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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.