Re: [PATCH dwarves] dwarf_loader: encode char type as signed

[Yonghong Song <yhs@fb.com>]

On 8/8/22 3:52 PM, Andrii Nakryiko wrote:
> On Sun, Aug 7, 2022 at 10:53 AM Yonghong Song <yhs@fb.com> wrote:
>>
>> Currently, the pahole treats 'char' or 'signed char' type
>> as unsigned in BTF generation. The following is an example,
>>    $ cat t.c
>>    signed char a;
>>    char b;
>>    $ clang -O2 -g -c t.c
>>    $ pahole -JV t.o
>>    ...
>>    [1] INT signed char size=1 nr_bits=8 encoding=(none)
>>    [2] INT char size=1 nr_bits=8 encoding=(none)
>> In the above encoding '(none)' implies unsigned type.
>>
>> But if the same program is compiled with bpf target,
>>    $ clang -target bpf -O2 -g -c t.c
>>    $ bpftool btf dump file t.o
>>    [1] INT 'signed char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED
>>    [2] VAR 'a' type_id=1, linkage=global
>>    [3] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED
>>    [4] VAR 'b' type_id=3, linkage=global
>>    [5] DATASEC '.bss' size=0 vlen=2
>>            type_id=2 offset=0 size=1 (VAR 'a')
>>            type_id=4 offset=0 size=1 (VAR 'b')
>> the 'char' and 'signed char' are encoded as SIGNED integers.
>>
>> Encode 'char' and 'signed char' as SIGNED should be a right to
>> do and it will be consistent with bpf implementation.
>>
>> With this patch,
>>    $ pahole -JV t.o
>>    ...
>>    [1] INT signed char size=1 nr_bits=8 encoding=SIGNED
>>    [2] INT char size=1 nr_bits=8 encoding=SIGNED
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
> 
> LGTM.
> 
> Is there a plan to also add CHAR encoding bit?

Not yet... Need more thinking about use cases and backward comparability 
issues.

> 
> 
>>   dwarf_loader.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>> index d892bc3..c2ad2a0 100644
>> --- a/dwarf_loader.c
>> +++ b/dwarf_loader.c
>> @@ -560,7 +560,7 @@ static struct base_type *base_type__new(Dwarf_Die *die, struct cu *cu, struct co
>>                  bt->bit_size = attr_numeric(die, DW_AT_byte_size) * 8;
>>                  uint64_t encoding = attr_numeric(die, DW_AT_encoding);
>>                  bt->is_bool = encoding == DW_ATE_boolean;
>> -               bt->is_signed = encoding == DW_ATE_signed;
>> +               bt->is_signed = (encoding == DW_ATE_signed) || (encoding == DW_ATE_signed_char);
>>                  bt->is_varargs = false;
>>                  bt->name_has_encoding = true;
>>                  bt->float_type = encoding_to_float_type(encoding);
>> --
>> 2.30.2
>>