Re: [PATCH dwarves 1/3] pahole: allow --btf_features to not participate in "all"

[Alan Maguire <alan.maguire@oracle.com>]

On 19/04/2024 01:06, Andrii Nakryiko wrote:
> On Tue, Apr 16, 2024 at 7:38 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>
>> Specifying --btf_features=all enables all supported BTF features.
>> However there are some features that are non-standard, so we should
>> support a way to use them in --btf_features but not participate in
>> the set of features enabled by "--btf_features=all".  As part of this,
>> also support all used in a list of --btf_features, i.e.
>>
>> --btf_features=all,nonstandard_feature
>>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  man-pages/pahole.1 |  2 +-
>>  pahole.c           | 36 +++++++++++++++++++++++-------------
>>  2 files changed, 24 insertions(+), 14 deletions(-)
>>
>> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
>> index 2be165d..2c08e97 100644
>> --- a/man-pages/pahole.1
>> +++ b/man-pages/pahole.1
>> @@ -290,7 +290,7 @@ 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 option can be used as an alternative to unsing multiple BTF-related options. Supported features are
>> +Encode BTF using the specified feature list, or specify 'all' for all standard features supported.  This option can be used as an alternative to unsing multiple BTF-related options. Supported standard features are
>>
>>  .nf
>>         encode_force       Ignore invalid symbols when encoding BTF; for example
>> diff --git a/pahole.c b/pahole.c
>> index 77772bb..890ef81 100644
>> --- a/pahole.c
>> +++ b/pahole.c
>> @@ -1266,23 +1266,26 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
>>   * BTF encoding apply; we encode type/decl tags, do not encode
>>   * floats, etc.  This ensures backwards compatibility.
>>   */
>> -#define BTF_FEATURE(name, alias, default_value)                        \
>> -       { #name, #alias, &conf_load.alias, default_value }
>> +#define BTF_FEATURE(name, alias, default_value, enable_for_all)                \
>> +       { #name, #alias, &conf_load.alias, default_value, enable_for_all }
>>
>>  struct btf_feature {
>>         const char      *name;
>>         const char      *option_alias;
>>         bool            *conf_value;
>>         bool            default_value;
>> +       bool            enable_for_all; /* some nonstandard features may not
>> +                                        * be enabled for --btf_features=all
>> +                                        */
>>  } 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_func, btf_gen_optimized, false),
>> -       BTF_FEATURE(consistent_func, skip_encoding_btf_inconsistent_proto, false),
>> +       BTF_FEATURE(encode_force, btf_encode_force, false, true),
>> +       BTF_FEATURE(var, skip_encoding_btf_vars, true, true),
>> +       BTF_FEATURE(float, btf_gen_floats, false, true),
>> +       BTF_FEATURE(decl_tag, skip_encoding_btf_decl_tag, true, true),
>> +       BTF_FEATURE(type_tag, skip_encoding_btf_type_tag, true, true),
>> +       BTF_FEATURE(enum64, skip_encoding_btf_enum64, true, true),
>> +       BTF_FEATURE(optimized_func, btf_gen_optimized, false, true),
>> +       BTF_FEATURE(consistent_func, skip_encoding_btf_inconsistent_proto, false, true),
>>  };
> 
> maybe keep those special features in a separate array instead?
> 

yeah, maybe that or have BTF_STANDARD_FEATURE() and BTF_EXTRA_FEATURE()
macros to clarify the difference.

> it is kind of weird to see --btf_features=all,and_then_some, maybe it
> makes sense to have --btf_extras or something for those?
>

yeah, the "all" is a problem. Arnaldo and I talked a bit about using
"default" or "standard" instead of "all"; the problem with default is
that there's already a notion of default BTF values - these are the ones
we get without specifying any --btf_features or other BTF-related flags.

Having an "extras" feature option (drawing from a separate option array)
as you suggest might be a cleaner way to make this distinction. What do
others think?

>>
>>  #define BTF_MAX_FEATURE_STR    1024
>> @@ -1350,8 +1353,10 @@ static void parse_btf_features(const char *features, bool strict)
>>         if (strcmp(features, "all") == 0) {
>>                 int i;
>>
>> -               for (i = 0; i < ARRAY_SIZE(btf_features); i++)
>> -                       enable_btf_feature(&btf_features[i]);
>> +               for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
>> +                       if (btf_features[i].enable_for_all)
>> +                               enable_btf_feature(&btf_features[i]);
>> +               }
>>                 return;
>>         }
>>
>> @@ -1361,7 +1366,12 @@ static void parse_btf_features(const char *features, bool strict)
>>                 struct btf_feature *feature = find_btf_feature(feature_name);
>>
>>                 if (!feature) {
>> -                       if (strict) {
>> +                       /* --btf_features=all,nonstandard_feature should be
>> +                        * allowed.
>> +                        */
>> +                       if (strcmp(feature_name, "all") == 0) {
>> +                               parse_btf_features(feature_name, strict);
>> +                       } else if (strict) {
>>                                 fprintf(stderr, "Feature '%s' in '%s' is not supported.  Supported BTF features are:\n",
>>                                         feature_name, features);
>>                                 show_supported_btf_features(stderr);
>> --
>> 2.39.3
>>
>>