[PATCH v3 dwarves 0/5] pahole, btf_encoder: support --btf_features

[PATCH v3 dwarves 0/5] pahole, btf_encoder: support --btf_features

[Alan Maguire]

Currently, the kernel uses pahole version checking as the way to
determine which BTF encoding features to request from pahole.  This
means that such features have to be tied to a specific version and
as new features are added, additional clauses in scripts/pahole-flags.sh
have to be added; for example

if [ "${pahole_ver}" -ge "125" ]; then
        extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
fi

To better future-proof this process, this series introduces a
single "btf_features" parameter that uses a comma-separated list
of encoding options.  This is helpful because

- the semantics are simpler for the user; the list comprises the set of
  BTF features asked for, rather than having to specify a combination of
  --skip_encoding_btf_feature and --btf_gen_feature options; and
- any version of pahole that supports --btf_features can accept the
  option list; unknown options are silently ignored.  As a result, there
  would be no need to add additional version clauses beyond

if [ "${pahole_ver}" -ge "126" ]; then
        extra_pahole_opt="-j --lang_exclude=rust
--btf_features=encode_force,var,float,decl_tag,type_tag,enum64,optimized_func,consistent_func"
fi

  Newly-supported features would simply be appended to the btf_features
  list, and these would have impact on BTF encoding only if the features
  were supported by pahole.  This means pahole will not require a version
  bump when new BTF features are added, and should ease the burden of
  coordinating such changes between bpf-next and dwarves.

Patches 1 and 2 are preparatory work, while patch 3 adds the
--btf_features support.  Patch 4 provides a means of querying
the supported feature set since --btf_features will not error
out when it encounters unrecognized features (this ensures
an older pahole without a requested feature will not dump warnings
in the build log for kernel/module BTF generation).  Patch 5
adds --btf_features_strict, which is identical to --btf_features
aside from the fact it will fail if an unrecognized feature is used.

See [1] for more background on this topic.

Changes since v2 [2]:
- added acks from Andrii and Jiri (patches 1-5)
- merged suggestions from Eduard which simplify and clean up code
  considerably; these changes fix issues with --btf_features_strict
  while providing better diagnostic output when an unknown feature
  is encountered (specifying the unknown feature if in verbose
  or strict modes).  Added Suggested-bys from Eduard for the
  relevant patches (Eduard, patches 3,5)

Changes since RFC [3]:

- ensure features are disabled unless requested; use "default" field in
  "struct btf_features" to specify the conf_load default value which
  corresponds to the feature being disabled.  For
  conf_load->btf_gen_floats for example, the default value is false,
  while for conf_load->skip_encoding_btf_type_tags the default is
  true; in both cases the intent is to _not_ encode the associated
  feature by default.  However if the user specifies "float" or
  "type_tag" in --btf_features, the default conf_load value is negated,
  resulting in a BTF encoding that contains floats and type tags
  (Eduard, patch 3)
- clarify feature default/setting behaviour and how it only applies
  when --btf_features is used (Eduard, patch 3)
- ensure we do not run off the end of the feature_list[] array
  (Eduard, patch 3)
- rather than having each struct btf_feature record the offset in the
  conf_load structure of the boolean (requiring us to later do pointer
  math to update it), record the pointers to the boolean conf_load
  values associated with each feature (Jiri, patch 3)
- allow for multiple specifications of --btf_features, enabling the
  union of all features specified (Andrii, patch 3)
- rename function-related optimized/consistent to optimized_func and
  consistent_func in recognition of the fact they are function-specific
  (Andrii, patch 3)
- add a strict version of --btf_features, --btf_features_strict that
  will error out if an unrecognized feature is used (Andrii, patch 5)

[1] https://lore.kernel.org/bpf/CAEf4Bzaz1UqqxuZ7Q+KQee-HLyY1nwhAurBE2n9YTWchqoYLbg@mail.gmail.com/
[2] https://lore.kernel.org/bpf/20231013153359.88274-1-alan.maguire@oracle.com/
[3] https://lore.kernel.org/bpf/20231011091732.93254-1-alan.maguire@oracle.com/

Alan Maguire (5):
  btf_encoder, pahole: move btf encoding options into conf_load
  dwarves: move ARRAY_SIZE() to dwarves.h
  pahole: add --btf_features support
  pahole: add --supported_btf_features
  pahole: add --btf_features_strict to reject unknown BTF features

 btf_encoder.c      |   8 +-
 btf_encoder.h      |   2 +-
 dwarves.c          |  16 ----
 dwarves.h          |  19 +++++
 man-pages/pahole.1 |  32 ++++++++
 pahole.c           | 191 ++++++++++++++++++++++++++++++++++++++++++---
 6 files changed, 234 insertions(+), 34 deletions(-)

-- 
2.31.1

[PATCH v3 dwarves 1/5] btf_encoder, pahole: move btf encoding options into conf_load

[Alan Maguire]

...rather than passing them to btf_encoder__new(); this tidies
up the encoder API and also allows us to use generalized methods
to translate from a BTF feature (forthcoming) to a conf_load
value.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 btf_encoder.c |  8 ++++----
 btf_encoder.h |  2 +-
 dwarves.h     |  3 +++
 pahole.c      | 21 ++++++++-------------
 4 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 65f6e71..fd04008 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -1625,7 +1625,7 @@ out:
 	return err;
 }
 
-struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool skip_encoding_vars, bool force, bool gen_floats, bool verbose)
+struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool verbose, struct conf_load *conf_load)
 {
 	struct btf_encoder *encoder = zalloc(sizeof(*encoder));
 
@@ -1639,9 +1639,9 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
 		if (encoder->btf == NULL)
 			goto out_delete;
 
-		encoder->force		 = force;
-		encoder->gen_floats	 = gen_floats;
-		encoder->skip_encoding_vars = skip_encoding_vars;
+		encoder->force		 = conf_load->btf_encode_force;
+		encoder->gen_floats	 = conf_load->btf_gen_floats;
+		encoder->skip_encoding_vars = conf_load->skip_encoding_btf_vars;
 		encoder->verbose	 = verbose;
 		encoder->has_index_type  = false;
 		encoder->need_index_type = false;
diff --git a/btf_encoder.h b/btf_encoder.h
index 34516bb..f54c95a 100644
--- a/btf_encoder.h
+++ b/btf_encoder.h
@@ -16,7 +16,7 @@ struct btf;
 struct cu;
 struct list_head;
 
-struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool skip_encoding_vars, bool force, bool gen_floats, bool verbose);
+struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filename, struct btf *base_btf, bool verbose, struct conf_load *conf_load);
 void btf_encoder__delete(struct btf_encoder *encoder);
 
 int btf_encoder__encode(struct btf_encoder *encoder);
