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

On Fri, Jan 23, 2026 at 05:26:46PM +0000, Alan Maguire wrote:
> 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]>

Looks good.

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

Oh, I like how you've split up the various skip reasonings.

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