On Wed, Oct 11, 2023 at 2:17 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> This allows consumers to specify an opt-in set of features
> they want to use in BTF encoding.
This is exactly what I had in mind, so thanks a lot for doing this!
Minor nits below, but otherwise a big thumb up from me for the overall
approach.
>
> Supported features are
>
> encode_force Ignore invalid symbols when encoding BTF.
ignore_invalid? Even then I don't really know what this means even
after reading the description, but that's ok :)
> 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.
both optimized and consistent refer to functions, so shouldn't the
feature name include func somewhere?
>
> 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).
As I mentioned in the cover letter reply, we might add a "strict mode"
flag, that will error out on unknown features. I don't have much
opinion here, up to Arnaldo and others whether this is useful.
>
> 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;
> + }
> + }
I see that pahole uses argp for argument parsing. argp supports
specifying the same parameter multiple times, so it's very natural to
support
--btf_feature=var --btf_feature=float --btf_feature enum64
without doing any of this parsing. Just find a matching feature and
set corresponding bool value in the callback.
> +
> + 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;
> + }
> +}
>
> 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;
> }
> --
> 2.31.1
>
