On Wed, Nov 29, 2023 at 3:24 AM Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx> wrote:
>
> On Tue, Nov 28, 2023 at 04:36:15PM -0800, Andrii Nakryiko wrote:
> > Similarly to subprog/callback logic, enforce return value of BPF program
> > using more precise umin/umax range.
> >
> > We need to adjust a bunch of tests due to a changed format of an error
> > message.
> >
> > Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> ...
>
> Q: should the missing register name and values be added?
>
Probably not, it makes future refactoring a bit less painful. If the
important part is to check that there *was* a message about invalid
return result, rather *what exact format* of that message was, then
matching for a substring is enough and makes the test a bit more
robust to future slight refactorings.
> I know relatively little about selftest, but scrolling through it looks
> as though the expect verifier message is incomplete. (Admittedly lots of
> them are like this even before this patch, and this patch improves the
> situation already)
>
Often times it's actually a mistake to expect exact format, it makes
for painful refactoring and improvements. I feel it every time I touch
verifier log formatting logic :( So I don't want to add to that pain.
> e.g.
>
> > --- a/tools/testing/selftests/bpf/progs/test_global_func15.c
> > +++ b/tools/testing/selftests/bpf/progs/test_global_func15.c
> > @@ -13,7 +13,7 @@ __noinline int foo(unsigned int *v)
> > }
> >
> > SEC("cgroup_skb/ingress")
> > -__failure __msg("At program exit the register R0 has value")
> > +__failure __msg("At program exit the register R0 has ")
> > int global_func15(struct __sk_buff *skb)
> > {
> > unsigned int v = 1;
>
> looks like it is missing umin/umax=1
>
> ...
>
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c b/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c
> > index d6c4a7f3f790..4655f01b24aa 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c
> > @@ -7,7 +7,7 @@
> >
> > SEC("cgroup/sock")
> > __description("bpf_exit with invalid return code. test1")
> > -__failure __msg("R0 has value (0x0; 0xffffffff)")
> > +__failure __msg("umax=4294967295 should have been in [0, 1]")
> > __naked void with_invalid_return_code_test1(void)
> > {
> > asm volatile (" \
>
> looks like it is missing mention of R0, etc.
