Re: [PATCH dwarves v3 7/9] dwarf_loader: Handle expression lists

Yonghong Song
2026-03-22T18:34:17Z

On 3/21/26 4:10 PM, Jiri Olsa wrote:
> On Fri, Mar 20, 2026 at 12:09:53PM -0700, Yonghong Song wrote:
>
> SNIP
>
>> + if (byte_size <= cu->addr_size || !cu->agg_use_two_regs) {
>> + switch (expr[0].atom) {
>> + case DW_OP_reg0 ... DW_OP_reg31:
>> + if (loc_num != 0)
>> + break;
>> + *ret = expr[0].atom;
>> + if (*ret == expected_reg)
>> + return *ret;
>> + break;
>> + case DW_OP_breg0 ... DW_OP_breg31:
>> + if (loc_num != 0)
>> + break;
>> + bool has_op_stack_value = false;
>> + for (int i = 1; i < exprlen; i++) {
>> + if (expr[i].atom == DW_OP_stack_value) {
>> + has_op_stack_value = true;
>> + break;
>> + }
>> + }
>> + if (!has_op_stack_value)
>> + break;
>> + /* The existence of DW_OP_stack_value means that
>> + * DW_OP_bregX register is used as value.
>> + */
>> + *ret = expr[0].atom - DW_OP_breg0 + DW_OP_reg0;
>> + if (*ret == expected_reg)
>> + return *ret;
>> + }
>> + } else {
>> + /* cu->addr * 2 */
>> + int off = 0;
>> + for (int i = 0; i < exprlen; i++) {
>> + if (expr[i].atom == DW_OP_piece) {
>> + int num = expr[i].number;
>> + if (i == 0) {
>> + off = num;
>> + continue;
>> + }
>> + if (off < cu->addr_size) (*lower_half) |= (1 << off);
>> + else (*upper_half) |= (1 << (off - cu->addr_size));
>> + off += num;
> this is really hard for me to read.. I think it needs to follow common
> formatting rules and it deserves some explanation either in comments
> or in changelog

Okay, I will add comments to explain what it is.

>
>> + } else if (expr[i].atom >= DW_OP_reg0 && expr[i].atom <= DW_OP_reg31) {
>> + if (off < cu->addr_size)
>> + *ret = expr[i].atom;
>> + else if (*ret < 0)
>> + *ret = expr[i].atom;
>> + }
>> + /* FIXME: not handling DW_OP_bregX yet since we do not have
>> + * a use case for it yet for linux kernel.
>> + */
>> + }
>> + }
>> +
>> return PARM_CONTINUE;
>> }
>>
>> +/* The lower_half and upper_half, computed in parameter__multi_exprs(), are handled here.
>> + */
>> +static int parameter__handle_two_addr_len(int expected_reg, unsigned long lower_half, unsigned long upper_half,
>> + int ret, Dwarf_Die *die, struct conf_load *conf, struct cu *cu,
>> + struct parameter *parm) {
>> + if (!lower_half && !upper_half)
>> + return ret;
>> +
>> + if (ret != expected_reg)
>> + return ret;
>> +
>> + if (!conf->true_signature)
>> + return PARM_DEFAULT_FAIL;
>> +
>> + /* Both halfs are used based on dwarf */
>> + if (lower_half && upper_half)
>> + return PARM_TWO_ADDR_LEN;
>> +
>> + /* FIXME: parm->name may be NULL due to abstract origin. We do not want to
>> + * update abstract origin as the type in abstract origin may be used
>> + * in some other places. We could remove abstract origin in this parameter
>> + * and add name and type in parameter itself. Right now, for current bpf-next
>> + * repo, we do not have instances below where parm->name is NULL for x86_64 arch.
>> + */
>> + if (!parm->name)
>> + return PARM_TO_BE_IMPROVED;
>> +
>> + /* FIXME: Only support single field now so we can have a good parameter name and
>> + * type for it.
>> + */
>> + if (__builtin_popcountll(lower_half) >= 2 || __builtin_popcountll(upper_half) >= 2)
>> + return PARM_TO_BE_IMPROVED;
>> +
>> + int field_offset;
>> + if (__builtin_popcountll(lower_half) == 1)
>> + field_offset = __builtin_ctzll(lower_half);
>> + else
>> + field_offset = cu->addr_size + __builtin_ctzll(upper_half);
>> +
>> + /* FIXME: Only struct type is supported. */
>> + Dwarf_Die member_die;
>> + if (!get_member_with_offset(die, field_offset, &member_die))
>> + return PARM_TO_BE_IMPROVED;
>> +
>> + const char *member_name = attr_string(&member_die, DW_AT_name, conf);
>> + int len = sizeof(parm->name) + strlen(member_name) + 3;
> this seems wrong, shoud be strlen for parm->name? maybe asprintf is
> better option in here?

Thanks for spotting this. It should be strlen. asprintf is even better.

>
>> + char *new_name = malloc(len);
> also there's cu__malloc, and we should check if the allocation failed

Ack

>
>> + sprintf(new_name, "%s__%s", parm->name, member_name);
>> + parm->name = new_name;
> I wonder this will leak, because normally the name is allocated with
> dwarf_formstring and we don't need to free it, but now we do

This will leak. I thought the number of functions for this pattern
is limited. But nevertheless, leaking is not good.

I will wait a little bit for more reviews before sending version 4.
Thanks for review!

>
> jirka