[RFC dwarves 0/4] pahole, btf_encoder: support --btf_features

[RFC dwarves 0/4] 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, we can have a catch-all
single "btf_features" parameter that uses a comma-separated list
of encoding options.  What makes this better is that any version
of pahole that supports btf_features can accept the option list;
unknown options are silently ignored. However 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,consistent"
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 an unrecognized features (this ensures
an older pahole without a requested feature will not dump warnings
in the build log for kernel/module BTF generation).

See [1] for more background on this topic.

[1] https://lore.kernel.org/bpf/CAEf4Bzaz1UqqxuZ7Q+KQee-HLyY1nwhAurBE2n9YTWchqoYLbg@mail.gmail.com/

Alan Maguire (4):
  btf_encoder, pahole: move btf encoding options into conf_load
  dwarves: move ARRAY_SIZE() to dwarves.h
  pahole: add --btf_features=feature1[,feature2...] support
  pahole: add --supported_btf_features to display feature support

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

-- 
2.31.1

[RFC dwarves 1/4] 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
parameter.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 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

Re: [RFC dwarves 1/4] btf_encoder, pahole: move btf encoding options into conf_load

[Jiri Olsa]

On Wed, Oct 11, 2023 at 10:17:29AM +0100, Alan Maguire wrote:
> ...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
> parameter.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

nice cleanup

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

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

[RFC dwarves 2/4] 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>
---
 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

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

[Alan Maguire]

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

Supported features are

	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     Encode representations of optimized functions
	              with suffixes like ".isra.0" etc
	consistent    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
it silently ignores it.  These properties allow us to use
the --btf_features option in the kernel pahole_flags.sh
script to specify the desired set of features.  If a new
feature is not present in pahole but requested, pahole
BTF encoding will not complain (but will not encode the
feature).

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 man-pages/pahole.1 | 20 +++++++++++
 pahole.c           | 87 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 106 insertions(+), 1 deletion(-)

diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
index c1b48de..7c072dc 100644
--- a/man-pages/pahole.1
+++ b/man-pages/pahole.1
@@ -273,6 +273,26 @@ 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 single parameter value can be used as an alternative to unsing multiple BTF-related options. Supported features are
+
+.nf
+	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     Encode representations of optimized functions
+	              with suffixes like ".isra.0" etc
+	consistent    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
+
 .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..4f00b08 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1229,6 +1229,83 @@ 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,..] option allows us to specify
+ * opt-in features (or "all"); these are translated into conf_load
+ * values by specifying the associated bool offset and whether it
+ * is a skip option or not; btf_features is for opting _into_ features
+ * so for skip options we have to reverse the logic.  For example
+ * "--skip_encoding_btf_type_tag --btf_gen_floats" translate to
+ * "--btf_features=type_tag,float"
+ */
+#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);
+		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

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

[Eduard Zingerman]

On Wed, 2023-10-11 at 10:17 +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
> 
> 	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     Encode representations of optimized functions
> 	              with suffixes like ".isra.0" etc
> 	consistent    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
> it silently ignores it.  These properties allow us to use
> the --btf_features option in the kernel pahole_flags.sh
> script to specify the desired set of features.  If a new
> feature is not present in pahole but requested, pahole
> BTF encoding will not complain (but will not encode the
> feature).
> 
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  man-pages/pahole.1 | 20 +++++++++++
>  pahole.c           | 87 +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 106 insertions(+), 1 deletion(-)
> 
> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
> index c1b48de..7c072dc 100644
> --- a/man-pages/pahole.1
> +++ b/man-pages/pahole.1
> @@ -273,6 +273,26 @@ 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 single parameter value can be used as an alternative to unsing multiple BTF-related options. Supported features are
> +
> +.nf
> +	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     Encode representations of optimized functions
> +	              with suffixes like ".isra.0" etc
> +	consistent    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
> +
>  .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..4f00b08 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -1229,6 +1229,83 @@ 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,..] option allows us to specify
> + * opt-in features (or "all"); these are translated into conf_load
> + * values by specifying the associated bool offset and whether it
> + * is a skip option or not; btf_features is for opting _into_ features
> + * so for skip options we have to reverse the logic.  For example
> + * "--skip_encoding_btf_type_tag --btf_gen_floats" translate to
> + * "--btf_features=type_tag,float"
> + */
> +#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;

Maybe guard against `n` >= BTF_MAX_FEATURES here?

> +		}
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
> +		bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset);
> +		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;

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?

Other than that I tested the patch-set with current kernel master and
a change to pahole-flags.sh and bpf tests pass.

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

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

[Alan Maguire]

On 11/10/2023 17:28, Eduard Zingerman wrote:
> On Wed, 2023-10-11 at 10:17 +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
>>
>> 	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     Encode representations of optimized functions
>> 	              with suffixes like ".isra.0" etc
>> 	consistent    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
>> it silently ignores it.  These properties allow us to use
>> the --btf_features option in the kernel pahole_flags.sh
>> script to specify the desired set of features.  If a new
>> feature is not present in pahole but requested, pahole
>> BTF encoding will not complain (but will not encode the
>> feature).
>>
>> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  man-pages/pahole.1 | 20 +++++++++++
>>  pahole.c           | 87 +++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 106 insertions(+), 1 deletion(-)
>>
>> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
>> index c1b48de..7c072dc 100644
>> --- a/man-pages/pahole.1
>> +++ b/man-pages/pahole.1
>> @@ -273,6 +273,26 @@ 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 single parameter value can be used as an alternative to unsing multiple BTF-related options. Supported features are
>> +
>> +.nf
>> +	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     Encode representations of optimized functions
>> +	              with suffixes like ".isra.0" etc
>> +	consistent    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
>> +
>>  .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..4f00b08 100644
>> --- a/pahole.c
>> +++ b/pahole.c
>> @@ -1229,6 +1229,83 @@ 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,..] option allows us to specify
>> + * opt-in features (or "all"); these are translated into conf_load
>> + * values by specifying the associated bool offset and whether it
>> + * is a skip option or not; btf_features is for opting _into_ features
>> + * so for skip options we have to reverse the logic.  For example
>> + * "--skip_encoding_btf_type_tag --btf_gen_floats" translate to
>> + * "--btf_features=type_tag,float"
>> + */
>> +#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;
> 
> Maybe guard against `n` >= BTF_MAX_FEATURES here?
>

good point - will fix.

>> +		}
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
>> +		bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset);
>> +		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;
> 
> 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?

Maybe a better way to express all this would be to rename the "skip"
field in "struct btf_feature" to "default" - so in the case of a "skip"
feature, the default is true, but for opt-in features, the default is false.

> Other than that I tested the patch-set with current kernel master and
> a change to pahole-flags.sh and bpf tests pass.
> 

Thanks so much for testing this!

Alan

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

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

[Eduard Zingerman]

On Wed, 2023-10-11 at 17:41 +0100, Alan Maguire wrote:
[...]
> > > +		}
> > > +	}
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
> > > +		bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset);
> > > +		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;
> > 
> > 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?
 
> Maybe a better way to express all this would be to rename the "skip"
> field in "struct btf_feature" to "default" - so in the case of a "skip"
> feature, the default is true, but for opt-in features, the default is false.

Yes, I agree, "default" is better as "skip" is a bit confusing.

[...]

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

[Alan Maguire]

On 11/10/2023 20:08, Eduard Zingerman wrote:
> On Wed, 2023-10-11 at 17:41 +0100, Alan Maguire wrote:
> [...]
>>>> +		}
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
>>>> +		bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset);
>>>> +		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;
>>>
>>> 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.
  */


>> Maybe a better way to express all this would be to rename the "skip"
>> field in "struct btf_feature" to "default" - so in the case of a "skip"
>> feature, the default is true, but for opt-in features, the default is false.
> 
> Yes, I agree, "default" is better as "skip" is a bit confusing.
>

Thanks; I'll use that next time.

Alan

> [...]

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

[Eduard Zingerman]

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

[...]

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

[Alan Maguire]

On 11/10/2023 23:14, Eduard Zingerman wrote:
> 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?
>

good idea, will do! Thanks!

Alan

> Thanks,
> Eduard
> 
> [...]

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

[Arnaldo Carvalho de Melo]

Em Thu, Oct 12, 2023 at 01:35:41PM +0100, Alan Maguire escreveu:
> On 11/10/2023 23:14, Eduard Zingerman wrote:
> > 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?
> >
> 
> good idea, will do! Thanks!

I have to catch up with all the comments on this thread, but does this
mean you're respinning the series now?

- Arnaldo

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

[Alan Maguire]

On 13/10/2023 15:17, Arnaldo Carvalho de Melo wrote:
> Em Thu, Oct 12, 2023 at 01:35:41PM +0100, Alan Maguire escreveu:
>> On 11/10/2023 23:14, Eduard Zingerman wrote:
>>> 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?
>>>
>>
>> good idea, will do! Thanks!
> 
> I have to catch up with all the comments on this thread, but does this
> mean you're respinning the series now?
>