diff --git a/dwarves.h b/dwarves.h
index eb1a6df..db68161 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -68,6 +68,9 @@ struct conf_load {
 	bool			skip_encoding_btf_enum64;
 	bool			btf_gen_optimized;
 	bool			skip_encoding_btf_inconsistent_proto;
+	bool			skip_encoding_btf_vars;
+	bool			btf_gen_floats;
+	bool			btf_encode_force;
 	uint8_t			hashtable_bits;
 	uint8_t			max_hashtable_bits;
 	uint16_t		kabi_prefix_len;
diff --git a/pahole.c b/pahole.c
index e843999..7a41dc3 100644
--- a/pahole.c
+++ b/pahole.c
@@ -32,13 +32,10 @@
 static struct btf_encoder *btf_encoder;
 static char *detached_btf_filename;
 static bool btf_encode;
-static bool btf_gen_floats;
 static bool ctf_encode;
 static bool sort_output;
 static bool need_resort;
 static bool first_obj_only;
-static bool skip_encoding_btf_vars;
-static bool btf_encode_force;
 static const char *base_btf_file;
 
 static const char *prettify_input_filename;
@@ -1786,9 +1783,9 @@ static error_t pahole__options_parser(int key, char *arg,
 	case ARGP_header_type:
 		conf.header_type = arg;			break;
 	case ARGP_skip_encoding_btf_vars:
-		skip_encoding_btf_vars = true;		break;
+		conf_load.skip_encoding_btf_vars = true;	break;
 	case ARGP_btf_encode_force:
-		btf_encode_force = true;		break;
+		conf_load.btf_encode_force = true;	break;
 	case ARGP_btf_base:
 		base_btf_file = arg;			break;
 	case ARGP_kabi_prefix:
@@ -1797,9 +1794,9 @@ static error_t pahole__options_parser(int key, char *arg,
 	case ARGP_numeric_version:
 		print_numeric_version = true;		break;
 	case ARGP_btf_gen_floats:
-		btf_gen_floats = true;			break;
+		conf_load.btf_gen_floats = true;	break;
 	case ARGP_btf_gen_all:
-		btf_gen_floats = true;			break;
+		conf_load.btf_gen_floats = true;	break;
 	case ARGP_with_flexible_array:
 		show_with_flexible_array = true;	break;
 	case ARGP_prettify_input_filename:
@@ -3063,8 +3060,8 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
 			 * And, it is used by the thread
 			 * create it.
 			 */
-			btf_encoder = btf_encoder__new(cu, detached_btf_filename, conf_load->base_btf, skip_encoding_btf_vars,
-						       btf_encode_force, btf_gen_floats, global_verbose);
+			btf_encoder = btf_encoder__new(cu, detached_btf_filename, conf_load->base_btf,
+						       global_verbose, conf_load);
 			if (btf_encoder && thr_data) {
 				struct thread_data *thread = thr_data;
 
@@ -3093,10 +3090,8 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
 				thread->encoder =
 					btf_encoder__new(cu, detached_btf_filename,
 							 NULL,
-							 skip_encoding_btf_vars,
-							 btf_encode_force,
-							 btf_gen_floats,
-							 global_verbose);
+							 global_verbose,
+							 conf_load);
 				thread->btf = btf_encoder__btf(thread->encoder);
 			}
 			encoder = thread->encoder;
-- 
2.31.1

[PATCH v3 dwarves 2/5] dwarves: move ARRAY_SIZE() to dwarves.h

[Alan Maguire]

...so it can be used by pahole.c too.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 dwarves.c | 16 ----------------
 dwarves.h | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/dwarves.c b/dwarves.c
index 218367b..9f97eda 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -2094,22 +2094,6 @@ int cus__load_file(struct cus *cus, struct conf_load *conf,
 	_min1 < _min2 ? _min1 : _min2; })
 #endif
 
-/* Force a compilation error if condition is true, but also produce a
-   result (of value 0 and type size_t), so the expression can be used
-   e.g. in a structure initializer (or where-ever else comma expressions
-   aren't permitted). */
-#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
-
-/* Are two types/vars the same type (ignoring qualifiers)? */
-#ifndef __same_type
-# define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
-#endif
-
-/* &a[0] degrades to a pointer: a different type from an array */
-#define __must_be_array(a)	BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
-
-#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
-
 #ifndef DW_LANG_C89
 #define DW_LANG_C89		0x0001
 #endif
diff --git a/dwarves.h b/dwarves.h
index db68161..857b37c 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -19,6 +19,22 @@
 #include "list.h"
 #include "rbtree.h"
 
+/* Force a compilation error if condition is true, but also produce a
+   result (of value 0 and type size_t), so the expression can be used
+   e.g. in a structure initializer (or where-ever else comma expressions
+   aren't permitted). */
+#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
+
+/* Are two types/vars the same type (ignoring qualifiers)? */
+#ifndef __same_type
+# define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
+#endif
+
+/* &a[0] degrades to a pointer: a different type from an array */
+#define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
+
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
+
 struct cu;
 
 enum load_steal_kind {
-- 
2.31.1

[PATCH v3 dwarves 3/5] pahole: add --btf_features support

[Alan Maguire]

This allows consumers to specify an opt-in set of features
they want to use in BTF encoding.

Supported features are a comma-separated combination of

	encode_force    Ignore invalid symbols when encoding BTF.
	var             Encode variables using BTF_KIND_VAR in BTF.
	float           Encode floating-point types in BTF.
	decl_tag        Encode declaration tags using BTF_KIND_DECL_TAG.
	type_tag        Encode type tags using BTF_KIND_TYPE_TAG.
	enum64          Encode enum64 values with BTF_KIND_ENUM64.
	optimized_func  Encode representations of optimized functions
	                with suffixes like ".isra.0" etc
	consistent_func Avoid encoding inconsistent static functions.
	                These occur when a parameter is optimized out
	                in some CUs and not others, or when the same
	                function name has inconsistent BTF descriptions
	                in different CUs.

Specifying "--btf_features=all" is the equivalent to setting
all of the above.  If pahole does not know about a feature
specified in --btf_features it silently ignores it.

The --btf_features can either be specified via a single comma-separated
list
	--btf_features=enum64,float

...or via multiple --btf_features values

	--btf_features=enum64 --btf_features=float

These properties allow us to use the --btf_features option in
the kernel scripts/pahole_flags.sh script to specify the desired
set of BTF features.

If a feature named in --btf_features is not present in the version
of pahole used, BTF encoding will not complain.  This is desired
because it means we no longer have to tie new features to a specific
pahole version.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 man-pages/pahole.1 |  24 ++++++++
 pahole.c           | 137 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 160 insertions(+), 1 deletion(-)

diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
index c1b48de..a09885f 100644
--- a/man-pages/pahole.1
+++ b/man-pages/pahole.1
@@ -273,6 +273,30 @@ Generate BTF for functions with optimization-related suffixes (.isra, .constprop
 .B \-\-btf_gen_all
 Allow using all the BTF features supported by pahole.
 
+.TP
+.B \-\-btf_features=FEATURE_LIST
+Encode BTF using the specified feature list, or specify 'all' for all features supported.  This option can be used as an alternative to unsing multiple BTF-related options. Supported features are
+
+.nf
+	encode_force       Ignore invalid symbols when encoding BTF; for example
+	                   if a symbol has an invalid name, it will be ignored
+	                   and BTF encoding will continue.
+	var                Encode variables using BTF_KIND_VAR in BTF.
+	float              Encode floating-point types in BTF.
+	decl_tag           Encode declaration tags using BTF_KIND_DECL_TAG.
+	type_tag           Encode type tags using BTF_KIND_TYPE_TAG.
+	enum64             Encode enum64 values with BTF_KIND_ENUM64.
+	optimized_func     Encode representations of optimized functions
+	                   with suffixes like ".isra.0".
+	consistent_func    Avoid encoding inconsistent static functions.
+	                   These occur when a parameter is optimized out
+	                   in some CUs and not others, or when the same
+	                   function name has inconsistent BTF descriptions
+	                   in different CUs.
+.fi
+
+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 \-l, \-\-show_first_biggest_size_base_type_member
 Show first biggest size base_type member.
diff --git a/pahole.c b/pahole.c
index 7a41dc3..0e889cf 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1229,6 +1229,133 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
 #define ARGP_skip_emitting_atomic_typedefs 338
 #define ARGP_btf_gen_optimized  339
 #define ARGP_skip_encoding_btf_inconsistent_proto 340
+#define ARGP_btf_features	341
+
+/* --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 a
+ * 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" keyword.
+ *
+ * So from the user perspective, all features specified via
+ * --btf_features are enabled, and if a feature is not specified,
+ * it is disabled.
+ *
+ * If --btf_features is not used, the usual pahole defaults for
+ * BTF encoding apply; we encode type/decl tags, do not encode
+ * floats, etc.  This ensures backwards compatibility.
+ */
+#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[] = {
+	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_func, btf_gen_optimized, false),
+	BTF_FEATURE(consistent_func, skip_encoding_btf_inconsistent_proto, false),
+};
+
+#define BTF_MAX_FEATURES	32
+#define BTF_MAX_FEATURE_STR	1024
+
+bool set_btf_features_defaults;
+
+static void init_btf_features(void)
+{
+	int i;
+
+	/* 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)
+		return;
+	for (i = 0; i < ARRAY_SIZE(btf_features); i++)
+		*btf_features[i].conf_value = btf_features[i].default_value;
+	set_btf_features_defaults = true;
+}
+
+static struct btf_feature *find_btf_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 enable_btf_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.
+ */
+static void parse_btf_features(const char *features)
+{
+	char *feature_list[BTF_MAX_FEATURES] = {};
+	char *saveptr = NULL, *s, *t;
+	char f[BTF_MAX_FEATURE_STR];
+	int i, n = 0;
+
+	init_btf_features();
+
+	if (strcmp(features, "all") == 0) {
+		for (i = 0; i < ARRAY_SIZE(btf_features); i++)
+			enable_btf_feature(&btf_features[i]);
+		return;
+	}
+
+	strncpy(f, features, sizeof(f));
+	s = f;
+	while ((t = strtok_r(s, ",", &saveptr)) != NULL && n < BTF_MAX_FEATURES) {
+		s = NULL;
+		feature_list[n++] = t;
+	}
+
+	for (i = 0; i < n; i++) {
+		struct btf_feature *feature = find_btf_feature(feature_list[i]);
+
+		if (!feature) {
+			if (global_verbose)
+				fprintf(stderr, "Ignoring unsupported feature '%s'\n",
+					feature_list[i]);
+		} else {
+			enable_btf_feature(feature);
+		}
+	}
+}
 
 static const struct argp_option pahole__options[] = {
 	{
@@ -1651,6 +1778,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 +1929,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");		break;
 	case ARGP_with_flexible_array:
 		show_with_flexible_array = true;	break;
 	case ARGP_prettify_input_filename:
@@ -1826,6 +1959,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);		break;
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
-- 
2.31.1

[PATCH v3 dwarves 4/5] pahole: add --supported_btf_features

[Alan Maguire]

By design --btf_features=FEATURE1[,FEATURE2,...] will not complain
if an unrecognized feature is specified.  This allows the kernel
build process to specify new features regardless of whether they
are supported by the version of pahole used; in such cases we do
not wish for every invocation of pahole to complain.  However it is
still valuable to have a way of knowing which BTF features pahole
supports; this could be logged as part of the build process for
example.  By specifying --supported_btf_features a comma-separated
list is returned; for example:

 $ pahole --supported_btf_features
 encode_force,var,float,decl_tag,type_tag,enum64,optimized_func,consistent_func

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 man-pages/pahole.1 |  4 ++++
 pahole.c           | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
index a09885f..6148915 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 \-\-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.
+
 .TP
 .B \-l, \-\-show_first_biggest_size_base_type_member
 Show first biggest size base_type member.
diff --git a/pahole.c b/pahole.c
index 0e889cf..b5790be 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1230,6 +1230,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
 #define ARGP_btf_gen_optimized  339
 #define ARGP_skip_encoding_btf_inconsistent_proto 340
 #define ARGP_btf_features	341
+#define ARGP_supported_btf_features 342
 
 /* --btf_features=feature1[,feature2,..] allows us to specify
  * a list of requested BTF features or "all" to enable all features.
@@ -1318,6 +1319,18 @@ static void enable_btf_feature(struct btf_feature *feature)
 	*feature->conf_value = !feature->default_value;
 }
 
+static void show_supported_btf_features(FILE *output)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
+		if (i > 0)
+			fprintf(output, ",");
+		fprintf(output, "%s", btf_features[i].name);
+	}
+	fprintf(output, "\n");
+}
+
 /* Translate --btf_features=feature1[,feature2] into conf_load values.
  * Explicitly ignores unrecognized features to allow future specification
  * of new opt-in features.
@@ -1784,6 +1797,11 @@ static const struct argp_option pahole__options[] = {
 		.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 = "supported_btf_features",
+		.key = ARGP_supported_btf_features,
+		.doc = "Show list of btf_features supported by pahole and exit."
+	},
 	{
 		.name = NULL,
 	}
@@ -1961,6 +1979,8 @@ static error_t pahole__options_parser(int key, char *arg,
 		conf_load.skip_encoding_btf_inconsistent_proto = true; break;
 	case ARGP_btf_features:
 		parse_btf_features(arg);		break;
+	case ARGP_supported_btf_features:
+		show_supported_btf_features(stdout);	exit(0);
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
-- 
2.31.1

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

[Alan Maguire]

--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>
Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 man-pages/pahole.1 |  4 ++++
 pahole.c           | 21 ++++++++++++++++++---
 2 files changed, 22 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 b5790be..2b6c965 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.
@@ -1335,7 +1336,7 @@ static void show_supported_btf_features(FILE *output)
  * 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 *saveptr = NULL, *s, *t;
@@ -1361,6 +1362,12 @@ static void parse_btf_features(const char *features)
 		struct btf_feature *feature = find_btf_feature(feature_list[i]);
 
 		if (!feature) {
+			if (strict) {
+				fprintf(stderr, "Feature '%s' in '%s' is not supported.  Supported BTF features are:\n",
+					feature_list[i], features);
+				show_supported_btf_features(stderr);
+				exit(EXIT_FAILURE);
+			}
 			if (global_verbose)
 				fprintf(stderr, "Ignoring unsupported feature '%s'\n",
 					feature_list[i]);
@@ -1802,6 +1809,12 @@ static const struct argp_option pahole__options[] = {
 		.key = ARGP_supported_btf_features,
 		.doc = "Show list of btf_features supported by pahole and exit."
 	},
+	{
+		.name = "btf_features_strict",
+		.key = ARGP_btf_features_strict,
+		.arg = "FEATURE_LIST_STRICT",
+		.doc = "Specify supported BTF features in FEATURE_LIST or 'all' for all supported features.  Unlike --btf_features, unrecognized features will trigger an error."
+	},
 	{
 		.name = NULL,
 	}
@@ -1947,7 +1960,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:
-		parse_btf_features("all");		break;
+		parse_btf_features("all", false);	break;
 	case ARGP_with_flexible_array:
 		show_with_flexible_array = true;	break;
 	case ARGP_prettify_input_filename:
@@ -1978,9 +1991,11 @@ static error_t pahole__options_parser(int key, char *arg,
 	case ARGP_skip_encoding_btf_inconsistent_proto:
 		conf_load.skip_encoding_btf_inconsistent_proto = true; break;
 	case ARGP_btf_features:
-		parse_btf_features(arg);		break;
+		parse_btf_features(arg, false);		break;
 	case ARGP_supported_btf_features:
 		show_supported_btf_features(stdout);	exit(0);
+	case ARGP_btf_features_strict:
+		parse_btf_features(arg, true);		break;
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
-- 
2.31.1

Re: [PATCH v3 dwarves 0/5] pahole, btf_encoder: support --btf_features

[Eduard Zingerman]

On Wed, 2023-10-18 at 13:29 +0100, Alan Maguire wrote:
> Currently, the kernel uses pahole version checking as the way to
> determine which BTF encoding features to request from pahole.  This
> means that such features have to be tied to a specific version and
> as new features are added, additional clauses in scripts/pahole-flags.sh
> have to be added; for example
> 
> if [ "${pahole_ver}" -ge "125" ]; then
>         extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> fi
> 
> To better future-proof this process, this series introduces a
> single "btf_features" parameter that uses a comma-separated list
> of encoding options.  This is helpful because
> 
> - the semantics are simpler for the user; the list comprises the set of
>   BTF features asked for, rather than having to specify a combination of
>   --skip_encoding_btf_feature and --btf_gen_feature options; and
> - any version of pahole that supports --btf_features can accept the
>   option list; unknown options are silently ignored.  As a result, there
>   would be no need to add additional version clauses beyond
> 
> if [ "${pahole_ver}" -ge "126" ]; then
>         extra_pahole_opt="-j --lang_exclude=rust
> --btf_features=encode_force,var,float,decl_tag,type_tag,enum64,optimized_func,consistent_func"
> fi
> 
>   Newly-supported features would simply be appended to the btf_features
>   list, and these would have impact on BTF encoding only if the features
>   were supported by pahole.  This means pahole will not require a version
>   bump when new BTF features are added, and should ease the burden of
>   coordinating such changes between bpf-next and dwarves.
> 
> Patches 1 and 2 are preparatory work, while patch 3 adds the
> --btf_features support.  Patch 4 provides a means of querying
> the supported feature set since --btf_features will not error
> out when it encounters unrecognized features (this ensures
> an older pahole without a requested feature will not dump warnings
> in the build log for kernel/module BTF generation).  Patch 5
> adds --btf_features_strict, which is identical to --btf_features
> aside from the fact it will fail if an unrecognized feature is used.
> 
> See [1] for more background on this topic.
> 
> Changes since v2 [2]:
> - added acks from Andrii and Jiri (patches 1-5)
> - merged suggestions from Eduard which simplify and clean up code
>   considerably; these changes fix issues with --btf_features_strict
>   while providing better diagnostic output when an unknown feature
>   is encountered (specifying the unknown feature if in verbose
>   or strict modes).  Added Suggested-bys from Eduard for the
>   relevant patches (Eduard, patches 3,5)
> 
> Changes since RFC [3]:
> 
> - ensure features are disabled unless requested; use "default" field in
>   "struct btf_features" to specify the conf_load default value which
>   corresponds to the feature being disabled.  For
>   conf_load->btf_gen_floats for example, the default value is false,
>   while for conf_load->skip_encoding_btf_type_tags the default is
>   true; in both cases the intent is to _not_ encode the associated
>   feature by default.  However if the user specifies "float" or
>   "type_tag" in --btf_features, the default conf_load value is negated,
>   resulting in a BTF encoding that contains floats and type tags
>   (Eduard, patch 3)
> - clarify feature default/setting behaviour and how it only applies
>   when --btf_features is used (Eduard, patch 3)
> - ensure we do not run off the end of the feature_list[] array
>   (Eduard, patch 3)
> - rather than having each struct btf_feature record the offset in the
>   conf_load structure of the boolean (requiring us to later do pointer
>   math to update it), record the pointers to the boolean conf_load
>   values associated with each feature (Jiri, patch 3)
> - allow for multiple specifications of --btf_features, enabling the
>   union of all features specified (Andrii, patch 3)
> - rename function-related optimized/consistent to optimized_func and
>   consistent_func in recognition of the fact they are function-specific
>   (Andrii, patch 3)
> - add a strict version of --btf_features, --btf_features_strict that
>   will error out if an unrecognized feature is used (Andrii, patch 5)
> 
> [1] https://lore.kernel.org/bpf/CAEf4Bzaz1UqqxuZ7Q+KQee-HLyY1nwhAurBE2n9YTWchqoYLbg@mail.gmail.com/
> [2] https://lore.kernel.org/bpf/20231013153359.88274-1-alan.maguire@oracle.com/
> [3] https://lore.kernel.org/bpf/20231011091732.93254-1-alan.maguire@oracle.com/

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

Hi Alan,

thank you for accommodating my changes, I've tested this patch-set and
everything seems to work fine.
I left one nitpick for patch #3 feel free to ignore it if you don't agree.

Thanks,
Eduard

Re: [PATCH v3 dwarves 3/5] pahole: add --btf_features support

[Eduard Zingerman]

On Wed, 2023-10-18 at 13:29 +0100, Alan Maguire wrote:
> This allows consumers to specify an opt-in set of features
> they want to use in BTF encoding.
> 
> Supported features are a comma-separated combination of
> 
> 	encode_force    Ignore invalid symbols when encoding BTF.
> 	var             Encode variables using BTF_KIND_VAR in BTF.
> 	float           Encode floating-point types in BTF.
> 	decl_tag        Encode declaration tags using BTF_KIND_DECL_TAG.
> 	type_tag        Encode type tags using BTF_KIND_TYPE_TAG.
> 	enum64          Encode enum64 values with BTF_KIND_ENUM64.
> 	optimized_func  Encode representations of optimized functions
> 	                with suffixes like ".isra.0" etc
> 	consistent_func Avoid encoding inconsistent static functions.
> 	                These occur when a parameter is optimized out
> 	                in some CUs and not others, or when the same
> 	                function name has inconsistent BTF descriptions
> 	                in different CUs.
> 
> Specifying "--btf_features=all" is the equivalent to setting
> all of the above.  If pahole does not know about a feature
> specified in --btf_features it silently ignores it.
> 
> The --btf_features can either be specified via a single comma-separated
> list
> 	--btf_features=enum64,float
> 
> ...or via multiple --btf_features values
> 
> 	--btf_features=enum64 --btf_features=float
> 
> These properties allow us to use the --btf_features option in
> the kernel scripts/pahole_flags.sh script to specify the desired
> set of BTF features.
> 
> If a feature named in --btf_features is not present in the version
> of pahole used, BTF encoding will not complain.  This is desired
> because it means we no longer have to tie new features to a specific
> pahole version.
> 
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  man-pages/pahole.1 |  24 ++++++++
>  pahole.c           | 137 ++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 160 insertions(+), 1 deletion(-)
> 
> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
> index c1b48de..a09885f 100644
> --- a/man-pages/pahole.1
> +++ b/man-pages/pahole.1
> @@ -273,6 +273,30 @@ Generate BTF for functions with optimization-related suffixes (.isra, .constprop
>  .B \-\-btf_gen_all
>  Allow using all the BTF features supported by pahole.
>  
> +.TP
> +.B \-\-btf_features=FEATURE_LIST
> +Encode BTF using the specified feature list, or specify 'all' for all features supported.  This option can be used as an alternative to unsing multiple BTF-related options. Supported features are
> +
> +.nf
> +	encode_force       Ignore invalid symbols when encoding BTF; for example
> +	                   if a symbol has an invalid name, it will be ignored
> +	                   and BTF encoding will continue.
> +	var                Encode variables using BTF_KIND_VAR in BTF.
> +	float              Encode floating-point types in BTF.
> +	decl_tag           Encode declaration tags using BTF_KIND_DECL_TAG.
> +	type_tag           Encode type tags using BTF_KIND_TYPE_TAG.
> +	enum64             Encode enum64 values with BTF_KIND_ENUM64.
> +	optimized_func     Encode representations of optimized functions
> +	                   with suffixes like ".isra.0".
> +	consistent_func    Avoid encoding inconsistent static functions.
> +	                   These occur when a parameter is optimized out
> +	                   in some CUs and not others, or when the same
> +	                   function name has inconsistent BTF descriptions
> +	                   in different CUs.
> +.fi
> +
> +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 \-l, \-\-show_first_biggest_size_base_type_member
>  Show first biggest size base_type member.
> diff --git a/pahole.c b/pahole.c
> index 7a41dc3..0e889cf 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -1229,6 +1229,133 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
>  #define ARGP_skip_emitting_atomic_typedefs 338
>  #define ARGP_btf_gen_optimized  339
>  #define ARGP_skip_encoding_btf_inconsistent_proto 340
> +#define ARGP_btf_features	341
> +
> +/* --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 a
> + * 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" keyword.
> + *
> + * So from the user perspective, all features specified via
> + * --btf_features are enabled, and if a feature is not specified,
> + * it is disabled.
> + *
> + * If --btf_features is not used, the usual pahole defaults for
> + * BTF encoding apply; we encode type/decl tags, do not encode
> + * floats, etc.  This ensures backwards compatibility.
> + */
> +#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[] = {
> +	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_func, btf_gen_optimized, false),
> +	BTF_FEATURE(consistent_func, skip_encoding_btf_inconsistent_proto, false),
> +};
> +
> +#define BTF_MAX_FEATURES	32
> +#define BTF_MAX_FEATURE_STR	1024
> +
> +bool set_btf_features_defaults;
> +
> +static void init_btf_features(void)
> +{
> +	int i;
> +
> +	/* 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)
> +		return;
> +	for (i = 0; i < ARRAY_SIZE(btf_features); i++)
> +		*btf_features[i].conf_value = btf_features[i].default_value;
> +	set_btf_features_defaults = true;
> +}
> +
> +static struct btf_feature *find_btf_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 enable_btf_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.
> + */
> +static void parse_btf_features(const char *features)
> +{
> +	char *feature_list[BTF_MAX_FEATURES] = {};
> +	char *saveptr = NULL, *s, *t;
> +	char f[BTF_MAX_FEATURE_STR];
> +	int i, n = 0;
> +
> +	init_btf_features();
> +
> +	if (strcmp(features, "all") == 0) {
> +		for (i = 0; i < ARRAY_SIZE(btf_features); i++)
> +			enable_btf_feature(&btf_features[i]);
> +		return;
> +	}
> +
> +	strncpy(f, features, sizeof(f));
> +	s = f;
> +	while ((t = strtok_r(s, ",", &saveptr)) != NULL && n < BTF_MAX_FEATURES) {
> +		s = NULL;
> +		feature_list[n++] = t;
> +	}
> +

Sorry, I should have realized it when I sent suggestion for v2.
It should be possible to merge the "while" and "for" loops and avoid
hypothetical edge case when old version of pahole is fed with 33 items
long feature list. As in the diff attached to the end of the email.
Feel free to ignore this if you think code is fine as it is.

> +	for (i = 0; i < n; i++) {
> +		struct btf_feature *feature = find_btf_feature(feature_list[i]);
> +
> +		if (!feature) {
> +			if (global_verbose)
> +				fprintf(stderr, "Ignoring unsupported feature '%s'\n",
> +					feature_list[i]);
> +		} else {
> +			enable_btf_feature(feature);
> +		}
> +	}
> +}
>  
>  static const struct argp_option pahole__options[] = {
>  	{
> @@ -1651,6 +1778,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 +1929,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");		break;
>  	case ARGP_with_flexible_array:
>  		show_with_flexible_array = true;	break;
>  	case ARGP_prettify_input_filename:
> @@ -1826,6 +1959,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);		break;
>  	default:
>  		return ARGP_ERR_UNKNOWN;
>  	}

---

diff --git a/pahole.c b/pahole.c
index e308dd1..b9bf395 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1280,7 +1280,6 @@ struct btf_feature {
 	BTF_FEATURE(consistent_func, skip_encoding_btf_inconsistent_proto, false),
 };
 
-#define BTF_MAX_FEATURES	32
 #define BTF_MAX_FEATURE_STR	1024
 
 bool set_btf_features_defaults;
@@ -1338,10 +1337,10 @@ static void show_supported_btf_features(FILE *output)
  */
 static void parse_btf_features(const char *features, bool strict)
 {
-	char *feature_list[BTF_MAX_FEATURES] = {};
-	char *saveptr = NULL, *s, *t;
+	char *saveptr = NULL, *s, *requested;
 	char f[BTF_MAX_FEATURE_STR];
-	int i, n = 0;
+	struct btf_feature *feature;
+	int i;
 
 	init_btf_features();
 
@@ -1353,24 +1352,19 @@ static void parse_btf_features(const char *features, bool strict)
 
 	strncpy(f, features, sizeof(f));
 	s = f;
-	while ((t = strtok_r(s, ",", &saveptr)) != NULL && n < BTF_MAX_FEATURES) {
+	while ((requested = strtok_r(s, ",", &saveptr)) != NULL) {
 		s = NULL;
-		feature_list[n++] = t;
-	}
-
-	for (i = 0; i < n; i++) {
-		struct btf_feature *feature = find_btf_feature(feature_list[i]);
-
+		feature = find_btf_feature(requested);
 		if (!feature) {
 			if (strict) {
 				fprintf(stderr, "Feature '%s' in '%s' is not supported.  Supported BTF features are:\n",
-					feature_list[i], features);
+					requested, features);
 				show_supported_btf_features(stderr);
 				exit(EXIT_FAILURE);
 			}
 			if (global_verbose)
 				fprintf(stderr, "Ignoring unsupported feature '%s'\n",
-					feature_list[i]);
+					requested);
 		} else {
 			enable_btf_feature(feature);
 		}

Re: [PATCH v3 dwarves 3/5] pahole: add --btf_features support

[Alan Maguire]

On 19/10/2023 12:07, Eduard Zingerman wrote:
> On Wed, 2023-10-18 at 13:29 +0100, Alan Maguire wrote:
>> This allows consumers to specify an opt-in set of features
>> they want to use in BTF encoding.
>>
>> Supported features are a comma-separated combination of
>>
>> 	encode_force    Ignore invalid symbols when encoding BTF.
>> 	var             Encode variables using BTF_KIND_VAR in BTF.
>> 	float           Encode floating-point types in BTF.
>> 	decl_tag        Encode declaration tags using BTF_KIND_DECL_TAG.
>> 	type_tag        Encode type tags using BTF_KIND_TYPE_TAG.
>> 	enum64          Encode enum64 values with BTF_KIND_ENUM64.
>> 	optimized_func  Encode representations of optimized functions
>> 	                with suffixes like ".isra.0" etc
>> 	consistent_func Avoid encoding inconsistent static functions.
>> 	                These occur when a parameter is optimized out
>> 	                in some CUs and not others, or when the same
>> 	                function name has inconsistent BTF descriptions
>> 	                in different CUs.
>>
>> Specifying "--btf_features=all" is the equivalent to setting
>> all of the above.  If pahole does not know about a feature
>> specified in --btf_features it silently ignores it.
>>
>> The --btf_features can either be specified via a single comma-separated
>> list
>> 	--btf_features=enum64,float
>>
>> ...or via multiple --btf_features values
>>
>> 	--btf_features=enum64 --btf_features=float
>>
>> These properties allow us to use the --btf_features option in
>> the kernel scripts/pahole_flags.sh script to specify the desired
>> set of BTF features.
>>
>> If a feature named in --btf_features is not present in the version
>> of pahole used, BTF encoding will not complain.  This is desired
>> because it means we no longer have to tie new features to a specific
>> pahole version.
>>
>> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
>> Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> Acked-by: Jiri Olsa <jolsa@kernel.org>
>> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>> ---
>>  man-pages/pahole.1 |  24 ++++++++
>>  pahole.c           | 137 ++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 160 insertions(+), 1 deletion(-)
>>
>> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
>> index c1b48de..a09885f 100644
>> --- a/man-pages/pahole.1
>> +++ b/man-pages/pahole.1
>> @@ -273,6 +273,30 @@ Generate BTF for functions with optimization-related suffixes (.isra, .constprop
>>  .B \-\-btf_gen_all
>>  Allow using all the BTF features supported by pahole.
>>  
>> +.TP
>> +.B \-\-btf_features=FEATURE_LIST
>> +Encode BTF using the specified feature list, or specify 'all' for all features supported.  This option can be used as an alternative to unsing multiple BTF-related options. Supported features are
>> +
>> +.nf
>> +	encode_force       Ignore invalid symbols when encoding BTF; for example
>> +	                   if a symbol has an invalid name, it will be ignored
>> +	                   and BTF encoding will continue.
>> +	var                Encode variables using BTF_KIND_VAR in BTF.
>> +	float              Encode floating-point types in BTF.
>> +	decl_tag           Encode declaration tags using BTF_KIND_DECL_TAG.
>> +	type_tag           Encode type tags using BTF_KIND_TYPE_TAG.
>> +	enum64             Encode enum64 values with BTF_KIND_ENUM64.
>> +	optimized_func     Encode representations of optimized functions
>> +	                   with suffixes like ".isra.0".
>> +	consistent_func    Avoid encoding inconsistent static functions.
>> +	                   These occur when a parameter is optimized out
>> +	                   in some CUs and not others, or when the same
>> +	                   function name has inconsistent BTF descriptions
>> +	                   in different CUs.
>> +.fi
>> +
>> +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 \-l, \-\-show_first_biggest_size_base_type_member
>>  Show first biggest size base_type member.
>> diff --git a/pahole.c b/pahole.c
>> index 7a41dc3..0e889cf 100644
>> --- a/pahole.c
>> +++ b/pahole.c
>> @@ -1229,6 +1229,133 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
>>  #define ARGP_skip_emitting_atomic_typedefs 338
>>  #define ARGP_btf_gen_optimized  339
>>  #define ARGP_skip_encoding_btf_inconsistent_proto 340
>> +#define ARGP_btf_features	341
>> +
>> +/* --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 a
>> + * 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" keyword.
>> + *
>> + * So from the user perspective, all features specified via
>> + * --btf_features are enabled, and if a feature is not specified,
>> + * it is disabled.
>> + *
>> + * If --btf_features is not used, the usual pahole defaults for
>> + * BTF encoding apply; we encode type/decl tags, do not encode
>> + * floats, etc.  This ensures backwards compatibility.
>> + */
>> +#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[] = {
>> +	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_func, btf_gen_optimized, false),
>> +	BTF_FEATURE(consistent_func, skip_encoding_btf_inconsistent_proto, false),
>> +};
>> +
>> +#define BTF_MAX_FEATURES	32
>> +#define BTF_MAX_FEATURE_STR	1024
>> +
>> +bool set_btf_features_defaults;
>> +
>> +static void init_btf_features(void)
>> +{
>> +	int i;
>> +
>> +	/* 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)
>> +		return;
>> +	for (i = 0; i < ARRAY_SIZE(btf_features); i++)
>> +		*btf_features[i].conf_value = btf_features[i].default_value;
>> +	set_btf_features_defaults = true;
>> +}
>> +
>> +static struct btf_feature *find_btf_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 enable_btf_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.
>> + */
>> +static void parse_btf_features(const char *features)
>> +{
>> +	char *feature_list[BTF_MAX_FEATURES] = {};
>> +	char *saveptr = NULL, *s, *t;
>> +	char f[BTF_MAX_FEATURE_STR];
>> +	int i, n = 0;
>> +
>> +	init_btf_features();
>> +
>> +	if (strcmp(features, "all") == 0) {
>> +		for (i = 0; i < ARRAY_SIZE(btf_features); i++)
>> +			enable_btf_feature(&btf_features[i]);
>> +		return;
>> +	}
>> +
>> +	strncpy(f, features, sizeof(f));
>> +	s = f;
>> +	while ((t = strtok_r(s, ",", &saveptr)) != NULL && n < BTF_MAX_FEATURES) {
>> +		s = NULL;
>> +		feature_list[n++] = t;
>> +	}
>> +
> 
> Sorry, I should have realized it when I sent suggestion for v2.
> It should be possible to merge the "while" and "for" loops and avoid
> hypothetical edge case when old version of pahole is fed with 33 items
> long feature list. As in the diff attached to the end of the email.
> Feel free to ignore this if you think code is fine as it is.

Yeah, it's definitely neater, thanks! I suggest we wait to see if anyone
else has additional suggestions, and I can roll the below into a v4
unless anyone objects.

Alan

> 
>> +	for (i = 0; i < n; i++) {
>> +		struct btf_feature *feature = find_btf_feature(feature_list[i]);
>> +
>> +		if (!feature) {
>> +			if (global_verbose)
>> +				fprintf(stderr, "Ignoring unsupported feature '%s'\n",
>> +					feature_list[i]);
>> +		} else {
>> +			enable_btf_feature(feature);
>> +		}
>> +	}
>> +}
>>  
>>  static const struct argp_option pahole__options[] = {
>>  	{
>> @@ -1651,6 +1778,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 +1929,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");		break;
>>  	case ARGP_with_flexible_array:
>>  		show_with_flexible_array = true;	break;
>>  	case ARGP_prettify_input_filename:
>> @@ -1826,6 +1959,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);		break;
>>  	default:
>>  		return ARGP_ERR_UNKNOWN;
>>  	}
> 
> ---
> 
> diff --git a/pahole.c b/pahole.c
> index e308dd1..b9bf395 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -1280,7 +1280,6 @@ struct btf_feature {
>  	BTF_FEATURE(consistent_func, skip_encoding_btf_inconsistent_proto, false),
>  };
>  
> -#define BTF_MAX_FEATURES	32
>  #define BTF_MAX_FEATURE_STR	1024
>  
>  bool set_btf_features_defaults;
> @@ -1338,10 +1337,10 @@ static void show_supported_btf_features(FILE *output)
>   */
>  static void parse_btf_features(const char *features, bool strict)
>  {
> -	char *feature_list[BTF_MAX_FEATURES] = {};
> -	char *saveptr = NULL, *s, *t;
> +	char *saveptr = NULL, *s, *requested;
>  	char f[BTF_MAX_FEATURE_STR];
> -	int i, n = 0;
> +	struct btf_feature *feature;
> +	int i;
>  
>  	init_btf_features();
>  
> @@ -1353,24 +1352,19 @@ static void parse_btf_features(const char *features, bool strict)
>  
>  	strncpy(f, features, sizeof(f));
>  	s = f;
> -	while ((t = strtok_r(s, ",", &saveptr)) != NULL && n < BTF_MAX_FEATURES) {
> +	while ((requested = strtok_r(s, ",", &saveptr)) != NULL) {
>  		s = NULL;
> -		feature_list[n++] = t;
> -	}
> -
> -	for (i = 0; i < n; i++) {
> -		struct btf_feature *feature = find_btf_feature(feature_list[i]);
> -
> +		feature = find_btf_feature(requested);
>  		if (!feature) {
>  			if (strict) {
>  				fprintf(stderr, "Feature '%s' in '%s' is not supported.  Supported BTF features are:\n",
> -					feature_list[i], features);
> +					requested, features);
>  				show_supported_btf_features(stderr);
>  				exit(EXIT_FAILURE);
>  			}
>  			if (global_verbose)
>  				fprintf(stderr, "Ignoring unsupported feature '%s'\n",
> -					feature_list[i]);
> +					requested);
>  		} else {
>  			enable_btf_feature(feature);
>  		}
> 
>