| Message ID | 20231011091732.93254-4-alan.maguire@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pahole, btf_encoder: support --btf_features | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, 2023-10-11 at 10:17 +0100, Alan Maguire wrote: > This allows consumers to specify an opt-in set of features > they want to use in BTF encoding. > > Supported features are > > encode_force Ignore invalid symbols when encoding BTF. > var Encode variables using BTF_KIND_VAR in BTF. > float Encode floating-point types in BTF. > decl_tag Encode declaration tags using BTF_KIND_DECL_TAG. > type_tag Encode type tags using BTF_KIND_TYPE_TAG. > enum64 Encode enum64 values with BTF_KIND_ENUM64. > optimized Encode representations of optimized functions > with suffixes like ".isra.0" etc > consistent Avoid encoding inconsistent static functions. > These occur when a parameter is optimized out > in some CUs and not others, or when the same > function name has inconsistent BTF descriptions > in different CUs. > > Specifying "--btf_features=all" is the equivalent to setting > all of the above. If pahole does not know about a feature > it silently ignores it. These properties allow us to use > the --btf_features option in the kernel pahole_flags.sh > script to specify the desired set of features. If a new > feature is not present in pahole but requested, pahole > BTF encoding will not complain (but will not encode the > feature). > > Suggested-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > --- > man-pages/pahole.1 | 20 +++++++++++ > pahole.c | 87 +++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 106 insertions(+), 1 deletion(-) > > diff --git a/man-pages/pahole.1 b/man-pages/pahole.1 > index c1b48de..7c072dc 100644 > --- a/man-pages/pahole.1 > +++ b/man-pages/pahole.1 > @@ -273,6 +273,26 @@ Generate BTF for functions with optimization-related suffixes (.isra, .constprop > .B \-\-btf_gen_all > Allow using all the BTF features supported by pahole. > > +.TP > +.B \-\-btf_features=FEATURE_LIST > +Encode BTF using the specified feature list, or specify 'all' for all features supported. This single parameter value can be used as an alternative to unsing multiple BTF-related options. Supported features are > + > +.nf > + encode_force Ignore invalid symbols when encoding BTF. > + var Encode variables using BTF_KIND_VAR in BTF. > + float Encode floating-point types in BTF. > + decl_tag Encode declaration tags using BTF_KIND_DECL_TAG. > + type_tag Encode type tags using BTF_KIND_TYPE_TAG. > + enum64 Encode enum64 values with BTF_KIND_ENUM64. > + optimized Encode representations of optimized functions > + with suffixes like ".isra.0" etc > + consistent Avoid encoding inconsistent static functions. > + These occur when a parameter is optimized out > + in some CUs and not others, or when the same > + function name has inconsistent BTF descriptions > + in different CUs. > +.fi > + > .TP > .B \-l, \-\-show_first_biggest_size_base_type_member > Show first biggest size base_type member. > diff --git a/pahole.c b/pahole.c > index 7a41dc3..4f00b08 100644 > --- a/pahole.c > +++ b/pahole.c > @@ -1229,6 +1229,83 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version; > #define ARGP_skip_emitting_atomic_typedefs 338 > #define ARGP_btf_gen_optimized 339 > #define ARGP_skip_encoding_btf_inconsistent_proto 340 > +#define ARGP_btf_features 341 > + > +/* --btf_features=feature1[,feature2,..] option allows us to specify > + * opt-in features (or "all"); these are translated into conf_load > + * values by specifying the associated bool offset and whether it > + * is a skip option or not; btf_features is for opting _into_ features > + * so for skip options we have to reverse the logic. For example > + * "--skip_encoding_btf_type_tag --btf_gen_floats" translate to > + * "--btf_features=type_tag,float" > + */ > +#define BTF_FEATURE(name, alias, skip) \ > + { #name, #alias, offsetof(struct conf_load, alias), skip } > + > +struct btf_feature { > + const char *name; > + const char *option_alias; > + size_t conf_load_offset; > + bool skip; > +} btf_features[] = { > + BTF_FEATURE(encode_force, btf_encode_force, false), > + BTF_FEATURE(var, skip_encoding_btf_vars, true), > + BTF_FEATURE(float, btf_gen_floats, false), > + BTF_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true), > + BTF_FEATURE(type_tag, skip_encoding_btf_type_tag, true), > + BTF_FEATURE(enum64, skip_encoding_btf_enum64, true), > + BTF_FEATURE(optimized, btf_gen_optimized, false), > + /* the "skip" in skip_encoding_btf_inconsistent_proto is misleading > + * here; this is a positive feature to ensure consistency of > + * representation rather than a negative option which we want > + * to invert. So as a result, "skip" is false here. > + */ > + BTF_FEATURE(consistent, skip_encoding_btf_inconsistent_proto, false), > +}; > + > +#define BTF_MAX_FEATURES 32 > +#define BTF_MAX_FEATURE_STR 256 > + > +/* Translate --btf_features=feature1[,feature2] into conf_load values. > + * Explicitly ignores unrecognized features to allow future specification > + * of new opt-in features. > + */ > +static void parse_btf_features(const char *features, struct conf_load *conf_load) > +{ > + char *feature_list[BTF_MAX_FEATURES] = {}; > + char f[BTF_MAX_FEATURE_STR]; > + bool encode_all = false; > + int i, j, n = 0; > + > + strncpy(f, features, sizeof(f)); > + > + if (strcmp(features, "all") == 0) { > + encode_all = true; > + } else { > + char *saveptr = NULL, *s = f, *t; > + > + while ((t = strtok_r(s, ",", &saveptr)) != NULL) { > + s = NULL; > + feature_list[n++] = t; Maybe guard against `n` >= BTF_MAX_FEATURES here? > + } > + } > + > + for (i = 0; i < ARRAY_SIZE(btf_features); i++) { > + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset); > + bool match = encode_all; > + > + if (!match) { > + for (j = 0; j < n; j++) { > + if (strcmp(feature_list[j], btf_features[i].name) == 0) { > + match = true; > + break; > + } > + } > + } > + if (match) > + *bval = btf_features[i].skip ? false : true; I'm not sure I understand the logic behind "skip" features. Take `decl_tag` for example: - by default conf_load->skip_encoding_btf_decl_tag is 0; - if `--btf_features=decl_tag` is passed it is still 0 because of the `skip ? false : true` logic. If there is no way to change "skip" features why listing these at all? Other than that I tested the patch-set with current kernel master and a change to pahole-flags.sh and bpf tests pass. > + } > +} > > static const struct argp_option pahole__options[] = { > { > @@ -1651,6 +1728,12 @@ static const struct argp_option pahole__options[] = { > .key = ARGP_skip_encoding_btf_inconsistent_proto, > .doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or that use unexpected registers for parameter values." > }, > + { > + .name = "btf_features", > + .key = ARGP_btf_features, > + .arg = "FEATURE_LIST", > + .doc = "Specify supported BTF features in FEATURE_LIST or 'all' for all supported features. See the pahole manual page for the list of supported features." > + }, > { > .name = NULL, > } > @@ -1796,7 +1879,7 @@ static error_t pahole__options_parser(int key, char *arg, > case ARGP_btf_gen_floats: > conf_load.btf_gen_floats = true; break; > case ARGP_btf_gen_all: > - conf_load.btf_gen_floats = true; break; > + parse_btf_features("all", &conf_load); break; > case ARGP_with_flexible_array: > show_with_flexible_array = true; break; > case ARGP_prettify_input_filename: > @@ -1826,6 +1909,8 @@ static error_t pahole__options_parser(int key, char *arg, > conf_load.btf_gen_optimized = true; break; > case ARGP_skip_encoding_btf_inconsistent_proto: > conf_load.skip_encoding_btf_inconsistent_proto = true; break; > + case ARGP_btf_features: > + parse_btf_features(arg, &conf_load); break; > default: > return ARGP_ERR_UNKNOWN; > }
On 11/10/2023 17:28, Eduard Zingerman wrote: > On Wed, 2023-10-11 at 10:17 +0100, Alan Maguire wrote: >> This allows consumers to specify an opt-in set of features >> they want to use in BTF encoding. >> >> Supported features are >> >> encode_force Ignore invalid symbols when encoding BTF. >> var Encode variables using BTF_KIND_VAR in BTF. >> float Encode floating-point types in BTF. >> decl_tag Encode declaration tags using BTF_KIND_DECL_TAG. >> type_tag Encode type tags using BTF_KIND_TYPE_TAG. >> enum64 Encode enum64 values with BTF_KIND_ENUM64. >> optimized Encode representations of optimized functions >> with suffixes like ".isra.0" etc >> consistent Avoid encoding inconsistent static functions. >> These occur when a parameter is optimized out >> in some CUs and not others, or when the same >> function name has inconsistent BTF descriptions >> in different CUs. >> >> Specifying "--btf_features=all" is the equivalent to setting >> all of the above. If pahole does not know about a feature >> it silently ignores it. These properties allow us to use >> the --btf_features option in the kernel pahole_flags.sh >> script to specify the desired set of features. If a new >> feature is not present in pahole but requested, pahole >> BTF encoding will not complain (but will not encode the >> feature). >> >> Suggested-by: Andrii Nakryiko <andrii@kernel.org> >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> >> --- >> man-pages/pahole.1 | 20 +++++++++++ >> pahole.c | 87 +++++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 106 insertions(+), 1 deletion(-) >> >> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1 >> index c1b48de..7c072dc 100644 >> --- a/man-pages/pahole.1 >> +++ b/man-pages/pahole.1 >> @@ -273,6 +273,26 @@ Generate BTF for functions with optimization-related suffixes (.isra, .constprop >> .B \-\-btf_gen_all >> Allow using all the BTF features supported by pahole. >> >> +.TP >> +.B \-\-btf_features=FEATURE_LIST >> +Encode BTF using the specified feature list, or specify 'all' for all features supported. This single parameter value can be used as an alternative to unsing multiple BTF-related options. Supported features are >> + >> +.nf >> + encode_force Ignore invalid symbols when encoding BTF. >> + var Encode variables using BTF_KIND_VAR in BTF. >> + float Encode floating-point types in BTF. >> + decl_tag Encode declaration tags using BTF_KIND_DECL_TAG. >> + type_tag Encode type tags using BTF_KIND_TYPE_TAG. >> + enum64 Encode enum64 values with BTF_KIND_ENUM64. >> + optimized Encode representations of optimized functions >> + with suffixes like ".isra.0" etc >> + consistent Avoid encoding inconsistent static functions. >> + These occur when a parameter is optimized out >> + in some CUs and not others, or when the same >> + function name has inconsistent BTF descriptions >> + in different CUs. >> +.fi >> + >> .TP >> .B \-l, \-\-show_first_biggest_size_base_type_member >> Show first biggest size base_type member. >> diff --git a/pahole.c b/pahole.c >> index 7a41dc3..4f00b08 100644 >> --- a/pahole.c >> +++ b/pahole.c >> @@ -1229,6 +1229,83 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version; >> #define ARGP_skip_emitting_atomic_typedefs 338 >> #define ARGP_btf_gen_optimized 339 >> #define ARGP_skip_encoding_btf_inconsistent_proto 340 >> +#define ARGP_btf_features 341 >> + >> +/* --btf_features=feature1[,feature2,..] option allows us to specify >> + * opt-in features (or "all"); these are translated into conf_load >> + * values by specifying the associated bool offset and whether it >> + * is a skip option or not; btf_features is for opting _into_ features >> + * so for skip options we have to reverse the logic. For example >> + * "--skip_encoding_btf_type_tag --btf_gen_floats" translate to >> + * "--btf_features=type_tag,float" >> + */ >> +#define BTF_FEATURE(name, alias, skip) \ >> + { #name, #alias, offsetof(struct conf_load, alias), skip } >> + >> +struct btf_feature { >> + const char *name; >> + const char *option_alias; >> + size_t conf_load_offset; >> + bool skip; >> +} btf_features[] = { >> + BTF_FEATURE(encode_force, btf_encode_force, false), >> + BTF_FEATURE(var, skip_encoding_btf_vars, true), >> + BTF_FEATURE(float, btf_gen_floats, false), >> + BTF_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true), >> + BTF_FEATURE(type_tag, skip_encoding_btf_type_tag, true), >> + BTF_FEATURE(enum64, skip_encoding_btf_enum64, true), >> + BTF_FEATURE(optimized, btf_gen_optimized, false), >> + /* the "skip" in skip_encoding_btf_inconsistent_proto is misleading >> + * here; this is a positive feature to ensure consistency of >> + * representation rather than a negative option which we want >> + * to invert. So as a result, "skip" is false here. >> + */ >> + BTF_FEATURE(consistent, skip_encoding_btf_inconsistent_proto, false), >> +}; >> + >> +#define BTF_MAX_FEATURES 32 >> +#define BTF_MAX_FEATURE_STR 256 >> + >> +/* Translate --btf_features=feature1[,feature2] into conf_load values. >> + * Explicitly ignores unrecognized features to allow future specification >> + * of new opt-in features. >> + */ >> +static void parse_btf_features(const char *features, struct conf_load *conf_load) >> +{ >> + char *feature_list[BTF_MAX_FEATURES] = {}; >> + char f[BTF_MAX_FEATURE_STR]; >> + bool encode_all = false; >> + int i, j, n = 0; >> + >> + strncpy(f, features, sizeof(f)); >> + >> + if (strcmp(features, "all") == 0) { >> + encode_all = true; >> + } else { >> + char *saveptr = NULL, *s = f, *t; >> + >> + while ((t = strtok_r(s, ",", &saveptr)) != NULL) { >> + s = NULL; >> + feature_list[n++] = t; > > Maybe guard against `n` >= BTF_MAX_FEATURES here? > good point - will fix. >> + } >> + } >> + >> + for (i = 0; i < ARRAY_SIZE(btf_features); i++) { >> + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset); >> + bool match = encode_all; >> + >> + if (!match) { >> + for (j = 0; j < n; j++) { >> + if (strcmp(feature_list[j], btf_features[i].name) == 0) { >> + match = true; >> + break; >> + } >> + } >> + } >> + if (match) >> + *bval = btf_features[i].skip ? false : true; > > I'm not sure I understand the logic behind "skip" features. > Take `decl_tag` for example: > - by default conf_load->skip_encoding_btf_decl_tag is 0; > - if `--btf_features=decl_tag` is passed it is still 0 because of the > `skip ? false : true` logic. > > If there is no way to change "skip" features why listing these at all? > You're right; in the case of a skip feature, I think we need the following behaviour 1. we skip the encoding by default (so the equivalent of --skip_encoding_btf_decl_tag, setting skip_encoding_btf_decl_tag to true 2. if the user however specifies the logical inversion of the skip feature in --btf_features (in this case "decl_tag" - or "all") skip_encoding_btf_decl_tag is set to false. So in my code we had 2 above but not 1. If both were in place I think we'd have the right set of behaviours. Does that sound right? Maybe a better way to express all this would be to rename the "skip" field in "struct btf_feature" to "default" - so in the case of a "skip" feature, the default is true, but for opt-in features, the default is false. > Other than that I tested the patch-set with current kernel master and > a change to pahole-flags.sh and bpf tests pass. > Thanks so much for testing this! Alan >> + } >> +} >> >> static const struct argp_option pahole__options[] = { >> { >> @@ -1651,6 +1728,12 @@ static const struct argp_option pahole__options[] = { >> .key = ARGP_skip_encoding_btf_inconsistent_proto, >> .doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or that use unexpected registers for parameter values." >> }, >> + { >> + .name = "btf_features", >> + .key = ARGP_btf_features, >> + .arg = "FEATURE_LIST", >> + .doc = "Specify supported BTF features in FEATURE_LIST or 'all' for all supported features. See the pahole manual page for the list of supported features." >> + }, >> { >> .name = NULL, >> } >> @@ -1796,7 +1879,7 @@ static error_t pahole__options_parser(int key, char *arg, >> case ARGP_btf_gen_floats: >> conf_load.btf_gen_floats = true; break; >> case ARGP_btf_gen_all: >> - conf_load.btf_gen_floats = true; break; >> + parse_btf_features("all", &conf_load); break; >> case ARGP_with_flexible_array: >> show_with_flexible_array = true; break; >> case ARGP_prettify_input_filename: >> @@ -1826,6 +1909,8 @@ static error_t pahole__options_parser(int key, char *arg, >> conf_load.btf_gen_optimized = true; break; >> case ARGP_skip_encoding_btf_inconsistent_proto: >> conf_load.skip_encoding_btf_inconsistent_proto = true; break; >> + case ARGP_btf_features: >> + parse_btf_features(arg, &conf_load); break; >> default: >> return ARGP_ERR_UNKNOWN; >> } >
On Wed, 2023-10-11 at 17:41 +0100, Alan Maguire wrote: [...] > > > + } > > > + } > > > + > > > + for (i = 0; i < ARRAY_SIZE(btf_features); i++) { > > > + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset); > > > + bool match = encode_all; > > > + > > > + if (!match) { > > > + for (j = 0; j < n; j++) { > > > + if (strcmp(feature_list[j], btf_features[i].name) == 0) { > > > + match = true; > > > + break; > > > + } > > > + } > > > + } > > > + if (match) > > > + *bval = btf_features[i].skip ? false : true; > > > > I'm not sure I understand the logic behind "skip" features. > > Take `decl_tag` for example: > > - by default conf_load->skip_encoding_btf_decl_tag is 0; > > - if `--btf_features=decl_tag` is passed it is still 0 because of the > > `skip ? false : true` logic. > > > > If there is no way to change "skip" features why listing these at all? > > > You're right; in the case of a skip feature, I think we need the > following behaviour > > 1. we skip the encoding by default (so the equivalent of > --skip_encoding_btf_decl_tag, setting skip_encoding_btf_decl_tag > to true > > 2. if the user however specifies the logical inversion of the skip > feature in --btf_features (in this case "decl_tag" - or "all") > skip_encoding_btf_decl_tag is set to false. > > So in my code we had 2 above but not 1. If both were in place I think > we'd have the right set of behaviours. Does that sound right? You mean when --features=? is specified we default to conf_load->skip_encoding_btf_decl_tag = true, and set it to false only if "all" or "decl_tag" is listed in features, right? > Maybe a better way to express all this would be to rename the "skip" > field in "struct btf_feature" to "default" - so in the case of a "skip" > feature, the default is true, but for opt-in features, the default is false. Yes, I agree, "default" is better as "skip" is a bit confusing. [...]
On 11/10/2023 20:08, Eduard Zingerman wrote: > On Wed, 2023-10-11 at 17:41 +0100, Alan Maguire wrote: > [...] >>>> + } >>>> + } >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(btf_features); i++) { >>>> + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset); >>>> + bool match = encode_all; >>>> + >>>> + if (!match) { >>>> + for (j = 0; j < n; j++) { >>>> + if (strcmp(feature_list[j], btf_features[i].name) == 0) { >>>> + match = true; >>>> + break; >>>> + } >>>> + } >>>> + } >>>> + if (match) >>>> + *bval = btf_features[i].skip ? false : true; >>> >>> I'm not sure I understand the logic behind "skip" features. >>> Take `decl_tag` for example: >>> - by default conf_load->skip_encoding_btf_decl_tag is 0; >>> - if `--btf_features=decl_tag` is passed it is still 0 because of the >>> `skip ? false : true` logic. >>> >>> If there is no way to change "skip" features why listing these at all? >>> >> You're right; in the case of a skip feature, I think we need the >> following behaviour >> >> 1. we skip the encoding by default (so the equivalent of >> --skip_encoding_btf_decl_tag, setting skip_encoding_btf_decl_tag >> to true >> >> 2. if the user however specifies the logical inversion of the skip >> feature in --btf_features (in this case "decl_tag" - or "all") >> skip_encoding_btf_decl_tag is set to false. >> >> So in my code we had 2 above but not 1. If both were in place I think >> we'd have the right set of behaviours. Does that sound right? > > You mean when --features=? is specified we default to > conf_load->skip_encoding_btf_decl_tag = true, and set it to false only > if "all" or "decl_tag" is listed in features, right? > Yep. Here's the comment I was thinking of adding for the next version, hopefully it clarifies this all a bit more than the original. +/* --btf_features=feature1[,feature2,..] allows us to specify + * a list of requested BTF features or "all" to enable all features. + * These are translated into the appropriate conf_load values via + * struct btf_feature which specifies the associated conf_load + * boolean field and whether its default (representing the feature being + * off) is false or true. + * + * btf_features is for opting _into_ features so for a case like + * conf_load->btf_gen_floats, the translation is simple; the presence + * of the "float" feature in --btf_features sets conf_load->btf_gen_floats + * to true. + * + * The more confusing case is for features that are enabled unless + * skipping them is specified; for example + * conf_load->skip_encoding_btf_type_tag. By default - to support + * the opt-in model of only enabling features the user asks for - + * conf_load->skip_encoding_btf_type_tag is set to true (meaning no + * type_tags) and it is only set to false if --btf_features contains + * the "type_tag" feature. + * + * So from the user perspective, all features specified via + * --btf_features are enabled, and if a feature is not specified, + * it is disabled. */ >> Maybe a better way to express all this would be to rename the "skip" >> field in "struct btf_feature" to "default" - so in the case of a "skip" >> feature, the default is true, but for opt-in features, the default is false. > > Yes, I agree, "default" is better as "skip" is a bit confusing. > Thanks; I'll use that next time. Alan > [...]
On Wed, 2023-10-11 at 23:05 +0100, Alan Maguire wrote: [...] > > > > I'm not sure I understand the logic behind "skip" features. > > > > Take `decl_tag` for example: > > > > - by default conf_load->skip_encoding_btf_decl_tag is 0; > > > > - if `--btf_features=decl_tag` is passed it is still 0 because of the > > > > `skip ? false : true` logic. > > > > > > > > If there is no way to change "skip" features why listing these at all? > > > > > > > You're right; in the case of a skip feature, I think we need the > > > following behaviour > > > > > > 1. we skip the encoding by default (so the equivalent of > > > --skip_encoding_btf_decl_tag, setting skip_encoding_btf_decl_tag > > > to true > > > > > > 2. if the user however specifies the logical inversion of the skip > > > feature in --btf_features (in this case "decl_tag" - or "all") > > > skip_encoding_btf_decl_tag is set to false. > > > > > > So in my code we had 2 above but not 1. If both were in place I think > > > we'd have the right set of behaviours. Does that sound right? > > > > You mean when --features=? is specified we default to > > conf_load->skip_encoding_btf_decl_tag = true, and set it to false only > > if "all" or "decl_tag" is listed in features, right? > > > > Yep. Here's the comment I was thinking of adding for the next version, > hopefully it clarifies this all a bit more than the original. > > +/* --btf_features=feature1[,feature2,..] allows us to specify > + * a list of requested BTF features or "all" to enable all features. > + * These are translated into the appropriate conf_load values via > + * struct btf_feature which specifies the associated conf_load > + * boolean field and whether its default (representing the feature being > + * off) is false or true. > + * > + * btf_features is for opting _into_ features so for a case like > + * conf_load->btf_gen_floats, the translation is simple; the presence > + * of the "float" feature in --btf_features sets conf_load->btf_gen_floats > + * to true. > + * > + * The more confusing case is for features that are enabled unless > + * skipping them is specified; for example > + * conf_load->skip_encoding_btf_type_tag. By default - to support > + * the opt-in model of only enabling features the user asks for - > + * conf_load->skip_encoding_btf_type_tag is set to true (meaning no > + * type_tags) and it is only set to false if --btf_features contains > + * the "type_tag" feature. > + * > + * So from the user perspective, all features specified via > + * --btf_features are enabled, and if a feature is not specified, > + * it is disabled. > */ Sounds reasonable. Maybe also add a line saying that skip_encoding_btf_decl_tag defaults to false if --btf_features is not specified to remain backwards compatible? Thanks, Eduard [...]
On 11/10/2023 23:14, Eduard Zingerman wrote: > On Wed, 2023-10-11 at 23:05 +0100, Alan Maguire wrote: > [...] >>>>> I'm not sure I understand the logic behind "skip" features. >>>>> Take `decl_tag` for example: >>>>> - by default conf_load->skip_encoding_btf_decl_tag is 0; >>>>> - if `--btf_features=decl_tag` is passed it is still 0 because of the >>>>> `skip ? false : true` logic. >>>>> >>>>> If there is no way to change "skip" features why listing these at all? >>>>> >>>> You're right; in the case of a skip feature, I think we need the >>>> following behaviour >>>> >>>> 1. we skip the encoding by default (so the equivalent of >>>> --skip_encoding_btf_decl_tag, setting skip_encoding_btf_decl_tag >>>> to true >>>> >>>> 2. if the user however specifies the logical inversion of the skip >>>> feature in --btf_features (in this case "decl_tag" - or "all") >>>> skip_encoding_btf_decl_tag is set to false. >>>> >>>> So in my code we had 2 above but not 1. If both were in place I think >>>> we'd have the right set of behaviours. Does that sound right? >>> >>> You mean when --features=? is specified we default to >>> conf_load->skip_encoding_btf_decl_tag = true, and set it to false only >>> if "all" or "decl_tag" is listed in features, right? >>> >> >> Yep. Here's the comment I was thinking of adding for the next version, >> hopefully it clarifies this all a bit more than the original. >> >> +/* --btf_features=feature1[,feature2,..] allows us to specify >> + * a list of requested BTF features or "all" to enable all features. >> + * These are translated into the appropriate conf_load values via >> + * struct btf_feature which specifies the associated conf_load >> + * boolean field and whether its default (representing the feature being >> + * off) is false or true. >> + * >> + * btf_features is for opting _into_ features so for a case like >> + * conf_load->btf_gen_floats, the translation is simple; the presence >> + * of the "float" feature in --btf_features sets conf_load->btf_gen_floats >> + * to true. >> + * >> + * The more confusing case is for features that are enabled unless >> + * skipping them is specified; for example >> + * conf_load->skip_encoding_btf_type_tag. By default - to support >> + * the opt-in model of only enabling features the user asks for - >> + * conf_load->skip_encoding_btf_type_tag is set to true (meaning no >> + * type_tags) and it is only set to false if --btf_features contains >> + * the "type_tag" feature. >> + * >> + * So from the user perspective, all features specified via >> + * --btf_features are enabled, and if a feature is not specified, >> + * it is disabled. >> */ > > Sounds reasonable. Maybe also add a line saying that > skip_encoding_btf_decl_tag defaults to false if --btf_features is not > specified to remain backwards compatible? > good idea, will do! Thanks! Alan > Thanks, > Eduard > > [...]
On Wed, Oct 11, 2023 at 10:17:31AM +0100, Alan Maguire wrote: SNIP > +#define BTF_FEATURE(name, alias, skip) \ > + { #name, #alias, offsetof(struct conf_load, alias), skip } > + > +struct btf_feature { > + const char *name; > + const char *option_alias; > + size_t conf_load_offset; > + bool skip; > +} btf_features[] = { > + BTF_FEATURE(encode_force, btf_encode_force, false), > + BTF_FEATURE(var, skip_encoding_btf_vars, true), > + BTF_FEATURE(float, btf_gen_floats, false), > + BTF_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true), > + BTF_FEATURE(type_tag, skip_encoding_btf_type_tag, true), > + BTF_FEATURE(enum64, skip_encoding_btf_enum64, true), > + BTF_FEATURE(optimized, btf_gen_optimized, false), > + /* the "skip" in skip_encoding_btf_inconsistent_proto is misleading > + * here; this is a positive feature to ensure consistency of > + * representation rather than a negative option which we want > + * to invert. So as a result, "skip" is false here. > + */ > + BTF_FEATURE(consistent, skip_encoding_btf_inconsistent_proto, false), > +}; > + > +#define BTF_MAX_FEATURES 32 > +#define BTF_MAX_FEATURE_STR 256 > + > +/* Translate --btf_features=feature1[,feature2] into conf_load values. > + * Explicitly ignores unrecognized features to allow future specification > + * of new opt-in features. > + */ > +static void parse_btf_features(const char *features, struct conf_load *conf_load) > +{ > + char *feature_list[BTF_MAX_FEATURES] = {}; > + char f[BTF_MAX_FEATURE_STR]; > + bool encode_all = false; > + int i, j, n = 0; > + > + strncpy(f, features, sizeof(f)); > + > + if (strcmp(features, "all") == 0) { > + encode_all = true; > + } else { > + char *saveptr = NULL, *s = f, *t; > + > + while ((t = strtok_r(s, ",", &saveptr)) != NULL) { > + s = NULL; > + feature_list[n++] = t; > + } > + } > + > + for (i = 0; i < ARRAY_SIZE(btf_features); i++) { > + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset); nit, would it be easier to have btf_features defined inside the function and pass specific bool pointers directly to BTF_FEATURE macro? jirka > + bool match = encode_all; > + > + if (!match) { > + for (j = 0; j < n; j++) { > + if (strcmp(feature_list[j], btf_features[i].name) == 0) { > + match = true; > + break; > + } > + } > + } > + if (match) > + *bval = btf_features[i].skip ? false : true; > + } > +} > > static const struct argp_option pahole__options[] = { > { > @@ -1651,6 +1728,12 @@ static const struct argp_option pahole__options[] = { > .key = ARGP_skip_encoding_btf_inconsistent_proto, > .doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or that use unexpected registers for parameter values." > }, > + { > + .name = "btf_features", > + .key = ARGP_btf_features, > + .arg = "FEATURE_LIST", > + .doc = "Specify supported BTF features in FEATURE_LIST or 'all' for all supported features. See the pahole manual page for the list of supported features." > + }, > { > .name = NULL, > } > @@ -1796,7 +1879,7 @@ static error_t pahole__options_parser(int key, char *arg, > case ARGP_btf_gen_floats: > conf_load.btf_gen_floats = true; break; > case ARGP_btf_gen_all: > - conf_load.btf_gen_floats = true; break; > + parse_btf_features("all", &conf_load); break; > case ARGP_with_flexible_array: > show_with_flexible_array = true; break; > case ARGP_prettify_input_filename: > @@ -1826,6 +1909,8 @@ static error_t pahole__options_parser(int key, char *arg, > conf_load.btf_gen_optimized = true; break; > case ARGP_skip_encoding_btf_inconsistent_proto: > conf_load.skip_encoding_btf_inconsistent_proto = true; break; > + case ARGP_btf_features: > + parse_btf_features(arg, &conf_load); break; > default: > return ARGP_ERR_UNKNOWN; > } > -- > 2.31.1 >
On 12/10/2023 13:53, Jiri Olsa wrote: > On Wed, Oct 11, 2023 at 10:17:31AM +0100, Alan Maguire wrote: > > SNIP > >> +#define BTF_FEATURE(name, alias, skip) \ >> + { #name, #alias, offsetof(struct conf_load, alias), skip } >> + >> +struct btf_feature { >> + const char *name; >> + const char *option_alias; >> + size_t conf_load_offset; >> + bool skip; >> +} btf_features[] = { >> + BTF_FEATURE(encode_force, btf_encode_force, false), >> + BTF_FEATURE(var, skip_encoding_btf_vars, true), >> + BTF_FEATURE(float, btf_gen_floats, false), >> + BTF_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true), >> + BTF_FEATURE(type_tag, skip_encoding_btf_type_tag, true), >> + BTF_FEATURE(enum64, skip_encoding_btf_enum64, true), >> + BTF_FEATURE(optimized, btf_gen_optimized, false), >> + /* the "skip" in skip_encoding_btf_inconsistent_proto is misleading >> + * here; this is a positive feature to ensure consistency of >> + * representation rather than a negative option which we want >> + * to invert. So as a result, "skip" is false here. >> + */ >> + BTF_FEATURE(consistent, skip_encoding_btf_inconsistent_proto, false), >> +}; >> + >> +#define BTF_MAX_FEATURES 32 >> +#define BTF_MAX_FEATURE_STR 256 >> + >> +/* Translate --btf_features=feature1[,feature2] into conf_load values. >> + * Explicitly ignores unrecognized features to allow future specification >> + * of new opt-in features. >> + */ >> +static void parse_btf_features(const char *features, struct conf_load *conf_load) >> +{ >> + char *feature_list[BTF_MAX_FEATURES] = {}; >> + char f[BTF_MAX_FEATURE_STR]; >> + bool encode_all = false; >> + int i, j, n = 0; >> + >> + strncpy(f, features, sizeof(f)); >> + >> + if (strcmp(features, "all") == 0) { >> + encode_all = true; >> + } else { >> + char *saveptr = NULL, *s = f, *t; >> + >> + while ((t = strtok_r(s, ",", &saveptr)) != NULL) { >> + s = NULL; >> + feature_list[n++] = t; >> + } >> + } >> + >> + for (i = 0; i < ARRAY_SIZE(btf_features); i++) { >> + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset); > > nit, would it be easier to have btf_features defined inside the function > and pass specific bool pointers directly to BTF_FEATURE macro? > thanks for taking a look! I _think_ I see what you mean; if we had conf_load we could encode the bool pointer directly using the BTF_FEATURE() definition, something like #define BTF_FEATURE(name, alias, default_value) \ { #name, #alias, &conf_load->alias, default_value } struct btf_feature { const char *name; const char *option_alias; bool *conf_value; bool default_value; } btf_features[] = { ... This will work I think because conf_load is a global variable, and I think we need to keep it global since it's also used by patch 4 to get the list of supported features. Is the above something like what you had in mind? Thanks! Alan > jirka > >> + bool match = encode_all; >> + >> + if (!match) { >> + for (j = 0; j < n; j++) { >> + if (strcmp(feature_list[j], btf_features[i].name) == 0) { >> + match = true; >> + break; >> + } >> + } >> + } >> + if (match) >> + *bval = btf_features[i].skip ? false : true; >> + } >> +} >> >> static const struct argp_option pahole__options[] = { >> { >> @@ -1651,6 +1728,12 @@ static const struct argp_option pahole__options[] = { >> .key = ARGP_skip_encoding_btf_inconsistent_proto, >> .doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or that use unexpected registers for parameter values." >> }, >> + { >> + .name = "btf_features", >> + .key = ARGP_btf_features, >> + .arg = "FEATURE_LIST", >> + .doc = "Specify supported BTF features in FEATURE_LIST or 'all' for all supported features. See the pahole manual page for the list of supported features." >> + }, >> { >> .name = NULL, >> } >> @@ -1796,7 +1879,7 @@ static error_t pahole__options_parser(int key, char *arg, >> case ARGP_btf_gen_floats: >> conf_load.btf_gen_floats = true; break; >> case ARGP_btf_gen_all: >> - conf_load.btf_gen_floats = true; break; >> + parse_btf_features("all", &conf_load); break; >> case ARGP_with_flexible_array: >> show_with_flexible_array = true; break; >> case ARGP_prettify_input_filename: >> @@ -1826,6 +1909,8 @@ static error_t pahole__options_parser(int key, char *arg, >> conf_load.btf_gen_optimized = true; break; >> case ARGP_skip_encoding_btf_inconsistent_proto: >> conf_load.skip_encoding_btf_inconsistent_proto = true; break; >> + case ARGP_btf_features: >> + parse_btf_features(arg, &conf_load); break; >> default: >> return ARGP_ERR_UNKNOWN; >> } >> -- >> 2.31.1 >> >
On Thu, Oct 12, 2023 at 02:48:50PM +0100, Alan Maguire wrote: > On 12/10/2023 13:53, Jiri Olsa wrote: > > On Wed, Oct 11, 2023 at 10:17:31AM +0100, Alan Maguire wrote: > > > > SNIP > > > >> +#define BTF_FEATURE(name, alias, skip) \ > >> + { #name, #alias, offsetof(struct conf_load, alias), skip } > >> + > >> +struct btf_feature { > >> + const char *name; > >> + const char *option_alias; > >> + size_t conf_load_offset; > >> + bool skip; > >> +} btf_features[] = { > >> + BTF_FEATURE(encode_force, btf_encode_force, false), > >> + BTF_FEATURE(var, skip_encoding_btf_vars, true), > >> + BTF_FEATURE(float, btf_gen_floats, false), > >> + BTF_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true), > >> + BTF_FEATURE(type_tag, skip_encoding_btf_type_tag, true), > >> + BTF_FEATURE(enum64, skip_encoding_btf_enum64, true), > >> + BTF_FEATURE(optimized, btf_gen_optimized, false), > >> + /* the "skip" in skip_encoding_btf_inconsistent_proto is misleading > >> + * here; this is a positive feature to ensure consistency of > >> + * representation rather than a negative option which we want > >> + * to invert. So as a result, "skip" is false here. > >> + */ > >> + BTF_FEATURE(consistent, skip_encoding_btf_inconsistent_proto, false), > >> +}; > >> + > >> +#define BTF_MAX_FEATURES 32 > >> +#define BTF_MAX_FEATURE_STR 256 > >> + > >> +/* Translate --btf_features=feature1[,feature2] into conf_load values. > >> + * Explicitly ignores unrecognized features to allow future specification > >> + * of new opt-in features. > >> + */ > >> +static void parse_btf_features(const char *features, struct conf_load *conf_load) > >> +{ > >> + char *feature_list[BTF_MAX_FEATURES] = {}; > >> + char f[BTF_MAX_FEATURE_STR]; > >> + bool encode_all = false; > >> + int i, j, n = 0; > >> + > >> + strncpy(f, features, sizeof(f)); > >> + > >> + if (strcmp(features, "all") == 0) { > >> + encode_all = true; > >> + } else { > >> + char *saveptr = NULL, *s = f, *t; > >> + > >> + while ((t = strtok_r(s, ",", &saveptr)) != NULL) { > >> + s = NULL; > >> + feature_list[n++] = t; > >> + } > >> + } > >> + > >> + for (i = 0; i < ARRAY_SIZE(btf_features); i++) { > >> + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset); > > > > nit, would it be easier to have btf_features defined inside the function > > and pass specific bool pointers directly to BTF_FEATURE macro? > > > > thanks for taking a look! I _think_ I see what you mean; if we had > conf_load we could encode the bool pointer directly using > the BTF_FEATURE() definition, something like > > #define BTF_FEATURE(name, alias, default_value) \ > { #name, #alias, &conf_load->alias, default_value } > > struct btf_feature { > const char *name; > const char *option_alias; > bool *conf_value; > bool default_value; > } btf_features[] = { > ... > > This will work I think because conf_load is a global variable, yes, it's global, but it's also passed as an argument to parse_btf_features, so it could be done on top of that conf_load pointer > and I think we need to keep it global since it's also used by > patch 4 to get the list of supported features. Is the above > something like what you had in mind? Thanks! yes, using the bools directly thanks, jirka
On Wed, Oct 11, 2023 at 2:17 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > This allows consumers to specify an opt-in set of features > they want to use in BTF encoding. This is exactly what I had in mind, so thanks a lot for doing this! Minor nits below, but otherwise a big thumb up from me for the overall approach. > > Supported features are > > encode_force Ignore invalid symbols when encoding BTF. ignore_invalid? Even then I don't really know what this means even after reading the description, but that's ok :) > var Encode variables using BTF_KIND_VAR in BTF. > float Encode floating-point types in BTF. > decl_tag Encode declaration tags using BTF_KIND_DECL_TAG. > type_tag Encode type tags using BTF_KIND_TYPE_TAG. > enum64 Encode enum64 values with BTF_KIND_ENUM64. > optimized Encode representations of optimized functions > with suffixes like ".isra.0" etc > consistent Avoid encoding inconsistent static functions. > These occur when a parameter is optimized out > in some CUs and not others, or when the same > function name has inconsistent BTF descriptions > in different CUs. both optimized and consistent refer to functions, so shouldn't the feature name include func somewhere? > > Specifying "--btf_features=all" is the equivalent to setting > all of the above. If pahole does not know about a feature > it silently ignores it. These properties allow us to use > the --btf_features option in the kernel pahole_flags.sh > script to specify the desired set of features. If a new > feature is not present in pahole but requested, pahole > BTF encoding will not complain (but will not encode the > feature). As I mentioned in the cover letter reply, we might add a "strict mode" flag, that will error out on unknown features. I don't have much opinion here, up to Arnaldo and others whether this is useful. > > Suggested-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > --- > man-pages/pahole.1 | 20 +++++++++++ > pahole.c | 87 +++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 106 insertions(+), 1 deletion(-) > > diff --git a/man-pages/pahole.1 b/man-pages/pahole.1 > index c1b48de..7c072dc 100644 > --- a/man-pages/pahole.1 > +++ b/man-pages/pahole.1 > @@ -273,6 +273,26 @@ Generate BTF for functions with optimization-related suffixes (.isra, .constprop > .B \-\-btf_gen_all > Allow using all the BTF features supported by pahole. > > +.TP > +.B \-\-btf_features=FEATURE_LIST > +Encode BTF using the specified feature list, or specify 'all' for all features supported. This single parameter value can be used as an alternative to unsing multiple BTF-related options. Supported features are > + > +.nf > + encode_force Ignore invalid symbols when encoding BTF. > + var Encode variables using BTF_KIND_VAR in BTF. > + float Encode floating-point types in BTF. > + decl_tag Encode declaration tags using BTF_KIND_DECL_TAG. > + type_tag Encode type tags using BTF_KIND_TYPE_TAG. > + enum64 Encode enum64 values with BTF_KIND_ENUM64. > + optimized Encode representations of optimized functions > + with suffixes like ".isra.0" etc > + consistent Avoid encoding inconsistent static functions. > + These occur when a parameter is optimized out > + in some CUs and not others, or when the same > + function name has inconsistent BTF descriptions > + in different CUs. > +.fi > + > .TP > .B \-l, \-\-show_first_biggest_size_base_type_member > Show first biggest size base_type member. > diff --git a/pahole.c b/pahole.c > index 7a41dc3..4f00b08 100644 > --- a/pahole.c > +++ b/pahole.c > @@ -1229,6 +1229,83 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version; > #define ARGP_skip_emitting_atomic_typedefs 338 > #define ARGP_btf_gen_optimized 339 > #define ARGP_skip_encoding_btf_inconsistent_proto 340 > +#define ARGP_btf_features 341 > + > +/* --btf_features=feature1[,feature2,..] option allows us to specify > + * opt-in features (or "all"); these are translated into conf_load > + * values by specifying the associated bool offset and whether it > + * is a skip option or not; btf_features is for opting _into_ features > + * so for skip options we have to reverse the logic. For example > + * "--skip_encoding_btf_type_tag --btf_gen_floats" translate to > + * "--btf_features=type_tag,float" > + */ > +#define BTF_FEATURE(name, alias, skip) \ > + { #name, #alias, offsetof(struct conf_load, alias), skip } > + > +struct btf_feature { > + const char *name; > + const char *option_alias; > + size_t conf_load_offset; > + bool skip; > +} btf_features[] = { > + BTF_FEATURE(encode_force, btf_encode_force, false), > + BTF_FEATURE(var, skip_encoding_btf_vars, true), > + BTF_FEATURE(float, btf_gen_floats, false), > + BTF_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true), > + BTF_FEATURE(type_tag, skip_encoding_btf_type_tag, true), > + BTF_FEATURE(enum64, skip_encoding_btf_enum64, true), > + BTF_FEATURE(optimized, btf_gen_optimized, false), > + /* the "skip" in skip_encoding_btf_inconsistent_proto is misleading > + * here; this is a positive feature to ensure consistency of > + * representation rather than a negative option which we want > + * to invert. So as a result, "skip" is false here. > + */ > + BTF_FEATURE(consistent, skip_encoding_btf_inconsistent_proto, false), > +}; > + > +#define BTF_MAX_FEATURES 32 > +#define BTF_MAX_FEATURE_STR 256 > + > +/* Translate --btf_features=feature1[,feature2] into conf_load values. > + * Explicitly ignores unrecognized features to allow future specification > + * of new opt-in features. > + */ > +static void parse_btf_features(const char *features, struct conf_load *conf_load) > +{ > + char *feature_list[BTF_MAX_FEATURES] = {}; > + char f[BTF_MAX_FEATURE_STR]; > + bool encode_all = false; > + int i, j, n = 0; > + > + strncpy(f, features, sizeof(f)); > + > + if (strcmp(features, "all") == 0) { > + encode_all = true; > + } else { > + char *saveptr = NULL, *s = f, *t; > + > + while ((t = strtok_r(s, ",", &saveptr)) != NULL) { > + s = NULL; > + feature_list[n++] = t; > + } > + } I see that pahole uses argp for argument parsing. argp supports specifying the same parameter multiple times, so it's very natural to support --btf_feature=var --btf_feature=float --btf_feature enum64 without doing any of this parsing. Just find a matching feature and set corresponding bool value in the callback. > + > + for (i = 0; i < ARRAY_SIZE(btf_features); i++) { > + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset); > + bool match = encode_all; > + > + if (!match) { > + for (j = 0; j < n; j++) { > + if (strcmp(feature_list[j], btf_features[i].name) == 0) { > + match = true; > + break; > + } > + } > + } > + if (match) > + *bval = btf_features[i].skip ? false : true; > + } > +} > > static const struct argp_option pahole__options[] = { > { > @@ -1651,6 +1728,12 @@ static const struct argp_option pahole__options[] = { > .key = ARGP_skip_encoding_btf_inconsistent_proto, > .doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or that use unexpected registers for parameter values." > }, > + { > + .name = "btf_features", > + .key = ARGP_btf_features, > + .arg = "FEATURE_LIST", > + .doc = "Specify supported BTF features in FEATURE_LIST or 'all' for all supported features. See the pahole manual page for the list of supported features." > + }, > { > .name = NULL, > } > @@ -1796,7 +1879,7 @@ static error_t pahole__options_parser(int key, char *arg, > case ARGP_btf_gen_floats: > conf_load.btf_gen_floats = true; break; > case ARGP_btf_gen_all: > - conf_load.btf_gen_floats = true; break; > + parse_btf_features("all", &conf_load); break; > case ARGP_with_flexible_array: > show_with_flexible_array = true; break; > case ARGP_prettify_input_filename: > @@ -1826,6 +1909,8 @@ static error_t pahole__options_parser(int key, char *arg, > conf_load.btf_gen_optimized = true; break; > case ARGP_skip_encoding_btf_inconsistent_proto: > conf_load.skip_encoding_btf_inconsistent_proto = true; break; > + case ARGP_btf_features: > + parse_btf_features(arg, &conf_load); break; > default: > return ARGP_ERR_UNKNOWN; > } > -- > 2.31.1 >
On 13/10/2023 01:21, Andrii Nakryiko wrote: > On Wed, Oct 11, 2023 at 2:17 AM Alan Maguire <alan.maguire@oracle.com> wrote: >> >> This allows consumers to specify an opt-in set of features >> they want to use in BTF encoding. > > This is exactly what I had in mind, so thanks a lot for doing this! > Minor nits below, but otherwise a big thumb up from me for the overall > approach. > Great! >> >> Supported features are >> >> encode_force Ignore invalid symbols when encoding BTF. > > ignore_invalid? Even then I don't really know what this means even > after reading the description, but that's ok :) > The only place it is currently used is when checking btf_name_valid() on a variable - if encode_force is specified we skip invalidly-named symbols and drive on. I'll try and flesh out the description a bit. >> var Encode variables using BTF_KIND_VAR in BTF. >> float Encode floating-point types in BTF. >> decl_tag Encode declaration tags using BTF_KIND_DECL_TAG. >> type_tag Encode type tags using BTF_KIND_TYPE_TAG. >> enum64 Encode enum64 values with BTF_KIND_ENUM64. >> optimized Encode representations of optimized functions >> with suffixes like ".isra.0" etc >> consistent Avoid encoding inconsistent static functions. >> These occur when a parameter is optimized out >> in some CUs and not others, or when the same >> function name has inconsistent BTF descriptions >> in different CUs. > > both optimized and consistent refer to functions, so shouldn't the > feature name include func somewhere? > Yeah, though consistent may eventually need to apply to variables too. As Stephen and I have been exploring adding global variable support for all variables, we've run across a bunch of cases where the same variable name refers to different types too. Worse, it often happens that the same variable name refers to a "struct foo" and a "struct foo *" which is liable to be very confusing. So I think we will either need to skip encoding such variables for now (the "consistent" approach used for functions) or we may have to sort out the symbol->address mapping issue in BTF for functions _and_ variables before we land variable support. My preference would be the latter - since it will solve the issues with functions too - but I think we can probably make either sequence work. So all of that is to say we can either stick with "consistent" with the expectation that it may be more broadly applied to variables, or convert to "consistent_func", I've no major preference which. Optimized definitely refers to functions so we can switch that to "optimized_func" if you like. >> >> Specifying "--btf_features=all" is the equivalent to setting >> all of the above. If pahole does not know about a feature >> it silently ignores it. These properties allow us to use >> the --btf_features option in the kernel pahole_flags.sh >> script to specify the desired set of features. If a new >> feature is not present in pahole but requested, pahole >> BTF encoding will not complain (but will not encode the >> feature). > > As I mentioned in the cover letter reply, we might add a "strict mode" > flag, that will error out on unknown features. I don't have much > opinion here, up to Arnaldo and others whether this is useful. > I think this is a good idea. I'll add it to v2 unless anyone has major objections. >> >> Suggested-by: Andrii Nakryiko <andrii@kernel.org> >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> >> --- >> man-pages/pahole.1 | 20 +++++++++++ >> pahole.c | 87 +++++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 106 insertions(+), 1 deletion(-) >> >> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1 >> index c1b48de..7c072dc 100644 >> --- a/man-pages/pahole.1 >> +++ b/man-pages/pahole.1 >> @@ -273,6 +273,26 @@ Generate BTF for functions with optimization-related suffixes (.isra, .constprop >> .B \-\-btf_gen_all >> Allow using all the BTF features supported by pahole. >> >> +.TP >> +.B \-\-btf_features=FEATURE_LIST >> +Encode BTF using the specified feature list, or specify 'all' for all features supported. This single parameter value can be used as an alternative to unsing multiple BTF-related options. Supported features are >> + >> +.nf >> + encode_force Ignore invalid symbols when encoding BTF. >> + var Encode variables using BTF_KIND_VAR in BTF. >> + float Encode floating-point types in BTF. >> + decl_tag Encode declaration tags using BTF_KIND_DECL_TAG. >> + type_tag Encode type tags using BTF_KIND_TYPE_TAG. >> + enum64 Encode enum64 values with BTF_KIND_ENUM64. >> + optimized Encode representations of optimized functions >> + with suffixes like ".isra.0" etc >> + consistent Avoid encoding inconsistent static functions. >> + These occur when a parameter is optimized out >> + in some CUs and not others, or when the same >> + function name has inconsistent BTF descriptions >> + in different CUs. >> +.fi >> + >> .TP >> .B \-l, \-\-show_first_biggest_size_base_type_member >> Show first biggest size base_type member. >> diff --git a/pahole.c b/pahole.c >> index 7a41dc3..4f00b08 100644 >> --- a/pahole.c >> +++ b/pahole.c >> @@ -1229,6 +1229,83 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version; >> #define ARGP_skip_emitting_atomic_typedefs 338 >> #define ARGP_btf_gen_optimized 339 >> #define ARGP_skip_encoding_btf_inconsistent_proto 340 >> +#define ARGP_btf_features 341 >> + >> +/* --btf_features=feature1[,feature2,..] option allows us to specify >> + * opt-in features (or "all"); these are translated into conf_load >> + * values by specifying the associated bool offset and whether it >> + * is a skip option or not; btf_features is for opting _into_ features >> + * so for skip options we have to reverse the logic. For example >> + * "--skip_encoding_btf_type_tag --btf_gen_floats" translate to >> + * "--btf_features=type_tag,float" >> + */ >> +#define BTF_FEATURE(name, alias, skip) \ >> + { #name, #alias, offsetof(struct conf_load, alias), skip } >> + >> +struct btf_feature { >> + const char *name; >> + const char *option_alias; >> + size_t conf_load_offset; >> + bool skip; >> +} btf_features[] = { >> + BTF_FEATURE(encode_force, btf_encode_force, false), >> + BTF_FEATURE(var, skip_encoding_btf_vars, true), >> + BTF_FEATURE(float, btf_gen_floats, false), >> + BTF_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true), >> + BTF_FEATURE(type_tag, skip_encoding_btf_type_tag, true), >> + BTF_FEATURE(enum64, skip_encoding_btf_enum64, true), >> + BTF_FEATURE(optimized, btf_gen_optimized, false), >> + /* the "skip" in skip_encoding_btf_inconsistent_proto is misleading >> + * here; this is a positive feature to ensure consistency of >> + * representation rather than a negative option which we want >> + * to invert. So as a result, "skip" is false here. >> + */ >> + BTF_FEATURE(consistent, skip_encoding_btf_inconsistent_proto, false), >> +}; >> + >> +#define BTF_MAX_FEATURES 32 >> +#define BTF_MAX_FEATURE_STR 256 >> + >> +/* Translate --btf_features=feature1[,feature2] into conf_load values. >> + * Explicitly ignores unrecognized features to allow future specification >> + * of new opt-in features. >> + */ >> +static void parse_btf_features(const char *features, struct conf_load *conf_load) >> +{ >> + char *feature_list[BTF_MAX_FEATURES] = {}; >> + char f[BTF_MAX_FEATURE_STR]; >> + bool encode_all = false; >> + int i, j, n = 0; >> + >> + strncpy(f, features, sizeof(f)); >> + >> + if (strcmp(features, "all") == 0) { >> + encode_all = true; >> + } else { >> + char *saveptr = NULL, *s = f, *t; >> + >> + while ((t = strtok_r(s, ",", &saveptr)) != NULL) { >> + s = NULL; >> + feature_list[n++] = t; >> + } >> + } > > I see that pahole uses argp for argument parsing. argp supports > specifying the same parameter multiple times, so it's very natural to > support > > --btf_feature=var --btf_feature=float --btf_feature enum64 > > without doing any of this parsing. Just find a matching feature and > set corresponding bool value in the callback. > Sure, will do. >> + >> + for (i = 0; i < ARRAY_SIZE(btf_features); i++) { >> + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset); >> + bool match = encode_all; >> + >> + if (!match) { >> + for (j = 0; j < n; j++) { >> + if (strcmp(feature_list[j], btf_features[i].name) == 0) { >> + match = true; >> + break; >> + } >> + } >> + } >> + if (match) >> + *bval = btf_features[i].skip ? false : true; >> + } >> +} >> >> static const struct argp_option pahole__options[] = { >> { >> @@ -1651,6 +1728,12 @@ static const struct argp_option pahole__options[] = { >> .key = ARGP_skip_encoding_btf_inconsistent_proto, >> .doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or that use unexpected registers for parameter values." >> }, >> + { >> + .name = "btf_features", >> + .key = ARGP_btf_features, >> + .arg = "FEATURE_LIST", >> + .doc = "Specify supported BTF features in FEATURE_LIST or 'all' for all supported features. See the pahole manual page for the list of supported features." >> + }, >> { >> .name = NULL, >> } >> @@ -1796,7 +1879,7 @@ static error_t pahole__options_parser(int key, char *arg, >> case ARGP_btf_gen_floats: >> conf_load.btf_gen_floats = true; break; >> case ARGP_btf_gen_all: >> - conf_load.btf_gen_floats = true; break; >> + parse_btf_features("all", &conf_load); break; >> case ARGP_with_flexible_array: >> show_with_flexible_array = true; break; >> case ARGP_prettify_input_filename: >> @@ -1826,6 +1909,8 @@ static error_t pahole__options_parser(int key, char *arg, >> conf_load.btf_gen_optimized = true; break; >> case ARGP_skip_encoding_btf_inconsistent_proto: >> conf_load.skip_encoding_btf_inconsistent_proto = true; break; >> + case ARGP_btf_features: >> + parse_btf_features(arg, &conf_load); break; >> default: >> return ARGP_ERR_UNKNOWN; >> } >> -- >> 2.31.1 >>
Em Thu, Oct 12, 2023 at 01:35:41PM +0100, Alan Maguire escreveu: > On 11/10/2023 23:14, Eduard Zingerman wrote: > > On Wed, 2023-10-11 at 23:05 +0100, Alan Maguire wrote: > > [...] > >>>>> I'm not sure I understand the logic behind "skip" features. > >>>>> Take `decl_tag` for example: > >>>>> - by default conf_load->skip_encoding_btf_decl_tag is 0; > >>>>> - if `--btf_features=decl_tag` is passed it is still 0 because of the > >>>>> `skip ? false : true` logic. > >>>>> > >>>>> If there is no way to change "skip" features why listing these at all? > >>>>> > >>>> You're right; in the case of a skip feature, I think we need the > >>>> following behaviour > >>>> > >>>> 1. we skip the encoding by default (so the equivalent of > >>>> --skip_encoding_btf_decl_tag, setting skip_encoding_btf_decl_tag > >>>> to true > >>>> > >>>> 2. if the user however specifies the logical inversion of the skip > >>>> feature in --btf_features (in this case "decl_tag" - or "all") > >>>> skip_encoding_btf_decl_tag is set to false. > >>>> > >>>> So in my code we had 2 above but not 1. If both were in place I think > >>>> we'd have the right set of behaviours. Does that sound right? > >>> > >>> You mean when --features=? is specified we default to > >>> conf_load->skip_encoding_btf_decl_tag = true, and set it to false only > >>> if "all" or "decl_tag" is listed in features, right? > >>> > >> > >> Yep. Here's the comment I was thinking of adding for the next version, > >> hopefully it clarifies this all a bit more than the original. > >> > >> +/* --btf_features=feature1[,feature2,..] allows us to specify > >> + * a list of requested BTF features or "all" to enable all features. > >> + * These are translated into the appropriate conf_load values via > >> + * struct btf_feature which specifies the associated conf_load > >> + * boolean field and whether its default (representing the feature being > >> + * off) is false or true. > >> + * > >> + * btf_features is for opting _into_ features so for a case like > >> + * conf_load->btf_gen_floats, the translation is simple; the presence > >> + * of the "float" feature in --btf_features sets conf_load->btf_gen_floats > >> + * to true. > >> + * > >> + * The more confusing case is for features that are enabled unless > >> + * skipping them is specified; for example > >> + * conf_load->skip_encoding_btf_type_tag. By default - to support > >> + * the opt-in model of only enabling features the user asks for - > >> + * conf_load->skip_encoding_btf_type_tag is set to true (meaning no > >> + * type_tags) and it is only set to false if --btf_features contains > >> + * the "type_tag" feature. > >> + * > >> + * So from the user perspective, all features specified via > >> + * --btf_features are enabled, and if a feature is not specified, > >> + * it is disabled. > >> */ > > > > Sounds reasonable. Maybe also add a line saying that > > skip_encoding_btf_decl_tag defaults to false if --btf_features is not > > specified to remain backwards compatible? > > > > good idea, will do! Thanks! I have to catch up with all the comments on this thread, but does this mean you're respinning the series now? - Arnaldo
On 13/10/2023 15:17, Arnaldo Carvalho de Melo wrote: > Em Thu, Oct 12, 2023 at 01:35:41PM +0100, Alan Maguire escreveu: >> On 11/10/2023 23:14, Eduard Zingerman wrote: >>> On Wed, 2023-10-11 at 23:05 +0100, Alan Maguire wrote: >>> [...] >>>>>>> I'm not sure I understand the logic behind "skip" features. >>>>>>> Take `decl_tag` for example: >>>>>>> - by default conf_load->skip_encoding_btf_decl_tag is 0; >>>>>>> - if `--btf_features=decl_tag` is passed it is still 0 because of the >>>>>>> `skip ? false : true` logic. >>>>>>> >>>>>>> If there is no way to change "skip" features why listing these at all? >>>>>>> >>>>>> You're right; in the case of a skip feature, I think we need the >>>>>> following behaviour >>>>>> >>>>>> 1. we skip the encoding by default (so the equivalent of >>>>>> --skip_encoding_btf_decl_tag, setting skip_encoding_btf_decl_tag >>>>>> to true >>>>>> >>>>>> 2. if the user however specifies the logical inversion of the skip >>>>>> feature in --btf_features (in this case "decl_tag" - or "all") >>>>>> skip_encoding_btf_decl_tag is set to false. >>>>>> >>>>>> So in my code we had 2 above but not 1. If both were in place I think >>>>>> we'd have the right set of behaviours. Does that sound right? >>>>> >>>>> You mean when --features=? is specified we default to >>>>> conf_load->skip_encoding_btf_decl_tag = true, and set it to false only >>>>> if "all" or "decl_tag" is listed in features, right? >>>>> >>>> >>>> Yep. Here's the comment I was thinking of adding for the next version, >>>> hopefully it clarifies this all a bit more than the original. >>>> >>>> +/* --btf_features=feature1[,feature2,..] allows us to specify >>>> + * a list of requested BTF features or "all" to enable all features. >>>> + * These are translated into the appropriate conf_load values via >>>> + * struct btf_feature which specifies the associated conf_load >>>> + * boolean field and whether its default (representing the feature being >>>> + * off) is false or true. >>>> + * >>>> + * btf_features is for opting _into_ features so for a case like >>>> + * conf_load->btf_gen_floats, the translation is simple; the presence >>>> + * of the "float" feature in --btf_features sets conf_load->btf_gen_floats >>>> + * to true. >>>> + * >>>> + * The more confusing case is for features that are enabled unless >>>> + * skipping them is specified; for example >>>> + * conf_load->skip_encoding_btf_type_tag. By default - to support >>>> + * the opt-in model of only enabling features the user asks for - >>>> + * conf_load->skip_encoding_btf_type_tag is set to true (meaning no >>>> + * type_tags) and it is only set to false if --btf_features contains >>>> + * the "type_tag" feature. >>>> + * >>>> + * So from the user perspective, all features specified via >>>> + * --btf_features are enabled, and if a feature is not specified, >>>> + * it is disabled. >>>> */ >>> >>> Sounds reasonable. Maybe also add a line saying that >>> skip_encoding_btf_decl_tag defaults to false if --btf_features is not >>> specified to remain backwards compatible? >>> >> >> good idea, will do! Thanks! > > I have to catch up with all the comments on this thread, but does this > mean you're respinning the series now? > Yep, I'm respinning now. There were a few small things in Andrii's suggestions that we might need to figure out - mostly around names - but there's enough change since the RFC that it would probably be best to discuss those with the v2 series. Thanks! Alan > - Arnaldo >
On Fri, Oct 13, 2023 at 4:54 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > On 13/10/2023 01:21, Andrii Nakryiko wrote: > > On Wed, Oct 11, 2023 at 2:17 AM Alan Maguire <alan.maguire@oracle.com> wrote: > >> > >> This allows consumers to specify an opt-in set of features > >> they want to use in BTF encoding. > > > > This is exactly what I had in mind, so thanks a lot for doing this! > > Minor nits below, but otherwise a big thumb up from me for the overall > > approach. > > > > Great! > > >> > >> Supported features are > >> > >> encode_force Ignore invalid symbols when encoding BTF. > > > > ignore_invalid? Even then I don't really know what this means even > > after reading the description, but that's ok :) > > > > The only place it is currently used is when checking btf_name_valid() > on a variable - if encode_force is specified we skip invalidly-named > symbols and drive on. I'll try and flesh out the description a bit. > > > >> var Encode variables using BTF_KIND_VAR in BTF. > >> float Encode floating-point types in BTF. > >> decl_tag Encode declaration tags using BTF_KIND_DECL_TAG. > >> type_tag Encode type tags using BTF_KIND_TYPE_TAG. > >> enum64 Encode enum64 values with BTF_KIND_ENUM64. > >> optimized Encode representations of optimized functions > >> with suffixes like ".isra.0" etc > >> consistent Avoid encoding inconsistent static functions. > >> These occur when a parameter is optimized out > >> in some CUs and not others, or when the same > >> function name has inconsistent BTF descriptions > >> in different CUs. > > > > both optimized and consistent refer to functions, so shouldn't the > > feature name include func somewhere? > > > > Yeah, though consistent may eventually need to apply to variables too. > As Stephen and I have been exploring adding global variable support for > all variables, we've run across a bunch of cases where the same variable > name refers to different types too. Worse, it often happens that the > same variable name refers to a "struct foo" and a "struct foo *" which > is liable to be very confusing. So I think we will either need to skip > encoding such variables for now (the "consistent" approach used for > functions) or we may have to sort out the symbol->address mapping issue > in BTF for functions _and_ variables before we land variable support. > My preference would be the latter - since it will solve the issues with > functions too - but I think we can probably make either sequence work. > > So all of that is to say we can either stick with "consistent" with > the expectation that it may be more broadly applied to variables, or > convert to "consistent_func", I've no major preference which. > > Optimized definitely refers to functions so we can switch that to > "optimized_func" if you like. > So I'd say optimized params will be its own feature, no? So yeah, I think optimized_funcs is a better and more specific name. We can probably add groups/aliases separate or later on, so then "optimized" will mean both optimized_funcs and optimized_params, etc. Just like you have all. But this starts to sounds like a feature creep, so let's go with specific names now, and worry about aliases when we need them. > >> > >> Specifying "--btf_features=all" is the equivalent to setting > >> all of the above. If pahole does not know about a feature > >> it silently ignores it. These properties allow us to use > >> the --btf_features option in the kernel pahole_flags.sh > >> script to specify the desired set of features. If a new > >> feature is not present in pahole but requested, pahole > >> BTF encoding will not complain (but will not encode the > >> feature). > > > > As I mentioned in the cover letter reply, we might add a "strict mode" > > flag, that will error out on unknown features. I don't have much > > opinion here, up to Arnaldo and others whether this is useful. > > > > I think this is a good idea. I'll add it to v2 unless anyone has major > objections. > SGTM > >> > >> Suggested-by: Andrii Nakryiko <andrii@kernel.org> > >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > >> --- > >> man-pages/pahole.1 | 20 +++++++++++ > >> pahole.c | 87 +++++++++++++++++++++++++++++++++++++++++++++- > >> 2 files changed, 106 insertions(+), 1 deletion(-) > >> [...]
diff --git a/man-pages/pahole.1 b/man-pages/pahole.1 index c1b48de..7c072dc 100644 --- a/man-pages/pahole.1 +++ b/man-pages/pahole.1 @@ -273,6 +273,26 @@ Generate BTF for functions with optimization-related suffixes (.isra, .constprop .B \-\-btf_gen_all Allow using all the BTF features supported by pahole. +.TP +.B \-\-btf_features=FEATURE_LIST +Encode BTF using the specified feature list, or specify 'all' for all features supported. This single parameter value can be used as an alternative to unsing multiple BTF-related options. Supported features are + +.nf + encode_force Ignore invalid symbols when encoding BTF. + var Encode variables using BTF_KIND_VAR in BTF. + float Encode floating-point types in BTF. + decl_tag Encode declaration tags using BTF_KIND_DECL_TAG. + type_tag Encode type tags using BTF_KIND_TYPE_TAG. + enum64 Encode enum64 values with BTF_KIND_ENUM64. + optimized Encode representations of optimized functions + with suffixes like ".isra.0" etc + consistent Avoid encoding inconsistent static functions. + These occur when a parameter is optimized out + in some CUs and not others, or when the same + function name has inconsistent BTF descriptions + in different CUs. +.fi + .TP .B \-l, \-\-show_first_biggest_size_base_type_member Show first biggest size base_type member. diff --git a/pahole.c b/pahole.c index 7a41dc3..4f00b08 100644 --- a/pahole.c +++ b/pahole.c @@ -1229,6 +1229,83 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version; #define ARGP_skip_emitting_atomic_typedefs 338 #define ARGP_btf_gen_optimized 339 #define ARGP_skip_encoding_btf_inconsistent_proto 340 +#define ARGP_btf_features 341 + +/* --btf_features=feature1[,feature2,..] option allows us to specify + * opt-in features (or "all"); these are translated into conf_load + * values by specifying the associated bool offset and whether it + * is a skip option or not; btf_features is for opting _into_ features + * so for skip options we have to reverse the logic. For example + * "--skip_encoding_btf_type_tag --btf_gen_floats" translate to + * "--btf_features=type_tag,float" + */ +#define BTF_FEATURE(name, alias, skip) \ + { #name, #alias, offsetof(struct conf_load, alias), skip } + +struct btf_feature { + const char *name; + const char *option_alias; + size_t conf_load_offset; + bool skip; +} btf_features[] = { + BTF_FEATURE(encode_force, btf_encode_force, false), + BTF_FEATURE(var, skip_encoding_btf_vars, true), + BTF_FEATURE(float, btf_gen_floats, false), + BTF_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true), + BTF_FEATURE(type_tag, skip_encoding_btf_type_tag, true), + BTF_FEATURE(enum64, skip_encoding_btf_enum64, true), + BTF_FEATURE(optimized, btf_gen_optimized, false), + /* the "skip" in skip_encoding_btf_inconsistent_proto is misleading + * here; this is a positive feature to ensure consistency of + * representation rather than a negative option which we want + * to invert. So as a result, "skip" is false here. + */ + BTF_FEATURE(consistent, skip_encoding_btf_inconsistent_proto, false), +}; + +#define BTF_MAX_FEATURES 32 +#define BTF_MAX_FEATURE_STR 256 + +/* Translate --btf_features=feature1[,feature2] into conf_load values. + * Explicitly ignores unrecognized features to allow future specification + * of new opt-in features. + */ +static void parse_btf_features(const char *features, struct conf_load *conf_load) +{ + char *feature_list[BTF_MAX_FEATURES] = {}; + char f[BTF_MAX_FEATURE_STR]; + bool encode_all = false; + int i, j, n = 0; + + strncpy(f, features, sizeof(f)); + + if (strcmp(features, "all") == 0) { + encode_all = true; + } else { + char *saveptr = NULL, *s = f, *t; + + while ((t = strtok_r(s, ",", &saveptr)) != NULL) { + s = NULL; + feature_list[n++] = t; + } + } + + for (i = 0; i < ARRAY_SIZE(btf_features); i++) { + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset); + bool match = encode_all; + + if (!match) { + for (j = 0; j < n; j++) { + if (strcmp(feature_list[j], btf_features[i].name) == 0) { + match = true; + break; + } + } + } + if (match) + *bval = btf_features[i].skip ? false : true; + } +} static const struct argp_option pahole__options[] = { { @@ -1651,6 +1728,12 @@ static const struct argp_option pahole__options[] = { .key = ARGP_skip_encoding_btf_inconsistent_proto, .doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or that use unexpected registers for parameter values." }, + { + .name = "btf_features", + .key = ARGP_btf_features, + .arg = "FEATURE_LIST", + .doc = "Specify supported BTF features in FEATURE_LIST or 'all' for all supported features. See the pahole manual page for the list of supported features." + }, { .name = NULL, } @@ -1796,7 +1879,7 @@ static error_t pahole__options_parser(int key, char *arg, case ARGP_btf_gen_floats: conf_load.btf_gen_floats = true; break; case ARGP_btf_gen_all: - conf_load.btf_gen_floats = true; break; + parse_btf_features("all", &conf_load); break; case ARGP_with_flexible_array: show_with_flexible_array = true; break; case ARGP_prettify_input_filename: @@ -1826,6 +1909,8 @@ static error_t pahole__options_parser(int key, char *arg, conf_load.btf_gen_optimized = true; break; case ARGP_skip_encoding_btf_inconsistent_proto: conf_load.skip_encoding_btf_inconsistent_proto = true; break; + case ARGP_btf_features: + parse_btf_features(arg, &conf_load); break; default: return ARGP_ERR_UNKNOWN; }
This allows consumers to specify an opt-in set of features they want to use in BTF encoding. Supported features are encode_force Ignore invalid symbols when encoding BTF. var Encode variables using BTF_KIND_VAR in BTF. float Encode floating-point types in BTF. decl_tag Encode declaration tags using BTF_KIND_DECL_TAG. type_tag Encode type tags using BTF_KIND_TYPE_TAG. enum64 Encode enum64 values with BTF_KIND_ENUM64. optimized Encode representations of optimized functions with suffixes like ".isra.0" etc consistent Avoid encoding inconsistent static functions. These occur when a parameter is optimized out in some CUs and not others, or when the same function name has inconsistent BTF descriptions in different CUs. Specifying "--btf_features=all" is the equivalent to setting all of the above. If pahole does not know about a feature it silently ignores it. These properties allow us to use the --btf_features option in the kernel pahole_flags.sh script to specify the desired set of features. If a new feature is not present in pahole but requested, pahole BTF encoding will not complain (but will not encode the feature). Suggested-by: Andrii Nakryiko <andrii@kernel.org> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> --- man-pages/pahole.1 | 20 +++++++++++ pahole.c | 87 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 1 deletion(-)