[PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions

[PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions

[Alan Maguire]

At optimization level -O2 or higher in gcc, static functions may be
optimized such that they have suffixes like .isra.0, .constprop.0 etc.
These represent 
    
- constant propagation (.constprop.0);
- interprocedural scalar replacement of aggregates, removal of
  unused parameters and replacement of parameters passed by
  reference by parameters passed by value (.isra.0)
  
See [1] for details. 
    
Currently BTF encoding does not handle such optimized functions
that get renamed with a "." suffix such as ".isra.0", ".constprop.0".
This is safer because such suffixes can often indicate parameters have
been optimized out.  This series addresses this by matching a
function to a suffixed version ("foo" matching "foo.isra.0") while
ensuring that the function signature does not contain optimized-out
parameters.  Note that if the function is found ("foo") it will
be preferred, only falling back to "foo.isra.0" if lookup of the
function fails.  Addition to BTF is skipped if the function has
optimized-out parameters, since the expected function signature
will not match. BTF encoding does not include the "."-suffix to
be consistent with DWARF. In addition, the kernel currently does
not allow a "." suffix in a BTF function name.

A problem with this approach however is that BTF carries out the
encoding process in parallel across multiple CUs, and sometimes
a function has optimized-out parameters in one CU but not others;
we see this for NF_HOOK.constprop.0 for example.  So in order to
determine if the function has optimized-out parameters in any
CU, its addition is not carried out until we have processed all
CUs and are about to merge BTF.  At this point we know if any
such optimizations have occurred.  Patches 1-5 handle the
optimized-out parameter identification and matching "."-suffixed
functions with the original function to facilitate BTF
encoding.  This feature can be enabled via the
"--btf_gen_optimized" option.

Patch 6 addresses a related problem - it is entirely possible
for a static function of the same name to exist in different
CUs with different function signatures.  Because BTF does not
currently encode any information that would help disambiguate
which BTF function specification matches which static function
(in the case of multiple different function signatures), it is
best to eliminate such functions from BTF for now.  The same
mechanism that is used to compare static "."-suffixed functions
is re-used for the static function comparison.  A superficial
comparison of number of parameters/parameter names is done to
see if such representations are consistent, and if inconsistent
prototypes are observed, the function is flagged for exclusion
from BTF.

When these methods are combined - the additive encoding of
"."-suffixed functions and the subtractive elimination of
functions with inconsistent parameters - we see an overall
drop in the number of functions in vmlinux BTF, from
51529 to 50246.  Skipping inconsistent functions is enabled
via "--skip_encoding_btf_inconsistent_proto".

Changes since v2 [2]
- Arnaldo incorporated some of the suggestions in the v2 thread;
  these patches are based on those; the relevant changes are
  noted as committer changes.
- Patch 1 is unchanged from v2, but the rest of the patches
  have been updated:
- Patch 2 separates out the changes to the struct btf_encoder
  that better support later addition of functions.
- Patch 3 then is changed insofar as these changes are no
  longer needed for the function addition refactoring.
- Patch 4 has a small change; we need to verify that an
  encoder has actually been added to the encoders list
  prior to removal
- Patch 5 changed significantly; when attempting to measure
  performance the relatively good numbers attained when using
  delayed function addition were not reproducible.
  Further analysis revealed that the large number of lookups
  caused by the presence of the separate function tree was
  a major cause of performance degradation in the multi
  threaded case.  So instead of maintaining a separate tree,
  we use the ELF function list which we already need to look
  up to match ELF -> DWARF function descriptions to store
  the function representation.  This has 2 benefits; firstly
  as mentioned, we already look up the ELF function so no
  additional lookup is required to save the function.
  Secondly, the ELF representation is identical for each
  encoder, so we can index the same function across multiple
  encoder function arrays - this greatly speeds up the
  processing of comparing function representations across
  encoders.  There is still a performance cost in this
  approach however; more details are provided in patch 6.
  An option specific to adding functions with "." suffixes
  is added "--btf_gen_optimized"
- Patch 6 builds on patch 5 in applying the save/merge/add
  approach for all functions using the same mechanisms.
  In addition the "--skip_encoding_btf_inconsistent_proto"
  option is introduced.
- Patches 7/8 document the new options in the pahole manual
  page.
  
Changes since v1 [3]

- Eduard noted that a DW_AT_const_value attribute can signal
  an optimized-out parameter, and that the lack of a location
  attribute signals optimization; ensure we handle those cases
  also (Eduard, patch 1).
- Jiri noted we can have inconsistencies between a static
  and non-static function; apply the comparison process to
  all functions (Jiri, patch 5)
- segmentation fault was observed when handling functions with
  > 10 parameters; needed parameter comparison loop to exit
  at BTF_ENCODER_MAX_PARAMETERS (patch 5)
- Kui-Feng Lee pointed out that having a global shared function
  tree would lead to a lot of contention; here a per-encoder 
  tree is used, and once the threads are collected the trees
  are merged. Performance numbers are provided in patch 5 
  (Kui-Feng Lee, patches 4/5)

[1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
[2] https://lore.kernel.org/bpf/1675088985-20300-1-git-send-email-alan.maguire@oracle.com/
[3] https://lore.kernel.org/bpf/1674567931-26458-1-git-send-email-alan.maguire@oracle.com/

Alan Maguire (8):
  dwarf_loader: Help spotting functions with optimized-out parameters
  btf_encoder: store type_id_off, unspecified type in encoder
  btf_encoder: Refactor function addition into dedicated
    btf_encoder__add_func
  btf_encoder: Rework btf_encoders__*() API to allow traversal of
    encoders
  btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF
  btf_encoder: support delaying function addition to check for function
    prototype inconsistencies
  dwarves: document --btf_gen_optimized option
  dwarves: document --skip_encoding_btf_inconsistent_proto option

 btf_encoder.c      | 360 +++++++++++++++++++++++++++++++++++++--------
 btf_encoder.h      |   6 -
 dwarf_loader.c     | 130 +++++++++++++++-
 dwarves.h          |  11 +-
 man-pages/pahole.1 |  10 ++
 pahole.c           |  30 +++-
 6 files changed, 468 insertions(+), 79 deletions(-)

-- 
2.31.1

[PATCH v3 dwarves 1/8] dwarf_loader: Help spotting functions with optimized-out parameters

[Alan Maguire]

Compilation generates DWARF at several stages, and often the later DWARF
representations more accurately represent optimizations that have
occurred during compilation.

In particular, parameter representations can be spotted by their
abstract origin references to the original parameter, but they often
have more accurate location information.  In most cases, the parameter
locations will match calling conventions, and be registers for the first
6 parameters on x86_64, first 8 on ARM64 etc.  If the parameter is not a
register when it should be however, it is likely passed via the stack or
the compiler has used a constant representation instead.  The latter can
often be spotted by checking for a DW_AT_const_value attribute, as noted
by Eduard.

In addition, absence of a location tag (either across the abstract
origin reference and the original parameter, or in the standalone
parameter description) is evidence of an optimized-out parameter.
Presence of a location tag is stored in the parameter description and
shared between abstract tags and their original referents.

This change adds a field to parameters and their associated ftype to
note if a parameter has been optimized out.  Having this information
allows us to skip such functions, as their presence in CUs makes BTF
encoding impossible.

Committer notes:

Changed the NR_REGISTER_PARAMS definition from a if/elif/endif for the
native architecture into a function that uses the ELF header e_machine
to find the target architecture, to allow for cross builds.

Also avoided looking at location expression in parameter__new() when the
param_idx argument is -1, as is the case when creating 'struct
parameter' instances for DW_TAG_subroutine_type, since we don't have
such info, only for the 'struct parameter' instances created from
DW_TAG_subprogram.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@chromium.org>
Cc: Kui-Feng Lee <sinquersw@gmail.com>
Cc: Martin KaFai Lau <martin.lau@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Timo Beckers <timo@incline.eu>
Cc: Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org
Link: https://lore.kernel.org/r/1675088985-20300-2-git-send-email-alan.maguire@oracle.com
Link: https://lore.kernel.org/r/9c330c78-e668-fa4c-e0ab-52aa445ccc00@oracle.com # DW_OP_reg0 is the first register on aarch64
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 dwarf_loader.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++---
 dwarves.h      |   6 ++-
 2 files changed, 128 insertions(+), 8 deletions(-)

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 5a74035..7aaf1d4 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -52,6 +52,10 @@
 #define DW_OP_addrx 0xa1
 #endif
 
+#ifndef EM_RISCV
+#define EM_RISCV	243
+#endif
+
 static pthread_mutex_t libdw__lock = PTHREAD_MUTEX_INITIALIZER;
 
 static uint32_t hashtags__bits = 12;
@@ -992,13 +996,98 @@ static struct class_member *class_member__new(Dwarf_Die *die, struct cu *cu,
 	return member;
 }
 
-static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu, struct conf_load *conf)
+/* How many function parameters are passed via registers?  Used below in
+ * determining if an argument has been optimized out or if it is simply
+ * an argument > cu__nr_register_params().  Making cu__nr_register_params()
+ * return 0 allows unsupported architectures to skip tagging optimized-out
+ * values.
+ */
+static int arch__nr_register_params(const GElf_Ehdr *ehdr)
+{
+	switch (ehdr->e_machine) {
+	case EM_S390:	 return 5;
+	case EM_SPARC:
+	case EM_SPARCV9:
+	case EM_X86_64:	 return 6;
+	case EM_AARCH64:
+	case EM_ARC:
+	case EM_ARM:
+	case EM_MIPS:
+	case EM_PPC:
+	case EM_PPC64:
+	case EM_RISCV:	 return 8;
+	default:	 break;
+	}
+
+	return 0;
+}
+
+static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
+					struct conf_load *conf, int param_idx)
 {
 	struct parameter *parm = tag__alloc(cu, sizeof(*parm));
 
 	if (parm != NULL) {
+		bool has_const_value;
+		Dwarf_Attribute attr;
+		struct location loc;
+
 		tag__init(&parm->tag, cu, die);
 		parm->name = attr_string(die, DW_AT_name, conf);
+
+		if (param_idx >= cu->nr_register_params || param_idx < 0)
+			return parm;
+		/* Parameters which use DW_AT_abstract_origin to point at
+		 * the original parameter definition (with no name in the DIE)
+		 * are the result of later DWARF generation during compilation
+		 * so often better take into account if arguments were
+		 * optimized out.
+		 *
+		 * By checking that locations for parameters that are expected
+		 * to be passed as registers are actually passed as registers,
+		 * we can spot optimized-out parameters.
+		 *
+		 * It can also be the case that a parameter DIE has
+		 * a constant value attribute reflecting optimization or
+		 * has no location attribute.
+		 *
+		 * From the DWARF spec:
+		 *
+		 * "4.1.10
+		 *
+		 * A DW_AT_const_value attribute for an entry describing a
+		 * variable or formal parameter whose value is constant and not
+		 * represented by an object in the address space of the program,
+		 * or an entry describing a named constant. (Note
+		 * that such an entry does not have a location attribute.)"
+		 *
+		 * So we can also use the absence of a location for a parameter
+		 * as evidence it has been optimized out.  This info will
+		 * need to be shared between a parameter and any abstract
+		 * origin references however, since gcc can have location
+		 * information in the parameter that refers back to the original
+		 * via abstract origin, so we need to share location presence
+		 * between these parameter representations.  See
+		 * ftype__recode_dwarf_types() below for how this is handled.
+		 */
+		parm->has_loc = dwarf_attr(die, DW_AT_location, &attr) != NULL;
+		has_const_value = dwarf_attr(die, DW_AT_const_value, &attr) != NULL;
+		if (parm->has_loc &&
+		    attr_location(die, &loc.expr, &loc.exprlen) == 0 &&
+			loc.exprlen != 0) {
+			Dwarf_Op *expr = loc.expr;
+
+			switch (expr->atom) {
+			case DW_OP_reg0 ... DW_OP_reg31:
+			case DW_OP_breg0 ... DW_OP_breg31:
+				break;
+			default:
+				parm->optimized = 1;
+				break;
+			}
+		} else if (has_const_value) {
+			parm->optimized = 1;
+		}
 	}
 
 	return parm;
@@ -1450,7 +1539,7 @@ static struct tag *die__create_new_parameter(Dwarf_Die *die,
 					     struct cu *cu, struct conf_load *conf,
 					     int param_idx)
 {
-	struct parameter *parm = parameter__new(die, cu, conf);
+	struct parameter *parm = parameter__new(die, cu, conf, param_idx);
 
 	if (parm == NULL)
 		return NULL;
@@ -2194,6 +2283,7 @@ static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu)
 
 	ftype__for_each_parameter(type, pos) {
 		struct dwarf_tag *dpos = pos->tag.priv;
+		struct parameter *opos;
 		struct dwarf_tag *dtype;
 
 		if (dpos->type.off == 0) {
@@ -2207,8 +2297,18 @@ static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu)
 				tag__print_abstract_origin_not_found(&pos->tag);
 				continue;
 			}
-			pos->name = tag__parameter(dtype->tag)->name;
+			opos = tag__parameter(dtype->tag);
+			pos->name = opos->name;
 			pos->tag.type = dtype->tag->type;
+			/* share location information between parameter and
+			 * abstract origin; if neither have location, we will
+			 * mark the parameter as optimized out.
+			 */
+			if (pos->has_loc)
+				opos->has_loc = pos->has_loc;
+
+			if (pos->optimized)
+				opos->optimized = pos->optimized;
 			continue;
 		}
 
@@ -2478,18 +2578,33 @@ out:
 	return 0;
 }
 
-static int cu__resolve_func_ret_types(struct cu *cu)
+static int cu__resolve_func_ret_types_optimized(struct cu *cu)
 {
 	struct ptr_table *pt = &cu->functions_table;
 	uint32_t i;
 
 	for (i = 0; i < pt->nr_entries; ++i) {
 		struct tag *tag = pt->entries[i];
+		struct parameter *pos;
+		struct function *fn = tag__function(tag);
+
+		/* mark function as optimized if parameter is, or
+		 * if parameter does not have a location; at this
+		 * point location presence has been marked in
+		 * abstract origins for cases where a parameter
+		 * location is not stored in the original function
+		 * parameter tag.
+		 */
+		ftype__for_each_parameter(&fn->proto, pos) {
+			if (pos->optimized || !pos->has_loc) {
+				fn->proto.optimized_parms = 1;
+				break;
+			}
+		}
 
 		if (tag == NULL || tag->type != 0)
 			continue;
 
-		struct function *fn = tag__function(tag);
 		if (!fn->abstract_origin)
 			continue;
 
@@ -2612,7 +2727,7 @@ static int die__process_and_recode(Dwarf_Die *die, struct cu *cu, struct conf_lo
 	if (ret != 0)
 		return ret;
 
-	return cu__resolve_func_ret_types(cu);
+	return cu__resolve_func_ret_types_optimized(cu);
 }
 
 static int class_member__cache_byte_size(struct tag *tag, struct cu *cu,
@@ -2753,6 +2868,7 @@ static int cu__set_common(struct cu *cu, struct conf_load *conf,
 		return DWARF_CB_ABORT;
 
 	cu->little_endian = ehdr.e_ident[EI_DATA] == ELFDATA2LSB;
+	cu->nr_register_params = arch__nr_register_params(&ehdr);
 	return 0;
 }
 
@@ -3132,7 +3248,7 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
 	 * encoded in another subprogram through abstract_origin
 	 * tag. Let us visit all subprograms again to resolve this.
 	 */
-	if (cu__resolve_func_ret_types(cu) != LSK__KEEPIT)
+	if (cu__resolve_func_ret_types_optimized(cu) != LSK__KEEPIT)
 		goto out_abort;
 
 	if (cus__finalize(cus, cu, conf, NULL) == LSK__STOP_LOADING)
diff --git a/dwarves.h b/dwarves.h
index 589588e..1cd95f7 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -262,6 +262,7 @@ struct cu {
 	uint8_t		 has_addr_info:1;
 	uint8_t		 uses_global_strings:1;
 	uint8_t		 little_endian:1;
+	uint8_t		 nr_register_params;
 	uint16_t	 language;
 	unsigned long	 nr_inline_expansions;
 	size_t		 size_inline_expansions;
@@ -808,6 +809,8 @@ size_t lexblock__fprintf(const struct lexblock *lexblock, const struct cu *cu,
 struct parameter {
 	struct tag tag;
 	const char *name;
+	uint8_t optimized:1;
+	uint8_t has_loc:1;
 };
 
 static inline struct parameter *tag__parameter(const struct tag *tag)
@@ -827,7 +830,8 @@ struct ftype {
 	struct tag	 tag;
 	struct list_head parms;
 	uint16_t	 nr_parms;
-	uint8_t		 unspec_parms; /* just one bit is needed */
+	uint8_t		 unspec_parms:1; /* just one bit is needed */
+	uint8_t		 optimized_parms:1;
 };
 
 static inline struct ftype *tag__ftype(const struct tag *tag)
-- 
2.31.1

[PATCH v3 dwarves 2/8] btf_encoder: store type_id_off, unspecified type in encoder

[Alan Maguire]

Store the type id offset and unspecified type in the
encoder.

This will be useful for postponing local function addition
since to support function addition later on, CU references
will not work.  Provision will have to be made to save the
current type_id_off to support later addition of a function
by setting the type_id_off for the encoder to the saved
value.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@chromium.org>
Cc: Kui-Feng Lee <sinquersw@gmail.com>
Cc: Martin KaFai Lau <martin.lau@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Timo Beckers <timo@incline.eu>
Cc: Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org
---
 btf_encoder.c | 59 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index a5fa04a..9063342 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -54,6 +54,8 @@ struct btf_encoder {
 	struct gobuffer   percpu_secinfo;
 	const char	  *filename;
 	struct elf_symtab *symtab;
+	uint32_t	  type_id_off;
+	uint32_t	  unspecified_type;
 	bool		  has_index_type,
 			  need_index_type,
 			  skip_encoding_vars,
@@ -593,20 +595,20 @@ static int32_t btf_encoder__add_func_param(struct btf_encoder *encoder, const ch
 	}
 }
 
-static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t type_id_off, uint32_t tag_type)
+static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_type)
 {
 	if (tag_type == 0)
 		return 0;
 
-	if (encoder->cu->unspecified_type.tag && tag_type == encoder->cu->unspecified_type.type) {
+	if (encoder->unspecified_type && tag_type == encoder->unspecified_type) {
 		// No provision for encoding this, turn it into void.
 		return 0;
 	}
 
-	return type_id_off + tag_type;
+	return encoder->type_id_off + tag_type;
 }
 
-static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype, uint32_t type_id_off)
+static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype)
 {
 	struct btf *btf = encoder->btf;
 	const struct btf_type *t;
@@ -616,7 +618,7 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
 
 	/* add btf_type for func_proto */
 	nr_params = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
-	type_id = btf_encoder__tag_type(encoder, type_id_off, ftype->tag.type);
+	type_id = btf_encoder__tag_type(encoder, ftype->tag.type);
 
 	id = btf__add_func_proto(btf, type_id);
 	if (id > 0) {
@@ -634,7 +636,7 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
 	ftype__for_each_parameter(ftype, param) {
 		const char *name = parameter__name(param);
 
-		type_id = param->tag.type == 0 ? 0 : type_id_off + param->tag.type;
+		type_id = param->tag.type == 0 ? 0 : encoder->type_id_off + param->tag.type;
 		++param_idx;
 		if (btf_encoder__add_func_param(encoder, name, type_id, param_idx == nr_params))
 			return -1;
@@ -859,22 +861,21 @@ static void dump_invalid_symbol(const char *msg, const char *sym,
 	fprintf(stderr, "PAHOLE: Error: Use '--btf_encode_force' to ignore such symbols and force emit the btf.\n");
 }
 
-static int tag__check_id_drift(const struct tag *tag,
-			       uint32_t core_id, uint32_t btf_type_id,
-			       uint32_t type_id_off)
+static int tag__check_id_drift(struct btf_encoder *encoder, const struct tag *tag,
+			       uint32_t core_id, uint32_t btf_type_id)
 {
-	if (btf_type_id != (core_id + type_id_off)) {
+	if (btf_type_id != (core_id + encoder->type_id_off)) {
 		fprintf(stderr,
 			"%s: %s id drift, core_id: %u, btf_type_id: %u, type_id_off: %u\n",
 			__func__, dwarf_tag_name(tag->tag),
-			core_id, btf_type_id, type_id_off);
+			core_id, btf_type_id, encoder->type_id_off);
 		return -1;
 	}
 
 	return 0;
 }
 
-static int32_t btf_encoder__add_struct_type(struct btf_encoder *encoder, struct tag *tag, uint32_t type_id_off)
+static int32_t btf_encoder__add_struct_type(struct btf_encoder *encoder, struct tag *tag)
 {
 	struct type *type = tag__type(tag);
 	struct class_member *pos;
@@ -896,7 +897,8 @@ static int32_t btf_encoder__add_struct_type(struct btf_encoder *encoder, struct
 		 * is required.
 		 */
 		name = class_member__name(pos);
-		if (btf_encoder__add_field(encoder, name, type_id_off + pos->tag.type, pos->bitfield_size, pos->bit_offset))
+		if (btf_encoder__add_field(encoder, name, encoder->type_id_off + pos->tag.type,
+					   pos->bitfield_size, pos->bit_offset))
 			return -1;
 	}
 
@@ -936,11 +938,11 @@ static int32_t btf_encoder__add_enum_type(struct btf_encoder *encoder, struct ta
 	return type_id;
 }
 
-static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag, uint32_t type_id_off,
+static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag,
 				   struct conf_load *conf_load)
 {
 	/* single out type 0 as it represents special type "void" */
-	uint32_t ref_type_id = tag->type == 0 ? 0 : type_id_off + tag->type;
+	uint32_t ref_type_id = tag->type == 0 ? 0 : encoder->type_id_off + tag->type;
 	struct base_type *bt;
 	const char *name;
 
@@ -970,7 +972,7 @@ static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag,
 		if (tag__type(tag)->declaration)
 			return btf_encoder__add_ref_type(encoder, BTF_KIND_FWD, 0, name, tag->tag == DW_TAG_union_type);
 		else
-			return btf_encoder__add_struct_type(encoder, tag, type_id_off);
+			return btf_encoder__add_struct_type(encoder, tag);
 	case DW_TAG_array_type:
 		/* TODO: Encode one dimension at a time. */
 		encoder->need_index_type = true;
@@ -978,7 +980,7 @@ static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag,
 	case DW_TAG_enumeration_type:
 		return btf_encoder__add_enum_type(encoder, tag, conf_load);
 	case DW_TAG_subroutine_type:
-		return btf_encoder__add_func_proto(encoder, tag__ftype(tag), type_id_off);
+		return btf_encoder__add_func_proto(encoder, tag__ftype(tag));
         case DW_TAG_unspecified_type:
 		/* Just don't encode this for now, converting anything with this type to void (0) instead.
 		 *
@@ -1281,7 +1283,7 @@ static bool ftype__has_arg_names(const struct ftype *ftype)
 	return true;
 }
 
-static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder, uint32_t type_id_off)
+static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
 {
 	struct cu *cu = encoder->cu;
 	uint32_t core_id;
@@ -1366,7 +1368,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder, uint32_
 			continue;
 		}
 
-		type = var->ip.tag.type + type_id_off;
+		type = var->ip.tag.type + encoder->type_id_off;
 		linkage = var->external ? BTF_VAR_GLOBAL_ALLOCATED : BTF_VAR_STATIC;
 
 		if (encoder->verbose) {
@@ -1507,7 +1509,6 @@ void btf_encoder__delete(struct btf_encoder *encoder)
 
 int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct conf_load *conf_load)
 {
-	uint32_t type_id_off = btf__type_cnt(encoder->btf) - 1;
 	struct llvm_annotation *annot;
 	int btf_type_id, tag_type_id, skipped_types = 0;
 	uint32_t core_id;
@@ -1516,21 +1517,24 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 	int err = 0;
 
 	encoder->cu = cu;
+	encoder->type_id_off = btf__type_cnt(encoder->btf) - 1;
+	if (encoder->cu->unspecified_type.tag)
+		encoder->unspecified_type = encoder->cu->unspecified_type.type;
 
 	if (!encoder->has_index_type) {
 		/* cu__find_base_type_by_name() takes "type_id_t *id" */
 		type_id_t id;
 		if (cu__find_base_type_by_name(cu, "int", &id)) {
 			encoder->has_index_type = true;
-			encoder->array_index_id = type_id_off + id;
+			encoder->array_index_id = encoder->type_id_off + id;
 		} else {
 			encoder->has_index_type = false;
-			encoder->array_index_id = type_id_off + cu->types_table.nr_entries;
+			encoder->array_index_id = encoder->type_id_off + cu->types_table.nr_entries;
 		}
 	}
 
 	cu__for_each_type(cu, core_id, pos) {
-		btf_type_id = btf_encoder__encode_tag(encoder, pos, type_id_off, conf_load);
+		btf_type_id = btf_encoder__encode_tag(encoder, pos, conf_load);
 
 		if (btf_type_id == 0) {
 			++skipped_types;
@@ -1538,7 +1542,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 		}
 
 		if (btf_type_id < 0 ||
-		    tag__check_id_drift(pos, core_id, btf_type_id + skipped_types, type_id_off)) {
+		    tag__check_id_drift(encoder, pos, core_id, btf_type_id + skipped_types)) {
 			err = -1;
 			goto out;
 		}
@@ -1572,7 +1576,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 			continue;
 		}
 
-		btf_type_id = type_id_off + core_id;
+		btf_type_id = encoder->type_id_off + core_id;
 		ns = tag__namespace(pos);
 		list_for_each_entry(annot, &ns->annots, node) {
 			tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_type_id, annot->component_idx);
@@ -1616,7 +1620,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 				continue;
 		}
 
-		btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto, type_id_off);
+		btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto);
 		name = function__name(fn);
 		btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
 		if (btf_fnproto_id < 0 || btf_fn_id < 0) {
@@ -1633,10 +1637,11 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 				goto out;
 			}
 		}
+
 	}
 
 	if (!encoder->skip_encoding_vars)
