From: Alan Maguire <alan.maguire@oracle.com>
To: Jiri Olsa <olsajiri@gmail.com>
Cc: acme@kernel.org, ast@kernel.org, andrii@kernel.org,
eddyz87@gmail.com, qmo@kernel.org, dwarves@vger.kernel.org,
daniel@iogearbox.net, ihor.solodrai@linux.dev,
yonghong.song@linux.dev, song@kernel.org, martin.lau@linux.dev,
memxor@gmail.com, emil@etsalapatis.com, bpf@vger.kernel.org,
nsc@kernel.org, puranjay@kernel.org, yatsenko@meta.com
Subject: Re: [PATCH v3 dwarves 1/9] dwarf_loader: Add parameter list to inlined expansions
Date: Fri, 2 Oct 2026 12:02:53 +0100 [thread overview]
Message-ID: <210d44c8-2404-4fe8-8084-31940d8aeece@oracle.com> (raw)
In-Reply-To: <ar0umERDiFj2sN3W@krava>
On 30/09/2026 16:45, Jiri Olsa wrote:
> On Fri, Sep 25, 2026 at 07:51:50PM +0100, Alan Maguire wrote:
>
> SNIP
>
>> -static int die__process_inline_expansion(Dwarf_Die *die, struct lexblock *lexblock, struct cu *cu, struct conf_load *conf)
>> +static int die__process_inline_expansion(Dwarf_Die *die,
>> + struct inline_expansion *exp,
>> + struct lexblock *lexblock,
>> + struct cu *cu, struct conf_load *conf)
>> {
>> Dwarf_Die child;
>> struct tag *tag;
>> + int parm_idx = 0;
>>
>> if (!dwarf_haschildren(die) || dwarf_child(die, &child) != 0)
>> return 0;
>> @@ -2957,6 +2965,7 @@ static int die__process_inline_expansion(Dwarf_Die *die, struct lexblock *lexblo
>> die = &child;
>> do {
>> uint32_t id;
>> + bool add_to_inline_expansion = false;
>>
>> switch (dwarf_tag(die)) {
>> case DW_TAG_call_site:
>> @@ -2975,13 +2984,10 @@ static int die__process_inline_expansion(Dwarf_Die *die, struct lexblock *lexblo
>> goto out_enomem;
>> continue;
>> case DW_TAG_formal_parameter:
>> - /*
>> - * Inline expansions can have their own formal
>> - * parameter children duplicating the abstract
>> - * origin's parameters. These are not needed
>> - * for type reconstruction — skip them.
>> - */
>> - continue;
>> + tag = die__create_new_parameter(die, NULL, lexblock, exp,
>> + cu, conf, parm_idx++);
>> + add_to_inline_expansion = true;
>> + break;
>> case DW_TAG_inlined_subroutine:
>> tag = die__create_new_inline_expansion(die, lexblock, cu, conf);
>> break;
>> @@ -3010,6 +3016,8 @@ static int die__process_inline_expansion(Dwarf_Die *die, struct lexblock *lexblo
>>
>> if (cu__table_add_tag(cu, tag, &id) < 0)
>> goto out_delete_tag;
>> + if (add_to_inline_expansion)
>
> could we just check (tag->tag == DW_TAG_formal_parameter) instead the
> new bool?
>
yep, will fix.
>
>> + inline_expansion__add_parameter(exp, tag__parameter(tag));
>> hash:
>> cu__hash(cu, tag);
>> struct dwarf_tag *dtag = tag__dwarf(tag);
>> @@ -3032,8 +3040,8 @@ static struct tag *die__create_new_inline_expansion(Dwarf_Die *die,
>> if (exp == NULL)
>> return NULL;
>>
>> - if (die__process_inline_expansion(die, lexblock, cu, conf) != 0) {
>> - tag__free(&exp->ip.tag, cu);
>> + if (die__process_inline_expansion(die, exp, lexblock, cu, conf) != 0) {
>> + tag__delete(&exp->ip.tag, cu);
>> return NULL;
>> }
>>
>> @@ -3115,7 +3123,8 @@ static int die__process_function(Dwarf_Die *die, struct ftype *ftype,
>> continue;
>> }
>> case DW_TAG_formal_parameter:
>> - tag = die__create_new_parameter(die, ftype, lexblock, cu, conf, param_idx++);
>> + tag = die__create_new_parameter(die, ftype, lexblock, NULL,
>> + cu, conf, param_idx++);
>> break;
>> case DW_TAG_variable:
>> tag = die__create_new_variable(die, cu, conf, 0);
>> @@ -3562,75 +3571,103 @@ static void __tag__print_abstract_origin_not_found(struct tag *tag,
>> #define tag__print_abstract_origin_not_found(tag) \
>> __tag__print_abstract_origin_not_found(tag, __func__, __LINE__)
>>
>> -static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu)
>> +static void parameter__share_state_with_abstract_origin(struct parameter *parm,
>> + struct parameter *oparm)
>> +{
>> + if (parm->has_loc)
>> + oparm->has_loc = parm->has_loc;
>> + if (parm->has_const_value)
>> + oparm->has_const_value = parm->has_const_value;
>> + if (parm->loc_const_value)
>> + oparm->loc_const_value = parm->loc_const_value;
>> + if (parm->loc_stack)
>> + oparm->loc_stack = parm->loc_stack;
>> + if (parm->loc_reg != PARAMETER_UNKNOWN_REG)
>> + oparm->loc_reg = parm->loc_reg;
>> + if (parm->type_byte_size != 0)
>> + oparm->type_byte_size = parm->type_byte_size;
>> + if (parm->passed_in_memory)
>> + oparm->passed_in_memory = parm->passed_in_memory;
>> + oparm->first_reg_fields |= parm->first_reg_fields;
>> + oparm->second_reg_fields |= parm->second_reg_fields;
>> + if (parm->true_sig_member_name && !oparm->true_sig_member_name) {
>> + oparm->true_sig_member_name = parm->true_sig_member_name;
>> + oparm->true_sig_type = parm->true_sig_type;
>> + oparm->true_sig_type_from_types = parm->true_sig_type_from_types;
>> + }
>> + if (parm->optimized)
>> + oparm->optimized = parm->optimized;
>> + if (parm->unexpected_reg)
>> + oparm->unexpected_reg = parm->unexpected_reg;
>> +}
>> +
>> +static void parameter__recode_dwarf_type(struct parameter *parm, struct cu *cu,
>> + struct ftype *ftype)
>
> could this be added in separate change? seems like it could
> ease up the readability a bit
>
ok so a separate patch to rework recoding/sharing with abstract origin?
Sure.
>> {
>> - struct parameter *pos;
>> struct dwarf_cu *dcu = cu->priv;
>> - struct ftype *type = tag__ftype(tag);
>> -
>> - ftype__for_each_parameter(type, pos) {
>> - struct dwarf_tag *dpos = tag__dwarf(&pos->tag);
>> - struct parameter *opos;
>> - struct dwarf_tag *dtype;
>> + struct dwarf_tag *dparm = tag__dwarf(&parm->tag);
>> + struct dwarf_tag *dtype;
>>
>> - if (dpos->type == 0) {
>> - if (dpos->abstract_origin == 0) {
>> - /* Function without parameters */
>> - pos->tag.type = 0;
>> - continue;
>> - }
>> - dtype = dwarf_cu__find_tag_by_ref(dcu, dpos, abstract_origin);
>> - if (dtype == NULL) {
>> - tag__print_abstract_origin_not_found(&pos->tag);
>> - continue;
>> - }
>> - 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
>> - * mark the parameter as optimized out. Also share
>> - * info regarding unexpected register use for
>> - * parameters.
>> - */
>> - if (pos->has_loc)
>> - opos->has_loc = pos->has_loc;
>> - if (pos->has_const_value)
>> - opos->has_const_value = pos->has_const_value;
>> - if (pos->loc_const_value)
>> - opos->loc_const_value = pos->loc_const_value;
>> - if (pos->loc_stack)
>> - opos->loc_stack = pos->loc_stack;
>> - if (pos->loc_reg != PARAMETER_UNKNOWN_REG)
>> - opos->loc_reg = pos->loc_reg;
>> - if (pos->type_byte_size != 0)
>> - opos->type_byte_size = pos->type_byte_size;
>> - if (pos->passed_in_memory)
>> - opos->passed_in_memory = pos->passed_in_memory;
>> - opos->first_reg_fields |= pos->first_reg_fields;
>> - opos->second_reg_fields |= pos->second_reg_fields;
>> - if (pos->true_sig_member_name && !opos->true_sig_member_name) {
>> - opos->true_sig_member_name = pos->true_sig_member_name;
>> - opos->true_sig_type = pos->true_sig_type;
>> - opos->true_sig_type_from_types = pos->true_sig_type_from_types;
>> - }
>> + if (dparm->type == 0) {
>> + struct parameter *oparm;
>>
>> - if (pos->optimized)
>> - opos->optimized = pos->optimized;
>> - if (pos->unexpected_reg)
>> - opos->unexpected_reg = pos->unexpected_reg;
>> - continue;
>> + if (dparm->abstract_origin == 0) {
>> + parm->tag.type = 0;
>> + return;
>> }
>>
>> - dtype = dwarf_cu__find_type_by_ref(dcu, dpos, type);
>> + dtype = dwarf_cu__find_tag_by_ref(dcu, dparm, abstract_origin);
>> if (dtype == NULL) {
>> - tag__print_type_not_found(&pos->tag);
>> - continue;
>> + tag__print_abstract_origin_not_found(&parm->tag);
>> + return;
>> }
>> - pos->tag.type = dtype->small_id;
>> +
>> + oparm = tag__parameter(dtag__tag(dtype));
>> + parm->name = oparm->name;
>> + if (ftype != NULL && parm->idx != oparm->idx)
>> + ftype->reordered_parm = 1;
>
> looks like we could end up with wrong argument index if the dwarf
> info is missing for the argument, my agent is suggesting this fix
>
> it gets me over 16k changes like this:
>
> -0xffffffff8100e322 [__show_trace_log_lvl+0xb2, .text +0xe322] get_stack_pointer(struct task_struct * task [const 0], struct pt_regs * regs [%r14])
> +0xffffffff8100e322 [__show_trace_log_lvl+0xb2, .text +0xe322] get_stack_pointer(struct task_struct * task [%r14], struct pt_regs * regs [const 0])
>
> I tried to validate one against dwarf info and it seems
> to be correct with the fix ;-)
>
thanks for catching this! So the solution is something like this
oparm = tag__parameter(dtag__tag(dtype));
parm->name = oparm->name;
- if (ftype != NULL && parm->idx != oparm->idx)
- ftype->reordered_parm = 1;
+ if (ftype != NULL) {
+ if (parm->idx != oparm->idx)
+ ftype->reordered_parm = 1;
+ } else {
+ parm->idx = oparm->idx;
+ }
parm->tag.type = dtag__tag(dtype)->type;
to ensure that we map to source parameter order for inlines, right?
Thanks for reviewing!
Alan
next prev parent reply other threads:[~2026-10-02 11:03 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-25 18:51 [PATCH v3 dwarves 0/9] Encoding of inline functions using BTF Alan Maguire
2026-09-25 18:51 ` [PATCH v3 dwarves 1/9] dwarf_loader: Add parameter list to inlined expansions Alan Maguire
2026-09-30 15:45 ` Jiri Olsa
2026-10-02 11:02 ` Alan Maguire [this message]
2026-09-25 18:51 ` [PATCH v3 dwarves 2/9] dwarf_loader: Resolve inline expansion names during recoding Alan Maguire
2026-09-25 18:51 ` [PATCH v3 dwarves 3/9] dwarf_loader: Collect inline expansion location data Alan Maguire
2026-09-25 18:51 ` [PATCH v3 dwarves 4/9] pahole: Support inline BTF location encoding Alan Maguire
2026-09-25 18:51 ` [PATCH v3 dwarves 5/9] btf_loader: Add "inline" annotation for inlined functions Alan Maguire
2026-09-25 18:51 ` [PATCH v3 dwarves 6/9] btf_loader: Record inline sites in CU for BTF Alan Maguire
2026-09-25 18:51 ` [PATCH v3 dwarves 7/9] pfunct: Print inline site information Alan Maguire
2026-09-25 18:51 ` [PATCH v3 dwarves 8/9] tests: Validate inline BTF encoding Alan Maguire
2026-09-25 18:51 ` [PATCH v3 dwarves 9/9] man-pages: Document --inline_sites option Alan Maguire
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=210d44c8-2404-4fe8-8084-31940d8aeece@oracle.com \
--to=alan.maguire@oracle.com \
--cc=acme@kernel.org \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=dwarves@vger.kernel.org \
--cc=eddyz87@gmail.com \
--cc=emil@etsalapatis.com \
--cc=ihor.solodrai@linux.dev \
--cc=martin.lau@linux.dev \
--cc=memxor@gmail.com \
--cc=nsc@kernel.org \
--cc=olsajiri@gmail.com \
--cc=puranjay@kernel.org \
--cc=qmo@kernel.org \
--cc=song@kernel.org \
--cc=yatsenko@meta.com \
--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