Yep, I'm respinning now. There were a few small things in Andrii's
suggestions that we might need to figure out - mostly around names - but
there's enough change since the RFC that it would probably be best to
discuss those with the v2 series. Thanks!

Alan
> - Arnaldo
> 

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

[Jiri Olsa]

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
> 

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

[Alan Maguire]

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,
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!

Alan

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

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

[Jiri Olsa]

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

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

[Andrii Nakryiko]

On Wed, Oct 11, 2023 at 2:17 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> This allows consumers to specify an opt-in set of features
> they want to use in BTF encoding.

This is exactly what I had in mind, so thanks a lot for doing this!
Minor nits below, but otherwise a big thumb up from me for the overall
approach.

>
> Supported features are
>
>         encode_force  Ignore invalid symbols when encoding BTF.

ignore_invalid? Even then I don't really know what this means even
after reading the description, but that's ok :)

>         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     Encode representations of optimized functions
>                       with suffixes like ".isra.0" etc
>         consistent    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.

both optimized and consistent refer to functions, so shouldn't the
feature name include func somewhere?

>
> Specifying "--btf_features=all" is the equivalent to setting
> all of the above.  If pahole does not know about a feature
> it silently ignores it.  These properties allow us to use
> the --btf_features option in the kernel pahole_flags.sh
> script to specify the desired set of features.  If a new
> feature is not present in pahole but requested, pahole
> BTF encoding will not complain (but will not encode the
> feature).

As I mentioned in the cover letter reply, we might add a "strict mode"
flag, that will error out on unknown features. I don't have much
opinion here, up to Arnaldo and others whether this is useful.

>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  man-pages/pahole.1 | 20 +++++++++++
>  pahole.c           | 87 +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
> index c1b48de..7c072dc 100644
> --- a/man-pages/pahole.1
> +++ b/man-pages/pahole.1
> @@ -273,6 +273,26 @@ 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 single parameter value can be used as an alternative to unsing multiple BTF-related options. Supported features are
> +
> +.nf
> +       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     Encode representations of optimized functions
> +                     with suffixes like ".isra.0" etc
> +       consistent    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
> +
>  .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..4f00b08 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -1229,6 +1229,83 @@ 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,..] option allows us to specify
> + * opt-in features (or "all"); these are translated into conf_load
> + * values by specifying the associated bool offset and whether it
> + * is a skip option or not; btf_features is for opting _into_ features
> + * so for skip options we have to reverse the logic.  For example
> + * "--skip_encoding_btf_type_tag --btf_gen_floats" translate to
> + * "--btf_features=type_tag,float"
> + */
> +#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;
> +               }
> +       }

I see that pahole uses argp for argument parsing. argp supports
specifying the same parameter multiple times, so it's very natural to
support

--btf_feature=var --btf_feature=float --btf_feature enum64

without doing any of this parsing. Just find a matching feature and
set corresponding bool value in the callback.

> +
> +       for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
> +               bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset);
> +               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
>

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

[Alan Maguire]

On 13/10/2023 01:21, Andrii Nakryiko wrote:
> On Wed, Oct 11, 2023 at 2:17 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>
>> This allows consumers to specify an opt-in set of features
>> they want to use in BTF encoding.
> 
> This is exactly what I had in mind, so thanks a lot for doing this!
> Minor nits below, but otherwise a big thumb up from me for the overall
> approach.
> 

Great!

>>
>> Supported features are
>>
>>         encode_force  Ignore invalid symbols when encoding BTF.
> 
> ignore_invalid? Even then I don't really know what this means even
> after reading the description, but that's ok :)
>

The only place it is currently used is when checking btf_name_valid()
on a variable - if encode_force is specified we skip invalidly-named
symbols and drive on. I'll try and flesh out the description a bit.


>>         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     Encode representations of optimized functions
>>                       with suffixes like ".isra.0" etc
>>         consistent    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.
> 
> both optimized and consistent refer to functions, so shouldn't the
> feature name include func somewhere?
> 

Yeah, though consistent may eventually need to apply to variables too.
As Stephen and I have been exploring adding global variable support for
all variables, we've run across a bunch of cases where the same variable
name refers to different types too. Worse, it often happens that the
same variable name refers to a "struct foo" and a "struct foo *" which
is liable to be very confusing. So I think we will either need to skip
encoding such variables for now (the "consistent" approach used for
functions) or we may have to sort out the symbol->address mapping issue
in BTF for functions _and_ variables before we land variable support.
My preference would be the latter - since it will solve the issues with
functions too - but I think we can probably make either sequence work.

