Re: [PATCH bpf-next 04/12] libbpf: Add btf enum64 support

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, May 10, 2022 at 4:02 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 5/10/22 3:40 PM, Yonghong Song wrote:
> >
> >
> > On 5/9/22 4:25 PM, Andrii Nakryiko wrote:
> >> On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote:
> >>>
> >>> Add BTF_KIND_ENUM64 support. Deprecated btf__add_enum() and
> >>> btf__add_enum_value() and introduced the following new APIs
> >>>    btf__add_enum32()
> >>>    btf__add_enum32_value()
> >>>    btf__add_enum64()
> >>>    btf__add_enum64_value()
> >>> due to new kind and introduction of kflag.
> >>>
> >>> To support old kernel with enum64, the sanitization is
> >>> added to replace BTF_KIND_ENUM64 with a bunch of
> >>> pointer-to-void types.
> >>>
> >>> The enum64 value relocation is also supported. The enum64
> >>> forward resolution, with enum type as forward declaration
> >>> and enum64 as the actual definition, is also supported.
> >>>
> >>> Signed-off-by: Yonghong Song <yhs@fb.com>
> >>> ---
> >>>   tools/lib/bpf/btf.c                           | 226 +++++++++++++++++-
> >>>   tools/lib/bpf/btf.h                           |  21 ++
> >>>   tools/lib/bpf/btf_dump.c                      |  94 ++++++--
> >>>   tools/lib/bpf/libbpf.c                        |  64 ++++-
> >>>   tools/lib/bpf/libbpf.map                      |   4 +
> >>>   tools/lib/bpf/libbpf_internal.h               |   2 +
> >>>   tools/lib/bpf/linker.c                        |   2 +
> >>>   tools/lib/bpf/relo_core.c                     |  93 ++++---
> >>>   .../selftests/bpf/prog_tests/btf_dump.c       |  10 +-
> >>>   .../selftests/bpf/prog_tests/btf_write.c      |   6 +-
> >>>   10 files changed, 450 insertions(+), 72 deletions(-)
> >>>
> >>
> [...]
> >>
> >>
> >>> +       t->size = tsize;
> >>> +
> >>> +       return btf_commit_type(btf, sz);
> >>> +}
> >>> +
> >>> +/*
> >>> + * Append new BTF_KIND_ENUM type with:
> >>> + *   - *name* - name of the enum, can be NULL or empty for anonymous
> >>> enums;
> >>> + *   - *is_unsigned* - whether the enum values are unsigned or not;
> >>> + *
> >>> + * Enum initially has no enum values in it (and corresponds to enum
> >>> forward
> >>> + * declaration). Enumerator values can be added by
> >>> btf__add_enum64_value()
> >>> + * immediately after btf__add_enum() succeeds.
> >>> + *
> >>> + * Returns:
> >>> + *   - >0, type ID of newly added BTF type;
> >>> + *   - <0, on error.
> >>> + */
> >>> +int btf__add_enum32(struct btf *btf, const char *name, bool
> >>> is_unsigned)
> >>
> >> given it's still BTF_KIND_ENUM in UAPI, let's keep 32-bit ones as just
> >> btf__add_enum()/btf__add_enum_value() and not deprecate anything.
> >> ENUM64 can be thought about as more of a special case, so I think it's
> >> ok.
> >
> > The current btf__add_enum api:
> > LIBBPF_API int btf__add_enum(struct btf *btf, const char *name, __u32
> > bytes_sz);
> >
> > The issue is it doesn't have signedness parameter. if the user input
> > is
> >     enum { A = -1, B = 0, C = 1 };
> > the actual printout btf format will be
> >     enum { A 4294967295, B = 0, C = 1}
> > does not match the original source.
>
> I think I found a way to keep the current btf__add_enum() API.
> Initially, the signedness will be unsigned. But during
> btf__add_enum_value() api calls, if any negative value
> is found, the signedness will change to signed. I think
> this should work.
>

Oops, didn't see this email when replying. Yeah, I guess this approach
will work for 32-bit enum. For 64-bit one we probably better specify
signedness explicitly and then accept __u64 as the value (which can be
negative value casted to __u64, in practice).

> >
> >>
> >>> +{
> >>> +       return btf_add_enum_common(btf, name, is_unsigned,
> >>> BTF_KIND_ENUM, 4);
> >>> +}
> >>> +
> >>
> >> [...]
> >>
> [...]