[PATCH v3 dwarves 1/5] dwarf_loader/btf_encoder: Detect reordered parameters

When encoding concrete instances of optimized functions it is possible
parameters get reordered, often due to a parameter being optimized out;
in such cases the order of abstract origin references to the abstract
function is different, and the parameters that are optimized out
usually appear after all the non-optimized parameters with no
DW_AT_location information [1].

As an example consider

static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu);

It has - as expected - an abstract representation as follows:

 <1><6392a2d>: Abbrev Number: 47 (DW_TAG_subprogram)
    <6392a2e>   DW_AT_name        : (indirect string, offset: 0x261e25): __blkcg_rstat_flush
    <6392a32>   DW_AT_decl_file   : 1
    <6392a33>   DW_AT_decl_line   : 1043
    <6392a35>   DW_AT_decl_column : 13
    <6392a36>   DW_AT_prototyped  : 1
    <6392a36>   DW_AT_inline      : 1   (inlined)
    <6392a37>   DW_AT_sibling     : <0x6392bac>
 <2><6392a3b>: Abbrev Number: 38 (DW_TAG_formal_parameter)
    <6392a3c>   DW_AT_name        : (indirect string, offset: 0xa7a9f): blkcg
    <6392a40>   DW_AT_decl_file   : 1
    <6392a41>   DW_AT_decl_line   : 1043
    <6392a43>   DW_AT_decl_column : 47
    <6392a44>   DW_AT_type        : <0x638b611>
 <2><6392a48>: Abbrev Number: 20 (DW_TAG_formal_parameter)
    <6392a49>   DW_AT_name        : cpu
    <6392a4d>   DW_AT_decl_file   : 1
    <6392a4e>   DW_AT_decl_line   : 1043
    <6392a50>   DW_AT_decl_column : 58
    <6392a51>   DW_AT_type        : <0x6377f8f>

However the concrete representation after optimization becomes:

ffffffff8186d180 t __blkcg_rstat_flush.isra.0

and has a concrete representation with parameter order switched:

<1><6399661>: Abbrev Number: 110 (DW_TAG_subprogram)
    <6399662>   DW_AT_abstract_origin: <0x6392a2d>
    <6399666>   DW_AT_low_pc      : 0xffffffff8186d180
    <639966e>   DW_AT_high_pc     : 0x169
    <6399676>   DW_AT_frame_base  : 1 byte block: 9c    (DW_OP_call_frame_cfa)
    <6399678>   DW_AT_GNU_all_call_sites: 1
    <6399678>   DW_AT_sibling     : <0x6399a8a>
 <2><639967c>: Abbrev Number: 4 (DW_TAG_formal_parameter)
    <639967d>   DW_AT_abstract_origin: <0x6392a48>
    <6399681>   DW_AT_location    : 0x1fe21fb (location list)
    <6399685>   DW_AT_GNU_locviews: 0x1fe21f5
 <2><63996e4>: Abbrev Number: 4 (DW_TAG_formal_parameter)
    <63996e5>   DW_AT_abstract_origin: <0x6392a3b>
    <63996e9>   DW_AT_location    : 0x1fe2387 (location list)
    <63996ed>   DW_AT_GNU_locviews: 0x1fe2385

In other words we end up with

static void __blkcg_rstat_flush.isra(int cpu, struct blkcg *blkcg);

We are not detecting cases like this in pahole, so we need to
catch it to exclude such cases since they could lead to incorrect
fentry attachment.

Future work around true function signatures will allow such functions
with their "." suffixes, but even for such cases it is good to
detect the reordering.

In practice we just end up excluding a few more .isra/.constprop
functions which we cannot fentry-attach by name anyway; see [2] for an
example list from CI.

[1] https://lore.kernel.org/bpf/[email protected]/
[2] https://github.com/alan-maguire/dwarves/actions/runs/20031993822

