| Message ID | 20231013153359.88274-6-alan.maguire@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | pahole, btf_encoder: support --btf_features | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, 2023-10-13 at 16:33 +0100, Alan Maguire wrote: > --btf_features is used to specify the list of requested features > for BTF encoding. However, it is not strict in rejecting requests > with unknown features; this allows us to use the same parameters > regardless of pahole version. --btf_features_strict carries out > the same encoding with the same feature set, but will fail if an > unrecognized feature is specified. > > So > > pahole -J --btf_features=enum64,foo > > will succeed, while > > pahole -J --btf_features_strict=enum64,foo > > will not. > > Suggested-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > --- > man-pages/pahole.1 | 4 ++++ > pahole.c | 20 +++++++++++++++++--- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/man-pages/pahole.1 b/man-pages/pahole.1 > index 6148915..ea9045c 100644 > --- a/man-pages/pahole.1 > +++ b/man-pages/pahole.1 > @@ -297,6 +297,10 @@ Encode BTF using the specified feature list, or specify 'all' for all features s > > So for example, specifying \-\-btf_encode=var,enum64 will result in a BTF encoding that (as well as encoding basic BTF information) will contain variables and enum64 values. > > +.TP > +.B \-\-btf_features_strict > +Identical to \-\-btf_features above, but pahole will exit if it encounters an unrecognized feature. > + > .TP > .B \-\-supported_btf_features > Show set of BTF features supported by \-\-btf_features option and exit. Useful for checking which features are supported since \-\-btf_features will not emit an error if an unrecognized feature is specified. > diff --git a/pahole.c b/pahole.c > index 816525a..e2a2440 100644 > --- a/pahole.c > +++ b/pahole.c > @@ -1231,6 +1231,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version; > #define ARGP_skip_encoding_btf_inconsistent_proto 340 > #define ARGP_btf_features 341 > #define ARGP_supported_btf_features 342 > +#define ARGP_btf_features_strict 343 > > /* --btf_features=feature1[,feature2,..] allows us to specify > * a list of requested BTF features or "all" to enable all features. > @@ -1288,7 +1289,7 @@ bool set_btf_features_defaults; > * Explicitly ignores unrecognized features to allow future specification > * of new opt-in features. > */ > -static void parse_btf_features(const char *features) > +static void parse_btf_features(const char *features, bool strict) > { > char *feature_list[BTF_MAX_FEATURES] = {}; > char f[BTF_MAX_FEATURE_STR]; > @@ -1325,6 +1326,11 @@ static void parse_btf_features(const char *features) > break; > } > } > + if (strict && !match) { > + fprintf(stderr, "Feature in '%s' is not supported. 'pahole --supported_btf_features' shows the list of features supported.\n", > + features); > + exit(EXIT_FAILURE); > + } Hi Alan, Sorry for late response. This won't work if --btf_features_strict specifies an incomplete list, e.g.: $ pahole --btf_features_strict=decl_tag,enum64 --btf_encode_detached=/dev/null ~/work/tmp/test.o Feature in 'decl_tag,enum64' is not supported. 'pahole --supported_btf_features' shows the list of features supported. Also, I think it would be good to print exactly which feature is not supported. What do you think about modification as in the end of this email? (applied on top of your series). Thanks, Eduard --- diff --git a/pahole.c b/pahole.c index e2a2440..cf87f83 100644 --- a/pahole.c +++ b/pahole.c @@ -1285,6 +1285,29 @@ struct btf_feature { bool set_btf_features_defaults; +static struct btf_feature *find_feature(char *name) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(btf_features); i++) + if (strcmp(name, btf_features[i].name) == 0) + return &btf_features[i]; + return NULL; +} + +static void init_feature(struct btf_feature *feature) +{ + *feature->conf_value = feature->default_value; +} + +static void enable_feature(struct btf_feature *feature) +{ + /* switch "default-off" features on, and "default-on" features + * off; i.e. negate the default value. + */ + *feature->conf_value = !feature->default_value; +} + /* Translate --btf_features=feature1[,feature2] into conf_load values. * Explicitly ignores unrecognized features to allow future specification * of new opt-in features. @@ -1294,7 +1317,7 @@ static void parse_btf_features(const char *features, bool strict) char *feature_list[BTF_MAX_FEATURES] = {}; char f[BTF_MAX_FEATURE_STR]; bool encode_all = false; - int i, j, n = 0; + int i, n = 0; strncpy(f, features, sizeof(f)); @@ -1309,36 +1332,36 @@ static void parse_btf_features(const char *features, bool strict) } } - for (i = 0; i < ARRAY_SIZE(btf_features); i++) { - bool match = encode_all; + /* Only set default values once, as multiple --btf_features= + * may be specified on command-line, and setting defaults + * again could clobber values. The aim is to enable + * all features set across all --btf_features options. + */ + if (!set_btf_features_defaults) { + for (i = 0; i < ARRAY_SIZE(btf_features); i++) + init_feature(&btf_features[i]); + set_btf_features_defaults = true; + } - /* Only set default values once, as multiple --btf_features= - * may be specified on command-line, and setting defaults - * again could clobber values. The aim is to enable - * all features set across all --btf_features options. - */ - if (!set_btf_features_defaults) - *(btf_features[i].conf_value) = btf_features[i].default_value; - if (!match) { - for (j = 0; j < n; j++) { - if (strcmp(feature_list[j], btf_features[i].name) == 0) { - match = true; - break; - } - } - if (strict && !match) { - fprintf(stderr, "Feature in '%s' is not supported. 'pahole --supported_btf_features' shows the list of features supported.\n", - features); + if (encode_all) { + for (i = 0; i < ARRAY_SIZE(btf_features); i++) + enable_feature(&btf_features[i]); + } else { + for (i = 0; i < n; i++) { + struct btf_feature *feature = find_feature(feature_list[i]); + + if (!feature && strict) { + fprintf(stderr, "Feature '%s' is not supported. 'pahole --supported_btf_features' shows the list of features supported.\n", + feature_list[i]); exit(EXIT_FAILURE); } + if (!feature && global_verbose) + fprintf(stdout, "Ignoring unsupported feature '%s'\n", + feature_list[i]); + if (feature) + enable_feature(feature); } - /* switch "default-off" features on, and "default-on" features - * off; i.e. negate the default value. - */ - if (match) - *(btf_features[i].conf_value) = !btf_features[i].default_value; } - set_btf_features_defaults = true; } static void show_supported_btf_features(void)
On 17/10/2023 13:53, Eduard Zingerman wrote: > On Fri, 2023-10-13 at 16:33 +0100, Alan Maguire wrote: >> --btf_features is used to specify the list of requested features >> for BTF encoding. However, it is not strict in rejecting requests >> with unknown features; this allows us to use the same parameters >> regardless of pahole version. --btf_features_strict carries out >> the same encoding with the same feature set, but will fail if an >> unrecognized feature is specified. >> >> So >> >> pahole -J --btf_features=enum64,foo >> >> will succeed, while >> >> pahole -J --btf_features_strict=enum64,foo >> >> will not. >> >> Suggested-by: Andrii Nakryiko <andrii@kernel.org> >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> >> --- >> man-pages/pahole.1 | 4 ++++ >> pahole.c | 20 +++++++++++++++++--- >> 2 files changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1 >> index 6148915..ea9045c 100644 >> --- a/man-pages/pahole.1 >> +++ b/man-pages/pahole.1 >> @@ -297,6 +297,10 @@ Encode BTF using the specified feature list, or specify 'all' for all features s >> >> So for example, specifying \-\-btf_encode=var,enum64 will result in a BTF encoding that (as well as encoding basic BTF information) will contain variables and enum64 values. >> >> +.TP >> +.B \-\-btf_features_strict >> +Identical to \-\-btf_features above, but pahole will exit if it encounters an unrecognized feature. >> + >> .TP >> .B \-\-supported_btf_features >> Show set of BTF features supported by \-\-btf_features option and exit. Useful for checking which features are supported since \-\-btf_features will not emit an error if an unrecognized feature is specified. >> diff --git a/pahole.c b/pahole.c >> index 816525a..e2a2440 100644 >> --- a/pahole.c >> +++ b/pahole.c >> @@ -1231,6 +1231,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version; >> #define ARGP_skip_encoding_btf_inconsistent_proto 340 >> #define ARGP_btf_features 341 >> #define ARGP_supported_btf_features 342 >> +#define ARGP_btf_features_strict 343 >> >> /* --btf_features=feature1[,feature2,..] allows us to specify >> * a list of requested BTF features or "all" to enable all features. >> @@ -1288,7 +1289,7 @@ bool set_btf_features_defaults; >> * Explicitly ignores unrecognized features to allow future specification >> * of new opt-in features. >> */ >> -static void parse_btf_features(const char *features) >> +static void parse_btf_features(const char *features, bool strict) >> { >> char *feature_list[BTF_MAX_FEATURES] = {}; >> char f[BTF_MAX_FEATURE_STR]; >> @@ -1325,6 +1326,11 @@ static void parse_btf_features(const char *features) >> break; >> } >> } >> + if (strict && !match) { >> + fprintf(stderr, "Feature in '%s' is not supported. 'pahole --supported_btf_features' shows the list of features supported.\n", >> + features); >> + exit(EXIT_FAILURE); >> + } > > Hi Alan, > > Sorry for late response. > > This won't work if --btf_features_strict specifies an incomplete list, e.g.: > > $ pahole --btf_features_strict=decl_tag,enum64 --btf_encode_detached=/dev/null ~/work/tmp/test.o > Feature in 'decl_tag,enum64' is not supported. 'pahole --supported_btf_features' shows the list of features supported. > > Also, I think it would be good to print exactly which feature is not supported. > What do you think about modification as in the end of this email? > (applied on top of your series). > Argh, apologies, I could have sworn I'd tested this. Thanks, the fix looks great, and I tested your modifications and all looks good. I can add a Co-developed-by: tag for v3, or let me know what attribution works best for you. I'll fix the cover letter as per your other email also. Thanks for the help! Alan > Thanks, > Eduard > > --- > > diff --git a/pahole.c b/pahole.c > index e2a2440..cf87f83 100644 > --- a/pahole.c > +++ b/pahole.c > @@ -1285,6 +1285,29 @@ struct btf_feature { > > bool set_btf_features_defaults; > > +static struct btf_feature *find_feature(char *name) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(btf_features); i++) > + if (strcmp(name, btf_features[i].name) == 0) > + return &btf_features[i]; > + return NULL; > +} > + > +static void init_feature(struct btf_feature *feature) > +{ > + *feature->conf_value = feature->default_value; > +} > + > +static void enable_feature(struct btf_feature *feature) > +{ > + /* switch "default-off" features on, and "default-on" features > + * off; i.e. negate the default value. > + */ > + *feature->conf_value = !feature->default_value; > +} > + > /* Translate --btf_features=feature1[,feature2] into conf_load values. > * Explicitly ignores unrecognized features to allow future specification > * of new opt-in features. > @@ -1294,7 +1317,7 @@ static void parse_btf_features(const char *features, bool strict) > char *feature_list[BTF_MAX_FEATURES] = {}; > char f[BTF_MAX_FEATURE_STR]; > bool encode_all = false; > - int i, j, n = 0; > + int i, n = 0; > > strncpy(f, features, sizeof(f)); > > @@ -1309,36 +1332,36 @@ static void parse_btf_features(const char *features, bool strict) > } > } > > - for (i = 0; i < ARRAY_SIZE(btf_features); i++) { > - bool match = encode_all; > + /* Only set default values once, as multiple --btf_features= > + * may be specified on command-line, and setting defaults > + * again could clobber values. The aim is to enable > + * all features set across all --btf_features options. > + */ > + if (!set_btf_features_defaults) { > + for (i = 0; i < ARRAY_SIZE(btf_features); i++) > + init_feature(&btf_features[i]); > + set_btf_features_defaults = true; > + } > > - /* Only set default values once, as multiple --btf_features= > - * may be specified on command-line, and setting defaults > - * again could clobber values. The aim is to enable > - * all features set across all --btf_features options. > - */ > - if (!set_btf_features_defaults) > - *(btf_features[i].conf_value) = btf_features[i].default_value; > - if (!match) { > - for (j = 0; j < n; j++) { > - if (strcmp(feature_list[j], btf_features[i].name) == 0) { > - match = true; > - break; > - } > - } > - if (strict && !match) { > - fprintf(stderr, "Feature in '%s' is not supported. 'pahole --supported_btf_features' shows the list of features supported.\n", > - features); > + if (encode_all) { > + for (i = 0; i < ARRAY_SIZE(btf_features); i++) > + enable_feature(&btf_features[i]); > + } else { > + for (i = 0; i < n; i++) { > + struct btf_feature *feature = find_feature(feature_list[i]); > + > + if (!feature && strict) { > + fprintf(stderr, "Feature '%s' is not supported. 'pahole --supported_btf_features' shows the list of features supported.\n", > + feature_list[i]); > exit(EXIT_FAILURE); > } > + if (!feature && global_verbose) > + fprintf(stdout, "Ignoring unsupported feature '%s'\n", > + feature_list[i]); > + if (feature) > + enable_feature(feature); > } > - /* switch "default-off" features on, and "default-on" features > - * off; i.e. negate the default value. > - */ > - if (match) > - *(btf_features[i].conf_value) = !btf_features[i].default_value; > } > - set_btf_features_defaults = true; > } > > static void show_supported_btf_features(void)
On Tue, 2023-10-17 at 16:59 +0100, Alan Maguire wrote: [...] > > Sorry for late response. > > > > This won't work if --btf_features_strict specifies an incomplete list, e.g.: > > > > $ pahole --btf_features_strict=decl_tag,enum64 --btf_encode_detached=/dev/null ~/work/tmp/test.o > > Feature in 'decl_tag,enum64' is not supported. 'pahole --supported_btf_features' shows the list of features supported. > > > > Also, I think it would be good to print exactly which feature is not supported. > > What do you think about modification as in the end of this email? > > (applied on top of your series). > > > > Argh, apologies, I could have sworn I'd tested this. No worries. > Thanks, the fix looks great, and I tested your modifications and all > looks good. I can add a Co-developed-by: tag for v3, or let me know > what attribution works best for you. I'll fix the cover letter as > per your other email also. Thanks for the help! > > Alan I don't think "Co-developed-by" is necessary, I can just add ack in the end. If you think that some attribution info is necessary for bookkeeping reasons maybe "Suggested-by"? Thanks, Eduard [...]
diff --git a/man-pages/pahole.1 b/man-pages/pahole.1 index 6148915..ea9045c 100644 --- a/man-pages/pahole.1 +++ b/man-pages/pahole.1 @@ -297,6 +297,10 @@ Encode BTF using the specified feature list, or specify 'all' for all features s So for example, specifying \-\-btf_encode=var,enum64 will result in a BTF encoding that (as well as encoding basic BTF information) will contain variables and enum64 values. +.TP +.B \-\-btf_features_strict +Identical to \-\-btf_features above, but pahole will exit if it encounters an unrecognized feature. + .TP .B \-\-supported_btf_features Show set of BTF features supported by \-\-btf_features option and exit. Useful for checking which features are supported since \-\-btf_features will not emit an error if an unrecognized feature is specified. diff --git a/pahole.c b/pahole.c index 816525a..e2a2440 100644 --- a/pahole.c +++ b/pahole.c @@ -1231,6 +1231,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version; #define ARGP_skip_encoding_btf_inconsistent_proto 340 #define ARGP_btf_features 341 #define ARGP_supported_btf_features 342 +#define ARGP_btf_features_strict 343 /* --btf_features=feature1[,feature2,..] allows us to specify * a list of requested BTF features or "all" to enable all features. @@ -1288,7 +1289,7 @@ bool set_btf_features_defaults; * Explicitly ignores unrecognized features to allow future specification * of new opt-in features. */ -static void parse_btf_features(const char *features) +static void parse_btf_features(const char *features, bool strict) { char *feature_list[BTF_MAX_FEATURES] = {}; char f[BTF_MAX_FEATURE_STR]; @@ -1325,6 +1326,11 @@ static void parse_btf_features(const char *features) break; } } + if (strict && !match) { + fprintf(stderr, "Feature in '%s' is not supported. 'pahole --supported_btf_features' shows the list of features supported.\n", + features); + exit(EXIT_FAILURE); + } } /* switch "default-off" features on, and "default-on" features * off; i.e. negate the default value. @@ -1779,6 +1785,12 @@ static const struct argp_option pahole__options[] = { .key = ARGP_supported_btf_features, .doc = "Show list of btf_features supported by pahole and exit." }, + { + .name = "btf_features_strict", + .key = ARGP_btf_features_strict, + .arg = "FEATURE_LIST_STRICT", + .doc = "Specify supported BTF features in FEATURE_LIST or 'all' for all supported features. Unlike --btf_features, unrecognized features will trigger an error." + }, { .name = NULL, } @@ -1924,7 +1936,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: - parse_btf_features("all"); break; + parse_btf_features("all", false); break; case ARGP_with_flexible_array: show_with_flexible_array = true; break; case ARGP_prettify_input_filename: @@ -1955,9 +1967,11 @@ static error_t pahole__options_parser(int key, char *arg, case ARGP_skip_encoding_btf_inconsistent_proto: conf_load.skip_encoding_btf_inconsistent_proto = true; break; case ARGP_btf_features: - parse_btf_features(arg); break; + parse_btf_features(arg, false); break; case ARGP_supported_btf_features: show_supported_btf_features(); exit(0); + case ARGP_btf_features_strict: + parse_btf_features(arg, true); break; default: return ARGP_ERR_UNKNOWN; }
--btf_features is used to specify the list of requested features for BTF encoding. However, it is not strict in rejecting requests with unknown features; this allows us to use the same parameters regardless of pahole version. --btf_features_strict carries out the same encoding with the same feature set, but will fail if an unrecognized feature is specified. So pahole -J --btf_features=enum64,foo will succeed, while pahole -J --btf_features_strict=enum64,foo will not. Suggested-by: Andrii Nakryiko <andrii@kernel.org> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> --- man-pages/pahole.1 | 4 ++++ pahole.c | 20 +++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-)