Re: [PATCH v2 dwarves 5/5] pahole: add --btf_features_strict to reject unknown BTF features

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2023-10-13 at 16:33 +0100, Alan Maguire wrote:
> --btf_features is used to specify the list of requested features
> for BTF encoding.  However, it is not strict in rejecting requests
> with unknown features; this allows us to use the same parameters
> regardless of pahole version.  --btf_features_strict carries out
> the same encoding with the same feature set, but will fail if an
> unrecognized feature is specified.
> 
> So
> 
>   pahole -J --btf_features=enum64,foo
> 
> will succeed, while
> 
>   pahole -J --btf_features_strict=enum64,foo
> 
> will not.
> 
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  man-pages/pahole.1 |  4 ++++
>  pahole.c           | 20 +++++++++++++++++---
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
> index 6148915..ea9045c 100644
> --- a/man-pages/pahole.1
> +++ b/man-pages/pahole.1
> @@ -297,6 +297,10 @@ Encode BTF using the specified feature list, or specify 'all' for all features s
>  
>  So for example, specifying \-\-btf_encode=var,enum64 will result in a BTF encoding that (as well as encoding basic BTF information) will contain variables and enum64 values.
>  
> +.TP
> +.B \-\-btf_features_strict
> +Identical to \-\-btf_features above, but pahole will exit if it encounters an unrecognized feature.
> +
>  .TP
>  .B \-\-supported_btf_features
>  Show set of BTF features supported by \-\-btf_features option and exit.  Useful for checking which features are supported since \-\-btf_features will not emit an error if an unrecognized feature is specified.
> diff --git a/pahole.c b/pahole.c
> index 816525a..e2a2440 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -1231,6 +1231,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
>  #define ARGP_skip_encoding_btf_inconsistent_proto 340
>  #define ARGP_btf_features	341
>  #define ARGP_supported_btf_features 342
> +#define ARGP_btf_features_strict 343
>  
>  /* --btf_features=feature1[,feature2,..] allows us to specify
>   * a list of requested BTF features or "all" to enable all features.
> @@ -1288,7 +1289,7 @@ bool set_btf_features_defaults;
>   * Explicitly ignores unrecognized features to allow future specification
>   * of new opt-in features.
>   */
> -static void parse_btf_features(const char *features)
> +static void parse_btf_features(const char *features, bool strict)
>  {
>  	char *feature_list[BTF_MAX_FEATURES] = {};
>  	char f[BTF_MAX_FEATURE_STR];
> @@ -1325,6 +1326,11 @@ static void parse_btf_features(const char *features)
>  					break;
>  				}
>  			}
> +			if (strict && !match) {
> +				fprintf(stderr, "Feature in '%s' is not supported.  'pahole --supported_btf_features' shows the list of features supported.\n",
> +					features);
> +				exit(EXIT_FAILURE);
> +			}

Hi Alan,

Sorry for late response.

This won't work if --btf_features_strict specifies an incomplete list, e.g.:

  $ pahole --btf_features_strict=decl_tag,enum64 --btf_encode_detached=/dev/null ~/work/tmp/test.o 
  Feature in 'decl_tag,enum64' is not supported.  'pahole --supported_btf_features' shows the list of features supported.

Also, I think it would be good to print exactly which feature is not supported.
What do you think about modification as in the end of this email?
(applied on top of your series).

Thanks,
Eduard

---

diff --git a/pahole.c b/pahole.c
index e2a2440..cf87f83 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1285,6 +1285,29 @@ struct btf_feature {
 
 bool set_btf_features_defaults;
 
+static struct btf_feature *find_feature(char *name)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(btf_features); i++)
+		if (strcmp(name, btf_features[i].name) == 0)
+			return &btf_features[i];
+	return NULL;
+}
+
+static void init_feature(struct btf_feature *feature)
+{
+	*feature->conf_value = feature->default_value;
+}
+
+static void enable_feature(struct btf_feature *feature)
+{
+	/* switch "default-off" features on, and "default-on" features
+	 * off; i.e. negate the default value.
+	 */
+	*feature->conf_value = !feature->default_value;
+}
+
 /* Translate --btf_features=feature1[,feature2] into conf_load values.
  * Explicitly ignores unrecognized features to allow future specification
  * of new opt-in features.
@@ -1294,7 +1317,7 @@ static void parse_btf_features(const char *features, bool strict)
 	char *feature_list[BTF_MAX_FEATURES] = {};
 	char f[BTF_MAX_FEATURE_STR];
 	bool encode_all = false;
-	int i, j, n = 0;
+	int i, n = 0;
 
 	strncpy(f, features, sizeof(f));
 
@@ -1309,36 +1332,36 @@ static void parse_btf_features(const char *features, bool strict)
 		}
 	}
 
-	for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
-		bool match = encode_all;
+	/* Only set default values once, as multiple --btf_features=
+	 * may be specified on command-line, and setting defaults
+	 * again could clobber values.   The aim is to enable
+	 * all features set across all --btf_features options.
+	 */
+	if (!set_btf_features_defaults) {
+		for (i = 0; i < ARRAY_SIZE(btf_features); i++)
+			init_feature(&btf_features[i]);
+		set_btf_features_defaults = true;
+	}
 
-		/* Only set default values once, as multiple --btf_features=
-		 * may be specified on command-line, and setting defaults
-		 * again could clobber values.   The aim is to enable
-		 * all features set across all --btf_features options.
-		 */
-		if (!set_btf_features_defaults)
-			*(btf_features[i].conf_value) = btf_features[i].default_value;
-		if (!match) {
-			for (j = 0; j < n; j++) {
-				if (strcmp(feature_list[j], btf_features[i].name) == 0) {
-					match = true;
-					break;
-				}
-			}
-			if (strict && !match) {
-				fprintf(stderr, "Feature in '%s' is not supported.  'pahole --supported_btf_features' shows the list of features supported.\n",
-					features);
+	if (encode_all) {
+		for (i = 0; i < ARRAY_SIZE(btf_features); i++)
+			enable_feature(&btf_features[i]);
+	} else {
+		for (i = 0; i < n; i++) {
+			struct btf_feature *feature = find_feature(feature_list[i]);
+
+			if (!feature && strict) {
+				fprintf(stderr, "Feature '%s' is not supported.  'pahole --supported_btf_features' shows the list of features supported.\n",
+					feature_list[i]);
 				exit(EXIT_FAILURE);
 			}
+			if (!feature && global_verbose)
+				fprintf(stdout, "Ignoring unsupported feature '%s'\n",
+					feature_list[i]);
+			if (feature)
+				enable_feature(feature);
 		}
-		/* switch "default-off" features on, and "default-on" features
-		 * off; i.e. negate the default value.
-		 */
-		if (match)
-			*(btf_features[i].conf_value) = !btf_features[i].default_value;
 	}
-	set_btf_features_defaults = true;
 }
 
 static void show_supported_btf_features(void)