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

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