public inbox for dwarves@vger.kernel.org
 help / color / mirror / Atom feed
From: "Ihor Solodrai" <ihor.solodrai@linux.dev>
To: "Alan Maguire" <alan.maguire@oracle.com>,
	dwarves@vger.kernel.org, bpf@vger.kernel.org
Cc: acme@kernel.org, ast@kernel.org, andrii@kernel.org,
	eddyz87@gmail.com, mykolal@fb.com, kernel-team@meta.com
Subject: Re: [PATCH dwarves v4 0/6] btf_encoder: emit type tags for bpf_arena pointers
Date: Mon, 24 Mar 2025 18:07:06 +0000	[thread overview]
Message-ID: <68a594e38c00ff3dd30d0a13fb1e1de71f19954c@linux.dev> (raw)
In-Reply-To: <b1a23727-098e-473b-8282-8fb0cbf97603@oracle.com>

On 3/23/25 4:11 AM, Alan Maguire wrote:
> [...]
>
> hi Ihor, I took a look at the series and merged it with latest next
> branch; results are in
>
> https://web.git.kernel.org/pub/scm/devel/pahole/pahole.git/log/?h=next.attributes-v4
>
> ...if you want to take a look.
>
> There are a few small things I think that it would be good to resolve
> before landing this.
>
> First, when testing this with -DLIBBPF_EMBEDDED=OFF and a packaged
> libbpf 1.5 - which means we wouldn't have the latest attributes-related
> libbpf function; I saw:
>
>   BTF     .tmp_vmlinux1.btf.o
> btf__add_type_attr is not available, is libbpf < 1.6?
> error: failed to encode function 'bbr_cwnd_event': invalid proto
> Failed to encode BTF
>   NM      .tmp_vmlinux1.syms

Hi Alan. Thanks for testing. This is my mistake, I should've checked
for attributes feature here:

@@ -731,6 +812,10 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
 
 	assert(ftype != NULL || state != NULL);
 
+	if (is_kfunc_state(state) && encoder->tag_kfuncs)
+		if (btf__add_bpf_arena_type_tags(encoder->btf, state) < 0)
+			return -1;

>
> ...and we got no BTF as a result. Ideally we'd like pahole to encode but
> without the attributes feature if not available. Related, we also report
> features that are not present, i.e. attributes with
> "--supported_btf_features".  So I propose that we make use of the weak
> declarations being NULL in an optional feature check function. It is
> optionally declared for a feature, and if declared must return true if
> the feature is available.
>
> Something like the below works (it's in the next.attributes-v4 branch
> too for reference) and it resolves the issue of BTF generation failure
> and accurate supported_btf_features reporting. What do you think? Thanks!

I think failing fast is a good approach here: check that all
requested/default features are available on startup.

>
> Alan
>
> From: Alan Maguire <alan.maguire@oracle.com>
> Date: Sun, 23 Mar 2025 11:06:18 +0000
> Subject: [PATCH] pahole: add a BTF feature check function
>
> It is used to see if functions that BTF features rely on are
> really there; weak declarations mean they will be NULL if not
> in non-embedded linked libbpf.
>
> This gives us more accurate --supported_btf_features reporting also.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  pahole.c | 39 +++++++++++++++++++++++++++++++++------
>  1 file changed, 33 insertions(+), 6 deletions(-)
>

Acked-by: Ihor Solodrai <ihor.solodrai@linux.dev>

