On 19/04/2024 01:06, Andrii Nakryiko wrote:
> On Tue, Apr 16, 2024 at 7:38 AM Alan Maguire <alan.maguire@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
>>
>>