-		err = btf_encoder__encode_cu_variables(encoder, type_id_off);
+		err = btf_encoder__encode_cu_variables(encoder);
 out:
 	encoder->cu = NULL;
 	return err;
-- 
2.31.1

[PATCH v3 dwarves 3/8] btf_encoder: Refactor function addition into dedicated btf_encoder__add_func

[Alan Maguire]

This will be useful for postponing local function addition later on.
As part of this, store the type id offset and unspecified type in
the encoder, as this will simplify late addition of local functions.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@chromium.org>
Cc: Kui-Feng Lee <sinquersw@gmail.com>
Cc: Martin KaFai Lau <martin.lau@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Timo Beckers <timo@incline.eu>
Cc: Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org
---
 btf_encoder.c | 46 +++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 9063342..71f67ae 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -764,6 +764,31 @@ static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char
 	return id;
 }
 
+static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn)
+{
+	int btf_fnproto_id, btf_fn_id, tag_type_id;
+	struct llvm_annotation *annot;
+	const char *name;
+
+	btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto);
+	name = function__name(fn);
+	btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
+	if (btf_fnproto_id < 0 || btf_fn_id < 0) {
+		printf("error: failed to encode function '%s'\n", function__name(fn));
+		return -1;
+	}
+	list_for_each_entry(annot, &fn->annots, node) {
+		tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_fn_id,
+							annot->component_idx);
+		if (tag_type_id < 0) {
+			fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
+				annot->value, name, annot->component_idx);
+			return -1;
+		}
+	}
+	return 0;
+}
+
 /*
  * This corresponds to the same macro defined in
  * include/linux/kallsyms.h
@@ -1589,8 +1614,6 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 	}
 
 	cu__for_each_function(cu, core_id, fn) {
-		int btf_fnproto_id, btf_fn_id;
-		const char *name;
 
 		/*
 		 * Skip functions that:
@@ -1620,24 +1643,9 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 				continue;
 		}
 
-		btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto);
-		name = function__name(fn);
-		btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
-		if (btf_fnproto_id < 0 || btf_fn_id < 0) {
-			err = -1;
-			printf("error: failed to encode function '%s'\n", function__name(fn));
+		err = btf_encoder__add_func(encoder, fn);
+		if (err)
 			goto out;
-		}
-
-		list_for_each_entry(annot, &fn->annots, node) {
-			tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_fn_id, annot->component_idx);
-			if (tag_type_id < 0) {
-				fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
-					annot->value, name, annot->component_idx);
-				goto out;
-			}
-		}
-
 	}
 
 	if (!encoder->skip_encoding_vars)
-- 
2.31.1

[PATCH v3 dwarves 4/8] btf_encoder: Rework btf_encoders__*() API to allow traversal of encoders

[Alan Maguire]

To coordinate across multiple encoders at collection time, there will be
a need to access the set of encoders.  Rework the unused
btf_encoders__*() API to facilitate this.

Committer notes:

Removed btf_encoders__for_each_encoder() duplicate define, pointed out
by Jiri Olsa.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@chromium.org>
Cc: Kui-Feng Lee <sinquersw@gmail.com>
Cc: Martin KaFai Lau <martin.lau@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Timo Beckers <timo@incline.eu>
Cc: Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org
---
 btf_encoder.c | 36 ++++++++++++++++++++++++++++--------
 btf_encoder.h |  6 ------
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 71f67ae..74ab61b 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -30,6 +30,7 @@
 
 #include <errno.h>
 #include <stdint.h>
+#include <pthread.h>
 
 struct elf_function {
 	const char	*name;
@@ -79,19 +80,36 @@ struct btf_encoder {
 	} functions;
 };
 
-void btf_encoders__add(struct list_head *encoders, struct btf_encoder *encoder)
-{
-	list_add_tail(&encoder->node, encoders);
-}
+static LIST_HEAD(encoders);
+static pthread_mutex_t encoders__lock = PTHREAD_MUTEX_INITIALIZER;
+
+/* mutex only needed for add/delete, as this can happen in multiple encoding
+ * threads.  Traversal of the list is currently confined to thread collection.
+ */
 
-struct btf_encoder *btf_encoders__first(struct list_head *encoders)
+#define btf_encoders__for_each_encoder(encoder)		\
+	list_for_each_entry(encoder, &encoders, node)
+
+static void btf_encoders__add(struct btf_encoder *encoder)
 {
-	return list_first_entry(encoders, struct btf_encoder, node);
+	pthread_mutex_lock(&encoders__lock);
+	list_add_tail(&encoder->node, &encoders);
+	pthread_mutex_unlock(&encoders__lock);
 }
 
-struct btf_encoder *btf_encoders__next(struct btf_encoder *encoder)
+static void btf_encoders__delete(struct btf_encoder *encoder)
 {
-	return list_next_entry(encoder, node);
+	struct btf_encoder *existing = NULL;
+
+	pthread_mutex_lock(&encoders__lock);
+	/* encoder may not have been added to list yet; check. */
+	btf_encoders__for_each_encoder(existing) {
+		if (encoder == existing)
+			break;
+	}
+	if (encoder == existing)
+		list_del(&encoder->node);
+	pthread_mutex_unlock(&encoders__lock);
 }
 
 #define PERCPU_SECTION ".data..percpu"
@@ -1505,6 +1523,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam
 
 		if (encoder->verbose)
 			printf("File %s:\n", cu->filename);
+		btf_encoders__add(encoder);
 	}
 out:
 	return encoder;
@@ -1519,6 +1538,7 @@ void btf_encoder__delete(struct btf_encoder *encoder)
 	if (encoder == NULL)
 		return;
 
+	btf_encoders__delete(encoder);
 	__gobuffer__delete(&encoder->percpu_secinfo);
 	zfree(&encoder->filename);
 	btf__free(encoder->btf);
diff --git a/btf_encoder.h b/btf_encoder.h
index a65120c..34516bb 100644
--- a/btf_encoder.h
+++ b/btf_encoder.h
@@ -23,12 +23,6 @@ int btf_encoder__encode(struct btf_encoder *encoder);
 
 int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct conf_load *conf_load);
 
-void btf_encoders__add(struct list_head *encoders, struct btf_encoder *encoder);
-
-struct btf_encoder *btf_encoders__first(struct list_head *encoders);
-
-struct btf_encoder *btf_encoders__next(struct btf_encoder *encoder);
-
 struct btf *btf_encoder__btf(struct btf_encoder *encoder);
 
 int btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder *other);
-- 
2.31.1

[PATCH v3 dwarves 5/8] btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF

[Alan Maguire]

At gcc optimization level O2 or higher, many function optimizations
occur such as

- constant propagation (.constprop.0);
- interprocedural scalar replacement of aggregates, removal of
  unused parameters and replacement of parameters passed by
  reference by parameters passed by value (.isra.0)

See [1] for details.

Currently BTF encoding does not handle such optimized functions that get
renamed with a "." suffix such as ".isra.0", ".constprop.0".  This is
safer because such suffixes can often indicate parameters have been
optimized out.  Since we can now spot this, support matching to a "."
suffix and represent the function in BTF if it does not have
optimized-out parameters.  First an attempt to match by exact name is
made; if that fails we fall back to checking for a "."-suffixed name.
The BTF representation will use the original function name "foo" not
"foo.isra.0" for consistency with DWARF representation.

There is a complication however, and this arises because we process each
CU separately and merge BTF when complete.  Different CUs may optimize
differently, so in one CU, a function may have optimized-out parameters
- and thus be ineligible for BTF - while in another it does not have
  optimized-out parameters - making it eligible for BTF.  The NF_HOOK
function is an example of this.

To avoid disrupting BTF generation parallelism, the approach taken is to
save pointers to the function representations in the ELF function table;
it is per-encoder, but the same representation is used across encoders,
so we can use the same array index to find the same function when
merging for efficiency.  Using the ELF function is ideal also as we
already have to carry out a name lookup for matching from DWARF to
ELF.

At thread collection time, observations about optimizations are
merged across encoders and we know whether it is safe to add a
"."-suffixed function or not.

The result of this is we add 1180 "."-suffixed functions to the BTF
representation.

Encoding of "."-suffixed functions is done only if the
"--btf_gen_optimized" function is specified.

Because we need to ensure consistency across encoders, there is
a performance cost to the save/merge/apply approach. Comparing
baseline to test times:

$ time LLVM_OBJCOPY=objcopy pahole -J -j4 vmlinux

real	0m7.788s
user	0m17.629s
sys	0m0.652s

$ time LLVM_OBJCOPY=objcopy pahole -J -j4 --btf_gen_optimized vmlinux

real	0m10.839s
user	0m19.473s
sys	0m5.932s

[1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@chromium.org>
Cc: Kui-Feng Lee <sinquersw@gmail.com>
Cc: Martin KaFai Lau <martin.lau@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Timo Beckers <timo@incline.eu>
Cc: Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org
---
 btf_encoder.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++---
 dwarves.h     |   3 +
 pahole.c      |  22 +++++---
 3 files changed, 159 insertions(+), 14 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 74ab61b..cb50401 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -30,11 +30,20 @@
 
 #include <errno.h>
 #include <stdint.h>
+#include <search.h> /* for tsearch(), tfind() and tdestroy() */
 #include <pthread.h>
 
+/* state used to do later encoding of saved functions */
+struct btf_encoder_state {
+	uint32_t type_id_off;
+};
+
 struct elf_function {
 	const char	*name;
 	bool		 generated;
+	size_t		prefixlen;
+	struct function	*function;
+	struct btf_encoder_state state;
 };
 
 #define MAX_PERCPU_VAR_CNT 4096
@@ -57,6 +66,7 @@ struct btf_encoder {
 	struct elf_symtab *symtab;
 	uint32_t	  type_id_off;
 	uint32_t	  unspecified_type;
+	int		  saved_func_cnt;
 	bool		  has_index_type,
 			  need_index_type,
 			  skip_encoding_vars,
@@ -77,12 +87,15 @@ struct btf_encoder {
 		struct elf_function *entries;
 		int		    allocated;
 		int		    cnt;
+		int		    suffix_cnt; /* number of .isra, .part etc */
 	} functions;
 };
 
 static LIST_HEAD(encoders);
 static pthread_mutex_t encoders__lock = PTHREAD_MUTEX_INITIALIZER;
 
+static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder);
+
 /* mutex only needed for add/delete, as this can happen in multiple encoding
  * threads.  Traversal of the list is currently confined to thread collection.
  */
@@ -707,6 +720,11 @@ int32_t btf_encoder__add_encoder(struct btf_encoder *encoder, struct btf_encoder
 	int32_t i, id;
 	struct btf_var_secinfo *vsi;
 
+	if (encoder == other)
+		return 0;
+
+	btf_encoder__add_saved_funcs(other);
+
 	for (i = 0; i < nr_var_secinfo; i++) {
 		vsi = (struct btf_var_secinfo *)var_secinfo_buf->entries + i;
 		type_id = next_type_id + vsi->type - 1; /* Type ID starts from 1 */
@@ -782,6 +800,27 @@ static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char
 	return id;
 }
 
+static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
+{
+	if (func->function) {
+		struct function *existing = func->function;
+
+		/* If saving and we find an existing entry, we want to merge
+		 * observations across both functions, checking that the
+		 * "seen optimized parameters" status is reflected in the func
+		 * entry. If the entry is new, record encoder state required
+		 * to add the local function later (encoder + type_id_off)
+		 * such that we can add the function later.
+		 */
+		existing->proto.optimized_parms |= fn->proto.optimized_parms;
+	} else {
+		func->state.type_id_off = encoder->type_id_off;
+		func->function = fn;
+		encoder->saved_func_cnt++;
+	}
+	return 0;
+}
+
 static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn)
 {
 	int btf_fnproto_id, btf_fn_id, tag_type_id;
@@ -807,6 +846,50 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct functio
 	return 0;
 }
 
+static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
+{
+	int i;
+
+	for (i = 0; i < encoder->functions.cnt; i++) {
+		struct function *fn = encoder->functions.entries[i].function;
+		struct btf_encoder *other_encoder;
+
+		if (!fn || fn->proto.processed)
+			continue;
+
+		/* merge optimized-out status across encoders; since each
+		 * encoder has the same elf symbol table we can use the
+		 * same index to access the same elf symbol.
+		 */
+		btf_encoders__for_each_encoder(other_encoder) {
+			struct function *other_fn;
+
+			if (other_encoder == encoder)
+				continue;
+
+			other_fn = other_encoder->functions.entries[i].function;
+			if (!other_fn)
+				continue;
+			fn->proto.optimized_parms |= other_fn->proto.optimized_parms;
+			other_fn->proto.processed = 1;
+		}
+		if (fn->proto.optimized_parms) {
+			if (encoder->verbose) {
+				const char *name = function__name(fn);
+
+				printf("skipping addition of '%s'(%s) due to optimized-out parameters\n",
+				       name, fn->alias ?: name);
+			}
+		} else {
+			struct elf_function *func = &encoder->functions.entries[i];
+
+			encoder->type_id_off = func->state.type_id_off;
+			btf_encoder__add_func(encoder, fn);
+		}
+		fn->proto.processed = 1;
+	}
+}
+
 /*
  * This corresponds to the same macro defined in
  * include/linux/kallsyms.h
@@ -818,6 +901,11 @@ static int functions_cmp(const void *_a, const void *_b)
 	const struct elf_function *a = _a;
 	const struct elf_function *b = _b;
 
+	/* if search key allows prefix match, verify target has matching
+	 * prefix len and prefix matches.
+	 */
+	if (a->prefixlen && a->prefixlen == b->prefixlen)
+		return strncmp(a->name, b->name, b->prefixlen);
 	return strcmp(a->name, b->name);
 }
 
@@ -850,14 +938,22 @@ static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *
 	}
 
 	encoder->functions.entries[encoder->functions.cnt].name = name;
+	if (strchr(name, '.')) {
+		const char *suffix = strchr(name, '.');
+
+		encoder->functions.suffix_cnt++;
+		encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
+	}
 	encoder->functions.entries[encoder->functions.cnt].generated = false;
+	encoder->functions.entries[encoder->functions.cnt].function = NULL;
 	encoder->functions.cnt++;
 	return 0;
 }
 
-static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder, const char *name)
+static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
+						       const char *name, size_t prefixlen)
 {
-	struct elf_function key = { .name = name };
+	struct elf_function key = { .name = name, .prefixlen = prefixlen };
 
 	return bsearch(&key, encoder->functions.entries, encoder->functions.cnt, sizeof(key), functions_cmp);
 }
@@ -1187,6 +1283,9 @@ int btf_encoder__encode(struct btf_encoder *encoder)
 {
 	int err;
 
+	/* for single-threaded case, saved funcs are added here */
+	btf_encoder__add_saved_funcs(encoder);
+
 	if (gobuffer__size(&encoder->percpu_secinfo) != 0)
 		btf_encoder__add_datasec(encoder, PERCPU_SECTION);
 
@@ -1634,6 +1733,8 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 	}
 
 	cu__for_each_function(cu, core_id, fn) {
+		struct elf_function *func = NULL;
+		bool save = false;
 
 		/*
 		 * Skip functions that:
@@ -1647,29 +1748,62 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 		if (!ftype__has_arg_names(&fn->proto))
 			continue;
 		if (encoder->functions.cnt) {
-			struct elf_function *func;
 			const char *name;
 
 			name = function__name(fn);
 			if (!name)
 				continue;
 
-			func = btf_encoder__find_function(encoder, name);
-			if (!func || func->generated)
+			/* prefer exact function name match... */
+			func = btf_encoder__find_function(encoder, name, 0);
+			if (func) {
+				if (func->generated)
+					continue;
+				func->generated = true;
+			} else if (encoder->functions.suffix_cnt &&
+				   conf_load->btf_gen_optimized) {
+				/* falling back to name.isra.0 match if no exact
+				 * match is found; only bother if we found any
+				 * .suffix function names.  The function
+				 * will be saved and added once we ensure
+				 * it does not have optimized-out parameters
+				 * in any cu.
+				 */
+				func = btf_encoder__find_function(encoder, name,
+								  strlen(name));
+				if (func) {
+					save = true;
+					if (encoder->verbose)
+						printf("matched function '%s' with '%s'%s\n",
+						       name, func->name,
+						       fn->proto.optimized_parms ?
+						       ", has optimized-out parameters" : "");
+					fn->alias = func->name;
+				}
+			}
+			if (!func)
 				continue;
-			func->generated = true;
 		} else {
 			if (!fn->external)
 				continue;
 		}
 
-		err = btf_encoder__add_func(encoder, fn);
+		if (save)
+			err = btf_encoder__save_func(encoder, fn, func);
+		else
+			err = btf_encoder__add_func(encoder, fn);
 		if (err)
 			goto out;
 	}
 
 	if (!encoder->skip_encoding_vars)
 		err = btf_encoder__encode_cu_variables(encoder);
+
+	/* It is only safe to delete this CU if we have not stashed any static
+	 * functions for later addition.
+	 */
+	if (!err)
+		err = encoder->saved_func_cnt > 0 ? LSK__KEEPIT : LSK__DELETE;
 out:
 	encoder->cu = NULL;
 	return err;
diff --git a/dwarves.h b/dwarves.h
index 1cd95f7..bb2c3bb 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -66,6 +66,7 @@ struct conf_load {
 	bool			skip_missing;
 	bool			skip_encoding_btf_type_tag;
 	bool			skip_encoding_btf_enum64;
+	bool			btf_gen_optimized;
 	uint8_t			hashtable_bits;
 	uint8_t			max_hashtable_bits;
 	uint16_t		kabi_prefix_len;
@@ -832,6 +833,7 @@ struct ftype {
 	uint16_t	 nr_parms;
 	uint8_t		 unspec_parms:1; /* just one bit is needed */
 	uint8_t		 optimized_parms:1;
+	uint8_t		 processed:1;
 };
 
 static inline struct ftype *tag__ftype(const struct tag *tag)
@@ -884,6 +886,7 @@ struct function {
 	struct rb_node	 rb_node;
 	const char	 *name;
 	const char	 *linkage_name;
+	const char	 *alias;	/* name.isra.0 */
 	uint32_t	 cu_total_size_inline_expansions;
 	uint16_t	 cu_total_nr_inline_expansions;
 	uint8_t		 inlined:2;
diff --git a/pahole.c b/pahole.c
index 6f4f87c..f48b66d 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1221,6 +1221,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
 #define ARGP_languages_exclude	   336
 #define ARGP_skip_encoding_btf_enum64 337
 #define ARGP_skip_emitting_atomic_typedefs 338
+#define ARGP_btf_gen_optimized  339
 
 static const struct argp_option pahole__options[] = {
 	{
@@ -1633,6 +1634,11 @@ static const struct argp_option pahole__options[] = {
 		.key  = ARGP_skip_emitting_atomic_typedefs,
 		.doc  = "Do not emit 'typedef _Atomic int atomic_int' & friends."
 	},
+	{
+		.name = "btf_gen_optimized",
+		.key  = ARGP_btf_gen_optimized,
+		.doc  = "Generate BTF for functions with optimization-related suffixes (.isra, .constprop).  BTF will only be generated if a function does not optimize out parameters."
+	},
 	{
 		.name = NULL,
 	}
@@ -1802,6 +1808,8 @@ static error_t pahole__options_parser(int key, char *arg,
 		conf_load.skip_encoding_btf_enum64 = true;	break;
 	case ARGP_skip_emitting_atomic_typedefs:
 		conf.skip_emitting_atomic_typedefs = true;	break;
+	case ARGP_btf_gen_optimized:
+		conf_load.btf_gen_optimized = true;		break;
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
@@ -2980,20 +2988,20 @@ static int pahole_threads_collect(struct conf_load *conf, int nr_threads, void *
 		 * Merge content of the btf instances of worker threads to the btf
 		 * instance of the primary btf_encoder.
                 */
-		if (!threads[i]->btf || threads[i]->encoder == btf_encoder)
-			continue; /* The primary btf_encoder */
+		if (!threads[i]->btf)
+			continue;
 		err = btf_encoder__add_encoder(btf_encoder, threads[i]->encoder);
 		if (err < 0)
 			goto out;
-		btf_encoder__delete(threads[i]->encoder);
-		threads[i]->encoder = NULL;
 	}
 	err = 0;
 
 out:
 	for (i = 0; i < nr_threads; i++) {
-		if (threads[i]->encoder && threads[i]->encoder != btf_encoder)
+		if (threads[i]->encoder && threads[i]->encoder != btf_encoder) {
 			btf_encoder__delete(threads[i]->encoder);
+			threads[i]->encoder = NULL;
+		}
 	}
 	free(threads[0]);
 
@@ -3077,11 +3085,11 @@ static enum load_steal_kind pahole_stealer(struct cu *cu,
 			encoder = btf_encoder;
 		}
 
-		if (btf_encoder__encode_cu(encoder, cu, conf_load)) {
+		ret = btf_encoder__encode_cu(encoder, cu, conf_load);
+		if (ret < 0) {
 			fprintf(stderr, "Encountered error while encoding BTF.\n");
 			exit(1);
 		}
-		ret = LSK__DELETE;
 out_btf:
 		return ret;
 	}
-- 
2.31.1

Re: [PATCH v3 dwarves 5/8] btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF

[Jiri Olsa]

On Tue, Feb 07, 2023 at 05:14:59PM +0000, Alan Maguire wrote:

SNIP

> +
>  /*
>   * This corresponds to the same macro defined in
>   * include/linux/kallsyms.h
> @@ -818,6 +901,11 @@ static int functions_cmp(const void *_a, const void *_b)
>  	const struct elf_function *a = _a;
>  	const struct elf_function *b = _b;
>  
> +	/* if search key allows prefix match, verify target has matching
> +	 * prefix len and prefix matches.
> +	 */
> +	if (a->prefixlen && a->prefixlen == b->prefixlen)
> +		return strncmp(a->name, b->name, b->prefixlen);
>  	return strcmp(a->name, b->name);
>  }
>  
> @@ -850,14 +938,22 @@ static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *
>  	}
>  
>  	encoder->functions.entries[encoder->functions.cnt].name = name;
> +	if (strchr(name, '.')) {
> +		const char *suffix = strchr(name, '.');
> +
> +		encoder->functions.suffix_cnt++;
> +		encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
> +	}
>  	encoder->functions.entries[encoder->functions.cnt].generated = false;
> +	encoder->functions.entries[encoder->functions.cnt].function = NULL;

should we zero functions.state in here? next patch adds other stuff
like got_parameter_names and parameter_names in it, so looks like it
could actually matter

jirka

>  	encoder->functions.cnt++;
>  	return 0;
>  }
>  
> -static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder, const char *name)
> +static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
> +						       const char *name, size_t prefixlen)
>  {
> -	struct elf_function key = { .name = name };
> +	struct elf_function key = { .name = name, .prefixlen = prefixlen };
>  

SNIP

Re: [PATCH v3 dwarves 5/8] btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF

[Arnaldo Carvalho de Melo]

Em Wed, Feb 08, 2023 at 02:19:20PM +0100, Jiri Olsa escreveu:
> On Tue, Feb 07, 2023 at 05:14:59PM +0000, Alan Maguire wrote:
> 
> SNIP
> 
> > +
> >  /*
> >   * This corresponds to the same macro defined in
> >   * include/linux/kallsyms.h
> > @@ -818,6 +901,11 @@ static int functions_cmp(const void *_a, const void *_b)
> >  	const struct elf_function *a = _a;
> >  	const struct elf_function *b = _b;
> >  
> > +	/* if search key allows prefix match, verify target has matching
> > +	 * prefix len and prefix matches.
> > +	 */
> > +	if (a->prefixlen && a->prefixlen == b->prefixlen)
> > +		return strncmp(a->name, b->name, b->prefixlen);
> >  	return strcmp(a->name, b->name);
> >  }
> >  
> > @@ -850,14 +938,22 @@ static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *
> >  	}
> >  
> >  	encoder->functions.entries[encoder->functions.cnt].name = name;
> > +	if (strchr(name, '.')) {
> > +		const char *suffix = strchr(name, '.');
> > +
> > +		encoder->functions.suffix_cnt++;
> > +		encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
> > +	}
> >  	encoder->functions.entries[encoder->functions.cnt].generated = false;
> > +	encoder->functions.entries[encoder->functions.cnt].function = NULL;
> 
> should we zero functions.state in here? next patch adds other stuff
> like got_parameter_names and parameter_names in it, so looks like it
> could actually matter

Probably, but that can come as a followup patch, right?

I've applied the patches, combining the patches documenting the two new
command line options with the patches where those options are
introduced.

Testing everything now.

Thanks,

- Arnaldo
 
