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

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