From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Yonghong Song <yhs@fb.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>,
dwarves@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH dwarves v2 2/2] btf: Support BTF_KIND_ENUM64
Date: Mon, 27 Jun 2022 15:30:07 -0700 [thread overview]
Message-ID: <CAEf4BzayE3nEdft-=nhxSAxkq_F1N0o0AdbFU0m=MKSsF+pZ4w@mail.gmail.com> (raw)
In-Reply-To: <20220615230317.852304-1-yhs@fb.com>
On Wed, Jun 15, 2022 at 4:03 PM Yonghong Song <yhs@fb.com> 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@fb.com>
> ---
> 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@kernel.org>
> {
> - 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);
[...]
next prev parent reply other threads:[~2022-06-27 22:30 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-15 23:03 [PATCH dwarves v2 0/2] btf: support BTF_KIND_ENUM64 Yonghong Song
2022-06-15 23:03 ` [PATCH dwarves v2 1/2] libbpf: Sync with latest libbpf repo Yonghong Song
2022-06-15 23:03 ` [PATCH dwarves v2 2/2] btf: Support BTF_KIND_ENUM64 Yonghong Song
2022-06-27 22:30 ` Andrii Nakryiko [this message]
2022-06-28 6:19 ` Yonghong Song
2022-06-28 9:51 ` [PATCH dwarves v2 0/2] btf: support BTF_KIND_ENUM64 Jiri Olsa
2022-06-29 16:30 ` Arnaldo Carvalho de Melo
2022-06-29 16:56 ` [PATCH] btf_loader: support BTF_KIND_ENUM64 was " Arnaldo Carvalho de Melo
2022-07-06 4:28 ` Andrii Nakryiko
2022-07-06 15:51 ` Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEf4BzayE3nEdft-=nhxSAxkq_F1N0o0AdbFU0m=MKSsF+pZ4w@mail.gmail.com' \
--to=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=arnaldo.melo@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=dwarves@vger.kernel.org \
--cc=kernel-team@fb.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).