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