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

Jiri Olsa
2026-03-21T23:10:13Z

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

> + } 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?

> + char *new_name = malloc(len);

also there's cu__malloc, and we should check if the allocation failed

> + 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

jirka