From: Eduard Zingerman <eddyz87@gmail.com>
To: Andrii Nakryiko <andrii@kernel.org>,
bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
martin.lau@kernel.org
Cc: kernel-team@meta.com
Subject: Re: [PATCH bpf-next 7/8] bpf: smarter verifier log number printing logic
Date: Sat, 11 Nov 2023 02:51:43 +0200 [thread overview]
Message-ID: <8506722472c49aafc5842ee3d39ddae3230882b7.camel@gmail.com> (raw)
In-Reply-To: <20231110161057.1943534-8-andrii@kernel.org>
On Fri, 2023-11-10 at 08:10 -0800, Andrii Nakryiko wrote:
> Instead of always printing numbers as either decimals (and in some
> cases, like for "imm=%llx", in hexadecimals), decide the form based on
> actual values. For numbers in a reasonably small range (currently,
> [0, U16_MAX] for unsigned values, and [S16_MIN, S16_MAX] for signed ones),
> emit them as decimals. In all other cases, even for signed values,
> emit them in hexadecimals.
>
> For large values hex form is often times way more useful: it's easier to
> see an exact difference between 0xffffffff80000000 and 0xffffffff7fffffff,
> than between 18446744071562067966 and 18446744071562067967, as one
> particular example.
>
> Small values representing small pointer offsets or application
> constants, on the other hand, are way more useful to be represented in
> decimal notation.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
[...]
> @@ -576,14 +627,14 @@ static void print_reg_state(struct bpf_verifier_env *env, const struct bpf_reg_s
> tnum_is_const(reg->var_off)) {
> /* reg->off should be 0 for SCALAR_VALUE */
> verbose(env, "%s", t == SCALAR_VALUE ? "" : reg_type_str(env, t));
> - verbose(env, "%lld", reg->var_off.value + reg->off);
> + verbose_snum(env, reg->var_off.value + reg->off);
> return;
> }
> /*
> * _a stands for append, was shortened to avoid multiline statements below.
> * This macro is used to output a comma separated list of attributes.
> */
> -#define verbose_a(fmt, ...) ({ verbose(env, "%s" fmt, sep, __VA_ARGS__); sep = ","; })
> +#define verbose_a(fmt, ...) ({ verbose(env, "%s" fmt, sep, ##__VA_ARGS__); sep = ","; })
>
> verbose(env, "%s", reg_type_str(env, t));
> if (base_type(t) == PTR_TO_BTF_ID)
> @@ -608,8 +659,10 @@ static void print_reg_state(struct bpf_verifier_env *env, const struct bpf_reg_s
> verbose_a("r=%d", reg->range);
> if (tnum_is_const(reg->var_off)) {
> /* a pointer register with fixed offset */
> - if (reg->var_off.value)
> - verbose_a("imm=%llx", reg->var_off.value);
> + if (reg->var_off.value) {
> + verbose_a("imm=");
> + verbose_snum(env, reg->var_off.value);
> + }
Maybe use same verbose_{u,s}num() for reg->off and reg->range in this
function?
next prev parent reply other threads:[~2023-11-11 0:51 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-10 16:10 [PATCH bpf-next 0/8] BPF verifier log improvements Andrii Nakryiko
2023-11-10 16:10 ` [PATCH bpf-next 1/8] bpf: move verbose_linfo() into kernel/bpf/log.c Andrii Nakryiko
2023-11-10 16:10 ` [PATCH bpf-next 2/8] bpf: move verifier state printing code to kernel/bpf/log.c Andrii Nakryiko
2023-11-10 17:37 ` Stanislav Fomichev
2023-11-10 17:49 ` Andrii Nakryiko
2023-11-10 16:10 ` [PATCH bpf-next 3/8] bpf: extract register state printing Andrii Nakryiko
2023-11-10 16:10 ` [PATCH bpf-next 4/8] bpf: print spilled register state in stack slot Andrii Nakryiko
2023-11-11 0:31 ` Eduard Zingerman
2023-11-10 16:10 ` [PATCH bpf-next 5/8] bpf: emit map name in register state if applicable and available Andrii Nakryiko
2023-11-10 16:10 ` [PATCH bpf-next 6/8] bpf: omit default off=0 and imm=0 in register state log Andrii Nakryiko
2023-11-10 16:10 ` [PATCH bpf-next 7/8] bpf: smarter verifier log number printing logic Andrii Nakryiko
2023-11-11 0:51 ` Eduard Zingerman [this message]
2023-11-11 6:31 ` Andrii Nakryiko
2023-11-10 16:10 ` [PATCH bpf-next 8/8] bpf: emit frameno for PTR_TO_STACK regs if it differs from current one Andrii Nakryiko
2023-11-10 18:50 ` [PATCH bpf-next 0/8] BPF verifier log improvements Stanislav Fomichev
2023-11-10 19:13 ` Andrii Nakryiko
2023-11-11 0:57 ` Eduard Zingerman
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=8506722472c49aafc5842ee3d39ddae3230882b7.camel@gmail.com \
--to=eddyz87@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@meta.com \
--cc=martin.lau@kernel.org \
/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