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@xxxxxxxxxx>
>> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>> ---
>> 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)
