On 19/10/2023 12:07, Eduard Zingerman wrote:
> On Wed, 2023-10-18 at 13:29 +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 a comma-separated combination of
>>
>> 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_func Encode representations of optimized functions
>> with suffixes like ".isra.0" etc
>> consistent_func 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
>> specified in --btf_features it silently ignores it.
>>
>> The --btf_features can either be specified via a single comma-separated
>> list
>> --btf_features=enum64,float
>>
>> ...or via multiple --btf_features values
>>
>> --btf_features=enum64 --btf_features=float
>>
>> These properties allow us to use the --btf_features option in
>> the kernel scripts/pahole_flags.sh script to specify the desired
>> set of BTF features.
>>
>> If a feature named in --btf_features is not present in the version
>> of pahole used, BTF encoding will not complain. This is desired
>> because it means we no longer have to tie new features to a specific
>> pahole version.
>>
>> Suggested-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>> Suggested-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>> Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
>> Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>> ---
>> man-pages/pahole.1 | 24 ++++++++
>> pahole.c | 137 ++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 160 insertions(+), 1 deletion(-)
>>
>> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
>> index c1b48de..a09885f 100644
>> --- a/man-pages/pahole.1
>> +++ b/man-pages/pahole.1
>> @@ -273,6 +273,30 @@ 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 option can be used as an alternative to unsing multiple BTF-related options. Supported features are
>> +
>> +.nf
>> + encode_force Ignore invalid symbols when encoding BTF; for example
>> + if a symbol has an invalid name, it will be ignored
>> + and BTF encoding will continue.
>> + 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_func Encode representations of optimized functions
>> + with suffixes like ".isra.0".
>> + consistent_func 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
>> +
>> +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 \-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..0e889cf 100644
>> --- a/pahole.c
>> +++ b/pahole.c
>> @@ -1229,6 +1229,133 @@ 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,..] allows us to specify
>> + * a list of requested BTF features or "all" to enable all features.
>> + * These are translated into the appropriate conf_load values via a
>> + * struct btf_feature which specifies the associated conf_load
>> + * boolean field and whether its default (representing the feature being
>> + * off) is false or true.
>> + *
>> + * btf_features is for opting _into_ features so for a case like
>> + * conf_load->btf_gen_floats, the translation is simple; the presence
>> + * of the "float" feature in --btf_features sets conf_load->btf_gen_floats
>> + * to true.
>> + *
>> + * The more confusing case is for features that are enabled unless
>> + * skipping them is specified; for example
>> + * conf_load->skip_encoding_btf_type_tag. By default - to support
>> + * the opt-in model of only enabling features the user asks for -
>> + * conf_load->skip_encoding_btf_type_tag is set to true (meaning no
>> + * type_tags) and it is only set to false if --btf_features contains
>> + * the "type_tag" keyword.
>> + *
>> + * So from the user perspective, all features specified via
>> + * --btf_features are enabled, and if a feature is not specified,
>> + * it is disabled.
>> + *
>> + * If --btf_features is not used, the usual pahole defaults for
>> + * 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 }
>> +
>> +struct btf_feature {
>> + const char *name;
>> + const char *option_alias;
>> + bool *conf_value;
>> + bool default_value;
>> +} 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),
>> +};
>> +
>> +#define BTF_MAX_FEATURES 32
>> +#define BTF_MAX_FEATURE_STR 1024
>> +
>> +bool set_btf_features_defaults;
>> +
>> +static void init_btf_features(void)
>> +{
>> + int i;
>> +
>> + /* 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)
>> + return;
>> + for (i = 0; i < ARRAY_SIZE(btf_features); i++)
>> + *btf_features[i].conf_value = btf_features[i].default_value;
>> + set_btf_features_defaults = true;
>> +}
>> +
>> +static struct btf_feature *find_btf_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 enable_btf_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.
>> + */
>> +static void parse_btf_features(const char *features)
>> +{
>> + char *feature_list[BTF_MAX_FEATURES] = {};
>> + char *saveptr = NULL, *s, *t;
>> + char f[BTF_MAX_FEATURE_STR];
>> + int i, n = 0;
>> +
>> + init_btf_features();
>> +
>> + if (strcmp(features, "all") == 0) {
>> + for (i = 0; i < ARRAY_SIZE(btf_features); i++)
>> + enable_btf_feature(&btf_features[i]);
>> + return;
>> + }
>> +
>> + strncpy(f, features, sizeof(f));
>> + s = f;
>> + while ((t = strtok_r(s, ",", &saveptr)) != NULL && n < BTF_MAX_FEATURES) {
>> + s = NULL;
>> + feature_list[n++] = t;
>> + }
>> +
>
> Sorry, I should have realized it when I sent suggestion for v2.
> It should be possible to merge the "while" and "for" loops and avoid
> hypothetical edge case when old version of pahole is fed with 33 items
> long feature list. As in the diff attached to the end of the email.
> Feel free to ignore this if you think code is fine as it is.
Yeah, it's definitely neater, thanks! I suggest we wait to see if anyone
else has additional suggestions, and I can roll the below into a v4
unless anyone objects.
Alan
>
>> + for (i = 0; i < n; i++) {
>> + struct btf_feature *feature = find_btf_feature(feature_list[i]);
>> +
>> + if (!feature) {
>> + if (global_verbose)
>> + fprintf(stderr, "Ignoring unsupported feature '%s'\n",
>> + feature_list[i]);
>> + } else {
>> + enable_btf_feature(feature);
>> + }
>> + }
>> +}
>>
>> static const struct argp_option pahole__options[] = {
>> {
>> @@ -1651,6 +1778,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 +1929,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"); break;
>> case ARGP_with_flexible_array:
>> show_with_flexible_array = true; break;
>> case ARGP_prettify_input_filename:
>> @@ -1826,6 +1959,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); break;
>> default:
>> return ARGP_ERR_UNKNOWN;
>> }
>
> ---
>
> diff --git a/pahole.c b/pahole.c
> index e308dd1..b9bf395 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -1280,7 +1280,6 @@ struct btf_feature {
> BTF_FEATURE(consistent_func, skip_encoding_btf_inconsistent_proto, false),
> };
>
> -#define BTF_MAX_FEATURES 32
> #define BTF_MAX_FEATURE_STR 1024
>
> bool set_btf_features_defaults;
> @@ -1338,10 +1337,10 @@ static void show_supported_btf_features(FILE *output)
> */
> static void parse_btf_features(const char *features, bool strict)
> {
> - char *feature_list[BTF_MAX_FEATURES] = {};
> - char *saveptr = NULL, *s, *t;
> + char *saveptr = NULL, *s, *requested;
> char f[BTF_MAX_FEATURE_STR];
> - int i, n = 0;
> + struct btf_feature *feature;
> + int i;
>
> init_btf_features();
>
> @@ -1353,24 +1352,19 @@ static void parse_btf_features(const char *features, bool strict)
>
> strncpy(f, features, sizeof(f));
> s = f;
> - while ((t = strtok_r(s, ",", &saveptr)) != NULL && n < BTF_MAX_FEATURES) {
> + while ((requested = strtok_r(s, ",", &saveptr)) != NULL) {
> s = NULL;
> - feature_list[n++] = t;
> - }
> -
> - for (i = 0; i < n; i++) {
> - struct btf_feature *feature = find_btf_feature(feature_list[i]);
> -
> + feature = find_btf_feature(requested);
> if (!feature) {
> if (strict) {
> fprintf(stderr, "Feature '%s' in '%s' is not supported. Supported BTF features are:\n",
> - feature_list[i], features);
> + requested, features);
> show_supported_btf_features(stderr);
> exit(EXIT_FAILURE);
> }
> if (global_verbose)
> fprintf(stderr, "Ignoring unsupported feature '%s'\n",
> - feature_list[i]);
> + requested);
> } else {
> enable_btf_feature(feature);
> }
>
>
