On Fri, Oct 13, 2023 at 4:54 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote: > > 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. > So I'd say optimized params will be its own feature, no? So yeah, I think optimized_funcs is a better and more specific name. We can probably add groups/aliases separate or later on, so then "optimized" will mean both optimized_funcs and optimized_params, etc. Just like you have all. But this starts to sounds like a feature creep, so let's go with specific names now, and worry about aliases when we need them. > >> > >> 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. > SGTM > >> > >> 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(-) > >> [...]
