On Fri, Nov 10, 2023 at 4:51 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> [...]
> > @@ -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?
sure, will do!
