* [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features
@ 2023-10-11 9:17 Alan Maguire
2023-10-11 9:17 ` [RFC dwarves 1/4] btf_encoder, pahole: move btf encoding options into conf_load Alan Maguire
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Alan Maguire @ 2023-10-11 9:17 UTC (permalink / raw
To: acme, andrii.nakryiko
Cc: jolsa, ast, daniel, eddyz87, martin.lau, song, yhs,
john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf, 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC dwarves 1/4] btf_encoder, pahole: move btf encoding options into conf_load
2023-10-11 9:17 [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features Alan Maguire
@ 2023-10-11 9:17 ` Alan Maguire
2023-10-12 12:54 ` Jiri Olsa
2023-10-11 9:17 ` [RFC dwarves 2/4] dwarves: move ARRAY_SIZE() to dwarves.h Alan Maguire
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Alan Maguire @ 2023-10-11 9:17 UTC (permalink / raw
To: acme, andrii.nakryiko
Cc: jolsa, ast, daniel, eddyz87, martin.lau, song, yhs,
john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [RFC dwarves 2/4] dwarves: move ARRAY_SIZE() to dwarves.h
2023-10-11 9:17 [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features Alan Maguire
2023-10-11 9:17 ` [RFC dwarves 1/4] btf_encoder, pahole: move btf encoding options into conf_load Alan Maguire
@ 2023-10-11 9:17 ` Alan Maguire
2023-10-11 9:17 ` [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support Alan Maguire
` (2 subsequent siblings)
4 siblings, 0 replies; 21+ messages in thread
From: Alan Maguire @ 2023-10-11 9:17 UTC (permalink / raw
To: acme, andrii.nakryiko
Cc: jolsa, ast, daniel, eddyz87, martin.lau, song, yhs,
john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-11 9:17 [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features Alan Maguire
2023-10-11 9:17 ` [RFC dwarves 1/4] btf_encoder, pahole: move btf encoding options into conf_load Alan Maguire
2023-10-11 9:17 ` [RFC dwarves 2/4] dwarves: move ARRAY_SIZE() to dwarves.h Alan Maguire
@ 2023-10-11 9:17 ` Alan Maguire
2023-10-11 16:28 ` Eduard Zingerman
` (2 more replies)
2023-10-11 9:17 ` [RFC dwarves 4/4] pahole: add --supported_btf_features to display feature support Alan Maguire
2023-10-13 0:14 ` [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features Andrii Nakryiko
4 siblings, 3 replies; 21+ messages in thread
From: Alan Maguire @ 2023-10-11 9:17 UTC (permalink / raw
To: acme, andrii.nakryiko
Cc: jolsa, ast, daniel, eddyz87, martin.lau, song, yhs,
john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf, Alan Maguire,
Andrii Nakryiko
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [RFC dwarves 4/4] pahole: add --supported_btf_features to display feature support
2023-10-11 9:17 [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features Alan Maguire
` (2 preceding siblings ...)
2023-10-11 9:17 ` [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support Alan Maguire
@ 2023-10-11 9:17 ` Alan Maguire
2023-10-13 0:14 ` [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features Andrii Nakryiko
4 siblings, 0 replies; 21+ messages in thread
From: Alan Maguire @ 2023-10-11 9:17 UTC (permalink / raw
To: acme, andrii.nakryiko
Cc: jolsa, ast, daniel, eddyz87, martin.lau, song, yhs,
john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf, 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-11 9:17 ` [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support Alan Maguire
@ 2023-10-11 16:28 ` Eduard Zingerman
2023-10-11 16:41 ` Alan Maguire
2023-10-12 12:53 ` Jiri Olsa
2023-10-13 0:21 ` Andrii Nakryiko
2 siblings, 1 reply; 21+ messages in thread
From: Eduard Zingerman @ 2023-10-11 16:28 UTC (permalink / raw
To: Alan Maguire, acme, andrii.nakryiko
Cc: jolsa, ast, daniel, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, mykolal, bpf, Andrii Nakryiko
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;
> }
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-11 16:28 ` Eduard Zingerman
@ 2023-10-11 16:41 ` Alan Maguire
2023-10-11 19:08 ` Eduard Zingerman
0 siblings, 1 reply; 21+ messages in thread
From: Alan Maguire @ 2023-10-11 16:41 UTC (permalink / raw
To: Eduard Zingerman, acme, andrii.nakryiko
Cc: jolsa, ast, daniel, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, mykolal, bpf, Andrii Nakryiko
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;
>> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-11 16:41 ` Alan Maguire
@ 2023-10-11 19:08 ` Eduard Zingerman
2023-10-11 22:05 ` Alan Maguire
0 siblings, 1 reply; 21+ messages in thread
From: Eduard Zingerman @ 2023-10-11 19:08 UTC (permalink / raw
To: Alan Maguire, acme, andrii.nakryiko
Cc: jolsa, ast, daniel, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, mykolal, bpf, Andrii Nakryiko
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.
[...]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-11 19:08 ` Eduard Zingerman
@ 2023-10-11 22:05 ` Alan Maguire
2023-10-11 22:14 ` Eduard Zingerman
0 siblings, 1 reply; 21+ messages in thread
From: Alan Maguire @ 2023-10-11 22:05 UTC (permalink / raw
To: Eduard Zingerman, acme, andrii.nakryiko
Cc: jolsa, ast, daniel, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, mykolal, bpf, Andrii Nakryiko
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
> [...]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-11 22:05 ` Alan Maguire
@ 2023-10-11 22:14 ` Eduard Zingerman
2023-10-12 12:35 ` Alan Maguire
0 siblings, 1 reply; 21+ messages in thread
From: Eduard Zingerman @ 2023-10-11 22:14 UTC (permalink / raw
To: Alan Maguire, acme, andrii.nakryiko
Cc: jolsa, ast, daniel, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, mykolal, bpf, Andrii Nakryiko
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
[...]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-11 22:14 ` Eduard Zingerman
@ 2023-10-12 12:35 ` Alan Maguire
2023-10-13 14:17 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 21+ messages in thread
From: Alan Maguire @ 2023-10-12 12:35 UTC (permalink / raw
To: Eduard Zingerman, acme, andrii.nakryiko
Cc: jolsa, ast, daniel, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, mykolal, bpf, Andrii Nakryiko
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
>
> [...]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-11 9:17 ` [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support Alan Maguire
2023-10-11 16:28 ` Eduard Zingerman
@ 2023-10-12 12:53 ` Jiri Olsa
2023-10-12 13:48 ` Alan Maguire
2023-10-13 0:21 ` Andrii Nakryiko
2 siblings, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2023-10-12 12:53 UTC (permalink / raw
To: Alan Maguire
Cc: acme, andrii.nakryiko, ast, daniel, eddyz87, martin.lau, song,
yhs, john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf,
Andrii Nakryiko
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 1/4] btf_encoder, pahole: move btf encoding options into conf_load
2023-10-11 9:17 ` [RFC dwarves 1/4] btf_encoder, pahole: move btf encoding options into conf_load Alan Maguire
@ 2023-10-12 12:54 ` Jiri Olsa
0 siblings, 0 replies; 21+ messages in thread
From: Jiri Olsa @ 2023-10-12 12:54 UTC (permalink / raw
To: Alan Maguire
Cc: acme, andrii.nakryiko, ast, daniel, eddyz87, martin.lau, song,
yhs, john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-12 12:53 ` Jiri Olsa
@ 2023-10-12 13:48 ` Alan Maguire
2023-10-12 21:19 ` Jiri Olsa
0 siblings, 1 reply; 21+ messages in thread
From: Alan Maguire @ 2023-10-12 13:48 UTC (permalink / raw
To: Jiri Olsa
Cc: acme, andrii.nakryiko, ast, daniel, eddyz87, martin.lau, song,
yhs, john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf,
Andrii Nakryiko
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
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-12 13:48 ` Alan Maguire
@ 2023-10-12 21:19 ` Jiri Olsa
0 siblings, 0 replies; 21+ messages in thread
From: Jiri Olsa @ 2023-10-12 21:19 UTC (permalink / raw
To: Alan Maguire
Cc: Jiri Olsa, acme, andrii.nakryiko, ast, daniel, eddyz87,
martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo,
mykolal, bpf, Andrii Nakryiko
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features
2023-10-11 9:17 [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features Alan Maguire
` (3 preceding siblings ...)
2023-10-11 9:17 ` [RFC dwarves 4/4] pahole: add --supported_btf_features to display feature support Alan Maguire
@ 2023-10-13 0:14 ` Andrii Nakryiko
4 siblings, 0 replies; 21+ messages in thread
From: Andrii Nakryiko @ 2023-10-13 0:14 UTC (permalink / raw
To: Alan Maguire
Cc: acme, jolsa, ast, daniel, eddyz87, martin.lau, song, yhs,
john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-11 9:17 ` [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support Alan Maguire
2023-10-11 16:28 ` Eduard Zingerman
2023-10-12 12:53 ` Jiri Olsa
@ 2023-10-13 0:21 ` Andrii Nakryiko
2023-10-13 11:54 ` Alan Maguire
2 siblings, 1 reply; 21+ messages in thread
From: Andrii Nakryiko @ 2023-10-13 0:21 UTC (permalink / raw
To: Alan Maguire
Cc: acme, jolsa, ast, daniel, eddyz87, martin.lau, song, yhs,
john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf,
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-13 0:21 ` Andrii Nakryiko
@ 2023-10-13 11:54 ` Alan Maguire
2023-10-13 18:39 ` Andrii Nakryiko
0 siblings, 1 reply; 21+ messages in thread
From: Alan Maguire @ 2023-10-13 11:54 UTC (permalink / raw
To: Andrii Nakryiko
Cc: acme, jolsa, ast, daniel, eddyz87, martin.lau, song, yhs,
john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf,
Andrii Nakryiko
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
>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-12 12:35 ` Alan Maguire
@ 2023-10-13 14:17 ` Arnaldo Carvalho de Melo
2023-10-13 14:43 ` Alan Maguire
0 siblings, 1 reply; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-10-13 14:17 UTC (permalink / raw
To: Alan Maguire
Cc: Eduard Zingerman, andrii.nakryiko, jolsa, ast, daniel, martin.lau,
song, yhs, john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf,
Andrii Nakryiko
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-13 14:17 ` Arnaldo Carvalho de Melo
@ 2023-10-13 14:43 ` Alan Maguire
0 siblings, 0 replies; 21+ messages in thread
From: Alan Maguire @ 2023-10-13 14:43 UTC (permalink / raw
To: Arnaldo Carvalho de Melo
Cc: Eduard Zingerman, andrii.nakryiko, jolsa, ast, daniel, martin.lau,
song, yhs, john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf,
Andrii Nakryiko
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support
2023-10-13 11:54 ` Alan Maguire
@ 2023-10-13 18:39 ` Andrii Nakryiko
0 siblings, 0 replies; 21+ messages in thread
From: Andrii Nakryiko @ 2023-10-13 18:39 UTC (permalink / raw
To: Alan Maguire
Cc: acme, jolsa, ast, daniel, eddyz87, martin.lau, song, yhs,
john.fastabend, kpsingh, sdf, haoluo, mykolal, bpf,
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(-)
> >>
[...]
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2023-10-13 18:40 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-11 9:17 [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features Alan Maguire
2023-10-11 9:17 ` [RFC dwarves 1/4] btf_encoder, pahole: move btf encoding options into conf_load Alan Maguire
2023-10-12 12:54 ` Jiri Olsa
2023-10-11 9:17 ` [RFC dwarves 2/4] dwarves: move ARRAY_SIZE() to dwarves.h Alan Maguire
2023-10-11 9:17 ` [RFC dwarves 3/4] pahole: add --btf_features=feature1[,feature2...] support Alan Maguire
2023-10-11 16:28 ` Eduard Zingerman
2023-10-11 16:41 ` Alan Maguire
2023-10-11 19:08 ` Eduard Zingerman
2023-10-11 22:05 ` Alan Maguire
2023-10-11 22:14 ` Eduard Zingerman
2023-10-12 12:35 ` Alan Maguire
2023-10-13 14:17 ` Arnaldo Carvalho de Melo
2023-10-13 14:43 ` Alan Maguire
2023-10-12 12:53 ` Jiri Olsa
2023-10-12 13:48 ` Alan Maguire
2023-10-12 21:19 ` Jiri Olsa
2023-10-13 0:21 ` Andrii Nakryiko
2023-10-13 11:54 ` Alan Maguire
2023-10-13 18:39 ` Andrii Nakryiko
2023-10-11 9:17 ` [RFC dwarves 4/4] pahole: add --supported_btf_features to display feature support Alan Maguire
2023-10-13 0:14 ` [RFC dwarves 0/4] pahole, btf_encoder: support --btf_features Andrii Nakryiko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.