Re: [RFC bpf-next 0/5] test_verifier tests migration to inline assembly

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2023-01-25 at 19:25 -0800, Alexei Starovoitov wrote:
> On Wed, Jan 25, 2023 at 05:33:42PM -0800, Andrii Nakryiko wrote:
> > 
> > > __naked void invalid_and_of_negative_number(void)
> > > 
> > > {
> > >         asm volatile (
> > > "       r1 = 0;                                         \n\
> > 
> > Kumar recently landed similarly formatted inline asm-based test, let's
> > make sure we stick to common style. \n at the end are pretty
> > distracting, IMO (though helpful to debug syntax errors in asm, of
> > course). I'd also move starting " into the same line as asm volatile:
> 
> +1. Pls drop \n.
> You don't have \n anyway in migrator's README on github.

I have --newlines switch there :)

> 
> > asm volatile ("                       \
> > 
> > this will make adding/removing asm lines at the beginning simpler (and
> > you already put closing quote on separate line, so that side is taken
> > care of)
> 
> +1
> 
> Also pls indent the asm code with two tabs the way Kumar did.
> I think it looks cleaner this way and single tab labels align
> with 'asm volatile ('.

Will do.

> 
> > > All in all the current script stats are as follows:
> > > - 62 out of 93 files from progs/*.c can be converted w/o warnings;
> 
> out of 98 in verifier/*.c ?

Sorry, yes, messed up twice in this statement:
- meant verifier/*.c not progs/*.c;
- counted 93 files after migrating one file picked to be an examle.
I just double checked and there 94 *.c files in that directory on
bpf-next master branch.

> 
> > > - 55 converted files could be compiled;
> > > - 40 pass testing, 15 fail.
> 
> I would land this 40 now and continue step by step.
> 
> > > 
> > > By submitting this RFC I seek the following feedback:
> > > - is community interested in such migration?
> > 
> > +1
> > 
> > This is a great work!
> 
> +1

Thanks.

> 
> > > - if yes, should I pursue partial or complete tests migration?
> > 
> > I'd start with partial
> > 
> > > - in case of partial migration which tests should be prioritized?
> > 
> > those that work out of the box?
> > 
> > > - should I offer migrated tests one by one or in big butches?
> 
> Can you do one patch one file in verifier/*.c that would map
> to one new file in progs/ ?

Will do.

> 
> > > 
> > > [1] https://github.com/eddyz87/verifier-tests-migrator
> 
> Having this link in patch series is enough.
> The 'migrator' itself doesn't need to be in the kernel tree.