Re: [PATCH bpf-next 00/43] First set of verifier/*.c migrated to inline assembly

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Sun, Mar 26, 2023 at 8:57 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sun, Mar 26, 2023 at 8:16 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Sat, Mar 25, 2023 at 6:19 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Sat, Mar 25, 2023 at 9:16 AM Stanislav Fomichev <sdf@google.com> wrote:
> > > >
> > > > >
> > > > > It was my understanding from the RFC feedback that this "lighter" way
> > > > > is preferable and we already have some tests written like that.
> > > > > Don't have a strong opinion on this topic.
> > > >
> > > > Ack, I'm obviously losing a bunch of context here :-(
> > > > I like coalescing better, but if the original suggestion was to use
> > > > this lighter way, I'll keep that in mind while reviewing.
> > >
> > > I still prefer the clean look of the tests, so I've applied this set.
> > >
> > > But I'm not going to insist that this is the only style developers
> > > should use moving forward.
> > > Whoever prefers "" style can use it in the future tests.
> >
> > Great, because I found out in practice that inability to add comments
> > to the manually written asm code is a pretty big limitation.
>
> What do you mean by "inability" ?
> The comments can be added. See verifier_and.c
>         r0 &= 0xFFFF1234;                               \
>         /* Upper bits are unknown but AND above masks out 1 zero'ing
> lower bits */\
>         if w0 < 1 goto l0_%=;                           \

My bad. I remembered that there were problems with comments in Eduards
previous revision and concluded that they don't work in this
"\-delimited mode". Especially that online documentation for GCC or
Clang didn't explicitly say that they support /* */ comments in asm
blocks (as far as I could google that).

So now I know it's possible, thanks. I still find it very tedious to
do manually, so I appreciate the flexibility in allowing to do
""-delimited style for new programs.

Just to explain where I'm coming from. I took one asm program I have
locally and converted it to a new style. It was tedious with all the
tab alignment. Then I realized that one comment block uses too long
lines and wanted to use vim to reformat them, and that doesn't work
with those '\' delimiters at the end (I didn't have such a problem
with the original style). So that turned into more tedious work. So
for something that needs iteration and adjustments, ""-delimited style
gives more flexibility. See below for reference.

'#' comments are dangerous, btw, they silently ignore everything till
the very end of asm block. No warning or error, just wrong and
incomplete asm is generated, unfortunately.


SEC("?raw_tp")
__failure __log_level(2)
__msg("XXX")
__naked int subprog_result_precise(void)
{
        asm volatile (
                "r6 = 3;"
                /* pass r6 through r1 into subprog to get it back as r0;
                 * this whole chain will have to be marked as precise later
                 */
                "r1 = r6;"
                "call identity_subprog;"
                /* now use subprog's returned value (which is a
                 * r6 -> r1 -> r0 chain), as index into vals array, forcing
                 * all of that to be known precisely
                 */
                "r0 *= 4;"
                "r1 = %[vals];"
                "r1 += r0;"
                /* here r0->r1->r6 chain is forced to be precise and has to be
                 * propagated back to the beginning, including through the
                 * subprog call
                 */
                "r0 = *(u32 *)(r1 + 0);"
                "exit;"
                :
                : __imm_ptr(vals)
                : __clobber_common, "r6"
        );
}

SEC("?raw_tp")
__failure __log_level(2)
__msg("XXX")
__naked int subprog_result_precise2(void)
{
        asm volatile ("
         \
                r6 = 3;
         \
                /* pass r6 through r1 into subprog to get it back as
r0;        \
                 * this whole chain will have to be marked as precise
later     \
                 */
         \
                r1 = r6;
         \
                call identity_subprog;
         \
                /* now use subprog's returned value (which is a
         \
                 * r6 -> r1 -> r0 chain), as index into vals array,
forcing     \
                 * all of that to be known precisely
         \
                 */
         \
                r0 *= 4;
         \
                r1 = %[vals];
         \
                r1 += r0;
         \
                /* here r0->r1->r6 chain is forced to be precise and
has to be  \
                 * propagated back to the beginning, including through
the      \
                 * subprog call
         \
                 */
         \
                r0 = *(u32 *)(r1 + 0);
         \
                exit;
         \
                "
                :
                : __imm_ptr(vals)
                : __clobber_common, "r6"
        );
}