> diff --git a/pahole.c b/pahole.c
> index 4a2b1ce..8304ba4 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -1183,10 +1183,31 @@ ARGP_PROGRAM_VERSION_HOOK_DEF =
> dwarves_print_version;
>   * floats, etc.  This ensures backwards compatibility.
>   */
>  #define BTF_DEFAULT_FEATURE(name, alias, initial_value)		\
> -	{ #name, #alias, &conf_load.alias, initial_value, true }
> +	{ #name, #alias, &conf_load.alias, initial_value, true, NULL }
> +
> +#define BTF_DEFAULT_FEATURE_CHECK(name, alias, initial_value,
> feature_check)	\
> +	{ #name, #alias, &conf_load.alias, initial_value, true, feature_check }
>
>  #define BTF_NON_DEFAULT_FEATURE(name, alias, initial_value)	\
> -	{ #name, #alias, &conf_load.alias, initial_value, false }
> +	{ #name, #alias, &conf_load.alias, initial_value, false, NULL }
> +
> +#define BTF_NON_DEFAULT_FEATURE_CHECK(name, alias, initial_value,
> feature_check) \
> +	{ #name, #alias, &conf_load.alias, initial_value, false, feature_check }
> +
> +static bool enum64_check(void)
> +{
> +	return btf__add_enum64 != NULL;
> +}
> +
> +static bool distilled_base_check(void)
> +{
> +	return btf__distill_base != NULL;
> +}
> +
> +static bool attributes_check(void)
> +{
> +	return btf__add_type_attr != NULL;
> +}
>
>  struct btf_feature {
>  	const char      *name;
> @@ -1196,20 +1217,23 @@ struct btf_feature {
>  	bool		default_enabled;	/* some nonstandard features may not
>  						 * be enabled for --btf_features=default
>  						 */
> +	bool		(*feature_check)(void);
>  } btf_features[] = {
>  	BTF_DEFAULT_FEATURE(encode_force, btf_encode_force, false),
>  	BTF_DEFAULT_FEATURE(var, skip_encoding_btf_vars, true),
>  	BTF_DEFAULT_FEATURE(float, btf_gen_floats, false),
>  	BTF_DEFAULT_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true),
>  	BTF_DEFAULT_FEATURE(type_tag, skip_encoding_btf_type_tag, true),
> -	BTF_DEFAULT_FEATURE(enum64, skip_encoding_btf_enum64, true),
> +	BTF_DEFAULT_FEATURE_CHECK(enum64, skip_encoding_btf_enum64, true,
> enum64_check),
>  	BTF_DEFAULT_FEATURE(optimized_func, btf_gen_optimized, false),
>  	BTF_DEFAULT_FEATURE(consistent_func,
> skip_encoding_btf_inconsistent_proto, false),
>  	BTF_DEFAULT_FEATURE(decl_tag_kfuncs, btf_decl_tag_kfuncs, false),
>  	BTF_NON_DEFAULT_FEATURE(reproducible_build, reproducible_build, false),
> -	BTF_NON_DEFAULT_FEATURE(distilled_base, btf_gen_distilled_base, false),
> +	BTF_NON_DEFAULT_FEATURE_CHECK(distilled_base, btf_gen_distilled_base,
> false,
> +				      distilled_base_check),
>  	BTF_NON_DEFAULT_FEATURE(global_var, encode_btf_global_vars, false),
> -	BTF_NON_DEFAULT_FEATURE(attributes, btf_attributes, false),
> +	BTF_NON_DEFAULT_FEATURE_CHECK(attributes, btf_attributes, false,
> +				      attributes_check),
>  };
>
>  #define BTF_MAX_FEATURE_STR	1024
> @@ -1248,7 +1272,8 @@ static void enable_btf_feature(struct btf_feature
> *feature)
>  	/* switch "initial-off" features on, and "initial-on" features
>  	 * off; i.e. negate the initial value.
>  	 */
> -	*feature->conf_value = !feature->initial_value;
> +	if (!feature->feature_check || feature->feature_check())
> +		*feature->conf_value = !feature->initial_value;
>  }
>
>  static void show_supported_btf_features(FILE *output)
> @@ -1256,6 +1281,8 @@ static void show_supported_btf_features(FILE *output)
>  	int i;
>
>  	for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
> +		if (btf_features[i].feature_check && !btf_features[i].feature_check())
> +			continue;
>  		if (i > 0)
>  			fprintf(output, ",");
>  		fprintf(output, "%s", btf_features[i].name);

  reply	other threads:[~2025-03-24 18:07 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-02-28 19:46 [PATCH dwarves v4 0/6] btf_encoder: emit type tags for bpf_arena pointers Ihor Solodrai
2025-02-28 19:46 ` [PATCH dwarves v4 1/6] btf_encoder: refactor btf_encoder__tag_kfuncs() Ihor Solodrai
2025-02-28 19:46 ` [PATCH dwarves v4 2/6] btf_encoder: use __weak declarations of version-dependent libbpf API Ihor Solodrai
2025-02-28 19:53   ` Ihor Solodrai
2025-03-06 17:14     ` Alan Maguire
2025-02-28 19:46 ` [PATCH dwarves v4 3/6] pahole: sync with libbpf mainline Ihor Solodrai
2025-02-28 19:46 ` [PATCH dwarves v4 4/6] btf_encoder: emit type tags for bpf_arena pointers Ihor Solodrai
2025-02-28 19:46 ` [PATCH dwarves v4 5/6] pahole: introduce --btf_feature=attributes Ihor Solodrai
2025-02-28 19:46 ` [PATCH dwarves v4 6/6] man-pages: describe attributes and remove reproducible_build Ihor Solodrai
2025-03-20 16:32 ` [PATCH dwarves v4 0/6] btf_encoder: emit type tags for bpf_arena pointers Ihor Solodrai
2025-03-20 20:34   ` Alan Maguire
2025-03-23 11:11     ` Alan Maguire
2025-03-24 18:07       ` Ihor Solodrai [this message]
2025-03-24 18:47         ` Ihor Solodrai
2025-03-25  9:59           ` Alan Maguire
2025-03-26 17:41             ` Ihor Solodrai
2025-03-27  8:22               ` Alan Maguire
2025-03-27 15:33                 ` Ihor Solodrai
2025-03-27 15:40                   ` Alan Maguire

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=68a594e38c00ff3dd30d0a13fb1e1de71f19954c@linux.dev \
    --to=ihor.solodrai@linux.dev \
    --cc=acme@kernel.org \
    --cc=alan.maguire@oracle.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=dwarves@vger.kernel.org \
    --cc=eddyz87@gmail.com \
    --cc=kernel-team@meta.com \
    --cc=mykolal@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox