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? Thanks, Eduard [...]