> jirka
> 
> >  	encoder->functions.cnt++;
> >  	return 0;
> >  }
> >  
> > -static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder, const char *name)
> > +static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
> > +						       const char *name, size_t prefixlen)
> >  {
> > -	struct elf_function key = { .name = name };
> > +	struct elf_function key = { .name = name, .prefixlen = prefixlen };
> >  
> 
> SNIP

-- 

- Arnaldo

Re: [PATCH v3 dwarves 5/8] btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF

[Jiri Olsa]

On Wed, Feb 08, 2023 at 11:43:18AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 08, 2023 at 02:19:20PM +0100, Jiri Olsa escreveu:
> > On Tue, Feb 07, 2023 at 05:14:59PM +0000, Alan Maguire wrote:
> > 
> > SNIP
> > 
> > > +
> > >  /*
> > >   * This corresponds to the same macro defined in
> > >   * include/linux/kallsyms.h
> > > @@ -818,6 +901,11 @@ static int functions_cmp(const void *_a, const void *_b)
> > >  	const struct elf_function *a = _a;
> > >  	const struct elf_function *b = _b;
> > >  
> > > +	/* if search key allows prefix match, verify target has matching
> > > +	 * prefix len and prefix matches.
> > > +	 */
> > > +	if (a->prefixlen && a->prefixlen == b->prefixlen)
> > > +		return strncmp(a->name, b->name, b->prefixlen);
> > >  	return strcmp(a->name, b->name);
> > >  }
> > >  
> > > @@ -850,14 +938,22 @@ static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *
> > >  	}
> > >  
> > >  	encoder->functions.entries[encoder->functions.cnt].name = name;
> > > +	if (strchr(name, '.')) {
> > > +		const char *suffix = strchr(name, '.');
> > > +
> > > +		encoder->functions.suffix_cnt++;
> > > +		encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
> > > +	}
> > >  	encoder->functions.entries[encoder->functions.cnt].generated = false;
> > > +	encoder->functions.entries[encoder->functions.cnt].function = NULL;
> > 
> > should we zero functions.state in here? next patch adds other stuff
> > like got_parameter_names and parameter_names in it, so looks like it
> > could actually matter
> 
> Probably, but that can come as a followup patch, right?

sure, if Alan is ok with that

jirka

> 
> I've applied the patches, combining the patches documenting the two new
> command line options with the patches where those options are
> introduced.
> 
> Testing everything now.
> 
> Thanks,
> 
> - Arnaldo
>  
> > jirka
> > 
> > >  	encoder->functions.cnt++;
> > >  	return 0;
> > >  }
> > >  
> > > -static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder, const char *name)
> > > +static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
> > > +						       const char *name, size_t prefixlen)
> > >  {
> > > -	struct elf_function key = { .name = name };
> > > +	struct elf_function key = { .name = name, .prefixlen = prefixlen };
> > >  
> > 
> > SNIP
> 
> -- 
> 
> - Arnaldo

Re: [PATCH v3 dwarves 5/8] btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF

[Alan Maguire]

On 08/02/2023 20:51, Jiri Olsa wrote:
> On Wed, Feb 08, 2023 at 11:43:18AM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Feb 08, 2023 at 02:19:20PM +0100, Jiri Olsa escreveu:
>>> On Tue, Feb 07, 2023 at 05:14:59PM +0000, Alan Maguire wrote:
>>>
>>> SNIP
>>>
>>>> +
>>>>  /*
>>>>   * This corresponds to the same macro defined in
>>>>   * include/linux/kallsyms.h
>>>> @@ -818,6 +901,11 @@ static int functions_cmp(const void *_a, const void *_b)
>>>>  	const struct elf_function *a = _a;
>>>>  	const struct elf_function *b = _b;
>>>>  
>>>> +	/* if search key allows prefix match, verify target has matching
>>>> +	 * prefix len and prefix matches.
>>>> +	 */
>>>> +	if (a->prefixlen && a->prefixlen == b->prefixlen)
>>>> +		return strncmp(a->name, b->name, b->prefixlen);
>>>>  	return strcmp(a->name, b->name);
>>>>  }
>>>>  
>>>> @@ -850,14 +938,22 @@ static int btf_encoder__collect_function(struct btf_encoder *encoder, GElf_Sym *
>>>>  	}
>>>>  
>>>>  	encoder->functions.entries[encoder->functions.cnt].name = name;
>>>> +	if (strchr(name, '.')) {
>>>> +		const char *suffix = strchr(name, '.');
>>>> +
>>>> +		encoder->functions.suffix_cnt++;
>>>> +		encoder->functions.entries[encoder->functions.cnt].prefixlen = suffix - name;
>>>> +	}
>>>>  	encoder->functions.entries[encoder->functions.cnt].generated = false;
>>>> +	encoder->functions.entries[encoder->functions.cnt].function = NULL;
>>>
>>> should we zero functions.state in here? next patch adds other stuff
>>> like got_parameter_names and parameter_names in it, so looks like it
>>> could actually matter
>>
>> Probably, but that can come as a followup patch, right?
> 
> sure, if Alan is ok with that
> 

it's a great catch; I sent:

https://lore.kernel.org/bpf/1675896868-26339-1-git-send-email-alan.maguire@oracle.com/

...as a followup. Thanks!