So all of that is to say we can either stick with "consistent" with
the expectation that it may be more broadly applied to variables, or
convert to "consistent_func", I've no major preference which.

Optimized definitely refers to functions so we can switch that to
"optimized_func" if you like.

>>
>> Specifying "--btf_features=all" is the equivalent to setting
>> all of the above.  If pahole does not know about a feature
>> it silently ignores it.  These properties allow us to use
>> the --btf_features option in the kernel pahole_flags.sh
>> script to specify the desired set of features.  If a new
>> feature is not present in pahole but requested, pahole
>> BTF encoding will not complain (but will not encode the
>> feature).
> 
> As I mentioned in the cover letter reply, we might add a "strict mode"
> flag, that will error out on unknown features. I don't have much
> opinion here, up to Arnaldo and others whether this is useful.
> 

I think this is a good idea. I'll add it to v2 unless anyone has major
objections.

>>
>> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  man-pages/pahole.1 | 20 +++++++++++
>>  pahole.c           | 87 +++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 106 insertions(+), 1 deletion(-)
>>
>> diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
>> index c1b48de..7c072dc 100644
>> --- a/man-pages/pahole.1
>> +++ b/man-pages/pahole.1
>> @@ -273,6 +273,26 @@ 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 single parameter value can be used as an alternative to unsing multiple BTF-related options. Supported features are
>> +
>> +.nf
>> +       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     Encode representations of optimized functions
>> +                     with suffixes like ".isra.0" etc
>> +       consistent    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
>> +
>>  .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..4f00b08 100644
>> --- a/pahole.c
>> +++ b/pahole.c
>> @@ -1229,6 +1229,83 @@ 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,..] option allows us to specify
>> + * opt-in features (or "all"); these are translated into conf_load
>> + * values by specifying the associated bool offset and whether it
>> + * is a skip option or not; btf_features is for opting _into_ features
>> + * so for skip options we have to reverse the logic.  For example
>> + * "--skip_encoding_btf_type_tag --btf_gen_floats" translate to
>> + * "--btf_features=type_tag,float"
>> + */
>> +#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;
>> +               }
>> +       }
> 
> I see that pahole uses argp for argument parsing. argp supports
> specifying the same parameter multiple times, so it's very natural to
> support
> 
> --btf_feature=var --btf_feature=float --btf_feature enum64
> 
> without doing any of this parsing. Just find a matching feature and
> set corresponding bool value in the callback.
>

Sure, will do.

>> +
>> +       for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
>> +               bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset);
>> +               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
>>

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

[Andrii Nakryiko]

On Fri, Oct 13, 2023 at 4:54 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 13/10/2023 01:21, Andrii Nakryiko wrote:
> > On Wed, Oct 11, 2023 at 2:17 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >>
> >> This allows consumers to specify an opt-in set of features
> >> they want to use in BTF encoding.
> >
> > This is exactly what I had in mind, so thanks a lot for doing this!
> > Minor nits below, but otherwise a big thumb up from me for the overall
> > approach.
> >
>
> Great!
>
> >>
> >> Supported features are
> >>
> >>         encode_force  Ignore invalid symbols when encoding BTF.
> >
> > ignore_invalid? Even then I don't really know what this means even
> > after reading the description, but that's ok :)
> >
>
> The only place it is currently used is when checking btf_name_valid()
> on a variable - if encode_force is specified we skip invalidly-named
> symbols and drive on. I'll try and flesh out the description a bit.
>
>
> >>         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     Encode representations of optimized functions
> >>                       with suffixes like ".isra.0" etc
> >>         consistent    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.
> >
> > both optimized and consistent refer to functions, so shouldn't the
> > feature name include func somewhere?
> >
>
> Yeah, though consistent may eventually need to apply to variables too.
> As Stephen and I have been exploring adding global variable support for
> all variables, we've run across a bunch of cases where the same variable
> name refers to different types too. Worse, it often happens that the
> same variable name refers to a "struct foo" and a "struct foo *" which
> is liable to be very confusing. So I think we will either need to skip
> encoding such variables for now (the "consistent" approach used for
> functions) or we may have to sort out the symbol->address mapping issue
> in BTF for functions _and_ variables before we land variable support.
> My preference would be the latter - since it will solve the issues with
> functions too - but I think we can probably make either sequence work.
>
> So all of that is to say we can either stick with "consistent" with
> the expectation that it may be more broadly applied to variables, or
> convert to "consistent_func", I've no major preference which.
>
> Optimized definitely refers to functions so we can switch that to
> "optimized_func" if you like.
>

