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

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Oct 12, 2023 at 02:48:50PM +0100, Alan Maguire wrote:
> On 12/10/2023 13:53, Jiri Olsa wrote:
> > On Wed, Oct 11, 2023 at 10:17:31AM +0100, Alan Maguire wrote:
> > 
> > SNIP
> > 
> >> +#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;
> >> +		}
> >> +	}
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
> >> +		bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset);
> > 
> > nit, would it be easier to have btf_features defined inside the function
> > and pass specific bool pointers directly to BTF_FEATURE macro?
> >
> 
> thanks for taking a look! I _think_ I see what you mean; if we had
> conf_load we could encode the bool pointer directly using
> the BTF_FEATURE() definition, something like
> 
> #define BTF_FEATURE(name, alias, default_value)                 \
>         { #name, #alias, &conf_load->alias, default_value }
> 
> struct btf_feature {
>         const char      *name;
>         const char      *option_alias;
>         bool		*conf_value;
>         bool            default_value;
> } btf_features[] = {
> ...
> 
> This will work I think because conf_load is a global variable,

yes, it's global, but it's also passed as an argument to parse_btf_features,
so it could be done on top of that conf_load pointer

> and I think we need to keep it global since it's also used by
> patch 4 to get the list of supported features. Is the above
> something like what you had in mind? Thanks!

yes, using the bools directly

thanks,
jirka