> jirka
> 
>>
>> I've applied the patches, combining the patches documenting the two new
>> command line options with the patches where those options are
>> introduced.
>>
>> Testing everything now.
>>
>> Thanks,
>>
>> - Arnaldo
>>  
>>> jirka
>>>
>>>>  	encoder->functions.cnt++;
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder, const char *name)
>>>> +static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
>>>> +						       const char *name, size_t prefixlen)
>>>>  {
>>>> -	struct elf_function key = { .name = name };
>>>> +	struct elf_function key = { .name = name, .prefixlen = prefixlen };
>>>>  
>>>
>>> SNIP
>>
>> -- 
>>
>> - Arnaldo

[PATCH v3 dwarves 6/8] btf_encoder: support delaying function addition to check for function prototype inconsistencies

[Alan Maguire]

There are multiple sources of inconsistency that can result in functions
of the same name having multiple prototypes:

- multiple static functions in different CUs share the same name
- static and external functions share the same name

In addition a function may have optimized-out parameters in some
CUs but not others.

Here we attempt to catch such cases by finding inconsistencies across
CUs using the save/compare/merge mechanisms that were previously
introduced to handle optimized-out parameters.

For two instances of a function to be considered consistent:

- number of parameters must match
- parameter names must match

The latter is a less strong method than a full type comparison but
suffices to match functions.

To enable inconsistency checking, the --skip_encoding_btf_inconsistent_proto
option is introduced.

With it, and the --btf_gen_optimized options in place:

- 285 functions are omitted due to inconsistent function prototypes
- 2495 functions are omitted due to optimized-out parameters, of these
  803 are functions with optimization-related suffixes ".isra", etc,
  leaving 1692 other functions without such suffixes.

Below performance effects and variability in encoded BTF are detailed.
It can be seen that due to the approach used - where functions are
marked "generated" on a per-encoder basis, we see quite variable
numbers of multiply-defined functions in the baseline case, some
with inconsistent prototypes.  With --skip_encoding_btf_inconsistent_proto
specified, this variability disappears, at the cost of a longer time
to carry out encoding due to the need to compare representations across
encoders at thread collection time.

Baseline:

Single-threaded:

$ time LLVM_OBJCOPY=objcopy pahole -J vmlinux

real    0m17.534s
user    0m17.019s
sys     0m0.514s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
51529
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
51529

2 threads:

$ time LLVM_OBJCOPY=objcopy pahole -J -j2 vmlinux

real    0m10.942s
user    0m17.309s
sys     0m0.592s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
51798
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
51529

4 threads:

$ time LLVM_OBJCOPY=objcopy pahole -J -j4 vmlinux

real    0m7.890s
user    0m18.067s
sys     0m0.661s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
52028
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
51529

Test:

Single-threaded:

$ time LLVM_OBJCOPY=objcopy pahole -J --skip_encoding_btf_inconsistent_proto --btf_gen_optimized vmlinux

real    0m19.216s
user    0m17.590s
sys     0m1.624s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
50246
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
50246

2 threads:

$ time LLVM_OBJCOPY=objcopy pahole -J -j2 --skip_encoding_btf_inconsistent_proto --btf_gen_optimized vmlinux

real    0m13.147s
user    0m18.179s
sys     0m3.486s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
50246
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
50246

4 threads:

$ time LLVM_OBJCOPY=objcopy pahole -J -j4 --skip_encoding_btf_inconsistent_proto --btf_gen_optimized vmlinux

real    0m11.090s
user    0m19.613s
sys     0m5.895s
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |wc -l
50246
$ bpftool btf dump file vmlinux|grep " FUNC "| awk '{print $3}'|sort |uniq|wc -l
50246

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@chromium.org>
Cc: Kui-Feng Lee <sinquersw@gmail.com>
Cc: Martin KaFai Lau <martin.lau@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Timo Beckers <timo@incline.eu>
Cc: Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org
---
 btf_encoder.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++-----
 dwarves.h     |  2 ++
 pahole.c      |  8 +++++
 3 files changed, 96 insertions(+), 9 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index cb50401..35fb60a 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -33,9 +33,13 @@
 #include <search.h> /* for tsearch(), tfind() and tdestroy() */
 #include <pthread.h>
 
+#define BTF_ENCODER_MAX_PARAMETERS	12
+
 /* state used to do later encoding of saved functions */
 struct btf_encoder_state {
 	uint32_t type_id_off;
+	bool got_parameter_names;
+	const char *parameter_names[BTF_ENCODER_MAX_PARAMETERS];
 };
 
 struct elf_function {
@@ -800,6 +804,66 @@ static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char
 	return id;
 }
 
+static void parameter_names__get(struct ftype *ftype, size_t nr_parameters,
+				 const char **parameter_names)
+{
+	struct parameter *parameter;
+	int i = 0;
+
+	ftype__for_each_parameter(ftype, parameter) {
+		if (i >= nr_parameters)
+			return;
+		parameter_names[i++] = parameter__name(parameter);
+	}
+}
+
+static bool funcs__match(struct btf_encoder *encoder, struct elf_function *func, struct function *f2)
+{
+	const char *parameter_names[BTF_ENCODER_MAX_PARAMETERS];
+	struct function *f1 = func->function;
+	const char *name;
+	int i;
+
+	if (!f1)
+		return false;
+
+	name = function__name(f1);
+
+	if (f1->proto.nr_parms != f2->proto.nr_parms) {
+		if (encoder->verbose)
+			printf("function mismatch for '%s'(%s): %d params != %d params\n",
+			       name, f1->alias ?: name,
+			       f1->proto.nr_parms, f2->proto.nr_parms);
+		return false;
+	}
+	if (f1->proto.nr_parms == 0)
+		return true;
+
+	if (!func->state.got_parameter_names) {
+		parameter_names__get(&f1->proto, BTF_ENCODER_MAX_PARAMETERS,
+				     func->state.parameter_names);
+		func->state.got_parameter_names = true;
+	}
+	parameter_names__get(&f2->proto, BTF_ENCODER_MAX_PARAMETERS, parameter_names);
+	for (i = 0; i < f1->proto.nr_parms && i < BTF_ENCODER_MAX_PARAMETERS; i++) {
+		if (!func->state.parameter_names[i]) {
+			if (!parameter_names[i])
+				continue;
+		} else if (parameter_names[i]) {
+			if (strcmp(func->state.parameter_names[i], parameter_names[i]) == 0)
+				continue;
+		}
+		if (encoder->verbose) {
+			printf("function mismatch for '%s'(%s): parameter #%d '%s' != '%s'\n",
+			       name, f1->alias ?: name, i,
+			       func->state.parameter_names[i] ?: "<null>",
+			       parameter_names[i] ?: "<null>");
+		}
+		return false;
+	}
+	return true;
+}
+
 static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
 {
 	if (func->function) {
@@ -807,12 +871,16 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
 
 		/* If saving and we find an existing entry, we want to merge
 		 * observations across both functions, checking that the
-		 * "seen optimized parameters" status is reflected in the func
-		 * entry. If the entry is new, record encoder state required
+		 * "seen optimized parameters" and "inconsistent prototype"
+		 * status is reflected in the func entry.
+		 * If the entry is new, record encoder state required
 		 * to add the local function later (encoder + type_id_off)
 		 * such that we can add the function later.
 		 */
 		existing->proto.optimized_parms |= fn->proto.optimized_parms;
+		if (!existing->proto.optimized_parms && !existing->proto.inconsistent_proto &&
+		     !funcs__match(encoder, func, fn))
+			existing->proto.inconsistent_proto = 1;
 	} else {
 		func->state.type_id_off = encoder->type_id_off;
 		func->function = fn;
@@ -851,7 +919,8 @@ static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
 	int i;
 
 	for (i = 0; i < encoder->functions.cnt; i++) {
-		struct function *fn = encoder->functions.entries[i].function;
+		struct elf_function *func = &encoder->functions.entries[i];
+		struct function *fn = func->function;
 		struct btf_encoder *other_encoder;
 
 		if (!fn || fn->proto.processed)
@@ -871,18 +940,23 @@ static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
 			if (!other_fn)
 				continue;
 			fn->proto.optimized_parms |= other_fn->proto.optimized_parms;
+			if (other_fn->proto.inconsistent_proto)
+				fn->proto.inconsistent_proto = 1;
+			if (!fn->proto.optimized_parms && !fn->proto.inconsistent_proto &&
+			    !funcs__match(encoder, func, other_fn))
+				fn->proto.inconsistent_proto = 1;
 			other_fn->proto.processed = 1;
 		}
-		if (fn->proto.optimized_parms) {
+		if (fn->proto.optimized_parms || fn->proto.inconsistent_proto) {
 			if (encoder->verbose) {
 				const char *name = function__name(fn);
 
-				printf("skipping addition of '%s'(%s) due to optimized-out parameters\n",
-				       name, fn->alias ?: name);
+				printf("skipping addition of '%s'(%s) due to %s\n",
+				       name, fn->alias ?: name,
+				       fn->proto.optimized_parms ? "optimized-out parameters" :
+								   "multiple inconsistent function prototypes");
 			}
 		} else {
-			struct elf_function *func = &encoder->functions.entries[i];
-
 			encoder->type_id_off = func->state.type_id_off;
 			btf_encoder__add_func(encoder, fn);
 		}
@@ -1759,7 +1833,10 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 			if (func) {
 				if (func->generated)
 					continue;
-				func->generated = true;
+				if (conf_load->skip_encoding_btf_inconsistent_proto)
+					save = true;
+				else
+					func->generated = true;
 			} else if (encoder->functions.suffix_cnt &&
 				   conf_load->btf_gen_optimized) {
 				/* falling back to name.isra.0 match if no exact
diff --git a/dwarves.h b/dwarves.h
index bb2c3bb..c9d2bf9 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -67,6 +67,7 @@ struct conf_load {
 	bool			skip_encoding_btf_type_tag;
 	bool			skip_encoding_btf_enum64;
 	bool			btf_gen_optimized;
+	bool			skip_encoding_btf_inconsistent_proto;
 	uint8_t			hashtable_bits;
 	uint8_t			max_hashtable_bits;
 	uint16_t		kabi_prefix_len;
@@ -834,6 +835,7 @@ struct ftype {
 	uint8_t		 unspec_parms:1; /* just one bit is needed */
 	uint8_t		 optimized_parms:1;
 	uint8_t		 processed:1;
+	uint8_t		 inconsistent_proto:1;
 };
 
 static inline struct ftype *tag__ftype(const struct tag *tag)
diff --git a/pahole.c b/pahole.c
index f48b66d..2992f43 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1222,6 +1222,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = dwarves_print_version;
 #define ARGP_skip_encoding_btf_enum64 337
 #define ARGP_skip_emitting_atomic_typedefs 338
 #define ARGP_btf_gen_optimized  339
+#define ARGP_skip_encoding_btf_inconsistent_proto 340
 
 static const struct argp_option pahole__options[] = {
 	{
@@ -1639,6 +1640,11 @@ static const struct argp_option pahole__options[] = {
 		.key  = ARGP_btf_gen_optimized,
 		.doc  = "Generate BTF for functions with optimization-related suffixes (.isra, .constprop).  BTF will only be generated if a function does not optimize out parameters."
 	},
+	{
+		.name = "skip_encoding_btf_inconsistent_proto",
+		.key = ARGP_skip_encoding_btf_inconsistent_proto,
+		.doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or have optimized-out parameters."
+	},
 	{
 		.name = NULL,
 	}
@@ -1810,6 +1816,8 @@ static error_t pahole__options_parser(int key, char *arg,
 		conf.skip_emitting_atomic_typedefs = true;	break;
 	case ARGP_btf_gen_optimized:
 		conf_load.btf_gen_optimized = true;		break;
+	case ARGP_skip_encoding_btf_inconsistent_proto:
+		conf_load.skip_encoding_btf_inconsistent_proto = true; break;
 	default:
 		return ARGP_ERR_UNKNOWN;
 	}
-- 
2.31.1

[PATCH v3 dwarves 7/8] dwarves: document --btf_gen_optimized option

[Alan Maguire]

Describe the option in the manual page.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@chromium.org>
Cc: Kui-Feng Lee <sinquersw@gmail.com>
Cc: Martin KaFai Lau <martin.lau@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Timo Beckers <timo@incline.eu>
Cc: Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org
---
 man-pages/pahole.1 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
index 7460104..1a85f6d 100644
--- a/man-pages/pahole.1
+++ b/man-pages/pahole.1
@@ -261,6 +261,11 @@ to "/sys/kernel/btf/vmlinux".
 Allow producing BTF_KIND_FLOAT entries in systems where the vmlinux DWARF
 information has float types.
 
+.TP
+.B \-\-btf_gen_optimized
+Generate BTF for functions with optimization-related suffixes (.isra, .constprop).
+BTF will only be generated if a function does not optimize out parameters.
+
 .TP
 .B \-\-btf_gen_all
 Allow using all the BTF features supported by pahole.
-- 
2.31.1

[PATCH v3 dwarves 8/8] dwarves: document --skip_encoding_btf_inconsistent_proto option

[Alan Maguire]

Describe the option in the manual page.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@chromium.org>
Cc: Kui-Feng Lee <sinquersw@gmail.com>
Cc: Martin KaFai Lau <martin.lau@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Timo Beckers <timo@incline.eu>
Cc: Yonghong Song <yhs@fb.com>
Cc: bpf@vger.kernel.org
---
 man-pages/pahole.1 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
index 1a85f6d..bfa20dc 100644
--- a/man-pages/pahole.1
+++ b/man-pages/pahole.1
@@ -229,6 +229,11 @@ Do not encode enum64 in BTF.
 .B \-\-skip_encoding_btf_type_tag
 Do not encode type tags in BTF.
 
+.TP
+.B \-\-skip_encoding_btf_inconsistent_proto
+Do not encode functions with multiple inconsistent prototypes or optimized-out
+parameters.
+
 .TP
 .B \-j, \-\-jobs=N
 Run N jobs in parallel. Defaults to number of online processors + 10% (like
-- 
2.31.1

Re: [PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions

[Jiri Olsa]

On Tue, Feb 07, 2023 at 05:14:54PM +0000, Alan Maguire wrote:

SNIP

> 
> Changes since v2 [2]
> - Arnaldo incorporated some of the suggestions in the v2 thread;
>   these patches are based on those; the relevant changes are
>   noted as committer changes.
> - Patch 1 is unchanged from v2, but the rest of the patches
>   have been updated:
> - Patch 2 separates out the changes to the struct btf_encoder
>   that better support later addition of functions.
> - Patch 3 then is changed insofar as these changes are no
>   longer needed for the function addition refactoring.
> - Patch 4 has a small change; we need to verify that an
>   encoder has actually been added to the encoders list
>   prior to removal
> - Patch 5 changed significantly; when attempting to measure
>   performance the relatively good numbers attained when using
>   delayed function addition were not reproducible.
>   Further analysis revealed that the large number of lookups
>   caused by the presence of the separate function tree was
>   a major cause of performance degradation in the multi
>   threaded case.  So instead of maintaining a separate tree,
>   we use the ELF function list which we already need to look
>   up to match ELF -> DWARF function descriptions to store
>   the function representation.  This has 2 benefits; firstly
>   as mentioned, we already look up the ELF function so no
>   additional lookup is required to save the function.
>   Secondly, the ELF representation is identical for each
>   encoder, so we can index the same function across multiple
>   encoder function arrays - this greatly speeds up the
>   processing of comparing function representations across
>   encoders.  There is still a performance cost in this

awesome.. great we can do it without the extra tree

I wonder we could save some cycles just by memdup-ing the encoder->functions
array for the subsequent encoders, but that's ok for another patch ;-)

thanks,
jirka

Re: [PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions

[Alan Maguire]

On 08/02/2023 13:20, Jiri Olsa wrote:
> On Tue, Feb 07, 2023 at 05:14:54PM +0000, Alan Maguire wrote:
> 
> SNIP
> 
>>
>> Changes since v2 [2]
>> - Arnaldo incorporated some of the suggestions in the v2 thread;
>>   these patches are based on those; the relevant changes are
>>   noted as committer changes.
>> - Patch 1 is unchanged from v2, but the rest of the patches
>>   have been updated:
>> - Patch 2 separates out the changes to the struct btf_encoder
>>   that better support later addition of functions.
>> - Patch 3 then is changed insofar as these changes are no
>>   longer needed for the function addition refactoring.
>> - Patch 4 has a small change; we need to verify that an
>>   encoder has actually been added to the encoders list
>>   prior to removal
>> - Patch 5 changed significantly; when attempting to measure
>>   performance the relatively good numbers attained when using
>>   delayed function addition were not reproducible.
>>   Further analysis revealed that the large number of lookups
>>   caused by the presence of the separate function tree was
>>   a major cause of performance degradation in the multi
>>   threaded case.  So instead of maintaining a separate tree,
>>   we use the ELF function list which we already need to look
>>   up to match ELF -> DWARF function descriptions to store
>>   the function representation.  This has 2 benefits; firstly
>>   as mentioned, we already look up the ELF function so no
>>   additional lookup is required to save the function.
>>   Secondly, the ELF representation is identical for each
>>   encoder, so we can index the same function across multiple
>>   encoder function arrays - this greatly speeds up the
>>   processing of comparing function representations across
>>   encoders.  There is still a performance cost in this
> 
> awesome.. great we can do it without the extra tree
> 
> I wonder we could save some cycles just by memdup-ing the encoder->functions
> array for the subsequent encoders, but that's ok for another patch ;-)
> 

great idea; also provides extra assurance the layout of the
ELF function arrays are identical! I'd started to explore having
ELF info allocated once in main encoder thread and just duped
for other threads; should definitely save some time. thanks!

Alan

> thanks,
> jirka
> 

Re: [PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions

[Arnaldo Carvalho de Melo]

Em Tue, Feb 07, 2023 at 05:14:54PM +0000, Alan Maguire escreveu:
> At optimization level -O2 or higher in gcc, static functions may be
> optimized such that they have suffixes like .isra.0, .constprop.0 etc.
> These represent 
>     
> - constant propagation (.constprop.0);
> - interprocedural scalar replacement of aggregates, removal of
>   unused parameters and replacement of parameters passed by
>   reference by parameters passed by value (.isra.0)

Initial test, without using the new options:

[acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | sort | uniq -c | sort -n | tail
      3 start_show
      3 timeout_show
      3 uuid_show
      4 m_next
      4 parse_options
      4 sk_diag_fill
      4 state_show
      4 state_store
      5 status_show
      6 type_show
[acme@pumpkin ~]$

Now I'll use --skip_encoding_btf_inconsistent_proto and --btf_gen_optimized

- Arnaldo
   
> See [1] for details. 
>     
> Currently BTF encoding does not handle such optimized functions
> that get renamed with a "." suffix such as ".isra.0", ".constprop.0".
> This is safer because such suffixes can often indicate parameters have
> been optimized out.  This series addresses this by matching a
> function to a suffixed version ("foo" matching "foo.isra.0") while
> ensuring that the function signature does not contain optimized-out
> parameters.  Note that if the function is found ("foo") it will
> be preferred, only falling back to "foo.isra.0" if lookup of the
> function fails.  Addition to BTF is skipped if the function has
> optimized-out parameters, since the expected function signature
> will not match. BTF encoding does not include the "."-suffix to
> be consistent with DWARF. In addition, the kernel currently does
> not allow a "." suffix in a BTF function name.
> 
> A problem with this approach however is that BTF carries out the
> encoding process in parallel across multiple CUs, and sometimes
> a function has optimized-out parameters in one CU but not others;
> we see this for NF_HOOK.constprop.0 for example.  So in order to
> determine if the function has optimized-out parameters in any
> CU, its addition is not carried out until we have processed all
> CUs and are about to merge BTF.  At this point we know if any
> such optimizations have occurred.  Patches 1-5 handle the
> optimized-out parameter identification and matching "."-suffixed
> functions with the original function to facilitate BTF
> encoding.  This feature can be enabled via the
> "--btf_gen_optimized" option.
> 
> Patch 6 addresses a related problem - it is entirely possible
> for a static function of the same name to exist in different
> CUs with different function signatures.  Because BTF does not
> currently encode any information that would help disambiguate
> which BTF function specification matches which static function
> (in the case of multiple different function signatures), it is
> best to eliminate such functions from BTF for now.  The same
> mechanism that is used to compare static "."-suffixed functions
> is re-used for the static function comparison.  A superficial
> comparison of number of parameters/parameter names is done to
> see if such representations are consistent, and if inconsistent
> prototypes are observed, the function is flagged for exclusion
> from BTF.
> 
> When these methods are combined - the additive encoding of
> "."-suffixed functions and the subtractive elimination of
> functions with inconsistent parameters - we see an overall
> drop in the number of functions in vmlinux BTF, from
> 51529 to 50246.  Skipping inconsistent functions is enabled
> via "--skip_encoding_btf_inconsistent_proto".
> 
> Changes since v2 [2]
> - Arnaldo incorporated some of the suggestions in the v2 thread;
>   these patches are based on those; the relevant changes are
>   noted as committer changes.
> - Patch 1 is unchanged from v2, but the rest of the patches
>   have been updated:
> - Patch 2 separates out the changes to the struct btf_encoder
>   that better support later addition of functions.
> - Patch 3 then is changed insofar as these changes are no
>   longer needed for the function addition refactoring.
> - Patch 4 has a small change; we need to verify that an
>   encoder has actually been added to the encoders list
>   prior to removal
> - Patch 5 changed significantly; when attempting to measure
>   performance the relatively good numbers attained when using
>   delayed function addition were not reproducible.
>   Further analysis revealed that the large number of lookups
>   caused by the presence of the separate function tree was
>   a major cause of performance degradation in the multi
>   threaded case.  So instead of maintaining a separate tree,
>   we use the ELF function list which we already need to look
>   up to match ELF -> DWARF function descriptions to store
>   the function representation.  This has 2 benefits; firstly
>   as mentioned, we already look up the ELF function so no
>   additional lookup is required to save the function.
>   Secondly, the ELF representation is identical for each
>   encoder, so we can index the same function across multiple
>   encoder function arrays - this greatly speeds up the
>   processing of comparing function representations across
>   encoders.  There is still a performance cost in this
>   approach however; more details are provided in patch 6.
>   An option specific to adding functions with "." suffixes
>   is added "--btf_gen_optimized"
> - Patch 6 builds on patch 5 in applying the save/merge/add
>   approach for all functions using the same mechanisms.
>   In addition the "--skip_encoding_btf_inconsistent_proto"
>   option is introduced.
> - Patches 7/8 document the new options in the pahole manual
>   page.
>   
> Changes since v1 [3]
> 
> - Eduard noted that a DW_AT_const_value attribute can signal
>   an optimized-out parameter, and that the lack of a location
>   attribute signals optimization; ensure we handle those cases
>   also (Eduard, patch 1).
> - Jiri noted we can have inconsistencies between a static
>   and non-static function; apply the comparison process to
>   all functions (Jiri, patch 5)
> - segmentation fault was observed when handling functions with
>   > 10 parameters; needed parameter comparison loop to exit
>   at BTF_ENCODER_MAX_PARAMETERS (patch 5)
> - Kui-Feng Lee pointed out that having a global shared function
>   tree would lead to a lot of contention; here a per-encoder 
>   tree is used, and once the threads are collected the trees
>   are merged. Performance numbers are provided in patch 5 
>   (Kui-Feng Lee, patches 4/5)
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
> [2] https://lore.kernel.org/bpf/1675088985-20300-1-git-send-email-alan.maguire@oracle.com/
> [3] https://lore.kernel.org/bpf/1674567931-26458-1-git-send-email-alan.maguire@oracle.com/
> 
> Alan Maguire (8):
>   dwarf_loader: Help spotting functions with optimized-out parameters
>   btf_encoder: store type_id_off, unspecified type in encoder
>   btf_encoder: Refactor function addition into dedicated
>     btf_encoder__add_func
>   btf_encoder: Rework btf_encoders__*() API to allow traversal of
>     encoders
>   btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF
>   btf_encoder: support delaying function addition to check for function
>     prototype inconsistencies
>   dwarves: document --btf_gen_optimized option
>   dwarves: document --skip_encoding_btf_inconsistent_proto option
> 
>  btf_encoder.c      | 360 +++++++++++++++++++++++++++++++++++++--------
>  btf_encoder.h      |   6 -
>  dwarf_loader.c     | 130 +++++++++++++++-
>  dwarves.h          |  11 +-
>  man-pages/pahole.1 |  10 ++
>  pahole.c           |  30 +++-
>  6 files changed, 468 insertions(+), 79 deletions(-)
> 
> -- 
> 2.31.1
> 

-- 

- Arnaldo

Re: [PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions

[Arnaldo Carvalho de Melo]

Em Wed, Feb 08, 2023 at 01:20:39PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Feb 07, 2023 at 05:14:54PM +0000, Alan Maguire escreveu:
> > At optimization level -O2 or higher in gcc, static functions may be
> > optimized such that they have suffixes like .isra.0, .constprop.0 etc.
> > These represent 
> >     
> > - constant propagation (.constprop.0);
> > - interprocedural scalar replacement of aggregates, removal of
> >   unused parameters and replacement of parameters passed by
> >   reference by parameters passed by value (.isra.0)
> 
> Initial test, without using the new options:
> 
> [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | sort | uniq -c | sort -n | tail
>       3 start_show
>       3 timeout_show
>       3 uuid_show
>       4 m_next
>       4 parse_options
>       4 sk_diag_fill
>       4 state_show
>       4 state_store
>       5 status_show
>       6 type_show
> [acme@pumpkin ~]$
> 
> Now I'll use --skip_encoding_btf_inconsistent_proto and --btf_gen_optimized

With:

⬢[acme@toolbox linux]$ git diff
diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
index 1f1f1d397c399afe..9dd185fb1ff1fc3b 100755
--- a/scripts/pahole-flags.sh
+++ b/scripts/pahole-flags.sh
@@ -21,7 +21,7 @@ if [ "${pahole_ver}" -ge "122" ]; then
 fi
 if [ "${pahole_ver}" -ge "124" ]; then
        # see PAHOLE_HAS_LANG_EXCLUDE
-       extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
+       extra_paholeopt="${extra_paholeopt} --lang_exclude=rust --btf_gen_optimized --skip_encoding_btf_inconsistent_proto"
 fi

 echo ${extra_paholeopt}
⬢[acme@toolbox linux]$

I get:

[acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | sort | uniq -c | sort -n | tail
      1 zswap_writeback_entry
      1 zswap_zpool_param_set
      1 zs_zpool_create
      1 zs_zpool_destroy
      1 zs_zpool_free
      1 zs_zpool_malloc
      1 zs_zpool_map
      1 zs_zpool_shrink
      1 zs_zpool_total_size
      1 zs_zpool_unmap
[acme@pumpkin ~]$

No functions with more than one entry:

[acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | sort | uniq -c | sort -n | grep -v ' 1 '
[acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | sort | uniq -c | sort -n | grep ' 1 ' | wc -l
54558
[acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | wc -l
54558
[acme@pumpkin ~]$

So I'll bump the release as we did in the past when testing features
that we need to test against a release on the pahole-flags.sh script so
that we can do further tests.

- Arnaldo
 
> - Arnaldo
>    
> > See [1] for details. 
> >     
> > Currently BTF encoding does not handle such optimized functions
> > that get renamed with a "." suffix such as ".isra.0", ".constprop.0".
> > This is safer because such suffixes can often indicate parameters have
> > been optimized out.  This series addresses this by matching a
> > function to a suffixed version ("foo" matching "foo.isra.0") while
> > ensuring that the function signature does not contain optimized-out
> > parameters.  Note that if the function is found ("foo") it will
> > be preferred, only falling back to "foo.isra.0" if lookup of the
> > function fails.  Addition to BTF is skipped if the function has
> > optimized-out parameters, since the expected function signature
> > will not match. BTF encoding does not include the "."-suffix to
> > be consistent with DWARF. In addition, the kernel currently does
> > not allow a "." suffix in a BTF function name.
> > 
> > A problem with this approach however is that BTF carries out the
> > encoding process in parallel across multiple CUs, and sometimes
> > a function has optimized-out parameters in one CU but not others;
> > we see this for NF_HOOK.constprop.0 for example.  So in order to
> > determine if the function has optimized-out parameters in any
> > CU, its addition is not carried out until we have processed all
> > CUs and are about to merge BTF.  At this point we know if any
> > such optimizations have occurred.  Patches 1-5 handle the
> > optimized-out parameter identification and matching "."-suffixed
> > functions with the original function to facilitate BTF
> > encoding.  This feature can be enabled via the
> > "--btf_gen_optimized" option.
> > 
> > Patch 6 addresses a related problem - it is entirely possible
> > for a static function of the same name to exist in different
> > CUs with different function signatures.  Because BTF does not
> > currently encode any information that would help disambiguate
> > which BTF function specification matches which static function
> > (in the case of multiple different function signatures), it is
> > best to eliminate such functions from BTF for now.  The same
> > mechanism that is used to compare static "."-suffixed functions
> > is re-used for the static function comparison.  A superficial
> > comparison of number of parameters/parameter names is done to
> > see if such representations are consistent, and if inconsistent
> > prototypes are observed, the function is flagged for exclusion
> > from BTF.
> > 
> > When these methods are combined - the additive encoding of
> > "."-suffixed functions and the subtractive elimination of
> > functions with inconsistent parameters - we see an overall
> > drop in the number of functions in vmlinux BTF, from
> > 51529 to 50246.  Skipping inconsistent functions is enabled
> > via "--skip_encoding_btf_inconsistent_proto".
> > 
> > Changes since v2 [2]
> > - Arnaldo incorporated some of the suggestions in the v2 thread;
> >   these patches are based on those; the relevant changes are
> >   noted as committer changes.
> > - Patch 1 is unchanged from v2, but the rest of the patches
> >   have been updated:
> > - Patch 2 separates out the changes to the struct btf_encoder
> >   that better support later addition of functions.
> > - Patch 3 then is changed insofar as these changes are no
> >   longer needed for the function addition refactoring.
> > - Patch 4 has a small change; we need to verify that an
> >   encoder has actually been added to the encoders list
> >   prior to removal
> > - Patch 5 changed significantly; when attempting to measure
> >   performance the relatively good numbers attained when using
> >   delayed function addition were not reproducible.
> >   Further analysis revealed that the large number of lookups
> >   caused by the presence of the separate function tree was
> >   a major cause of performance degradation in the multi
> >   threaded case.  So instead of maintaining a separate tree,
> >   we use the ELF function list which we already need to look
> >   up to match ELF -> DWARF function descriptions to store
> >   the function representation.  This has 2 benefits; firstly
> >   as mentioned, we already look up the ELF function so no
> >   additional lookup is required to save the function.
> >   Secondly, the ELF representation is identical for each
> >   encoder, so we can index the same function across multiple
> >   encoder function arrays - this greatly speeds up the
> >   processing of comparing function representations across
> >   encoders.  There is still a performance cost in this
> >   approach however; more details are provided in patch 6.
> >   An option specific to adding functions with "." suffixes
> >   is added "--btf_gen_optimized"
> > - Patch 6 builds on patch 5 in applying the save/merge/add
> >   approach for all functions using the same mechanisms.
> >   In addition the "--skip_encoding_btf_inconsistent_proto"
> >   option is introduced.
> > - Patches 7/8 document the new options in the pahole manual
> >   page.
> >   
> > Changes since v1 [3]
> > 
> > - Eduard noted that a DW_AT_const_value attribute can signal
> >   an optimized-out parameter, and that the lack of a location
> >   attribute signals optimization; ensure we handle those cases
> >   also (Eduard, patch 1).
> > - Jiri noted we can have inconsistencies between a static
> >   and non-static function; apply the comparison process to
> >   all functions (Jiri, patch 5)
> > - segmentation fault was observed when handling functions with
> >   > 10 parameters; needed parameter comparison loop to exit
> >   at BTF_ENCODER_MAX_PARAMETERS (patch 5)
> > - Kui-Feng Lee pointed out that having a global shared function
> >   tree would lead to a lot of contention; here a per-encoder 
> >   tree is used, and once the threads are collected the trees
> >   are merged. Performance numbers are provided in patch 5 
> >   (Kui-Feng Lee, patches 4/5)
> > 
> > [1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
> > [2] https://lore.kernel.org/bpf/1675088985-20300-1-git-send-email-alan.maguire@oracle.com/
> > [3] https://lore.kernel.org/bpf/1674567931-26458-1-git-send-email-alan.maguire@oracle.com/
> > 
> > Alan Maguire (8):
> >   dwarf_loader: Help spotting functions with optimized-out parameters
> >   btf_encoder: store type_id_off, unspecified type in encoder
> >   btf_encoder: Refactor function addition into dedicated
> >     btf_encoder__add_func
> >   btf_encoder: Rework btf_encoders__*() API to allow traversal of
> >     encoders
> >   btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF
> >   btf_encoder: support delaying function addition to check for function
> >     prototype inconsistencies
> >   dwarves: document --btf_gen_optimized option
> >   dwarves: document --skip_encoding_btf_inconsistent_proto option
> > 
> >  btf_encoder.c      | 360 +++++++++++++++++++++++++++++++++++++--------
> >  btf_encoder.h      |   6 -
> >  dwarf_loader.c     | 130 +++++++++++++++-
> >  dwarves.h          |  11 +-
> >  man-pages/pahole.1 |  10 ++
> >  pahole.c           |  30 +++-
> >  6 files changed, 468 insertions(+), 79 deletions(-)
> > 
> > -- 
> > 2.31.1
> > 
> 
> -- 
> 
> - Arnaldo

-- 

- Arnaldo

Re: [PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions

[Jiri Olsa]

On Wed, Feb 08, 2023 at 01:50:27PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 08, 2023 at 01:20:39PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Feb 07, 2023 at 05:14:54PM +0000, Alan Maguire escreveu:
> > > At optimization level -O2 or higher in gcc, static functions may be
> > > optimized such that they have suffixes like .isra.0, .constprop.0 etc.
> > > These represent 
> > >     
> > > - constant propagation (.constprop.0);
> > > - interprocedural scalar replacement of aggregates, removal of
> > >   unused parameters and replacement of parameters passed by
> > >   reference by parameters passed by value (.isra.0)
> > 
> > Initial test, without using the new options:
> > 
> > [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | sort | uniq -c | sort -n | tail
> >       3 start_show
> >       3 timeout_show
> >       3 uuid_show
> >       4 m_next
> >       4 parse_options
> >       4 sk_diag_fill
> >       4 state_show
> >       4 state_store
> >       5 status_show
> >       6 type_show
> > [acme@pumpkin ~]$
> > 
> > Now I'll use --skip_encoding_btf_inconsistent_proto and --btf_gen_optimized
> 
> With:
> 
> ⬢[acme@toolbox linux]$ git diff
> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> index 1f1f1d397c399afe..9dd185fb1ff1fc3b 100755
> --- a/scripts/pahole-flags.sh
> +++ b/scripts/pahole-flags.sh
> @@ -21,7 +21,7 @@ if [ "${pahole_ver}" -ge "122" ]; then
>  fi
>  if [ "${pahole_ver}" -ge "124" ]; then
>         # see PAHOLE_HAS_LANG_EXCLUDE
> -       extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> +       extra_paholeopt="${extra_paholeopt} --lang_exclude=rust --btf_gen_optimized --skip_encoding_btf_inconsistent_proto"
>  fi
> 
>  echo ${extra_paholeopt}
> ⬢[acme@toolbox linux]$
> 
> I get:
> 
> [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | sort | uniq -c | sort -n | tail
>       1 zswap_writeback_entry
>       1 zswap_zpool_param_set
>       1 zs_zpool_create
>       1 zs_zpool_destroy
>       1 zs_zpool_free
>       1 zs_zpool_malloc
>       1 zs_zpool_map
>       1 zs_zpool_shrink
>       1 zs_zpool_total_size
>       1 zs_zpool_unmap
> [acme@pumpkin ~]$
> 
> No functions with more than one entry:
> 
> [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | sort | uniq -c | sort -n | grep -v ' 1 '
> [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | sort | uniq -c | sort -n | grep ' 1 ' | wc -l
> 54558
> [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | wc -l
> 54558
> [acme@pumpkin ~]$
> 
> So I'll bump the release as we did in the past when testing features
> that we need to test against a release on the pahole-flags.sh script so
> that we can do further tests.

I did similar test and ran bpf selftests built with new pahole,
all looks good

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

thanks,
jirka

Re: [PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions

[Arnaldo Carvalho de Melo]

Em Thu, Feb 09, 2023 at 10:36:51AM +0100, Jiri Olsa escreveu:
> On Wed, Feb 08, 2023 at 01:50:27PM -0300, Arnaldo Carvalho de Melo wrote:
> > No functions with more than one entry:

> > [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | sort | uniq -c | sort -n | grep -v ' 1 '
> > [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | sort | uniq -c | sort -n | grep ' 1 ' | wc -l
> > 54558
> > [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux  | wc -l
> > 54558
> > [acme@pumpkin ~]$

> > So I'll bump the release as we did in the past when testing features
> > that we need to test against a release on the pahole-flags.sh script so
> > that we can do further tests.

> I did similar test and ran bpf selftests built with new pahole,
> all looks good

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

Thanks, added to the patches in this series.

- Arnaldo

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Arnaldo Carvalho de Melo]

Em Wed, Feb 08, 2023 at 05:17:02PM +0000, Alan Maguire escreveu:
> From: Alan Maguire <alan.maguire@oracle.com>
> Date: Thu, 2 Feb 2023 12:26:20 +0000
> Subject: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto,
>  --btf_gen_optimized to pahole flags for v1.25
> 
> v1.25 of pahole supports filtering out functions with multiple
> inconsistent function prototypes or optimized-out parameters
> from the BTF representation.  These present problems because
> there is no additional info in BTF saying which inconsistent
> prototype matches which function instance to help guide
> attachment, and functions with optimized-out parameters can
> lead to incorrect assumptions about register contents.
> 
> So for now, filter out such functions while adding BTF
> representations for functions that have "."-suffixes
> (foo.isra.0) but not optimized-out parameters.
> 
> This patch assumes changes in [1] land and pahole is bumped
> to v1.25.
> 
> [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  scripts/pahole-flags.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> index 1f1f1d3..728d551 100755
> --- a/scripts/pahole-flags.sh
> +++ b/scripts/pahole-flags.sh
> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>  	# see PAHOLE_HAS_LANG_EXCLUDE
>  	extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>  fi
> +if [ "${pahole_ver}" -ge "125" ]; then
> +	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> +fi
>  
>  echo ${extra_paholeopt}
> -- 
> 1.8.3.1


I applied the patch and it works as advertised using what is in pahole's
'next' branch, that should go to 'master' later today.

Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo

[PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Alan Maguire]

v1.25 of pahole supports filtering out functions with multiple
inconsistent function prototypes or optimized-out parameters
from the BTF representation.  These present problems because
there is no additional info in BTF saying which inconsistent
prototype matches which function instance to help guide
attachment, and functions with optimized-out parameters can
lead to incorrect assumptions about register contents.

So for now, filter out such functions while adding BTF
representations for functions that have "."-suffixes
(foo.isra.0) but not optimized-out parameters.

This patch assumes changes in [1] land and pahole is bumped
to v1.25.

[1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>

---
 scripts/pahole-flags.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
index 1f1f1d3..728d551 100755
--- a/scripts/pahole-flags.sh
+++ b/scripts/pahole-flags.sh
@@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
 	# see PAHOLE_HAS_LANG_EXCLUDE
 	extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
 fi
+if [ "${pahole_ver}" -ge "125" ]; then
+	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
+fi
 
 echo ${extra_paholeopt}
-- 
1.8.3.1

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Jiri Olsa]

On Thu, Feb 09, 2023 at 01:28:51PM +0000, Alan Maguire wrote:
> v1.25 of pahole supports filtering out functions with multiple
> inconsistent function prototypes or optimized-out parameters
> from the BTF representation.  These present problems because
> there is no additional info in BTF saying which inconsistent
> prototype matches which function instance to help guide
> attachment, and functions with optimized-out parameters can
> lead to incorrect assumptions about register contents.
> 
> So for now, filter out such functions while adding BTF
> representations for functions that have "."-suffixes
> (foo.isra.0) but not optimized-out parameters.
> 
> This patch assumes changes in [1] land and pahole is bumped
> to v1.25.
> 
> [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>

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

jirka

> 
> ---
>  scripts/pahole-flags.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> index 1f1f1d3..728d551 100755
> --- a/scripts/pahole-flags.sh
> +++ b/scripts/pahole-flags.sh
> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>  	# see PAHOLE_HAS_LANG_EXCLUDE
>  	extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>  fi
> +if [ "${pahole_ver}" -ge "125" ]; then
> +	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> +fi
>  
>  echo ${extra_paholeopt}
> -- 
> 1.8.3.1
> 

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Thu,  9 Feb 2023 13:28:51 +0000 you wrote:
> v1.25 of pahole supports filtering out functions with multiple
> inconsistent function prototypes or optimized-out parameters
> from the BTF representation.  These present problems because
> there is no additional info in BTF saying which inconsistent
> prototype matches which function instance to help guide
> attachment, and functions with optimized-out parameters can
> lead to incorrect assumptions about register contents.
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25
    https://git.kernel.org/bpf/bpf-next/c/0243d3dfe274

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Alexei Starovoitov]

On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> v1.25 of pahole supports filtering out functions with multiple
> inconsistent function prototypes or optimized-out parameters
> from the BTF representation.  These present problems because
> there is no additional info in BTF saying which inconsistent
> prototype matches which function instance to help guide
> attachment, and functions with optimized-out parameters can
> lead to incorrect assumptions about register contents.
>
> So for now, filter out such functions while adding BTF
> representations for functions that have "."-suffixes
> (foo.isra.0) but not optimized-out parameters.
>
> This patch assumes changes in [1] land and pahole is bumped
> to v1.25.
>
> [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> ---
>  scripts/pahole-flags.sh | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> index 1f1f1d3..728d551 100755
> --- a/scripts/pahole-flags.sh
> +++ b/scripts/pahole-flags.sh
> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>         # see PAHOLE_HAS_LANG_EXCLUDE
>         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>  fi
> +if [ "${pahole_ver}" -ge "125" ]; then
> +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> +fi

We landed this too soon.
#229     tracing_struct:FAIL
is failing now.
since bpf_testmod.ko is missing a bunch of functions though they're global.

I've tried a bunch of different flags and attributes, but none of them
helped.
The only thing that works is:
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 46500636d8cd..5fd0f75d5d20 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 {
        long b;
 };

+__attribute__((optimize("-O0")))
 noinline int
 bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int
b, int c) {

We cannot do:
--- a/tools/testing/selftests/bpf/bpf_testmod/Makefile
+++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile
@@ -10,7 +10,7 @@ endif
 MODULES = bpf_testmod.ko

 obj-m += bpf_testmod.o
-CFLAGS_bpf_testmod.o = -I$(src)
+CFLAGS_bpf_testmod.o = -I$(src) -O0

The build fails due to asm stuff.

Maybe we should make scripts/pahole-flags.sh selective
and don't apply skip_encoding_btf_inconsiste to bpf_testmod ?

Thoughts?

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Alexei Starovoitov]

On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > v1.25 of pahole supports filtering out functions with multiple
> > inconsistent function prototypes or optimized-out parameters
> > from the BTF representation.  These present problems because
> > there is no additional info in BTF saying which inconsistent
> > prototype matches which function instance to help guide
> > attachment, and functions with optimized-out parameters can
> > lead to incorrect assumptions about register contents.
> >
> > So for now, filter out such functions while adding BTF
> > representations for functions that have "."-suffixes
> > (foo.isra.0) but not optimized-out parameters.
> >
> > This patch assumes changes in [1] land and pahole is bumped
> > to v1.25.
> >
> > [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
> >
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> >
> > ---
> >  scripts/pahole-flags.sh | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> > index 1f1f1d3..728d551 100755
> > --- a/scripts/pahole-flags.sh
> > +++ b/scripts/pahole-flags.sh
> > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
> >         # see PAHOLE_HAS_LANG_EXCLUDE
> >         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> >  fi
> > +if [ "${pahole_ver}" -ge "125" ]; then
> > +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> > +fi
>
> We landed this too soon.
> #229     tracing_struct:FAIL
> is failing now.
> since bpf_testmod.ko is missing a bunch of functions though they're global.
>
> I've tried a bunch of different flags and attributes, but none of them
> helped.
> The only thing that works is:
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 46500636d8cd..5fd0f75d5d20 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 {
>         long b;
>  };
>
> +__attribute__((optimize("-O0")))
>  noinline int
>  bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int
> b, int c) {
>
> We cannot do:
> --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile
> +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile
> @@ -10,7 +10,7 @@ endif
>  MODULES = bpf_testmod.ko
>
>  obj-m += bpf_testmod.o
> -CFLAGS_bpf_testmod.o = -I$(src)
> +CFLAGS_bpf_testmod.o = -I$(src) -O0
>
> The build fails due to asm stuff.
>
> Maybe we should make scripts/pahole-flags.sh selective
> and don't apply skip_encoding_btf_inconsiste to bpf_testmod ?
>
> Thoughts?

It's even worse with clang compiled kernel:
    WARN: resolve_btfids: unresolved symbol tcp_reno_cong_avoid
    WARN: resolve_btfids: unresolved symbol dctcp_update_alpha
    WARN: resolve_btfids: unresolved symbol cubictcp_cong_avoid
    WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_timestamp
    WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
    WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
    WARN: resolve_btfids: unresolved symbol bpf_task_acquire_not_zero
    WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast
    WARN: resolve_btfids: unresolved symbol
bpf_kfunc_call_test_static_unused_arg
    WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref

so I reverted this commit for now.
Looks like pahole with skip_encoding_btf_inconsistent_proto needs
to be more accurate.
It's way too aggressive removing valid functions.

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Arnaldo Carvalho de Melo]

Em Mon, Feb 13, 2023 at 10:09:21PM -0800, Alexei Starovoitov escreveu:
> On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > +++ b/scripts/pahole-flags.sh
> > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
> > >         # see PAHOLE_HAS_LANG_EXCLUDE
> > >         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> > >  fi
> > > +if [ "${pahole_ver}" -ge "125" ]; then
> > > +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> > > +fi
> >
> > We landed this too soon.
> > #229     tracing_struct:FAIL
> > is failing now.
> > since bpf_testmod.ko is missing a bunch of functions though they're global.
> >
> > I've tried a bunch of different flags and attributes, but none of them
> > helped.
> > The only thing that works is:
> > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > index 46500636d8cd..5fd0f75d5d20 100644
> > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 {
> >         long b;
> >  };
> >
> > +__attribute__((optimize("-O0")))
> >  noinline int
> >  bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int
> > b, int c) {
> >
> > We cannot do:
> > --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile
> > @@ -10,7 +10,7 @@ endif
> >  MODULES = bpf_testmod.ko
> >
> >  obj-m += bpf_testmod.o
> > -CFLAGS_bpf_testmod.o = -I$(src)
> > +CFLAGS_bpf_testmod.o = -I$(src) -O0
> >
> > The build fails due to asm stuff.
> >
> > Maybe we should make scripts/pahole-flags.sh selective
> > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ?
> >
> > Thoughts?
> 
> It's even worse with clang compiled kernel:

I tested what is now in the master branch with both gcc and clang, on
fedora:37, Alan also tested it, Jiri, it would be great if you could
check if reverting the revert works for you as well.

Thanks,

- Arnaldo

>     WARN: resolve_btfids: unresolved symbol tcp_reno_cong_avoid
>     WARN: resolve_btfids: unresolved symbol dctcp_update_alpha
>     WARN: resolve_btfids: unresolved symbol cubictcp_cong_avoid
>     WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_timestamp
>     WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
>     WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
>     WARN: resolve_btfids: unresolved symbol bpf_task_acquire_not_zero
>     WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast
>     WARN: resolve_btfids: unresolved symbol
> bpf_kfunc_call_test_static_unused_arg
>     WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref
> 
> so I reverted this commit for now.
> Looks like pahole with skip_encoding_btf_inconsistent_proto needs
> to be more accurate.
> It's way too aggressive removing valid functions.

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Jiri Olsa]

On Wed, Mar 08, 2023 at 10:53:51PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Feb 13, 2023 at 10:09:21PM -0800, Alexei Starovoitov escreveu:
> > On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > > +++ b/scripts/pahole-flags.sh
> > > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
> > > >         # see PAHOLE_HAS_LANG_EXCLUDE
> > > >         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> > > >  fi
> > > > +if [ "${pahole_ver}" -ge "125" ]; then
> > > > +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> > > > +fi
> > >
> > > We landed this too soon.
> > > #229     tracing_struct:FAIL
> > > is failing now.
> > > since bpf_testmod.ko is missing a bunch of functions though they're global.
> > >
> > > I've tried a bunch of different flags and attributes, but none of them
> > > helped.
> > > The only thing that works is:
> > > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > > index 46500636d8cd..5fd0f75d5d20 100644
> > > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > > @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 {
> > >         long b;
> > >  };
> > >
> > > +__attribute__((optimize("-O0")))
> > >  noinline int
> > >  bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int
> > > b, int c) {
> > >
> > > We cannot do:
> > > --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile
> > > +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile
> > > @@ -10,7 +10,7 @@ endif
> > >  MODULES = bpf_testmod.ko
> > >
> > >  obj-m += bpf_testmod.o
> > > -CFLAGS_bpf_testmod.o = -I$(src)
> > > +CFLAGS_bpf_testmod.o = -I$(src) -O0
> > >
> > > The build fails due to asm stuff.
> > >
> > > Maybe we should make scripts/pahole-flags.sh selective
> > > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ?
> > >
> > > Thoughts?
> > 
> > It's even worse with clang compiled kernel:
> 
> I tested what is now in the master branch with both gcc and clang, on
> fedora:37, Alan also tested it, Jiri, it would be great if you could
> check if reverting the revert works for you as well.

ok, will check your master branch

jirka

> 
> Thanks,
> 
> - Arnaldo
> 
> >     WARN: resolve_btfids: unresolved symbol tcp_reno_cong_avoid
> >     WARN: resolve_btfids: unresolved symbol dctcp_update_alpha
> >     WARN: resolve_btfids: unresolved symbol cubictcp_cong_avoid
> >     WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_timestamp
> >     WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> >     WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> >     WARN: resolve_btfids: unresolved symbol bpf_task_acquire_not_zero
> >     WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast
> >     WARN: resolve_btfids: unresolved symbol
> > bpf_kfunc_call_test_static_unused_arg
> >     WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref
> > 
> > so I reverted this commit for now.
> > Looks like pahole with skip_encoding_btf_inconsistent_proto needs
> > to be more accurate.
> > It's way too aggressive removing valid functions.

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Jiri Olsa]

On Thu, Mar 09, 2023 at 09:16:53AM +0100, Jiri Olsa wrote:
> On Wed, Mar 08, 2023 at 10:53:51PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Feb 13, 2023 at 10:09:21PM -0800, Alexei Starovoitov escreveu:
> > > On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > > > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > > > +++ b/scripts/pahole-flags.sh
> > > > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
> > > > >         # see PAHOLE_HAS_LANG_EXCLUDE
> > > > >         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> > > > >  fi
> > > > > +if [ "${pahole_ver}" -ge "125" ]; then
> > > > > +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> > > > > +fi
> > > >
> > > > We landed this too soon.
> > > > #229     tracing_struct:FAIL
> > > > is failing now.
> > > > since bpf_testmod.ko is missing a bunch of functions though they're global.
> > > >
> > > > I've tried a bunch of different flags and attributes, but none of them
> > > > helped.
> > > > The only thing that works is:
> > > > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > > > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > > > index 46500636d8cd..5fd0f75d5d20 100644
> > > > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > > > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > > > @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 {
> > > >         long b;
> > > >  };
> > > >
> > > > +__attribute__((optimize("-O0")))
> > > >  noinline int
> > > >  bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int
> > > > b, int c) {
> > > >
> > > > We cannot do:
> > > > --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile
> > > > +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile
> > > > @@ -10,7 +10,7 @@ endif
> > > >  MODULES = bpf_testmod.ko
> > > >
> > > >  obj-m += bpf_testmod.o
> > > > -CFLAGS_bpf_testmod.o = -I$(src)
> > > > +CFLAGS_bpf_testmod.o = -I$(src) -O0
> > > >
> > > > The build fails due to asm stuff.
> > > >
> > > > Maybe we should make scripts/pahole-flags.sh selective
> > > > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ?
> > > >
> > > > Thoughts?
> > > 
> > > It's even worse with clang compiled kernel:
> > 
> > I tested what is now in the master branch with both gcc and clang, on
> > fedora:37, Alan also tested it, Jiri, it would be great if you could
> > check if reverting the revert works for you as well.
> 
> ok, will check your master branch

looks good.. got no duplicates and passing bpf tests for both
gcc and clang setups

jirka

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Arnaldo Carvalho de Melo]

Em Thu, Mar 09, 2023 at 11:11:27AM +0100, Jiri Olsa escreveu:
> On Thu, Mar 09, 2023 at 09:16:53AM +0100, Jiri Olsa wrote:
> > On Wed, Mar 08, 2023 at 10:53:51PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Feb 13, 2023 at 10:09:21PM -0800, Alexei Starovoitov escreveu:
> > > > On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > > > > Maybe we should make scripts/pahole-flags.sh selective
> > > > > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ?

> > > > > Thoughts?

> > > > It's even worse with clang compiled kernel:

> > > I tested what is now in the master branch with both gcc and clang, on
> > > fedora:37, Alan also tested it, Jiri, it would be great if you could
> > > check if reverting the revert works for you as well.

> > ok, will check your master branch

> looks good.. got no duplicates and passing bpf tests for both
> gcc and clang setups

Thanks for testing!

Alexei, since you hit those problems, please consider redoing those
tests in your environment so that we triple check all this.

Thanks,

- Arnaldo

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Alan Maguire]

On 09/03/2023 10:11, Jiri Olsa wrote:
> On Thu, Mar 09, 2023 at 09:16:53AM +0100, Jiri Olsa wrote:
>> On Wed, Mar 08, 2023 at 10:53:51PM -0300, Arnaldo Carvalho de Melo wrote:
>>> Em Mon, Feb 13, 2023 at 10:09:21PM -0800, Alexei Starovoitov escreveu:
>>>> On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>>>> On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>>>>> +++ b/scripts/pahole-flags.sh
>>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>>>>>>         # see PAHOLE_HAS_LANG_EXCLUDE
>>>>>>         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>>>>>>  fi
>>>>>> +if [ "${pahole_ver}" -ge "125" ]; then
>>>>>> +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
>>>>>> +fi
>>>>>
>>>>> We landed this too soon.
>>>>> #229     tracing_struct:FAIL
>>>>> is failing now.
>>>>> since bpf_testmod.ko is missing a bunch of functions though they're global.
>>>>>
>>>>> I've tried a bunch of different flags and attributes, but none of them
>>>>> helped.
>>>>> The only thing that works is:
>>>>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>>>>> b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>>>>> index 46500636d8cd..5fd0f75d5d20 100644
>>>>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>>>>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>>>>> @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 {
>>>>>         long b;
>>>>>  };
>>>>>
>>>>> +__attribute__((optimize("-O0")))
>>>>>  noinline int
>>>>>  bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int
>>>>> b, int c) {
>>>>>
>>>>> We cannot do:
>>>>> --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile
>>>>> +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile
>>>>> @@ -10,7 +10,7 @@ endif
>>>>>  MODULES = bpf_testmod.ko
>>>>>
>>>>>  obj-m += bpf_testmod.o
>>>>> -CFLAGS_bpf_testmod.o = -I$(src)
>>>>> +CFLAGS_bpf_testmod.o = -I$(src) -O0
>>>>>
>>>>> The build fails due to asm stuff.
>>>>>
>>>>> Maybe we should make scripts/pahole-flags.sh selective
>>>>> and don't apply skip_encoding_btf_inconsiste to bpf_testmod ?
>>>>>
>>>>> Thoughts?
>>>>
>>>> It's even worse with clang compiled kernel:
>>>
>>> I tested what is now in the master branch with both gcc and clang, on
>>> fedora:37, Alan also tested it, Jiri, it would be great if you could
>>> check if reverting the revert works for you as well.
>>
>> ok, will check your master branch
> 
> looks good.. got no duplicates and passing bpf tests for both
> gcc and clang setups
>

thanks for re-testing! I just did the same for latest bpf-next
on x86_64/aarch64; all looks good.

Alan

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Jiri Olsa]

On Mon, Feb 13, 2023 at 07:12:33PM -0800, Alexei Starovoitov wrote:
> On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > v1.25 of pahole supports filtering out functions with multiple
> > inconsistent function prototypes or optimized-out parameters
> > from the BTF representation.  These present problems because
> > there is no additional info in BTF saying which inconsistent
> > prototype matches which function instance to help guide
> > attachment, and functions with optimized-out parameters can
> > lead to incorrect assumptions about register contents.
> >
> > So for now, filter out such functions while adding BTF
> > representations for functions that have "."-suffixes
> > (foo.isra.0) but not optimized-out parameters.
> >
> > This patch assumes changes in [1] land and pahole is bumped
> > to v1.25.
> >
> > [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
> >
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> >
> > ---
> >  scripts/pahole-flags.sh | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> > index 1f1f1d3..728d551 100755
> > --- a/scripts/pahole-flags.sh
> > +++ b/scripts/pahole-flags.sh
> > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
> >         # see PAHOLE_HAS_LANG_EXCLUDE
> >         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> >  fi
> > +if [ "${pahole_ver}" -ge "125" ]; then
> > +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> > +fi
> 
> We landed this too soon.
> #229     tracing_struct:FAIL
> is failing now.
> since bpf_testmod.ko is missing a bunch of functions though they're global.
> 

hum, didn't see this one failing.. I'll try that again

jirka

> I've tried a bunch of different flags and attributes, but none of them
> helped.
> The only thing that works is:
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 46500636d8cd..5fd0f75d5d20 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 {
>         long b;
>  };
> 
> +__attribute__((optimize("-O0")))
>  noinline int
>  bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int
> b, int c) {
> 
> We cannot do:
> --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile
> +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile
> @@ -10,7 +10,7 @@ endif
>  MODULES = bpf_testmod.ko
> 
>  obj-m += bpf_testmod.o
> -CFLAGS_bpf_testmod.o = -I$(src)
> +CFLAGS_bpf_testmod.o = -I$(src) -O0
> 
> The build fails due to asm stuff.
> 
> Maybe we should make scripts/pahole-flags.sh selective
> and don't apply skip_encoding_btf_inconsiste to bpf_testmod ?
> 
> Thoughts?

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Arnaldo Carvalho de Melo]

Em Tue, Feb 14, 2023 at 01:27:43PM +0100, Jiri Olsa escreveu:
> On Mon, Feb 13, 2023 at 07:12:33PM -0800, Alexei Starovoitov wrote:
> > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > >
> > > v1.25 of pahole supports filtering out functions with multiple
> > > inconsistent function prototypes or optimized-out parameters
> > > from the BTF representation.  These present problems because
> > > there is no additional info in BTF saying which inconsistent
> > > prototype matches which function instance to help guide
> > > attachment, and functions with optimized-out parameters can
> > > lead to incorrect assumptions about register contents.
> > >
> > > So for now, filter out such functions while adding BTF
> > > representations for functions that have "."-suffixes
> > > (foo.isra.0) but not optimized-out parameters.
> > >
> > > This patch assumes changes in [1] land and pahole is bumped
> > > to v1.25.
> > >
> > > [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
> > >
> > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > >
> > > ---
> > >  scripts/pahole-flags.sh | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> > > index 1f1f1d3..728d551 100755
> > > --- a/scripts/pahole-flags.sh
> > > +++ b/scripts/pahole-flags.sh
> > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
> > >         # see PAHOLE_HAS_LANG_EXCLUDE
> > >         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> > >  fi
> > > +if [ "${pahole_ver}" -ge "125" ]; then
> > > +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> > > +fi
> > 
> > We landed this too soon.
> > #229     tracing_struct:FAIL
> > is failing now.
> > since bpf_testmod.ko is missing a bunch of functions though they're global.
> > 
> 
> hum, didn't see this one failing.. I'll try that again

/me too, redoing tests her, with gcc and clang, running selftests on a
system booted with a kernel built with pahole 1.25, etc.

- Arnaldo
 
> jirka
> 
> > I've tried a bunch of different flags and attributes, but none of them
> > helped.
> > The only thing that works is:
> > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > index 46500636d8cd..5fd0f75d5d20 100644
> > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 {
> >         long b;
> >  };
> > 
> > +__attribute__((optimize("-O0")))
> >  noinline int
> >  bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int
> > b, int c) {
> > 
> > We cannot do:
> > --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile
> > @@ -10,7 +10,7 @@ endif
> >  MODULES = bpf_testmod.ko
> > 
> >  obj-m += bpf_testmod.o
> > -CFLAGS_bpf_testmod.o = -I$(src)
> > +CFLAGS_bpf_testmod.o = -I$(src) -O0
> > 
> > The build fails due to asm stuff.
> > 
> > Maybe we should make scripts/pahole-flags.sh selective
> > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ?
> > 
> > Thoughts?

-- 

- Arnaldo

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Jiri Olsa]

On Tue, Feb 14, 2023 at 01:17:56PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Feb 14, 2023 at 01:27:43PM +0100, Jiri Olsa escreveu:
> > On Mon, Feb 13, 2023 at 07:12:33PM -0800, Alexei Starovoitov wrote:
> > > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > >
> > > > v1.25 of pahole supports filtering out functions with multiple
> > > > inconsistent function prototypes or optimized-out parameters
> > > > from the BTF representation.  These present problems because
> > > > there is no additional info in BTF saying which inconsistent
> > > > prototype matches which function instance to help guide
> > > > attachment, and functions with optimized-out parameters can
> > > > lead to incorrect assumptions about register contents.
> > > >
> > > > So for now, filter out such functions while adding BTF
> > > > representations for functions that have "."-suffixes
> > > > (foo.isra.0) but not optimized-out parameters.
> > > >
> > > > This patch assumes changes in [1] land and pahole is bumped
> > > > to v1.25.
> > > >
> > > > [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
> > > >
> > > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > > > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > >
> > > > ---
> > > >  scripts/pahole-flags.sh | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> > > > index 1f1f1d3..728d551 100755
> > > > --- a/scripts/pahole-flags.sh
> > > > +++ b/scripts/pahole-flags.sh
> > > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
> > > >         # see PAHOLE_HAS_LANG_EXCLUDE
> > > >         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> > > >  fi
> > > > +if [ "${pahole_ver}" -ge "125" ]; then
> > > > +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> > > > +fi
> > > 
> > > We landed this too soon.
> > > #229     tracing_struct:FAIL
> > > is failing now.
> > > since bpf_testmod.ko is missing a bunch of functions though they're global.
> > > 
> > 
> > hum, didn't see this one failing.. I'll try that again
> 
> /me too, redoing tests her, with gcc and clang, running selftests on a
> system booted with a kernel built with pahole 1.25, etc.

ok, can't see that with gcc, but reproduced with clang 16

resolve_btfids complains because those functions are not in btf

  BTFIDS  vmlinux
WARN: resolve_btfids: unresolved symbol tcp_reno_cong_avoid
WARN: resolve_btfids: unresolved symbol should_failslab
WARN: resolve_btfids: unresolved symbol should_fail_alloc_page
WARN: resolve_btfids: unresolved symbol cubictcp_cong_avoid
WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_timestamp
WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
WARN: resolve_btfids: unresolved symbol bpf_task_acquire_not_zero
WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_static_unused_arg
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass_ctx
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass2
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass1
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_pass1
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail2
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail1
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_kptr_get
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail3
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail2
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail1
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_acquire
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test2
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test1
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb_release
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb1_release
WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_int_mem_release
  NM      System.map

jirka

Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Alan Maguire]

On 14/02/2023 22:12, Jiri Olsa wrote:
> On Tue, Feb 14, 2023 at 01:17:56PM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Tue, Feb 14, 2023 at 01:27:43PM +0100, Jiri Olsa escreveu:
>>> On Mon, Feb 13, 2023 at 07:12:33PM -0800, Alexei Starovoitov wrote:
>>>> On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>>>>
>>>>> v1.25 of pahole supports filtering out functions with multiple
>>>>> inconsistent function prototypes or optimized-out parameters
>>>>> from the BTF representation.  These present problems because
>>>>> there is no additional info in BTF saying which inconsistent
>>>>> prototype matches which function instance to help guide
>>>>> attachment, and functions with optimized-out parameters can
>>>>> lead to incorrect assumptions about register contents.
>>>>>
>>>>> So for now, filter out such functions while adding BTF
>>>>> representations for functions that have "."-suffixes
>>>>> (foo.isra.0) but not optimized-out parameters.
>>>>>
>>>>> This patch assumes changes in [1] land and pahole is bumped
>>>>> to v1.25.
>>>>>
>>>>> [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
>>>>>
>>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>>>> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>>>>>
>>>>> ---
>>>>>  scripts/pahole-flags.sh | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
>>>>> index 1f1f1d3..728d551 100755
>>>>> --- a/scripts/pahole-flags.sh
>>>>> +++ b/scripts/pahole-flags.sh
>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>>>>>         # see PAHOLE_HAS_LANG_EXCLUDE
>>>>>         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>>>>>  fi
>>>>> +if [ "${pahole_ver}" -ge "125" ]; then
>>>>> +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
>>>>> +fi
>>>>
>>>> We landed this too soon.
>>>> #229     tracing_struct:FAIL
>>>> is failing now.
>>>> since bpf_testmod.ko is missing a bunch of functions though they're global.
>>>>
>>>
>>> hum, didn't see this one failing.. I'll try that again
>>
>> /me too, redoing tests her, with gcc and clang, running selftests on a
>> system booted with a kernel built with pahole 1.25, etc.
> 
> ok, can't see that with gcc, but reproduced with clang 16
> 
> resolve_btfids complains because those functions are not in btf
> 
>   BTFIDS  vmlinux
> WARN: resolve_btfids: unresolved symbol tcp_reno_cong_avoid
> WARN: resolve_btfids: unresolved symbol should_failslab
> WARN: resolve_btfids: unresolved symbol should_fail_alloc_page
> WARN: resolve_btfids: unresolved symbol cubictcp_cong_avoid
> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_timestamp
> WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash
> WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get
> WARN: resolve_btfids: unresolved symbol bpf_task_acquire_not_zero
> WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_static_unused_arg
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass_ctx
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass2
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass1
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_pass1
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail2
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail1
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_kptr_get
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail3
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail2
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail1
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_acquire
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test2
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test1
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb_release
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb1_release
> WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_int_mem_release
>   NM      System.map
>

Jiri kindly provided a clang-generated vmlinux, and I also managed to reproduce
issues by building the kernel with clang 17.

The first question is if we're detecting optimizations correctly. From
an initial look, I _think_ we are, in some cases at least.

For tcp_reno_cong_avoid(), the function signature is

__bpf_kfunc void tcp_reno_cong_avoid(struct sock *sk, u32 ack, u32 acked)

...but our handling of the DWARF generated spots that the "ack" parameter
has no location info; and looking at the source it is never used.  Here is 
the DWARF - note no location info for the second parameter ("ack"):

0x0891dab0:   DW_TAG_subprogram
                DW_AT_low_pc    (0xffffffff82031180)
                DW_AT_high_pc   (0xffffffff820311d8)
                DW_AT_frame_base        (DW_OP_reg7 RSP)
                DW_AT_call_all_calls    (true)
                DW_AT_name      ("tcp_reno_cong_avoid")
                DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c")
                DW_AT_decl_line (446)
                DW_AT_prototyped        (true)
                DW_AT_external  (true)

0x0891dabe:     DW_TAG_formal_parameter
                  DW_AT_location        (indexed (0x7a) loclist = 0x00f50eb1:
                     [0xffffffff82031185, 0xffffffff8203119e): DW_OP_reg5 RDI
                     [0xffffffff8203119e, 0xffffffff820311cc): DW_OP_reg3 RBX
                     [0xffffffff820311cc, 0xffffffff820311d1): DW_OP_reg5 RDI
                     [0xffffffff820311d1, 0xffffffff820311d2): DW_OP_reg3 RBX
                     [0xffffffff820311d2, 0xffffffff820311d8): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)
                  DW_AT_name    ("sk")
                  DW_AT_decl_file       ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c")
                  DW_AT_decl_line       (446)
                  DW_AT_type    (0x08902c4d "sock *")

0x0891dac9:     DW_TAG_formal_parameter
                  DW_AT_name    ("ack")
                  DW_AT_decl_file       ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c")
                  DW_AT_decl_line       (446)
                  DW_AT_type    (0x08902c3d "u32")

0x0891dad4:     DW_TAG_formal_parameter
                  DW_AT_location        (indexed (0x7b) loclist = 0x00f50eda:
                     [0xffffffff82031185, 0xffffffff820311bc): DW_OP_reg1 RDX
                     [0xffffffff820311bc, 0xffffffff820311c8): DW_OP_reg0 RAX
                     [0xffffffff820311c8, 0xffffffff820311d1): DW_OP_reg1 RDX)
                  DW_AT_name    ("acked")
                  DW_AT_decl_file       ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c")
                  DW_AT_decl_line       (446)
                  DW_AT_type    (0x08902c3d "u32")

Disassembling we see the following:

(gdb) disassemble/s tcp_reno_cong_avoid
Dump of assembler code for function tcp_reno_cong_avoid:
net/ipv4/tcp_cong.c:
447	{
   0xffffffff82031180 <+0>:	nopl   0x0(%rax,%rax,1)
   0xffffffff82031185 <+5>:	push   %rbx
   0xffffffff82031186 <+6>:	mov    %rdi,%rbx

./include/net/tcp.h:
1305		if (tp->is_cwnd_limited)
   0xffffffff82031189 <+9>:	cmpb   $0x0,0x95f(%rdi)
   0xffffffff82031190 <+16>:	mov    0x9e4(%rdi),%eax
   0xffffffff82031196 <+22>:	mov    0x9e8(%rdi),%esi
   0xffffffff8203119c <+28>:	js     0xffffffff820311ae <tcp_reno_cong_avoid+46>

1238		return tcp_snd_cwnd(tp) < tp->snd_ssthresh;
   0xffffffff8203119e <+30>:	cmp    %eax,%esi

1306			return true;
1307	
1308		/* If in slow start, ensure cwnd grows to twice what was ACKed. */
1309		if (tcp_in_slow_start(tp))
   0xffffffff820311a0 <+32>:	jae    0xffffffff820311d1 <tcp_reno_cong_avoid+81>

1310			return tcp_snd_cwnd(tp) < 2 * tp->max_packets_out;
   0xffffffff820311a2 <+34>:	mov    0x9b4(%rbx),%ecx
   0xffffffff820311a8 <+40>:	add    %ecx,%ecx
   0xffffffff820311aa <+42>:	cmp    %ecx,%esi

net/ipv4/tcp_cong.c:
450		if (!tcp_is_cwnd_limited(sk))
   0xffffffff820311ac <+44>:	jae    0xffffffff820311d1 <tcp_reno_cong_avoid+81>

./include/net/tcp.h:
1238		return tcp_snd_cwnd(tp) < tp->snd_ssthresh;
   0xffffffff820311ae <+46>:	cmp    %eax,%esi

net/ipv4/tcp_cong.c:
454		if (tcp_in_slow_start(tp)) {
   0xffffffff820311b0 <+48>:	jae    0xffffffff820311c8 <tcp_reno_cong_avoid+72>

455			acked = tcp_slow_start(tp, acked);
   0xffffffff820311b2 <+50>:	mov    %rbx,%rdi
   0xffffffff820311b5 <+53>:	mov    %edx,%esi
   0xffffffff820311b7 <+55>:	call   0xffffffff82031080 <tcp_slow_start>

--Type <RET> for more, q to quit, c to continue without paging--
456			if (!acked)
   0xffffffff820311bc <+60>:	test   %eax,%eax
   0xffffffff820311be <+62>:	je     0xffffffff820311d1 <tcp_reno_cong_avoid+81>
   0xffffffff820311c0 <+64>:	mov    %eax,%edx

./include/net/tcp.h:
1227		return tp->snd_cwnd;
   0xffffffff820311c2 <+66>:	mov    0x9e8(%rbx),%esi

net/ipv4/tcp_cong.c:
460		tcp_cong_avoid_ai(tp, tcp_snd_cwnd(tp), acked);
   0xffffffff820311c8 <+72>:	mov    %rbx,%rdi
   0xffffffff820311cb <+75>:	pop    %rbx
   0xffffffff820311cc <+76>:	jmp    0xffffffff820310d0 <tcp_cong_avoid_ai>

461	}
   0xffffffff820311d1 <+81>:	pop    %rbx
   0xffffffff820311d2 <+82>:	cs jmp 0xffffffff8223c240 <__x86_return_thunk>
End of assembler dump.

From what I can see above - unless I'm misreading it - we see something
interesting here. Note that in preparing the call to tcp_cong_avoid_ai(),
we get the tcp_snd_cwnd() value into %esi, but nothing needs to be done with 
%rdx because it's already got the "acked" value in it. Now this is good
news, because if we're calling this kfunc - that only uses the first and
third parameters - preparing all three will not lead to any nasty surprises
(we just wasted a bit of time preparing the second unused parameter).
If this held true for all kfuncs it would mean that skipping representing
them in BTF due to optimized-out parameters would be the wrong answer.
The key thing to figure out is this - if we optimize out a parameter, will
the subsequent parameters that are not optimized out still use the registers
that they would be expected to if no optimization had happened? So if I 
optimize out the first parameter say, will the second parameter use the 
"right" register (%rsi on x86_64)? If that were guaranteed, we could relax
the cases where we skip BTF generation to the following:

1. cases where multiple inconsistent function prototypes for the same name
   exist.
2. cases where a function has multiple instances with different optimization
   states (optimized out parameter in one CU but not another). This is another
   instance of 1 really, and shouldn't be an issue for kfuncs.
3. cases where an optimized-out parameter has knock-on effects for the
   registers used to handle other unoptimized parameters such that assumptions
   we would make from the function signature are violated for non-optimized out
   parameters. So the parameter would be flagged as using an unexpected
   register, and that unexpected case would instead lead to skipping BTF
   encoding.

I can have a go at implementing the above in pahole and seeing how it effects
our list of functions.

Note though that what's good for kfuncs isn't necessarily good for tracing
accuracy; if assumptions I make about parameter presence are violated, I will
see strange trace results based upon reading the code, whereas if I am
preparing kfunc parameters, a bit of extra work is done preparing an unused
parameter, but no harm is done. For the tracing case, function annotations
flagging optimized-out parameters via BTF tags could help.

However, none of the above will help problematic cases like
bpf_xdp_metadata_rx_timestamp() or bpf_xdp_metadata_rx_hash(); 
again we see missing location info in their DWARF representations

0x07449011:   DW_TAG_subprogram
                DW_AT_low_pc    (0xffffffff81ec8c80)
                DW_AT_high_pc   (0xffffffff81ec8c90)
                DW_AT_frame_base        (DW_OP_reg7 RSP)
                DW_AT_call_all_calls    (true)
                DW_AT_name      ("bpf_xdp_metadata_rx_timestamp")
                DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/core/xdp.c")
                DW_AT_decl_line (725)
                DW_AT_prototyped        (true)
                DW_AT_type      (0x0742f1ae "int")
                DW_AT_external  (true)

0x07449023:     DW_TAG_formal_parameter
                  DW_AT_name    ("ctx")
                  DW_AT_decl_file       ("/home/jolsa/kernel/linux-qemu/net/core/xdp.c")
                  DW_AT_decl_line       (725)
                  DW_AT_type    (0x0743dff0 "const xdp_md *")

0x0744902d:     DW_TAG_formal_parameter
                  DW_AT_name    ("timestamp")
                  DW_AT_decl_file       ("/home/jolsa/kernel/linux-qemu/net/core/xdp.c")
                  DW_AT_decl_line       (725)
                  DW_AT_type    (0x0743217a "u64 *")


...due to the function just being a "return -EOPNOTSUPP;":

Dump of assembler code for function bpf_xdp_metadata_rx_timestamp:
   0xffffffff81ec8c80 <+0>:	nopl   0x0(%rax,%rax,1)
   0xffffffff81ec8c85 <+5>:	mov    $0xffffffa1,%eax
   0xffffffff81ec8c8a <+10>:	cs jmp 0xffffffff8223c240 <__x86_return_thunk>
End of assembler dump.

__bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *timestamp)
{
        return -EOPNOTSUPP;
}

...and playing around with various attributes here does not seem to help.

should_failslab() has a similar story, I suspect because __should_failslab()
got optimized out due to CONFIG_FAILSLAB=n and we get

(gdb) disassemble/s should_failslab
Dump of assembler code for function should_failslab:
mm/slab_common.c:
1462	{
   0xffffffff81422490 <+0>:	nopl   0x0(%rax,%rax,1)

1463		if (__should_failslab(s, gfpflags))
1464			return -ENOMEM;
1465		return 0;
1466	}
   0xffffffff81422495 <+5>:	xor    %eax,%eax
   0xffffffff81422497 <+7>:	cs jmp 0xffffffff8223c240 <__x86_return_thunk>
End of assembler dump.

However, the caller of this function still prepares parameters as if
they were going to be used:

(gdb) disassemble slab_pre_alloc_hook
Dump of assembler code for function slab_pre_alloc_hook:
   0xffffffff813bc5b0 <+0>:	push   %rbp
   0xffffffff813bc5b1 <+1>:	mov    %rsp,%rbp
   0xffffffff813bc5b4 <+4>:	push   %r15
   0xffffffff813bc5b6 <+6>:	push   %r14
   0xffffffff813bc5b8 <+8>:	push   %r13
   0xffffffff813bc5ba <+10>:	push   %r12
   0xffffffff813bc5bc <+12>:	push   %rbx
   0xffffffff813bc5bd <+13>:	sub    $0x20,%rsp
   0xffffffff813bc5c1 <+17>:	mov    %r8d,%r15d
   0xffffffff813bc5c4 <+20>:	mov    %rcx,%r12
   0xffffffff813bc5c7 <+23>:	mov    %rdx,-0x48(%rbp)
   0xffffffff813bc5cb <+27>:	mov    %rsi,%rbx
   0xffffffff813bc5ce <+30>:	mov    %rdi,%r13
   0xffffffff813bc5d1 <+33>:	and    0x219ff00(%rip),%r15d        # 0xffffffff8355c4d8 <gfp_allowed_mask>
   0xffffffff813bc5d8 <+40>:	test   $0x400,%r15d
   0xffffffff813bc5df <+47>:	je     0xffffffff813bc5e6 <slab_pre_alloc_hook+54>
   0xffffffff813bc5e1 <+49>:	callq  0xffffffff81d27b68 <__SCT__might_resched>
   0xffffffff813bc5e6 <+54>:	mov    %r13,%rdi
   0xffffffff813bc5e9 <+57>:	mov    %r15d,%esi
   0xffffffff813bc5ec <+60>:	callq  0xffffffff81340a70 <should_failslab>

...so the function is still being called with the right register
values. This points to a conceptual flaw in the approach I was
taking - we cannot equate register _state_ on entry to a function
with register _use_ by that function. So from a DWARF perspective,
the fact that there is no location information does not necessarily
tell us the function is not _called_ with that parameter; rather it 
tells us it is not used within the body of the function.

This all combines to suggest that the only time we should be
definitive in rejecting a function for BTF encoding due to 
optimizations is when they interfere with the expectations we
have about _used_ parameter->register mappings. So concretely,
where the second parameter uses a register other than the
one the calling conventions dictate should be used by the 
second parameter say, or indeed does not use a register at
all. It is possible that we could be more definitive by examining
DWARF call site info, but there is none for some of the above
functions like should_failslab(), so that will not help here
as far as I can see.

Does this seem reasonable, or am I missing something here?
I'm worried we're going to end up playing whack-a-mole with
increasingly clever compiler optimizations, so my instinct
is that we need to be conservative in choosing when to skip
BTF encoding, only doing so when we are certain it will mislead
badly. I _think_ there is some justification to that based on
the above. What do you think?

Alan

[PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Alan Maguire]

v1.25 of pahole supports filtering out functions with multiple inconsistent
function prototypes or optimized-out parameters from the BTF representation.
These present problems because there is no additional info in BTF saying which
inconsistent prototype matches which function instance to help guide attachment,
and functions with optimized-out parameters can lead to incorrect assumptions
about register contents.

So for now, filter out such functions while adding BTF representations for
functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters.
This patch assumes that below linked changes land in pahole for v1.25.

Issues with pahole filtering being too aggressive in removing functions
appear to be resolved now, but CI and further testing will confirm.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 scripts/pahole-flags.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
index 1f1f1d397c39..728d55190d97 100755
--- a/scripts/pahole-flags.sh
+++ b/scripts/pahole-flags.sh
@@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
 	# see PAHOLE_HAS_LANG_EXCLUDE
 	extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
 fi
+if [ "${pahole_ver}" -ge "125" ]; then
+	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
+fi
 
 echo ${extra_paholeopt}
-- 
2.31.1

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Jiri Olsa]

On Wed, May 10, 2023 at 02:02:41PM +0100, Alan Maguire wrote:
> v1.25 of pahole supports filtering out functions with multiple inconsistent
> function prototypes or optimized-out parameters from the BTF representation.
> These present problems because there is no additional info in BTF saying which
> inconsistent prototype matches which function instance to help guide attachment,
> and functions with optimized-out parameters can lead to incorrect assumptions
> about register contents.
> 
> So for now, filter out such functions while adding BTF representations for
> functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters.
> This patch assumes that below linked changes land in pahole for v1.25.
> 
> Issues with pahole filtering being too aggressive in removing functions
> appear to be resolved now, but CI and further testing will confirm.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

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

jirka

> ---
>  scripts/pahole-flags.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> index 1f1f1d397c39..728d55190d97 100755
> --- a/scripts/pahole-flags.sh
> +++ b/scripts/pahole-flags.sh
> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>  	# see PAHOLE_HAS_LANG_EXCLUDE
>  	extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>  fi
> +if [ "${pahole_ver}" -ge "125" ]; then
> +	extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> +fi
>  
>  echo ${extra_paholeopt}
> -- 
> 2.31.1
> 

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Yafang Shao]

On Wed, May 10, 2023 at 9:03 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> v1.25 of pahole supports filtering out functions with multiple inconsistent
> function prototypes or optimized-out parameters from the BTF representation.
> These present problems because there is no additional info in BTF saying which
> inconsistent prototype matches which function instance to help guide attachment,
> and functions with optimized-out parameters can lead to incorrect assumptions
> about register contents.
>
> So for now, filter out such functions while adding BTF representations for
> functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters.
> This patch assumes that below linked changes land in pahole for v1.25.
>
> Issues with pahole filtering being too aggressive in removing functions
> appear to be resolved now, but CI and further testing will confirm.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  scripts/pahole-flags.sh | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> index 1f1f1d397c39..728d55190d97 100755
> --- a/scripts/pahole-flags.sh
> +++ b/scripts/pahole-flags.sh
> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>         # see PAHOLE_HAS_LANG_EXCLUDE
>         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>  fi
> +if [ "${pahole_ver}" -ge "125" ]; then
> +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> +fi
>
>  echo ${extra_paholeopt}
> --
> 2.31.1
>

That change looks like a workaround to me.
There may be multiple functions that have the same proto, e.g.:

  $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
kernel/bpf/ net/core/
  kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
bpf_iter_aux_info *aux)
  net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct
bpf_iter_aux_info *aux)

  $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
bpf_iter_detach_map
  [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
  'aux' type_id=2638
  [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static

We don't know which one it is in the BTF.
However, I'm not against this change, as it can avoid some issues.

-- 
Regards
Yafang

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Alan Maguire]

On 12/05/2023 03:51, Yafang Shao wrote:
> On Wed, May 10, 2023 at 9:03 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>>
>> v1.25 of pahole supports filtering out functions with multiple inconsistent
>> function prototypes or optimized-out parameters from the BTF representation.
>> These present problems because there is no additional info in BTF saying which
>> inconsistent prototype matches which function instance to help guide attachment,
>> and functions with optimized-out parameters can lead to incorrect assumptions
>> about register contents.
>>
>> So for now, filter out such functions while adding BTF representations for
>> functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters.
>> This patch assumes that below linked changes land in pahole for v1.25.
>>
>> Issues with pahole filtering being too aggressive in removing functions
>> appear to be resolved now, but CI and further testing will confirm.
>>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  scripts/pahole-flags.sh | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
>> index 1f1f1d397c39..728d55190d97 100755
>> --- a/scripts/pahole-flags.sh
>> +++ b/scripts/pahole-flags.sh
>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>>         # see PAHOLE_HAS_LANG_EXCLUDE
>>         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>>  fi
>> +if [ "${pahole_ver}" -ge "125" ]; then
>> +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
>> +fi
>>
>>  echo ${extra_paholeopt}
>> --
>> 2.31.1
>>
> 
> That change looks like a workaround to me.
> There may be multiple functions that have the same proto, e.g.:
> 
>   $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
> kernel/bpf/ net/core/
>   kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
> bpf_iter_aux_info *aux)
>   net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct
> bpf_iter_aux_info *aux)
> 
>   $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
> bpf_iter_detach_map
>   [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
>   'aux' type_id=2638
>   [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static
> 
> We don't know which one it is in the BTF.
> However, I'm not against this change, as it can avoid some issues.
> 

In the above case, the BTF representation is consistent though.
That is, if I attach fentry progs to either of these functions
based on that BTF representation, nothing will crash.

That's ultimately what those changes are about; ensuring
consistency in BTF representation, so when a function is in
BTF we can know the signature of the function can be safely
used by fentry for example.

The question of being able to identify functions (as opposed
to having a consistent representation) is the next step.
Finding a way to link between kallsyms and BTF would allow us to
have multiple inconsistent functions in BTF, since we could map
from BTF -> kallsyms safely. So two functions called "foo"
with different function signatures would be okay, because
we'd know which was which in kallsyms and could attach
safely. Something like a BTF tag for the function that could
clarify that mapping - but just for cases where it would
otherwise be ambiguous - is probably the way forward
longer term.

Jiri's talking about this topic at LSF/MM/BPF this week I believe.

Thanks!

Alan

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Alexei Starovoitov]

On Fri, May 12, 2023 at 9:04 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 12/05/2023 03:51, Yafang Shao wrote:
> > On Wed, May 10, 2023 at 9:03 PM Alan Maguire <alan.maguire@oracle.com> wrote:
> >>
> >> v1.25 of pahole supports filtering out functions with multiple inconsistent
> >> function prototypes or optimized-out parameters from the BTF representation.
> >> These present problems because there is no additional info in BTF saying which
> >> inconsistent prototype matches which function instance to help guide attachment,
> >> and functions with optimized-out parameters can lead to incorrect assumptions
> >> about register contents.
> >>
> >> So for now, filter out such functions while adding BTF representations for
> >> functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters.
> >> This patch assumes that below linked changes land in pahole for v1.25.
> >>
> >> Issues with pahole filtering being too aggressive in removing functions
> >> appear to be resolved now, but CI and further testing will confirm.
> >>
> >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> >> ---
> >>  scripts/pahole-flags.sh | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> >> index 1f1f1d397c39..728d55190d97 100755
> >> --- a/scripts/pahole-flags.sh
> >> +++ b/scripts/pahole-flags.sh
> >> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
> >>         # see PAHOLE_HAS_LANG_EXCLUDE
> >>         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> >>  fi
> >> +if [ "${pahole_ver}" -ge "125" ]; then
> >> +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> >> +fi
> >>
> >>  echo ${extra_paholeopt}
> >> --
> >> 2.31.1
> >>
> >
> > That change looks like a workaround to me.
> > There may be multiple functions that have the same proto, e.g.:
> >
> >   $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
> > kernel/bpf/ net/core/
> >   kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
> > bpf_iter_aux_info *aux)
> >   net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct
> > bpf_iter_aux_info *aux)
> >
> >   $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
> > bpf_iter_detach_map
> >   [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
> >   'aux' type_id=2638
> >   [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static
> >
> > We don't know which one it is in the BTF.
> > However, I'm not against this change, as it can avoid some issues.
> >
>
> In the above case, the BTF representation is consistent though.
> That is, if I attach fentry progs to either of these functions
> based on that BTF representation, nothing will crash.
>
> That's ultimately what those changes are about; ensuring
> consistency in BTF representation, so when a function is in
> BTF we can know the signature of the function can be safely
> used by fentry for example.
>
> The question of being able to identify functions (as opposed
> to having a consistent representation) is the next step.
> Finding a way to link between kallsyms and BTF would allow us to
> have multiple inconsistent functions in BTF, since we could map
> from BTF -> kallsyms safely. So two functions called "foo"
> with different function signatures would be okay, because
> we'd know which was which in kallsyms and could attach
> safely. Something like a BTF tag for the function that could
> clarify that mapping - but just for cases where it would
> otherwise be ambiguous - is probably the way forward
> longer term.
>
> Jiri's talking about this topic at LSF/MM/BPF this week I believe.

Jiri presented a few ideas during LSFMMBPF.

I feel the best approach is to add a set of addr-s to BTF
via a special decl_tag.
We can also consider extending KIND_FUNC.
The advantage that every BTF func will have one or more addrs
associated with it and bpf prog loading logic wouldn't need to do
fragile name comparison between btf and kallsyms.
pahole can take addrs from dwarf and optionally double check with kallsyms.

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Jiri Olsa]

On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote:
> On Fri, May 12, 2023 at 9:04 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On 12/05/2023 03:51, Yafang Shao wrote:
> > > On Wed, May 10, 2023 at 9:03 PM Alan Maguire <alan.maguire@oracle.com> wrote:
> > >>
> > >> v1.25 of pahole supports filtering out functions with multiple inconsistent
> > >> function prototypes or optimized-out parameters from the BTF representation.
> > >> These present problems because there is no additional info in BTF saying which
> > >> inconsistent prototype matches which function instance to help guide attachment,
> > >> and functions with optimized-out parameters can lead to incorrect assumptions
> > >> about register contents.
> > >>
> > >> So for now, filter out such functions while adding BTF representations for
> > >> functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters.
> > >> This patch assumes that below linked changes land in pahole for v1.25.
> > >>
> > >> Issues with pahole filtering being too aggressive in removing functions
> > >> appear to be resolved now, but CI and further testing will confirm.
> > >>
> > >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > >> ---
> > >>  scripts/pahole-flags.sh | 3 +++
> > >>  1 file changed, 3 insertions(+)
> > >>
> > >> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> > >> index 1f1f1d397c39..728d55190d97 100755
> > >> --- a/scripts/pahole-flags.sh
> > >> +++ b/scripts/pahole-flags.sh
> > >> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
> > >>         # see PAHOLE_HAS_LANG_EXCLUDE
> > >>         extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> > >>  fi
> > >> +if [ "${pahole_ver}" -ge "125" ]; then
> > >> +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
> > >> +fi
> > >>
> > >>  echo ${extra_paholeopt}
> > >> --
> > >> 2.31.1
> > >>
> > >
> > > That change looks like a workaround to me.
> > > There may be multiple functions that have the same proto, e.g.:
> > >
> > >   $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
> > > kernel/bpf/ net/core/
> > >   kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
> > > bpf_iter_aux_info *aux)
> > >   net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct
> > > bpf_iter_aux_info *aux)
> > >
> > >   $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
> > > bpf_iter_detach_map
> > >   [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
> > >   'aux' type_id=2638
> > >   [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static
> > >
> > > We don't know which one it is in the BTF.
> > > However, I'm not against this change, as it can avoid some issues.
> > >
> >
> > In the above case, the BTF representation is consistent though.
> > That is, if I attach fentry progs to either of these functions
> > based on that BTF representation, nothing will crash.
> >
> > That's ultimately what those changes are about; ensuring
> > consistency in BTF representation, so when a function is in
> > BTF we can know the signature of the function can be safely
> > used by fentry for example.
> >
> > The question of being able to identify functions (as opposed
> > to having a consistent representation) is the next step.
> > Finding a way to link between kallsyms and BTF would allow us to
> > have multiple inconsistent functions in BTF, since we could map
> > from BTF -> kallsyms safely. So two functions called "foo"
> > with different function signatures would be okay, because
> > we'd know which was which in kallsyms and could attach
> > safely. Something like a BTF tag for the function that could
> > clarify that mapping - but just for cases where it would
> > otherwise be ambiguous - is probably the way forward
> > longer term.
> >
> > Jiri's talking about this topic at LSF/MM/BPF this week I believe.
> 
> Jiri presented a few ideas during LSFMMBPF.
> 
> I feel the best approach is to add a set of addr-s to BTF
> via a special decl_tag.
> We can also consider extending KIND_FUNC.
> The advantage that every BTF func will have one or more addrs
> associated with it and bpf prog loading logic wouldn't need to do
> fragile name comparison between btf and kallsyms.
> pahole can take addrs from dwarf and optionally double check with kallsyms.

Yonghong summed it up in another email discussion, pasting it in here:

  So overall we have three options as kallsyms representation now:
    (a) "addr module:foo:dir_a/dir_b/core.c"
    (b) "addr module:foo"
    (c) "addr module:foo:btf_id"

  option (a):
    'dir_a/dir_b/core.c' needs to be encoded in BTF.
    user space either check file path or func signature
    to find attach_btf_id and pass to the kernel.
    kernel can find file path in BTF and then lookup
    kallsyms to find addr.

  option (b):
    "addr" needs to be encoded in BTF.
    user space checks func signature to find
    attach_btf_id and pass to the kernel.
    kernel can find addr in BTF and use it.

  option (c):
    if user can decide which function to attach, e.g.,
    through func signature, then no BTF encoding
    is necessary. attach_btf_id is passed to the
    kernel and search kallsyms to find the matching
    btf_id and 'addr' will be available then.

  For option (b) and (c), user space needs to check
  func signature to find which btf_id to use. If
  same-name static functions having the identical
  signatures, then user space would have a hard time
  to differentiate. I think it should be very
  rare same-name static functions in the kernel will have
  identical signatures. But if we want 100% correctness,
  we may need file path in which case option (a)
  is preferable.

  Current option (a) kallsyms format is under review.
  option (c) also needs kallsyms change...


my thoughts so far is that I like the idea of storing functions address
in BTF (option b), because it's the easiest way

on the other hand, user would need debug info to find address for the function
to trace.. but still just for small subset of functions that share same name

also I like Lorenz's idea of storing BTF ID in kalsyms and verifier being
able to lookup address based on BTF ID.. seems like easier kallsyms change,
but it would still require storing objects paths in BTF to pick up the
correct one

cc-ing other folks

jirka

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Yonghong Song]

On 5/12/23 2:54 PM, Jiri Olsa wrote:
> On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote:
>> On Fri, May 12, 2023 at 9:04 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>>
>>> On 12/05/2023 03:51, Yafang Shao wrote:
>>>> On Wed, May 10, 2023 at 9:03 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>>>>>
>>>>> v1.25 of pahole supports filtering out functions with multiple inconsistent
>>>>> function prototypes or optimized-out parameters from the BTF representation.
>>>>> These present problems because there is no additional info in BTF saying which
>>>>> inconsistent prototype matches which function instance to help guide attachment,
>>>>> and functions with optimized-out parameters can lead to incorrect assumptions
>>>>> about register contents.
>>>>>
>>>>> So for now, filter out such functions while adding BTF representations for
>>>>> functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters.
>>>>> This patch assumes that below linked changes land in pahole for v1.25.
>>>>>
>>>>> Issues with pahole filtering being too aggressive in removing functions
>>>>> appear to be resolved now, but CI and further testing will confirm.
>>>>>
>>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>>>> ---
>>>>>   scripts/pahole-flags.sh | 3 +++
>>>>>   1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
>>>>> index 1f1f1d397c39..728d55190d97 100755
>>>>> --- a/scripts/pahole-flags.sh
>>>>> +++ b/scripts/pahole-flags.sh
>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>>>>>          # see PAHOLE_HAS_LANG_EXCLUDE
>>>>>          extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>>>>>   fi
>>>>> +if [ "${pahole_ver}" -ge "125" ]; then
>>>>> +       extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
>>>>> +fi
>>>>>
>>>>>   echo ${extra_paholeopt}
>>>>> --
>>>>> 2.31.1
>>>>>
>>>>
>>>> That change looks like a workaround to me.
>>>> There may be multiple functions that have the same proto, e.g.:
>>>>
>>>>    $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
>>>> kernel/bpf/ net/core/
>>>>    kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
>>>> bpf_iter_aux_info *aux)
>>>>    net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct
>>>> bpf_iter_aux_info *aux)
>>>>
>>>>    $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
>>>> bpf_iter_detach_map
>>>>    [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
>>>>    'aux' type_id=2638
>>>>    [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static
>>>>
>>>> We don't know which one it is in the BTF.
>>>> However, I'm not against this change, as it can avoid some issues.
>>>>
>>>
>>> In the above case, the BTF representation is consistent though.
>>> That is, if I attach fentry progs to either of these functions
>>> based on that BTF representation, nothing will crash.
>>>
>>> That's ultimately what those changes are about; ensuring
>>> consistency in BTF representation, so when a function is in
>>> BTF we can know the signature of the function can be safely
>>> used by fentry for example.
>>>
>>> The question of being able to identify functions (as opposed
>>> to having a consistent representation) is the next step.
>>> Finding a way to link between kallsyms and BTF would allow us to
>>> have multiple inconsistent functions in BTF, since we could map
>>> from BTF -> kallsyms safely. So two functions called "foo"
>>> with different function signatures would be okay, because
>>> we'd know which was which in kallsyms and could attach
>>> safely. Something like a BTF tag for the function that could
>>> clarify that mapping - but just for cases where it would
>>> otherwise be ambiguous - is probably the way forward
>>> longer term.
>>>
>>> Jiri's talking about this topic at LSF/MM/BPF this week I believe.
>>
>> Jiri presented a few ideas during LSFMMBPF.
>>
>> I feel the best approach is to add a set of addr-s to BTF
>> via a special decl_tag.
>> We can also consider extending KIND_FUNC.
>> The advantage that every BTF func will have one or more addrs
>> associated with it and bpf prog loading logic wouldn't need to do
>> fragile name comparison between btf and kallsyms.
>> pahole can take addrs from dwarf and optionally double check with kallsyms.
> 
> Yonghong summed it up in another email discussion, pasting it in here:
> 
>    So overall we have three options as kallsyms representation now:
>      (a) "addr module:foo:dir_a/dir_b/core.c"
>      (b) "addr module:foo"
>      (c) "addr module:foo:btf_id"
> 
>    option (a):
>      'dir_a/dir_b/core.c' needs to be encoded in BTF.
>      user space either check file path or func signature
>      to find attach_btf_id and pass to the kernel.
>      kernel can find file path in BTF and then lookup
>      kallsyms to find addr.
> 
>    option (b):
>      "addr" needs to be encoded in BTF.
>      user space checks func signature to find
>      attach_btf_id and pass to the kernel.
>      kernel can find addr in BTF and use it.
> 
>    option (c):
>      if user can decide which function to attach, e.g.,
>      through func signature, then no BTF encoding
>      is necessary. attach_btf_id is passed to the
>      kernel and search kallsyms to find the matching
>      btf_id and 'addr' will be available then.
> 
>    For option (b) and (c), user space needs to check
>    func signature to find which btf_id to use. If
>    same-name static functions having the identical
>    signatures, then user space would have a hard time
>    to differentiate. I think it should be very
>    rare same-name static functions in the kernel will have
>    identical signatures. But if we want 100% correctness,
>    we may need file path in which case option (a)
>    is preferable.

As Alexei mentioned in previous email, for such a extreme case,
if user is willing to go through extra step to check dwarf
to find and match file path, then (b) and (c) should work
perfectly as well.

> 
>    Current option (a) kallsyms format is under review.
>    option (c) also needs kallsyms change...
> 
> 
> my thoughts so far is that I like the idea of storing functions address
> in BTF (option b), because it's the easiest way
> 
> on the other hand, user would need debug info to find address for the function
> to trace.. but still just for small subset of functions that share same name
> 
> also I like Lorenz's idea of storing BTF ID in kalsyms and verifier being
> able to lookup address based on BTF ID.. seems like easier kallsyms change,
> but it would still require storing objects paths in BTF to pick up the
> correct one
> 
> cc-ing other folks
> 
> jirka

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Yonghong Song]

On 5/12/23 7:59 PM, Yonghong Song wrote:
> 
> 
> On 5/12/23 2:54 PM, Jiri Olsa wrote:
>> On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote:
>>> On Fri, May 12, 2023 at 9:04 AM Alan Maguire 
>>> <alan.maguire@oracle.com> wrote:
>>>>
>>>> On 12/05/2023 03:51, Yafang Shao wrote:
>>>>> On Wed, May 10, 2023 at 9:03 PM Alan Maguire 
>>>>> <alan.maguire@oracle.com> wrote:
>>>>>>
>>>>>> v1.25 of pahole supports filtering out functions with multiple 
>>>>>> inconsistent
>>>>>> function prototypes or optimized-out parameters from the BTF 
>>>>>> representation.
>>>>>> These present problems because there is no additional info in BTF 
>>>>>> saying which
>>>>>> inconsistent prototype matches which function instance to help 
>>>>>> guide attachment,
>>>>>> and functions with optimized-out parameters can lead to incorrect 
>>>>>> assumptions
>>>>>> about register contents.
>>>>>>
>>>>>> So for now, filter out such functions while adding BTF 
>>>>>> representations for
>>>>>> functions that have "."-suffixes (foo.isra.0) but not 
>>>>>> optimized-out parameters.
>>>>>> This patch assumes that below linked changes land in pahole for 
>>>>>> v1.25.
>>>>>>
>>>>>> Issues with pahole filtering being too aggressive in removing 
>>>>>> functions
>>>>>> appear to be resolved now, but CI and further testing will confirm.
>>>>>>
>>>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>>>>> ---
>>>>>>   scripts/pahole-flags.sh | 3 +++
>>>>>>   1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
>>>>>> index 1f1f1d397c39..728d55190d97 100755
>>>>>> --- a/scripts/pahole-flags.sh
>>>>>> +++ b/scripts/pahole-flags.sh
>>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>>>>>>          # see PAHOLE_HAS_LANG_EXCLUDE
>>>>>>          extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>>>>>>   fi
>>>>>> +if [ "${pahole_ver}" -ge "125" ]; then
>>>>>> +       extra_paholeopt="${extra_paholeopt} 
>>>>>> --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
>>>>>> +fi
>>>>>>
>>>>>>   echo ${extra_paholeopt}
>>>>>> -- 
>>>>>> 2.31.1
>>>>>>
>>>>>
>>>>> That change looks like a workaround to me.
>>>>> There may be multiple functions that have the same proto, e.g.:
>>>>>
>>>>>    $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
>>>>> kernel/bpf/ net/core/
>>>>>    kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
>>>>> bpf_iter_aux_info *aux)
>>>>>    net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct
>>>>> bpf_iter_aux_info *aux)
>>>>>
>>>>>    $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
>>>>> bpf_iter_detach_map
>>>>>    [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
>>>>>    'aux' type_id=2638
>>>>>    [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static
>>>>>
>>>>> We don't know which one it is in the BTF.
>>>>> However, I'm not against this change, as it can avoid some issues.
>>>>>
>>>>
>>>> In the above case, the BTF representation is consistent though.
>>>> That is, if I attach fentry progs to either of these functions
>>>> based on that BTF representation, nothing will crash.
>>>>
>>>> That's ultimately what those changes are about; ensuring
>>>> consistency in BTF representation, so when a function is in
>>>> BTF we can know the signature of the function can be safely
>>>> used by fentry for example.
>>>>
>>>> The question of being able to identify functions (as opposed
>>>> to having a consistent representation) is the next step.
>>>> Finding a way to link between kallsyms and BTF would allow us to
>>>> have multiple inconsistent functions in BTF, since we could map
>>>> from BTF -> kallsyms safely. So two functions called "foo"
>>>> with different function signatures would be okay, because
>>>> we'd know which was which in kallsyms and could attach
>>>> safely. Something like a BTF tag for the function that could
>>>> clarify that mapping - but just for cases where it would
>>>> otherwise be ambiguous - is probably the way forward
>>>> longer term.
>>>>
>>>> Jiri's talking about this topic at LSF/MM/BPF this week I believe.
>>>
>>> Jiri presented a few ideas during LSFMMBPF.
>>>
>>> I feel the best approach is to add a set of addr-s to BTF
>>> via a special decl_tag.
>>> We can also consider extending KIND_FUNC.
>>> The advantage that every BTF func will have one or more addrs
>>> associated with it and bpf prog loading logic wouldn't need to do
>>> fragile name comparison between btf and kallsyms.
>>> pahole can take addrs from dwarf and optionally double check with 
>>> kallsyms.
>>
>> Yonghong summed it up in another email discussion, pasting it in here:
>>
>>    So overall we have three options as kallsyms representation now:
>>      (a) "addr module:foo:dir_a/dir_b/core.c"
>>      (b) "addr module:foo"
>>      (c) "addr module:foo:btf_id"
>>
>>    option (a):
>>      'dir_a/dir_b/core.c' needs to be encoded in BTF.
>>      user space either check file path or func signature
>>      to find attach_btf_id and pass to the kernel.
>>      kernel can find file path in BTF and then lookup
>>      kallsyms to find addr.
>>
>>    option (b):
>>      "addr" needs to be encoded in BTF.
>>      user space checks func signature to find
>>      attach_btf_id and pass to the kernel.
>>      kernel can find addr in BTF and use it.
>>
>>    option (c):
>>      if user can decide which function to attach, e.g.,
>>      through func signature, then no BTF encoding
>>      is necessary. attach_btf_id is passed to the
>>      kernel and search kallsyms to find the matching
>>      btf_id and 'addr' will be available then.
>>
>>    For option (b) and (c), user space needs to check
>>    func signature to find which btf_id to use. If
>>    same-name static functions having the identical
>>    signatures, then user space would have a hard time
>>    to differentiate. I think it should be very
>>    rare same-name static functions in the kernel will have
>>    identical signatures. But if we want 100% correctness,
>>    we may need file path in which case option (a)
>>    is preferable.
> 
> As Alexei mentioned in previous email, for such a extreme case,
> if user is willing to go through extra step to check dwarf
> to find and match file path, then (b) and (c) should work
> perfectly as well.

Okay, it looks like this is more complex if the function signature is
the same. In such cases, current BTF dedup will merge these
functions as a single BTF func. In such cases, we could have:

    decl_tag_1   ----> dedup'ed static_func
                          ^
                          |
    decl_tag_2   ---------

For such cases, just passing btf_id of static func to kernel
won't work since the kernel won't be able to know which
decl_tag to be associated with.

(I did a simple test with vmlinux, it looks we have
  issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func
  as well since only one of decl_tag survives.
  But this is a different issue.
)

So if we intend to add decl tag (addr or file_path), we
should not dedup static functions or generally any functions.

> 
>>
>>    Current option (a) kallsyms format is under review.
>>    option (c) also needs kallsyms change...
>>
>>
>> my thoughts so far is that I like the idea of storing functions address
>> in BTF (option b), because it's the easiest way
>>
>> on the other hand, user would need debug info to find address for the 
>> function
>> to trace.. but still just for small subset of functions that share 
>> same name
>>
>> also I like Lorenz's idea of storing BTF ID in kalsyms and verifier being
>> able to lookup address based on BTF ID.. seems like easier kallsyms 
>> change,
>> but it would still require storing objects paths in BTF to pick up the
>> correct one
>>
>> cc-ing other folks
>>
>> jirka

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Jiri Olsa]

On Sun, May 14, 2023 at 10:37:08AM -0700, Yonghong Song wrote:
> 
> 
> On 5/12/23 7:59 PM, Yonghong Song wrote:
> > 
> > 
> > On 5/12/23 2:54 PM, Jiri Olsa wrote:
> > > On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote:
> > > > On Fri, May 12, 2023 at 9:04 AM Alan Maguire
> > > > <alan.maguire@oracle.com> wrote:
> > > > > 
> > > > > On 12/05/2023 03:51, Yafang Shao wrote:
> > > > > > On Wed, May 10, 2023 at 9:03 PM Alan Maguire
> > > > > > <alan.maguire@oracle.com> wrote:
> > > > > > > 
> > > > > > > v1.25 of pahole supports filtering out functions
> > > > > > > with multiple inconsistent
> > > > > > > function prototypes or optimized-out parameters from
> > > > > > > the BTF representation.
> > > > > > > These present problems because there is no
> > > > > > > additional info in BTF saying which
> > > > > > > inconsistent prototype matches which function
> > > > > > > instance to help guide attachment,
> > > > > > > and functions with optimized-out parameters can lead
> > > > > > > to incorrect assumptions
> > > > > > > about register contents.
> > > > > > > 
> > > > > > > So for now, filter out such functions while adding
> > > > > > > BTF representations for
> > > > > > > functions that have "."-suffixes (foo.isra.0) but
> > > > > > > not optimized-out parameters.
> > > > > > > This patch assumes that below linked changes land in
> > > > > > > pahole for v1.25.
> > > > > > > 
> > > > > > > Issues with pahole filtering being too aggressive in
> > > > > > > removing functions
> > > > > > > appear to be resolved now, but CI and further testing will confirm.
> > > > > > > 
> > > > > > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > > > > > > ---
> > > > > > >   scripts/pahole-flags.sh | 3 +++
> > > > > > >   1 file changed, 3 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
> > > > > > > index 1f1f1d397c39..728d55190d97 100755
> > > > > > > --- a/scripts/pahole-flags.sh
> > > > > > > +++ b/scripts/pahole-flags.sh
> > > > > > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
> > > > > > >          # see PAHOLE_HAS_LANG_EXCLUDE
> > > > > > >          extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> > > > > > >   fi
> > > > > > > +if [ "${pahole_ver}" -ge "125" ]; then
> > > > > > > +       extra_paholeopt="${extra_paholeopt}
> > > > > > > --skip_encoding_btf_inconsistent_proto
> > > > > > > --btf_gen_optimized"
> > > > > > > +fi
> > > > > > > 
> > > > > > >   echo ${extra_paholeopt}
> > > > > > > -- 
> > > > > > > 2.31.1
> > > > > > > 
> > > > > > 
> > > > > > That change looks like a workaround to me.
> > > > > > There may be multiple functions that have the same proto, e.g.:
> > > > > > 
> > > > > >    $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
> > > > > > kernel/bpf/ net/core/
> > > > > >    kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
> > > > > > bpf_iter_aux_info *aux)
> > > > > >    net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct
> > > > > > bpf_iter_aux_info *aux)
> > > > > > 
> > > > > >    $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
> > > > > > bpf_iter_detach_map
> > > > > >    [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
> > > > > >    'aux' type_id=2638
> > > > > >    [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static
> > > > > > 
> > > > > > We don't know which one it is in the BTF.
> > > > > > However, I'm not against this change, as it can avoid some issues.
> > > > > > 
> > > > > 
> > > > > In the above case, the BTF representation is consistent though.
> > > > > That is, if I attach fentry progs to either of these functions
> > > > > based on that BTF representation, nothing will crash.
> > > > > 
> > > > > That's ultimately what those changes are about; ensuring
> > > > > consistency in BTF representation, so when a function is in
> > > > > BTF we can know the signature of the function can be safely
> > > > > used by fentry for example.
> > > > > 
> > > > > The question of being able to identify functions (as opposed
> > > > > to having a consistent representation) is the next step.
> > > > > Finding a way to link between kallsyms and BTF would allow us to
> > > > > have multiple inconsistent functions in BTF, since we could map
> > > > > from BTF -> kallsyms safely. So two functions called "foo"
> > > > > with different function signatures would be okay, because
> > > > > we'd know which was which in kallsyms and could attach
> > > > > safely. Something like a BTF tag for the function that could
> > > > > clarify that mapping - but just for cases where it would
> > > > > otherwise be ambiguous - is probably the way forward
> > > > > longer term.
> > > > > 
> > > > > Jiri's talking about this topic at LSF/MM/BPF this week I believe.
> > > > 
> > > > Jiri presented a few ideas during LSFMMBPF.
> > > > 
> > > > I feel the best approach is to add a set of addr-s to BTF
> > > > via a special decl_tag.
> > > > We can also consider extending KIND_FUNC.
> > > > The advantage that every BTF func will have one or more addrs
> > > > associated with it and bpf prog loading logic wouldn't need to do
> > > > fragile name comparison between btf and kallsyms.
> > > > pahole can take addrs from dwarf and optionally double check
> > > > with kallsyms.
> > > 
> > > Yonghong summed it up in another email discussion, pasting it in here:
> > > 
> > >    So overall we have three options as kallsyms representation now:
> > >      (a) "addr module:foo:dir_a/dir_b/core.c"
> > >      (b) "addr module:foo"
> > >      (c) "addr module:foo:btf_id"
> > > 
> > >    option (a):
> > >      'dir_a/dir_b/core.c' needs to be encoded in BTF.
> > >      user space either check file path or func signature
> > >      to find attach_btf_id and pass to the kernel.
> > >      kernel can find file path in BTF and then lookup
> > >      kallsyms to find addr.
> > > 
> > >    option (b):
> > >      "addr" needs to be encoded in BTF.
> > >      user space checks func signature to find
> > >      attach_btf_id and pass to the kernel.
> > >      kernel can find addr in BTF and use it.
> > > 
> > >    option (c):
> > >      if user can decide which function to attach, e.g.,
> > >      through func signature, then no BTF encoding
> > >      is necessary. attach_btf_id is passed to the
> > >      kernel and search kallsyms to find the matching
> > >      btf_id and 'addr' will be available then.
> > > 
> > >    For option (b) and (c), user space needs to check
> > >    func signature to find which btf_id to use. If
> > >    same-name static functions having the identical
> > >    signatures, then user space would have a hard time
> > >    to differentiate. I think it should be very
> > >    rare same-name static functions in the kernel will have
> > >    identical signatures. But if we want 100% correctness,
> > >    we may need file path in which case option (a)
> > >    is preferable.
> > 
> > As Alexei mentioned in previous email, for such a extreme case,
> > if user is willing to go through extra step to check dwarf
> > to find and match file path, then (b) and (c) should work
> > perfectly as well.
> 
> Okay, it looks like this is more complex if the function signature is
> the same. In such cases, current BTF dedup will merge these
> functions as a single BTF func. In such cases, we could have:
> 
>    decl_tag_1   ----> dedup'ed static_func
>                          ^
>                          |
>    decl_tag_2   ---------
> 
> For such cases, just passing btf_id of static func to kernel
> won't work since the kernel won't be able to know which
> decl_tag to be associated with.
> 
> (I did a simple test with vmlinux, it looks we have
>  issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func
>  as well since only one of decl_tag survives.
>  But this is a different issue.
> )
> 
> So if we intend to add decl tag (addr or file_path), we
> should not dedup static functions or generally any functions.

I did not think functions would be dedup-ed, they are ;-) with the
declaration tags in place we could perhaps switch it off, right?

or perhaps I can't think of all the cases we need functions dedup for,
so maybe the dedup code could check also the associated decl tag when
comparing functions

jirka

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Yonghong Song]

On 5/14/23 2:49 PM, Jiri Olsa wrote:
> On Sun, May 14, 2023 at 10:37:08AM -0700, Yonghong Song wrote:
>>
>>
>> On 5/12/23 7:59 PM, Yonghong Song wrote:
>>>
>>>
>>> On 5/12/23 2:54 PM, Jiri Olsa wrote:
>>>> On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote:
>>>>> On Fri, May 12, 2023 at 9:04 AM Alan Maguire
>>>>> <alan.maguire@oracle.com> wrote:
>>>>>>
>>>>>> On 12/05/2023 03:51, Yafang Shao wrote:
>>>>>>> On Wed, May 10, 2023 at 9:03 PM Alan Maguire
>>>>>>> <alan.maguire@oracle.com> wrote:
>>>>>>>>
>>>>>>>> v1.25 of pahole supports filtering out functions
>>>>>>>> with multiple inconsistent
>>>>>>>> function prototypes or optimized-out parameters from
>>>>>>>> the BTF representation.
>>>>>>>> These present problems because there is no
>>>>>>>> additional info in BTF saying which
>>>>>>>> inconsistent prototype matches which function
>>>>>>>> instance to help guide attachment,
>>>>>>>> and functions with optimized-out parameters can lead
>>>>>>>> to incorrect assumptions
>>>>>>>> about register contents.
>>>>>>>>
>>>>>>>> So for now, filter out such functions while adding
>>>>>>>> BTF representations for
>>>>>>>> functions that have "."-suffixes (foo.isra.0) but
>>>>>>>> not optimized-out parameters.
>>>>>>>> This patch assumes that below linked changes land in
>>>>>>>> pahole for v1.25.
>>>>>>>>
>>>>>>>> Issues with pahole filtering being too aggressive in
>>>>>>>> removing functions
>>>>>>>> appear to be resolved now, but CI and further testing will confirm.
>>>>>>>>
>>>>>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>>>>>>> ---
>>>>>>>>    scripts/pahole-flags.sh | 3 +++
>>>>>>>>    1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
>>>>>>>> index 1f1f1d397c39..728d55190d97 100755
>>>>>>>> --- a/scripts/pahole-flags.sh
>>>>>>>> +++ b/scripts/pahole-flags.sh
>>>>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>>>>>>>>           # see PAHOLE_HAS_LANG_EXCLUDE
>>>>>>>>           extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>>>>>>>>    fi
>>>>>>>> +if [ "${pahole_ver}" -ge "125" ]; then
>>>>>>>> +       extra_paholeopt="${extra_paholeopt}
>>>>>>>> --skip_encoding_btf_inconsistent_proto
>>>>>>>> --btf_gen_optimized"
>>>>>>>> +fi
>>>>>>>>
>>>>>>>>    echo ${extra_paholeopt}
>>>>>>>> -- 
>>>>>>>> 2.31.1
>>>>>>>>
>>>>>>>
>>>>>>> That change looks like a workaround to me.
>>>>>>> There may be multiple functions that have the same proto, e.g.:
>>>>>>>
>>>>>>>     $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
>>>>>>> kernel/bpf/ net/core/
>>>>>>>     kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
>>>>>>> bpf_iter_aux_info *aux)
>>>>>>>     net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct
>>>>>>> bpf_iter_aux_info *aux)
>>>>>>>
>>>>>>>     $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
>>>>>>> bpf_iter_detach_map
>>>>>>>     [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
>>>>>>>     'aux' type_id=2638
>>>>>>>     [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static
>>>>>>>
>>>>>>> We don't know which one it is in the BTF.
>>>>>>> However, I'm not against this change, as it can avoid some issues.
>>>>>>>
>>>>>>
>>>>>> In the above case, the BTF representation is consistent though.
>>>>>> That is, if I attach fentry progs to either of these functions
>>>>>> based on that BTF representation, nothing will crash.
>>>>>>
>>>>>> That's ultimately what those changes are about; ensuring
>>>>>> consistency in BTF representation, so when a function is in
>>>>>> BTF we can know the signature of the function can be safely
>>>>>> used by fentry for example.
>>>>>>
>>>>>> The question of being able to identify functions (as opposed
>>>>>> to having a consistent representation) is the next step.
>>>>>> Finding a way to link between kallsyms and BTF would allow us to
>>>>>> have multiple inconsistent functions in BTF, since we could map
>>>>>> from BTF -> kallsyms safely. So two functions called "foo"
>>>>>> with different function signatures would be okay, because
>>>>>> we'd know which was which in kallsyms and could attach
>>>>>> safely. Something like a BTF tag for the function that could
>>>>>> clarify that mapping - but just for cases where it would
>>>>>> otherwise be ambiguous - is probably the way forward
>>>>>> longer term.
>>>>>>
>>>>>> Jiri's talking about this topic at LSF/MM/BPF this week I believe.
>>>>>
>>>>> Jiri presented a few ideas during LSFMMBPF.
>>>>>
>>>>> I feel the best approach is to add a set of addr-s to BTF
>>>>> via a special decl_tag.
>>>>> We can also consider extending KIND_FUNC.
>>>>> The advantage that every BTF func will have one or more addrs
>>>>> associated with it and bpf prog loading logic wouldn't need to do
>>>>> fragile name comparison between btf and kallsyms.
>>>>> pahole can take addrs from dwarf and optionally double check
>>>>> with kallsyms.
>>>>
>>>> Yonghong summed it up in another email discussion, pasting it in here:
>>>>
>>>>     So overall we have three options as kallsyms representation now:
>>>>       (a) "addr module:foo:dir_a/dir_b/core.c"
>>>>       (b) "addr module:foo"
>>>>       (c) "addr module:foo:btf_id"
>>>>
>>>>     option (a):
>>>>       'dir_a/dir_b/core.c' needs to be encoded in BTF.
>>>>       user space either check file path or func signature
>>>>       to find attach_btf_id and pass to the kernel.
>>>>       kernel can find file path in BTF and then lookup
>>>>       kallsyms to find addr.
>>>>
>>>>     option (b):
>>>>       "addr" needs to be encoded in BTF.
>>>>       user space checks func signature to find
>>>>       attach_btf_id and pass to the kernel.
>>>>       kernel can find addr in BTF and use it.
>>>>
>>>>     option (c):
>>>>       if user can decide which function to attach, e.g.,
>>>>       through func signature, then no BTF encoding
>>>>       is necessary. attach_btf_id is passed to the
>>>>       kernel and search kallsyms to find the matching
>>>>       btf_id and 'addr' will be available then.
>>>>
>>>>     For option (b) and (c), user space needs to check
>>>>     func signature to find which btf_id to use. If
>>>>     same-name static functions having the identical
>>>>     signatures, then user space would have a hard time
>>>>     to differentiate. I think it should be very
>>>>     rare same-name static functions in the kernel will have
>>>>     identical signatures. But if we want 100% correctness,
>>>>     we may need file path in which case option (a)
>>>>     is preferable.
>>>
>>> As Alexei mentioned in previous email, for such a extreme case,
>>> if user is willing to go through extra step to check dwarf
>>> to find and match file path, then (b) and (c) should work
>>> perfectly as well.
>>
>> Okay, it looks like this is more complex if the function signature is
>> the same. In such cases, current BTF dedup will merge these
>> functions as a single BTF func. In such cases, we could have:
>>
>>     decl_tag_1   ----> dedup'ed static_func
>>                           ^
>>                           |
>>     decl_tag_2   ---------
>>
>> For such cases, just passing btf_id of static func to kernel
>> won't work since the kernel won't be able to know which
>> decl_tag to be associated with.
>>
>> (I did a simple test with vmlinux, it looks we have
>>   issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func
>>   as well since only one of decl_tag survives.
>>   But this is a different issue.
>> )
>>
>> So if we intend to add decl tag (addr or file_path), we
>> should not dedup static functions or generally any functions.
> 
> I did not think functions would be dedup-ed, they are ;-) with the
> declaration tags in place we could perhaps switch it off, right?

That is my hope too. If with decl tag func won't be dedup'ed,
then we should be okay. In the kernel, based on attach_btf_id,
through btf, kernel can find the corresponding decl tag (file path
or addr) or through attach_btf_id itself if the btf id is
encoded in kallsym entries.

> 
> or perhaps I can't think of all the cases we need functions dedup for,
> so maybe the dedup code could check also the associated decl tag when
> comparing functions
> 
> jirka

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Alan Maguire]

On 15/05/2023 05:06, Yonghong Song wrote:
> 
> 
> On 5/14/23 2:49 PM, Jiri Olsa wrote:
>> On Sun, May 14, 2023 at 10:37:08AM -0700, Yonghong Song wrote:
>>>
>>>
>>> On 5/12/23 7:59 PM, Yonghong Song wrote:
>>>>
>>>>
>>>> On 5/12/23 2:54 PM, Jiri Olsa wrote:
>>>>> On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote:
>>>>>> On Fri, May 12, 2023 at 9:04 AM Alan Maguire
>>>>>> <alan.maguire@oracle.com> wrote:
>>>>>>>
>>>>>>> On 12/05/2023 03:51, Yafang Shao wrote:
>>>>>>>> On Wed, May 10, 2023 at 9:03 PM Alan Maguire
>>>>>>>> <alan.maguire@oracle.com> wrote:
>>>>>>>>>
>>>>>>>>> v1.25 of pahole supports filtering out functions
>>>>>>>>> with multiple inconsistent
>>>>>>>>> function prototypes or optimized-out parameters from
>>>>>>>>> the BTF representation.
>>>>>>>>> These present problems because there is no
>>>>>>>>> additional info in BTF saying which
>>>>>>>>> inconsistent prototype matches which function
>>>>>>>>> instance to help guide attachment,
>>>>>>>>> and functions with optimized-out parameters can lead
>>>>>>>>> to incorrect assumptions
>>>>>>>>> about register contents.
>>>>>>>>>
>>>>>>>>> So for now, filter out such functions while adding
>>>>>>>>> BTF representations for
>>>>>>>>> functions that have "."-suffixes (foo.isra.0) but
>>>>>>>>> not optimized-out parameters.
>>>>>>>>> This patch assumes that below linked changes land in
>>>>>>>>> pahole for v1.25.
>>>>>>>>>
>>>>>>>>> Issues with pahole filtering being too aggressive in
>>>>>>>>> removing functions
>>>>>>>>> appear to be resolved now, but CI and further testing will
>>>>>>>>> confirm.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>>>>>>>> ---
>>>>>>>>>    scripts/pahole-flags.sh | 3 +++
>>>>>>>>>    1 file changed, 3 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
>>>>>>>>> index 1f1f1d397c39..728d55190d97 100755
>>>>>>>>> --- a/scripts/pahole-flags.sh
>>>>>>>>> +++ b/scripts/pahole-flags.sh
>>>>>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>>>>>>>>>           # see PAHOLE_HAS_LANG_EXCLUDE
>>>>>>>>>           extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>>>>>>>>>    fi
>>>>>>>>> +if [ "${pahole_ver}" -ge "125" ]; then
>>>>>>>>> +       extra_paholeopt="${extra_paholeopt}
>>>>>>>>> --skip_encoding_btf_inconsistent_proto
>>>>>>>>> --btf_gen_optimized"
>>>>>>>>> +fi
>>>>>>>>>
>>>>>>>>>    echo ${extra_paholeopt}
>>>>>>>>> -- 
>>>>>>>>> 2.31.1
>>>>>>>>>
>>>>>>>>
>>>>>>>> That change looks like a workaround to me.
>>>>>>>> There may be multiple functions that have the same proto, e.g.:
>>>>>>>>
>>>>>>>>     $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
>>>>>>>> kernel/bpf/ net/core/
>>>>>>>>     kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
>>>>>>>> bpf_iter_aux_info *aux)
>>>>>>>>     net/core/bpf_sk_storage.c:static void
>>>>>>>> bpf_iter_detach_map(struct
>>>>>>>> bpf_iter_aux_info *aux)
>>>>>>>>
>>>>>>>>     $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
>>>>>>>> bpf_iter_detach_map
>>>>>>>>     [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
>>>>>>>>     'aux' type_id=2638
>>>>>>>>     [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static
>>>>>>>>
>>>>>>>> We don't know which one it is in the BTF.
>>>>>>>> However, I'm not against this change, as it can avoid some issues.
>>>>>>>>
>>>>>>>
>>>>>>> In the above case, the BTF representation is consistent though.
>>>>>>> That is, if I attach fentry progs to either of these functions
>>>>>>> based on that BTF representation, nothing will crash.
>>>>>>>
>>>>>>> That's ultimately what those changes are about; ensuring
>>>>>>> consistency in BTF representation, so when a function is in
>>>>>>> BTF we can know the signature of the function can be safely
>>>>>>> used by fentry for example.
>>>>>>>
>>>>>>> The question of being able to identify functions (as opposed
>>>>>>> to having a consistent representation) is the next step.
>>>>>>> Finding a way to link between kallsyms and BTF would allow us to
>>>>>>> have multiple inconsistent functions in BTF, since we could map
>>>>>>> from BTF -> kallsyms safely. So two functions called "foo"
>>>>>>> with different function signatures would be okay, because
>>>>>>> we'd know which was which in kallsyms and could attach
>>>>>>> safely. Something like a BTF tag for the function that could
>>>>>>> clarify that mapping - but just for cases where it would
>>>>>>> otherwise be ambiguous - is probably the way forward
>>>>>>> longer term.
>>>>>>>
>>>>>>> Jiri's talking about this topic at LSF/MM/BPF this week I believe.
>>>>>>
>>>>>> Jiri presented a few ideas during LSFMMBPF.
>>>>>>
>>>>>> I feel the best approach is to add a set of addr-s to BTF
>>>>>> via a special decl_tag.
>>>>>> We can also consider extending KIND_FUNC.
>>>>>> The advantage that every BTF func will have one or more addrs
>>>>>> associated with it and bpf prog loading logic wouldn't need to do
>>>>>> fragile name comparison between btf and kallsyms.
>>>>>> pahole can take addrs from dwarf and optionally double check
>>>>>> with kallsyms.
>>>>>
>>>>> Yonghong summed it up in another email discussion, pasting it in here:
>>>>>
>>>>>     So overall we have three options as kallsyms representation now:
>>>>>       (a) "addr module:foo:dir_a/dir_b/core.c"
>>>>>       (b) "addr module:foo"
>>>>>       (c) "addr module:foo:btf_id"
>>>>>
>>>>>     option (a):
>>>>>       'dir_a/dir_b/core.c' needs to be encoded in BTF.
>>>>>       user space either check file path or func signature
>>>>>       to find attach_btf_id and pass to the kernel.
>>>>>       kernel can find file path in BTF and then lookup
>>>>>       kallsyms to find addr.
>>>>>

I like the source-centric nature of this, but the only
danger here is we might not get a 1:1 mapping between
source file location and address; consider the case
of a static inline function in a .h file which doesn't
get inlined. It could have multiple addresses associated
with the same source. For example:

static inline void __list_del_entry(struct list_head *entry)
{
	if (!__list_del_entry_valid(entry))
		return;

	__list_del(entry->prev, entry->next);
}

$ grep __list_del_entry /proc/kallsyms
ffffffff982cc5c0 t __pfx___list_del_entry
ffffffff982cc5d0 t __list_del_entry
ffffffff985b0860 t __pfx___list_del_entry
ffffffff985b0870 t __list_del_entry
ffffffff9862d800 t __pfx___list_del_entry
ffffffff9862d810 t __list_del_entry
ffffffff987d3dd0 t __pfx___list_del_entry
ffffffff987d3de0 t __list_del_entry
ffffffff987f37a0 T __pfx___list_del_entry_valid
ffffffff987f37b0 T __list_del_entry_valid
ffffffff989fdd10 t __pfx___list_del_entry
ffffffff989fdd20 t __list_del_entry
ffffffff99baf08c r __ksymtab___list_del_entry_valid
ffffffffc12da2e0 t __list_del_entry	[bnep]
ffffffffc12da2d0 t __pfx___list_del_entry	[bnep]
ffffffffc092d6b0 t __list_del_entry	[videobuf2_common]
ffffffffc092d6a0 t __pfx___list_del_entry	[videobuf2_common]

>>>>>     option (b):
>>>>>       "addr" needs to be encoded in BTF.
>>>>>       user space checks func signature to find
>>>>>       attach_btf_id and pass to the kernel.
>>>>>       kernel can find addr in BTF and use it.
>>>>>

This seems like the safest option to me. Ideally we wouldn't
need such information for every function - only ones with
multiple sites and ambiguous signatures - but the problem
is a function could have the same name in a kernel and
a module too. So it seems like we're stuck with providing
additional info to clarify which signature goes with which
function for each static function.

>>>>>     option (c):
>>>>>       if user can decide which function to attach, e.g.,
>>>>>       through func signature, then no BTF encoding
>>>>>       is necessary. attach_btf_id is passed to the
>>>>>       kernel and search kallsyms to find the matching
>>>>>       btf_id and 'addr' will be available then.
>>>>>
>>>>>     For option (b) and (c), user space needs to check
>>>>>     func signature to find which btf_id to use. If
>>>>>     same-name static functions having the identical
>>>>>     signatures, then user space would have a hard time
>>>>>     to differentiate. I think it should be very
>>>>>     rare same-name static functions in the kernel will have
>>>>>     identical signatures. But if we want 100% correctness,
>>>>>     we may need file path in which case option (a)
>>>>>     is preferable.
>>>>
>>>> As Alexei mentioned in previous email, for such a extreme case,
>>>> if user is willing to go through extra step to check dwarf
>>>> to find and match file path, then (b) and (c) should work
>>>> perfectly as well.
>>>
>>> Okay, it looks like this is more complex if the function signature is
>>> the same. In such cases, current BTF dedup will merge these
>>> functions as a single BTF func. In such cases, we could have:
>>>
>>>     decl_tag_1   ----> dedup'ed static_func
>>>                           ^
>>>                           |
>>>     decl_tag_2   ---------
>>>
>>> For such cases, just passing btf_id of static func to kernel
>>> won't work since the kernel won't be able to know which
>>> decl_tag to be associated with.
>>>
>>> (I did a simple test with vmlinux, it looks we have
>>>   issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func
>>>   as well since only one of decl_tag survives.
>>>   But this is a different issue.
>>> )
>>>
>>> So if we intend to add decl tag (addr or file_path), we
>>> should not dedup static functions or generally any functions.
>>
>> I did not think functions would be dedup-ed, they are ;-) with the
>> declaration tags in place we could perhaps switch it off, right?
> 
> That is my hope too. If with decl tag func won't be dedup'ed,
> then we should be okay. In the kernel, based on attach_btf_id,
> through btf, kernel can find the corresponding decl tag (file path
> or addr) or through attach_btf_id itself if the btf id is
> encoded in kallsym entries.
> 
>>
>> or perhaps I can't think of all the cases we need functions dedup for,
>> so maybe the dedup code could check also the associated decl tag when
>> comparing functions
>>

Would using the BTF decl tag id instead of the function BTF id to
guide attachment be an option? That way if multiple functions shared
the same signature, we could still get the info to figure out which to
attach to from the decl tag, and we wouldn't need to mess with dedup.
I'm probably missing something which makes that unworkable, but just a
thought. Thanks!

Alan

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Yonghong Song]

On 5/15/23 7:53 AM, Alan Maguire wrote:
> On 15/05/2023 05:06, Yonghong Song wrote:
>>
>>
>> On 5/14/23 2:49 PM, Jiri Olsa wrote:
>>> On Sun, May 14, 2023 at 10:37:08AM -0700, Yonghong Song wrote:
>>>>
>>>>
>>>> On 5/12/23 7:59 PM, Yonghong Song wrote:
>>>>>
>>>>>
>>>>> On 5/12/23 2:54 PM, Jiri Olsa wrote:
>>>>>> On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote:
>>>>>>> On Fri, May 12, 2023 at 9:04 AM Alan Maguire
>>>>>>> <alan.maguire@oracle.com> wrote:
>>>>>>>>
>>>>>>>> On 12/05/2023 03:51, Yafang Shao wrote:
>>>>>>>>> On Wed, May 10, 2023 at 9:03 PM Alan Maguire
>>>>>>>>> <alan.maguire@oracle.com> wrote:
>>>>>>>>>>
>>>>>>>>>> v1.25 of pahole supports filtering out functions
>>>>>>>>>> with multiple inconsistent
>>>>>>>>>> function prototypes or optimized-out parameters from
>>>>>>>>>> the BTF representation.
>>>>>>>>>> These present problems because there is no
>>>>>>>>>> additional info in BTF saying which
>>>>>>>>>> inconsistent prototype matches which function
>>>>>>>>>> instance to help guide attachment,
>>>>>>>>>> and functions with optimized-out parameters can lead
>>>>>>>>>> to incorrect assumptions
>>>>>>>>>> about register contents.
>>>>>>>>>>
>>>>>>>>>> So for now, filter out such functions while adding
>>>>>>>>>> BTF representations for
>>>>>>>>>> functions that have "."-suffixes (foo.isra.0) but
>>>>>>>>>> not optimized-out parameters.
>>>>>>>>>> This patch assumes that below linked changes land in
>>>>>>>>>> pahole for v1.25.
>>>>>>>>>>
>>>>>>>>>> Issues with pahole filtering being too aggressive in
>>>>>>>>>> removing functions
>>>>>>>>>> appear to be resolved now, but CI and further testing will
>>>>>>>>>> confirm.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>>>>>>>>> ---
>>>>>>>>>>     scripts/pahole-flags.sh | 3 +++
>>>>>>>>>>     1 file changed, 3 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
>>>>>>>>>> index 1f1f1d397c39..728d55190d97 100755
>>>>>>>>>> --- a/scripts/pahole-flags.sh
>>>>>>>>>> +++ b/scripts/pahole-flags.sh
>>>>>>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>>>>>>>>>>            # see PAHOLE_HAS_LANG_EXCLUDE
>>>>>>>>>>            extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>>>>>>>>>>     fi
>>>>>>>>>> +if [ "${pahole_ver}" -ge "125" ]; then
>>>>>>>>>> +       extra_paholeopt="${extra_paholeopt}
>>>>>>>>>> --skip_encoding_btf_inconsistent_proto
>>>>>>>>>> --btf_gen_optimized"
>>>>>>>>>> +fi
>>>>>>>>>>
>>>>>>>>>>     echo ${extra_paholeopt}
>>>>>>>>>> -- 
>>>>>>>>>> 2.31.1
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That change looks like a workaround to me.
>>>>>>>>> There may be multiple functions that have the same proto, e.g.:
>>>>>>>>>
>>>>>>>>>      $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
>>>>>>>>> kernel/bpf/ net/core/
>>>>>>>>>      kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
>>>>>>>>> bpf_iter_aux_info *aux)
>>>>>>>>>      net/core/bpf_sk_storage.c:static void
>>>>>>>>> bpf_iter_detach_map(struct
>>>>>>>>> bpf_iter_aux_info *aux)
>>>>>>>>>
>>>>>>>>>      $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
>>>>>>>>> bpf_iter_detach_map
>>>>>>>>>      [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
>>>>>>>>>      'aux' type_id=2638
>>>>>>>>>      [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static
>>>>>>>>>
>>>>>>>>> We don't know which one it is in the BTF.
>>>>>>>>> However, I'm not against this change, as it can avoid some issues.
>>>>>>>>>
>>>>>>>>
>>>>>>>> In the above case, the BTF representation is consistent though.
>>>>>>>> That is, if I attach fentry progs to either of these functions
>>>>>>>> based on that BTF representation, nothing will crash.
>>>>>>>>
>>>>>>>> That's ultimately what those changes are about; ensuring
>>>>>>>> consistency in BTF representation, so when a function is in
>>>>>>>> BTF we can know the signature of the function can be safely
>>>>>>>> used by fentry for example.
>>>>>>>>
>>>>>>>> The question of being able to identify functions (as opposed
>>>>>>>> to having a consistent representation) is the next step.
>>>>>>>> Finding a way to link between kallsyms and BTF would allow us to
>>>>>>>> have multiple inconsistent functions in BTF, since we could map
>>>>>>>> from BTF -> kallsyms safely. So two functions called "foo"
>>>>>>>> with different function signatures would be okay, because
>>>>>>>> we'd know which was which in kallsyms and could attach
>>>>>>>> safely. Something like a BTF tag for the function that could
>>>>>>>> clarify that mapping - but just for cases where it would
>>>>>>>> otherwise be ambiguous - is probably the way forward
>>>>>>>> longer term.
>>>>>>>>
>>>>>>>> Jiri's talking about this topic at LSF/MM/BPF this week I believe.
>>>>>>>
>>>>>>> Jiri presented a few ideas during LSFMMBPF.
>>>>>>>
>>>>>>> I feel the best approach is to add a set of addr-s to BTF
>>>>>>> via a special decl_tag.
>>>>>>> We can also consider extending KIND_FUNC.
>>>>>>> The advantage that every BTF func will have one or more addrs
>>>>>>> associated with it and bpf prog loading logic wouldn't need to do
>>>>>>> fragile name comparison between btf and kallsyms.
>>>>>>> pahole can take addrs from dwarf and optionally double check
>>>>>>> with kallsyms.
>>>>>>
>>>>>> Yonghong summed it up in another email discussion, pasting it in here:
>>>>>>
>>>>>>      So overall we have three options as kallsyms representation now:
>>>>>>        (a) "addr module:foo:dir_a/dir_b/core.c"
>>>>>>        (b) "addr module:foo"
>>>>>>        (c) "addr module:foo:btf_id"
>>>>>>
>>>>>>      option (a):
>>>>>>        'dir_a/dir_b/core.c' needs to be encoded in BTF.
>>>>>>        user space either check file path or func signature
>>>>>>        to find attach_btf_id and pass to the kernel.
>>>>>>        kernel can find file path in BTF and then lookup
>>>>>>        kallsyms to find addr.
>>>>>>
> 
> I like the source-centric nature of this, but the only
> danger here is we might not get a 1:1 mapping between
> source file location and address; consider the case
> of a static inline function in a .h file which doesn't
> get inlined. It could have multiple addresses associated
> with the same source. For example:
> 
> static inline void __list_del_entry(struct list_head *entry)
> {
> 	if (!__list_del_entry_valid(entry))
> 		return;
> 
> 	__list_del(entry->prev, entry->next);
> }
> 
> $ grep __list_del_entry /proc/kallsyms
> ffffffff982cc5c0 t __pfx___list_del_entry
> ffffffff982cc5d0 t __list_del_entry
> ffffffff985b0860 t __pfx___list_del_entry
> ffffffff985b0870 t __list_del_entry
> ffffffff9862d800 t __pfx___list_del_entry
> ffffffff9862d810 t __list_del_entry
> ffffffff987d3dd0 t __pfx___list_del_entry
> ffffffff987d3de0 t __list_del_entry
> ffffffff987f37a0 T __pfx___list_del_entry_valid
> ffffffff987f37b0 T __list_del_entry_valid
> ffffffff989fdd10 t __pfx___list_del_entry
> ffffffff989fdd20 t __list_del_entry
> ffffffff99baf08c r __ksymtab___list_del_entry_valid
> ffffffffc12da2e0 t __list_del_entry	[bnep]
> ffffffffc12da2d0 t __pfx___list_del_entry	[bnep]
> ffffffffc092d6b0 t __list_del_entry	[videobuf2_common]
> ffffffffc092d6a0 t __pfx___list_del_entry	[videobuf2_common]

Until now, we are okay with static inline function carried
into .o file since all inline functions are marked as notrace
(fentry cannot attach to it.)

However, now we have
https://lore.kernel.org/live-patching/20230502164102.1a51cdb4@gandalf.local.home/T/#u

If the patch is merged, then the above inline function
in the header survived in .o file indeed a problem.

Typically users won't trace inline functions in
the header file. But for completeness, agree that
file path may not fully reliable.

> 
>>>>>>      option (b):
>>>>>>        "addr" needs to be encoded in BTF.
>>>>>>        user space checks func signature to find
>>>>>>        attach_btf_id and pass to the kernel.
>>>>>>        kernel can find addr in BTF and use it.
>>>>>>
> 
> This seems like the safest option to me. Ideally we wouldn't
> need such information for every function - only ones with
> multiple sites and ambiguous signatures - but the problem
> is a function could have the same name in a kernel and
> a module too. So it seems like we're stuck with providing
> additional info to clarify which signature goes with which
> function for each static function.

When passing attach_btf_id to kernel, we have attach_btf_obj_fd
as well, which should differentiate whether it is kernel or which module 
it is.

> 
>>>>>>      option (c):
>>>>>>        if user can decide which function to attach, e.g.,
>>>>>>        through func signature, then no BTF encoding
>>>>>>        is necessary. attach_btf_id is passed to the
>>>>>>        kernel and search kallsyms to find the matching
>>>>>>        btf_id and 'addr' will be available then.
>>>>>>
>>>>>>      For option (b) and (c), user space needs to check
>>>>>>      func signature to find which btf_id to use. If
>>>>>>      same-name static functions having the identical
>>>>>>      signatures, then user space would have a hard time
>>>>>>      to differentiate. I think it should be very
>>>>>>      rare same-name static functions in the kernel will have
>>>>>>      identical signatures. But if we want 100% correctness,
>>>>>>      we may need file path in which case option (a)
>>>>>>      is preferable.
>>>>>
>>>>> As Alexei mentioned in previous email, for such a extreme case,
>>>>> if user is willing to go through extra step to check dwarf
>>>>> to find and match file path, then (b) and (c) should work
>>>>> perfectly as well.
>>>>
>>>> Okay, it looks like this is more complex if the function signature is
>>>> the same. In such cases, current BTF dedup will merge these
>>>> functions as a single BTF func. In such cases, we could have:
>>>>
>>>>      decl_tag_1   ----> dedup'ed static_func
>>>>                            ^
>>>>                            |
>>>>      decl_tag_2   ---------
>>>>
>>>> For such cases, just passing btf_id of static func to kernel
>>>> won't work since the kernel won't be able to know which
>>>> decl_tag to be associated with.
>>>>
>>>> (I did a simple test with vmlinux, it looks we have
>>>>    issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func
>>>>    as well since only one of decl_tag survives.
>>>>    But this is a different issue.
>>>> )
>>>>
>>>> So if we intend to add decl tag (addr or file_path), we
>>>> should not dedup static functions or generally any functions.
>>>
>>> I did not think functions would be dedup-ed, they are ;-) with the
>>> declaration tags in place we could perhaps switch it off, right?
>>
>> That is my hope too. If with decl tag func won't be dedup'ed,
>> then we should be okay. In the kernel, based on attach_btf_id,
>> through btf, kernel can find the corresponding decl tag (file path
>> or addr) or through attach_btf_id itself if the btf id is
>> encoded in kallsym entries.
>>
>>>
>>> or perhaps I can't think of all the cases we need functions dedup for,
>>> so maybe the dedup code could check also the associated decl tag when
>>> comparing functions
>>>
> 
> Would using the BTF decl tag id instead of the function BTF id to
> guide attachment be an option? That way if multiple functions shared
> the same signature, we could still get the info to figure out which to
> attach to from the decl tag, and we wouldn't need to mess with dedup.
> I'm probably missing something which makes that unworkable, but just a
> thought. Thanks!

The issue is due to current bpf syscall UAPI. it passed attach_btf_id
and attach_btf_obj_fd to the kernel. If we change UAPI to pass
decl_tag_id instead of attach_btf_id to the kernel, we should be
okay, but this requires a bpf syscall UAPI change. Maybe this works too.

> 
> Alan

Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Wed, 10 May 2023 14:02:41 +0100 you wrote:
> v1.25 of pahole supports filtering out functions with multiple inconsistent
> function prototypes or optimized-out parameters from the BTF representation.
> These present problems because there is no additional info in BTF saying which
> inconsistent prototype matches which function instance to help guide attachment,
> and functions with optimized-out parameters can lead to incorrect assumptions
> about register contents.
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25
    https://git.kernel.org/bpf/bpf-next/c/7b99f75942da

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html