[PATCH dwarves] dwarf_loader: encode char type as signed

[PATCH dwarves] dwarf_loader: encode char type as signed

[Yonghong Song]

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

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

[Andrii Nakryiko]

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?


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

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

[Andrii Nakryiko]

On Mon, Aug 8, 2022 at 3:52 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> 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.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>
> Is there a plan to also add CHAR encoding bit?
>
>
> >  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
> >

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

[Yonghong Song]

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

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

[Arnaldo Carvalho de Melo]

Em Mon, Aug 08, 2022 at 03:52:38PM -0700, Andrii Nakryiko escreveu:
> On Mon, Aug 8, 2022 at 3:52 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> 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.
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>

Thanks, tested before/after, applied.

Pushing out next for CI testing.

- Arnaldo