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.
