Re: [PATCH dwarves 2/9] dwarf_loader: Handle signatures with dead arguments

On 3/19/26 10:00 PM, Yonghong Song wrote:
>
>
> On 3/19/26 11:55 AM, Alan Maguire wrote:
>> On 05/03/2026 22:55, Yonghong Song wrote:
>>> For llvm dwarf, the dead argument may be in the middle of
>>> DW_TAG_subprogram. So we introduce skip_idx in order to
>>> match expected registers properly.
>>>
>>> For example:
>>>    0x00042897:   DW_TAG_subprogram
>>>                    DW_AT_name      ("create_dev")
>>>                    DW_AT_calling_convention (DW_CC_nocall)
>>>                    DW_AT_type      (0x0002429a "int")
>>>                    ...
>>>
>>>    0x000428ab:     DW_TAG_formal_parameter
>>>                      DW_AT_name    ("name")
>>>                      DW_AT_type    (0x000242ed "char *")
>>>                      ...
>>>
>>>    0x000428b5:     DW_TAG_formal_parameter
>>>                      DW_AT_location        (indexed (0x3f) loclist = 
>>> 0x000027f8:
>>>                         [0xffffffff87681370, 0xffffffff8768137a): 
>>> DW_OP_reg5 RDI
>>>                         [0xffffffff8768137a, 0xffffffff87681392): 
>>> DW_OP_reg3 RBX
>>>                         [0xffffffff87681392, 0xffffffff876813ae): 
>>> DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)
>>>                      DW_AT_name    ("dev")
>>>                      DW_AT_type    (0x00026859 "dev_t")
>>>                      ...
>>>
>>> With skip_idx, we can identify that the second original argument
>>> 'dev' becomes the first one after optimization.
>>>
>>> Signed-off-by: Yonghong Song <[email protected]>
>>> ---
>>>   dwarf_loader.c | 27 +++++++++++++++++++++++----
>>>   1 file changed, 23 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>>> index 610b69e..1ced5e2 100644
>>> --- a/dwarf_loader.c
>>> +++ b/dwarf_loader.c
>>> @@ -1192,6 +1192,7 @@ static ptrdiff_t 
>>> __dwarf_getlocations(Dwarf_Attribute *attr,
>>>     struct func_info {
>>>       bool signature_changed;
>>> +    int skip_idx;
>>>   };
>>>     /* For DW_AT_location 'attr':
>>> @@ -1264,13 +1265,28 @@ static struct parameter 
>>> *parameter__new(Dwarf_Die *die, struct cu *cu,
>>>       if (parm != NULL) {
>>>           bool has_const_value;
>>>           Dwarf_Attribute attr;
>>> +        int reg_idx;
>>>             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)
>>> +        if (param_idx < 0)
>>>               return parm;
>>> -        if (cu->producer_clang && !info->signature_changed)
>>> +        if (!cu->producer_clang && param_idx >= 
>>> cu->nr_register_params)
>>> +            return parm;
>>> +        if (cu->producer_clang) {
>>> +            if (!info->signature_changed)
>>> +                return parm;
>>> +            /* if true_signature is not enabled, mark parameter as
>>> +             * unexpected_reg since there is a skipped parameter 
>>> before.
>>> +             */
>>> +            if (!conf->true_signature && info->skip_idx) {
>>> +                parm->unexpected_reg = 1;
>>> +                return parm;
>>> +            }
>>> +        }
>>> +        reg_idx = param_idx - info->skip_idx;
>>> +        if (reg_idx >= cu->nr_register_params)
>>>               return parm;
>>>           /* Parameters which use DW_AT_abstract_origin to point at
>>>            * the original parameter definition (with no name in the 
>>> DIE)
>>> @@ -1309,7 +1325,7 @@ static struct parameter 
>>> *parameter__new(Dwarf_Die *die, struct cu *cu,
>>>           parm->has_loc = dwarf_attr(die, DW_AT_location, &attr) != 
>>> NULL;
>>>             if (parm->has_loc) {
>>> -            int expected_reg = cu->register_params[param_idx];
>>> +            int expected_reg = cu->register_params[reg_idx];
>>>               int actual_reg = parameter__reg(&attr, expected_reg);
>>>                 if (actual_reg < 0)
>>> @@ -1322,8 +1338,11 @@ static struct parameter 
>>> *parameter__new(Dwarf_Die *die, struct cu *cu,
>>>                    * contents.
>>>                    */
>>>                   parm->unexpected_reg = 1;
>>> -        } else if (has_const_value) {
>>> +        } else if (!cu->producer_clang && has_const_value) {
>>> +            parm->optimized = 1;
>>> +        } else if (cu->producer_clang) {
>>>               parm->optimized = 1;
>>> +            info->skip_idx++;
>>>           }
>>>       }
>> In [1]  (dwarf_loader/btf_encoder: Detect reordered parameters)
>>
>> we detect reordered/missing parameters for gcc by comparing abstract 
>> origin to concrete representation
>> and mark any such functions by setting the reordered_parm bitfield in 
>> the encoder func state. In this
>> approach reordered encompasses both missing and actual reordering; in 
>> practice it's always the former, but
>> the DWARF representation was confusing because it had the 
>> actually-used parameters (with location info)
>> followed by the unused ones (without location info). Before the above 
>> commit we were treating these
>> function signatures incorrectly as valid leading to wrong functions 
>> signatures.
>>
>> All of this is to say: should we mark with reordered_parm instead? 
>> The reason I suggest this is that
>> btf_encoder__save_func() has handling for skipping functions without 
>> locations with reordered_parm set;
>> maybe we could find a way to unify that across clang/gcc cases?
>
> For v2, I focused on clang side. I have not studied such unification 
> yet. I think it is possible
> since ultimately both clang and gcc tries to skip some parameters.
>
>>
>> Now in the clang case, I guess reordered is a poor description; for 
>> gcc it really means "reordered as
>> compared to abstract origin". If we could find a better way to 
>> describe/encompass both cases that would
>> be ideal I think. The terminology used in the gcc true signature code 
>> isn't great today.
>
> Agree that we want *common* names to represent both gcc and clang for 
> skipped parameters.
> BTW, please review v2 instead:
> https://lore.kernel.org/bpf/[email protected]/ 
>

Jiri, Alan,

Thanks for the previous review. I just updated with v3:

   https://lore.kernel.org/bpf/[email protected]/

which addressed the issue to simplify getting cu->producer_clang, rewrite tests
based on latest convention, and try to use info->signature_changed instead of cu->producer_clang
to avoid clear separation between gcc and clang.

Please review v3.

Thanks!

>
> Thanks!
>
>>
>> [1] 
>> https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?id=109e5e4554f663e5f12fb634bc54cceb710aec61
>
>