Re: [PATCH 1/2] libtraceevent: Add loading of BTF to the tep handle

On 04-08-2025 15:41, Steven Rostedt wrote:
> On Mon, 4 Aug 2025 15:18:41 +0100
> Douglas Raillard <[email protected]> wrote:
> 
>>>>> +		case BTF_KIND_INT:
>>>>> +			encode = *(int *)((void *)t + sizeof(*t));
>>>>> +			/* Print unsigned ints as hex */
>>>>> +			if (BTF_INT_ENCODING(encode) & BTF_INT_SIGNED)
>>>>
>>>> BTF_INT_OFFSET() and BTF_INT_VAL() values should also be used to shift and mask
>>>> appropriately.
>>>
>>> You meant to the "arg" passed in?
>>
>> Yes, something like that:
>>
>>     unsigned long long mask = (1 << BTF_INT_BITS(encode)) - 1;
>>     unsigned long long shift = BTF_INT_OFFSET(encode);
>>     arg = (arg >> shift) & mask;
>>
>> I suppose that how BTF_INT_OFFSET() is to be used depends on endianness, and I'm not
>> sure what is the endianness of the arg value. t->size would probably be involved as
>> well in the big endian case: https://docs.kernel.org/bpf/btf.html#btf-kind-int
> 
> Hmm, I guess I'll need to update that. The endianness is stored in the
> tep_handle descriptor and will need to be used. Note, this needs to handle
> big endian machine reading little endian machine data and vice versa. As
> well as little and big reading their own endianness.
> 
> Too bad my big endian machine died. I used that to test all the combinations.
> 
>>
>>>    
>>>>
>>>> I assume that regardless of BTF_INT_CHAR and BTF_INT_BOOL value, BTF_INT_SIGNED is
>>>> set appropriately.
>>>>   
>>>>> +				trace_seq_printf(s, "%lld", arg);
>>>>> +			else
>>>>> +				trace_seq_printf(s, "0x%llx", arg);
>>>>> +			break;
>>>>> +		case BTF_KIND_ENUM:
>>>>
>>>> Could add as well: case BTF_KIND_ENUM64:
>>>
>>> Of course then we would need to check if this is a 32 bit infrastructure.
>>> I'm assuming it would be treated differently. More like a struct?
>>
>> AFAIR the only point of BTF_KIND_ENUM64 is to represent enum with enumerator values that are
>> larger than 32 bits, since BTF_KIND_ENUM can only represent 32 bit values:
>>
>>     struct btf_enum {
>>         __u32   name_off;
>>         __s32   val;
>>     };
>>
>> In contrast, BTF_KIND_ENUM64 encodes the enumerators with this:
>>
>>     struct btf_enum64 {
>>         __u32   name_off;
>>         __u32   val_lo32;
>>         __u32   val_hi32;
>>     };
>>
>> So each value can have 64bits, split over 2 fields. I assume this quirk is
>> just to have alignof(struct btf_enum64) == 4 so that the BTF wire format
>> can be mapped on that C struct easily.
>>
>> AFAIR BTF_KIND_ENUM encoding was simply an oversight in the initial BTF spec, so they corrected it
>> by adding another BTF_KIND_*.
>>
>> Since you make no use of the enumerators array for now, I think the handling is identical.
>>
>>
>> Wrt to 32 vs 64 bits, in ISO C, the enum size does not depend on the ABI:
>> https://port70.net/~nsz/c/c11/n1570.html#6.7.2.2p4
> 
> When I brought up 64 bit, I meant with the parameters being passed in. As
> this is for parsing function parameters. On 32bit an enum64 would require
> two registers whereas on 64bit it would only require one.

Ah yes hadn't thought of that. From a godbolt experiment, both C and Rust seem
to just use 2 registers in the arm 32 bits case, like a struct.

> -- Steve