On Fri, Nov 17, 2023 at 7:38 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Nov 17, 2023 at 9:33 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Sat, Nov 11, 2023 at 12:17 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> > >
> > > +static void verbose_unum(struct bpf_verifier_env *env, u64 num)
> > > +{
> > > + if (is_unum_decimal(num))
> > > + verbose(env, "%llu", num);
> > > + else
> > > + verbose(env, "%#llx", num);
> >
> > I didn't know about %#.
> > The kernel printk doc doesn't describe it.
> > Great find.
> > Curious, how did you discover this modifier?
> > Not sure whether it's worth adding a comment here
> > that # adds 0x. Probably not ?
>
> This # is called "an alternative form" and is a standard printf()
> feature. I saw it somewhere, don't remember where, so yeah, probably
> an overkill to add a comment for that.
>
> >
> > > + if (type_is_pkt_pointer(t)) {
> > > + verbose_a("r=");
> > > + verbose_snum(env, reg->range);
> > > + }
> >
> > A tiny nit...
> > The pkt range cannot be negative, so using Snum here
> > begs the question... why?
>
> original code was using "r=%d" format string, so I was preserving
> signedness. But if it's supposed to be unsigned, then yeah, no reason
> to do snum here.
>
> > The rest looks great.
> > If you're ok I can fix it up to unum while applying or respin?
>
> This patch requires fixes for reg_bounds.c tester in the part that
> parses register state. It's not a lot, but not really trivially
> fixup-able. I already have all that ready locally, so I'll repost v3
> with unum change.
btw, please also double-check off= and imm= outputs, they are
currently verbose_snum() just like original code, but if it makes more
sense to emit them as unsigned, please fix up while applying, thanks!
