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.