So I'd say optimized params will be its own feature, no? So yeah, I
think optimized_funcs is a better and more specific name. We can
probably add groups/aliases separate or later on, so then "optimized"
will mean both optimized_funcs and optimized_params, etc. Just like
you have all.

But this starts to sounds like a feature creep, so let's go with
specific names now, and worry about aliases when we need them.

> >>
> >> Specifying "--btf_features=all" is the equivalent to setting
> >> all of the above.  If pahole does not know about a feature
> >> it silently ignores it.  These properties allow us to use
> >> the --btf_features option in the kernel pahole_flags.sh
> >> script to specify the desired set of features.  If a new
> >> feature is not present in pahole but requested, pahole
> >> BTF encoding will not complain (but will not encode the
> >> feature).
> >
> > As I mentioned in the cover letter reply, we might add a "strict mode"
> > flag, that will error out on unknown features. I don't have much
> > opinion here, up to Arnaldo and others whether this is useful.
> >
>
> I think this is a good idea. I'll add it to v2 unless anyone has major
> objections.
>

SGTM

> >>
> >> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> >> ---
> >>  man-pages/pahole.1 | 20 +++++++++++
> >>  pahole.c           | 87 +++++++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 106 insertions(+), 1 deletion(-)
> >>

[...]

[RFC dwarves 4/4] pahole: add --supported_btf_features to display feature support

[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,consistent

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 man-pages/pahole.1 |  4 ++++
 pahole.c           | 21 +++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
index 7c072dc..b094195 100644
--- a/man-pages/pahole.1
+++ b/man-pages/pahole.1
@@ -293,6 +293,10 @@ Encode BTF using the specified feature list, or specify 'all' for all features s
 	              in different CUs.
 .fi
 
+.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 4f00b08..e828961 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,..] option allows us to specify
  * opt-in features (or "all"); these are translated into conf_load
@@ -1307,6 +1308,19 @@ static void parse_btf_features(const char *features, struct conf_load *conf_load
 	}
 }
 
+static void show_supported_btf_features(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
+		if (i > 0)
+			printf(",");
+		printf("%s", btf_features[i].name);
+	}
+	printf("\n");
+	exit(0);
+}
+
 static const struct argp_option pahole__options[] = {
 	{
 		.name = "bit_holes",
@@ -1734,6 +1748,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,
 	}
@@ -1911,6 +1930,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, &conf_load);	break;
+	case ARGP_supported_btf_features:
+		show_supported_btf_features();		break;
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
-- 
2.31.1

Re: [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features

[Andrii Nakryiko]

On Wed, Oct 11, 2023 at 2:17 AM Alan Maguire <alan.maguire@oracle.com> 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, we can have a catch-all
> single "btf_features" parameter that uses a comma-separated list
> of encoding options.  What makes this better is that any version
> of pahole that supports btf_features can accept the option list;
> unknown options are silently ignored. However 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,consistent"

I've seen a nice convention used in a lot of CLIs that allow multiple
values for a given parameter:

--btf_feature=encode_force --btf=var --btf=float and so on

It simplifies parsing code, and is also a bit more "modular" in
scripts. But it's just a minor nit, feel free to ignore.

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

Yep, this is great.

>
> 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 an unrecognized features (this ensures
> an older pahole without a requested feature will not dump warnings
> in the build log for kernel/module BTF generation).

List of features is great. I think we can also have something like
--btf_features_strict to make pahole error out if an unsupported
feature is specified.


>
> See [1] for more background on this topic.
>
> [1] https://lore.kernel.org/bpf/CAEf4Bzaz1UqqxuZ7Q+KQee-HLyY1nwhAurBE2n9YTWchqoYLbg@mail.gmail.com/
>
> Alan Maguire (4):
>   btf_encoder, pahole: move btf encoding options into conf_load
>   dwarves: move ARRAY_SIZE() to dwarves.h
>   pahole: add --btf_features=feature1[,feature2...] support
>   pahole: add --supported_btf_features to display feature support
>
>  btf_encoder.c      |   8 +--
>  btf_encoder.h      |   2 +-
>  dwarves.c          |  16 ------
>  dwarves.h          |  19 +++++++
>  man-pages/pahole.1 |  24 +++++++++
>  pahole.c           | 127 ++++++++++++++++++++++++++++++++++++++++-----
>  6 files changed, 162 insertions(+), 34 deletions(-)
>
> --
> 2.31.1
>