Re: [PATCH dwarves v2 2/2] btf: Support BTF_KIND_ENUM64

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 15, 2022 at 4:03 PM Yonghong Song <yhs@xxxxxx> wrote:
>
> BTF_KIND_ENUM64 is supported with latest libbpf, which
> supports 64-bit enum values. Latest libbpf also supports
> signedness for enum values. Add enum64 support in
> dwarf-to-btf conversion.
>
> The following is an example of new encoding which covers
> signed/unsigned enum64/enum variations.
>
>   $cat t.c
>   enum { /* signed, enum64 */
>     A = -1,
>     B = 0xffffffff,
>   } g1;
>   enum { /* unsigned, enum64 */
>     C = 1,
>     D = 0xfffffffff,
>   } g2;
>   enum { /* signed, enum */
>     E = -1,
>     F = 0xfffffff,
>   } g3;
>   enum { /* unsigned, enum */
>     G = 1,
>     H = 0xfffffff,
>   } g4;
>   $ clang -g -c t.c
>   $ pahole -JV t.o
>   btf_encoder__new: 't.o' doesn't have '.data..percpu' section
>   Found 0 per-CPU variables!
>   File t.o:
>   [1] ENUM64 (anon) size=8
>           A val=-1
>           B val=4294967295
>   [2] INT long size=8 nr_bits=64 encoding=SIGNED
>   [3] ENUM64 (anon) size=8
>           C val=1
>           D val=68719476735
>   [4] INT unsigned long size=8 nr_bits=64 encoding=(none)
>   [5] ENUM (anon) size=4
>           E val=-1
>           F val=268435455
>   [6] INT int size=4 nr_bits=32 encoding=SIGNED
>   [7] ENUM (anon) size=4
>           G val=1
>           H val=268435455
>   [8] INT unsigned int size=4 nr_bits=32 encoding=(none)
>
> With the flag to skip enum64 encoding,
>
>   $ pahole -JV t.o --skip_encoding_btf_enum64
>   btf_encoder__new: 't.o' doesn't have '.data..percpu' section
>   Found 0 per-CPU variables!
>   File t.o:
>   [1] ENUM (anon) size=8
>         A val=4294967295
>         B val=4294967295
>   [2] INT long size=8 nr_bits=64 encoding=SIGNED
>   [3] ENUM (anon) size=8
>         C val=1
>         D val=4294967295
>   [4] INT unsigned long size=8 nr_bits=64 encoding=(none)
>   [5] ENUM (anon) size=4
>         E val=4294967295
>         F val=268435455
>   [6] INT int size=4 nr_bits=32 encoding=SIGNED
>   [7] ENUM (anon) size=4
>         G val=1
>         H val=268435455
>   [8] INT unsigned int size=4 nr_bits=32 encoding=(none)
>
> In the above btf encoding without enum64, all enum types
> with the same type size as the corresponding enum64. All these
> enum types have unsigned type (kflag = 0) which is required
> before kernel enum64 support.
>
> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> ---
>  btf_encoder.c     | 65 +++++++++++++++++++++++++++++++++++------------
>  btf_encoder.h     |  2 +-
>  dwarf_loader.c    | 12 +++++++++
>  dwarves.h         |  4 ++-
>  dwarves_fprintf.c |  6 ++++-
>  pahole.c          | 10 +++++++-
>  6 files changed, 79 insertions(+), 20 deletions(-)
>

Sorry for late review, I don't always catch up on emails from older
emails first :(

[...]

>         size = BITS_ROUNDUP_BYTES(bit_size);
> -       id = btf__add_enum(btf, name, size);
> +       is_enum32 = size <= 4 || no_enum64;
> +       if (is_enum32)
> +               id = btf__add_enum(btf, name, size);
> +       else
> +               id = btf__add_enum64(btf, name, size, is_signed);
>         if (id > 0) {
>                 t = btf__type_by_id(btf, id);
>                 btf_encoder__log_type(encoder, t, false, true, "size=%u", t->size);
>         } else {
> -               btf__log_err(btf, BTF_KIND_ENUM, name, true,
> +               btf__log_err(btf, is_enum32 ? BTF_KIND_ENUM : BTF_KIND_ENUM64, name, true,
>                               "size=%u Error emitting BTF type", size);
>         }
>         return id;
>  }
>
> -static int btf_encoder__add_enum_val(struct btf_encoder *encoder, const char *name, int32_t value)
> +static int btf_encoder__add_enum_val(struct btf_encoder *encoder, const char *name, int64_t value,
> +                                    bool is_signed, bool is_enum64, bool no_enum64)

It was quite confusing to see "is_enum64" and "no_enum64" as arguments
to the same function :)

I'll let Arnaldo decide for himself, but I think it would be cleaner
to pass such configuration switches as fields in struct btf_encoder
itself and just check such flags from relevant btf_encoder__add_xxx()
functions. Such flags are global by nature, so it seems fitting.

But other than that looks good to me.

Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>

>  {
> -       int err = btf__add_enum_value(encoder->btf, name, value);
> +       const char *fmt_str;
> +       int err;
> +
> +       /* If enum64 is not allowed, generate enum32 with unsigned int value. In enum64-supported
> +        * libbpf library, btf__add_enum_value() will set the kflag (sign bit) in common_type
> +        * if the value is negative.
> +        */
> +       if (no_enum64)
> +               err = btf__add_enum_value(encoder->btf, name, (uint32_t)value);
> +       else if (is_enum64)
> +               err = btf__add_enum64_value(encoder->btf, name, value);
> +       else
> +               err = btf__add_enum_value(encoder->btf, name, value);

[...]



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux