On 13/10/2023 01:21, Andrii Nakryiko wrote:
> 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.
>
Great!
>>
>> 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 :)
>
The only place it is currently used is when checking btf_name_valid()
on a variable - if encode_force is specified we skip invalidly-named
symbols and drive on. I'll try and flesh out the description a bit.
>> 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?
>
Yeah, though consistent may eventually need to apply to variables too.
As Stephen and I have been exploring adding global variable support for
all variables, we've run across a bunch of cases where the same variable
name refers to different types too. Worse, it often happens that the
same variable name refers to a "struct foo" and a "struct foo *" which
is liable to be very confusing. So I think we will either need to skip
encoding such variables for now (the "consistent" approach used for
functions) or we may have to sort out the symbol->address mapping issue
in BTF for functions _and_ variables before we land variable support.
My preference would be the latter - since it will solve the issues with
functions too - but I think we can probably make either sequence work.
So all of that is to say we can either stick with "consistent" with
the expectation that it may be more broadly applied to variables, or
convert to "consistent_func", I've no major preference which.
Optimized definitely refers to functions so we can switch that to
"optimized_func" if you like.
>>
>> 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.
>
I think this is a good idea. I'll add it to v2 unless anyone has major
objections.
>>
>> 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.
>
Sure, will do.
>> +
>> + 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
>>
