From: Matt Bobrowski <mattbobrowski@google.com>
To: Alan Maguire <alan.maguire@oracle.com>
Cc: yonghong.song@linux.dev, ihor.solodrai@linux.dev,
eddyz87@gmail.com, jolsa@kernel.org, andrii@kernel.org,
ast@kernel.org, david.faust@oracle.com, dwarves@vger.kernel.org,
bpf@vger.kernel.org
Subject: Re: [PATCH v2 dwarves 1/5] dwarf_loader/btf_encoder: Detect reordered parameters
Date: Mon, 26 Jan 2026 09:30:45 +0000 [thread overview]
Message-ID: <aXc0RdcUFM-UMSNZ@google.com> (raw)
In-Reply-To: <20260123172650.4062362-2-alan.maguire@oracle.com>
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/101b74c9-949a-4bf4-a766-a5343b70bdd2@oracle.com/
> [2] https://github.com/alan-maguire/dwarves/actions/runs/20031993822
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
Looks good.
Acked-by: Matt Bobrowski <mattbobrowski@google.com>
> ---
> 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
>
next prev parent reply other threads:[~2026-01-26 9:30 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-23 17:26 [PATCH v2 dwarves 0/5] Improve BTF concrete function accuracy Alan Maguire
2026-01-23 17:26 ` [PATCH v2 dwarves 1/5] dwarf_loader/btf_encoder: Detect reordered parameters Alan Maguire
2026-01-26 9:30 ` Matt Bobrowski [this message]
2026-01-23 17:26 ` [PATCH v2 dwarves 2/5] btf_encoder: Add true_signature feature support for "."-suffixed functions Alan Maguire
2026-01-23 19:05 ` Yonghong Song
2026-01-26 9:52 ` Matt Bobrowski
2026-01-26 10:49 ` Alan Maguire
2026-01-26 11:18 ` Matt Bobrowski
2026-01-27 12:18 ` Alan Maguire
2026-01-23 17:26 ` [PATCH v2 dwarves 3/5] test: add gcc true signature test Alan Maguire
2026-01-23 19:06 ` Yonghong Song
2026-01-23 17:26 ` [PATCH v2 dwarves 4/5] man-pages: document true_signature btf_feature Alan Maguire
2026-01-23 19:07 ` Yonghong Song
2026-01-26 10:02 ` Matt Bobrowski
2026-01-26 10:51 ` Alan Maguire
2026-01-26 11:21 ` Matt Bobrowski
2026-01-26 17:10 ` Alan Maguire
2026-01-26 18:13 ` Yonghong Song
2026-01-23 17:26 ` [PATCH v2 dwarves 5/5] btf_encoder: Prefer strong function definitions for BTF generation Alan Maguire
2026-01-23 19:09 ` Yonghong Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aXc0RdcUFM-UMSNZ@google.com \
--to=mattbobrowski@google.com \
--cc=alan.maguire@oracle.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=david.faust@oracle.com \
--cc=dwarves@vger.kernel.org \
--cc=eddyz87@gmail.com \
--cc=ihor.solodrai@linux.dev \
--cc=jolsa@kernel.org \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox