Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support

[Alan Maguire <alan.maguire@oracle.com>]

On 11/10/2023 23:14, Eduard Zingerman wrote:
> On Wed, 2023-10-11 at 23:05 +0100, Alan Maguire wrote:
> [...]
>>>>> 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?
>>>
>>> You mean when --features=? is specified we default to
>>> conf_load->skip_encoding_btf_decl_tag = true, and set it to false only
>>> if "all" or "decl_tag" is listed in features, right?
>>>
>>
>> Yep. Here's the comment I was thinking of adding for the next version,
>> hopefully it clarifies this all a bit more than the original.
>>
>> +/* --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
>> + * 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" feature.
>> + *
>> + * So from the user perspective, all features specified via
>> + * --btf_features are enabled, and if a feature is not specified,
>> + * it is disabled.
>>   */
> 
> Sounds reasonable. Maybe also add a line saying that
> skip_encoding_btf_decl_tag defaults to false if --btf_features is not
> specified to remain backwards compatible?
>

good idea, will do! Thanks!

Alan

> Thanks,
> Eduard
> 
> [...]