Signed-off-by: Alan Maguire <[email protected]>
Acked-by: Yonghong Song <[email protected]>
Acked-by: Matt Bobrowski <[email protected]>
---
 btf_encoder.c  | 29 ++++++++++++++++++++---------
 dwarf_loader.c |  5 +++--
 dwarves.h      |  2 ++
 3 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index ec6933e..9a567e4 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -87,6 +87,7 @@ struct btf_encoder_func_state {
 	uint8_t unexpected_reg:1;
 	uint8_t inconsistent_proto:1;
 	uint8_t uncertain_parm_loc:1;
+	uint8_t reordered_parm:1;
 	uint8_t ambiguous_addr:1;
 	int ret_type_id;
 	struct btf_encoder_func_parm *parms;
@@ -1273,6 +1274,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
 	state->unexpected_reg = ftype->unexpected_reg;
 	state->optimized_parms = ftype->optimized_parms;
 	state->uncertain_parm_loc = ftype->uncertain_parm_loc;
+	state->reordered_parm = ftype->reordered_parm;
 	ftype__for_each_parameter(ftype, param) {
 		const char *name = parameter__name(param) ?: "";
 
@@ -1442,7 +1444,7 @@ static int saved_functions_combine(struct btf_encoder *encoder,
 				   struct btf_encoder_func_state *a,
 				   struct btf_encoder_func_state *b)
 {
-	uint8_t optimized, unexpected, inconsistent, uncertain_parm_loc;
+	uint8_t optimized, unexpected, inconsistent, uncertain_parm_loc, reordered_parm;
 
 	if (a->elf != b->elf)
 		return 1;
@@ -1451,12 +1453,14 @@ static int saved_functions_combine(struct btf_encoder *encoder,
 	unexpected = a->unexpected_reg | b->unexpected_reg;
 	inconsistent = a->inconsistent_proto | b->inconsistent_proto;
 	uncertain_parm_loc = a->uncertain_parm_loc | b->uncertain_parm_loc;
-	if (!unexpected && !inconsistent && !funcs__match(encoder, a, b))
+	reordered_parm = a->reordered_parm | b->reordered_parm;
+	if (!unexpected && !inconsistent && !reordered_parm && !funcs__match(encoder, a, b))
 		inconsistent = 1;
 	a->optimized_parms = b->optimized_parms = optimized;
 	a->unexpected_reg = b->unexpected_reg = unexpected;
 	a->inconsistent_proto = b->inconsistent_proto = inconsistent;
 	a->uncertain_parm_loc = b->uncertain_parm_loc = uncertain_parm_loc;
+	a->reordered_parm = b->reordered_parm = reordered_parm;
 
 	return 0;
 }
@@ -1494,7 +1498,7 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e
 
 	for (i = 0; i < nr_saved_fns; i = j) {
 		struct btf_encoder_func_state *state = &saved_fns[i];
-		bool add_to_btf = !skip_encoding_inconsistent_proto;
+		char *skip_reason = NULL;
 
 		/* Compare across sorted functions that match by name/prefix;
 		 * share inconsistent/unexpected reg state between them.
@@ -1510,14 +1514,21 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e
 		 * unexpected register use, multiple inconsistent prototypes or
 		 * uncertain parameters location
 		 */
-		add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto && !state->uncertain_parm_loc && !state->elf->ambiguous_addr;
-
+		if (state->unexpected_reg)
+			skip_reason = "unexpected register usage for parameter\n";
+		if (skip_encoding_inconsistent_proto && state->inconsistent_proto)
+			skip_reason = "inconsistet prototype\n";
 		if (state->uncertain_parm_loc)
-			btf_encoder__log_func_skip(encoder, saved_fns[i].elf,
-					"uncertain parameter location\n",
-					0, 0);
+			skip_reason = "uncertain parameter location\n";
+		if (state->reordered_parm)
+			skip_reason = "reordered parameters\n";
+		if (state->elf->ambiguous_addr)
+			skip_reason = "ambiguous address\n";
 
-		if (add_to_btf) {
+		if (skip_reason) {
+			btf_encoder__log_func_skip(encoder, saved_fns[i].elf,
+						   skip_reason, 0, 0);
+		} else {
 			if (is_kfunc_state(state))
 				err = btf_encoder__add_bpf_kfunc(encoder, state);
 			else
diff --git a/dwarf_loader.c b/dwarf_loader.c
index 77aab8a..16fb7be 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -1262,7 +1262,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
 
 		tag__init(&parm->tag, cu, die);
 		parm->name = attr_string(die, DW_AT_name, conf);
-
+		parm->idx = param_idx;
 		if (param_idx >= cu->nr_register_params || param_idx < 0)
 			return parm;
 		/* Parameters which use DW_AT_abstract_origin to point at
@@ -2636,6 +2636,8 @@ static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu)
 			}
 			opos = tag__parameter(dtag__tag(dtype));
 			pos->name = opos->name;
+			if (pos->idx != opos->idx)
+				type->reordered_parm = 1;
 			pos->tag.type = dtag__tag(dtype)->type;
 			/* share location information between parameter and
 			 * abstract origin; if neither have location, we will
@@ -2838,7 +2840,6 @@ static int tag__recode_dwarf_type(struct tag *tag, struct cu *cu)
 		lexblock__recode_dwarf_types(&fn->lexblock, cu);
 	}
 		/* Fall thru */
-
 	case DW_TAG_subroutine_type:
 		ftype__recode_dwarf_types(tag, cu);
 		/* Fall thru, for the function return type */
diff --git a/dwarves.h b/dwarves.h
index 21d4166..78bedf5 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -944,6 +944,7 @@ struct parameter {
 	uint8_t optimized:1;
 	uint8_t unexpected_reg:1;
 	uint8_t has_loc:1;
+	uint8_t idx;
 };
 
 static inline struct parameter *tag__parameter(const struct tag *tag)
@@ -1023,6 +1024,7 @@ struct ftype {
 	uint8_t		 processed:1;
 	uint8_t		 inconsistent_proto:1;
 	uint8_t		 uncertain_parm_loc:1;
+	uint8_t		 reordered_parm:1;
 	struct list_head template_type_params;
 	struct list_head template_value_params;
 	struct template_parameter_pack *template_parameter_pack;
-- 
2.43.5