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

[Jiri Olsa <olsajiri@gmail.com>]

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?

jirka

> +		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
>