Re: [PATCH v2 1/1] objdump, as: Add callx support for BPF CPU v1

[Will Hawkins <hawkinsw@obs.cr>]

On Tue, Feb 13, 2024 at 6:57 AM Nick Clifton <nickc@redhat.com> wrote:
>
> Hi Will,
>
> > Add support for (dis)assembling the callx instruction back to CPU v1.
>
> I am not going to comment on the callx instruction itself, since I am
> not a BPF expert and it seems like you are already working on that with
> Jose and YYonghong.  Instead I thought that I would comment on the patch
> instead...

Nick,

First, as I said yesterday in my direct message to you, thank you for
making binutils such a pleasant place to contribute to FOSS. You have
no idea what that means to contributors like me!

Second, thank you for this helpful critique. I really appreciated
reading your feedback and will reply inline below (including with an
offer for a patch unrelated to callx that may clean up some of the
non-constant uses throughout the bpf-specific code).

Third, there is good news: The heavy lifting of this patch is largely
"overcome by events" -- clang/llvm developers are changing their
encoding of the callx instruction to more closely match what gcc does.
In fact, v3 of this patch will look much more like v1 than v2.

And now, on with the action ...


>
>
> > Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
>
> Just to be clear: your sign off here does mean that you agree to the
> Developer Certificate of Origin (v1.1) right ?  [I am being paranoid...:-]
>

Yes, 100%! I appreciate you checking but I specifically included the
sign off as my agreement. If there is a better way to positively
signal that intention, please let me know!

>
>
> > @@ -935,7 +937,13 @@ encode_insn (struct bpf_insn *insn, char *bytes,
> >         if (immediate_overflow (imm, 32))
> >           as_bad (_("immediate out of range, shall fit in 32 bits"));
> >         else
> > -        encode_int32 (insn->imm32.X_add_number, bytes + 4);
> > +        encode_int32 (insn->imm32.X_add_number, bytes + 4);
> > +    }
> > +
> > +  if (insn->has_rmm32)
> > +    {
> > +      int64_t imm = insn->rmm32;
> > +      encode_int32 (imm, bytes + 4);
> >       }
> >
> >     if (insn->has_disp32 && insn->disp32.X_op == O_constant)
>
> This frag struck me as strange since it appears that the first change is to
> replace a line with itself.  Looking a bit closer I saw that you are removing
> some unnecessary spaces at the end of the line.  Which is good and not a
> problem.  It just looked odd when I was reviewing the patch.

I was hesitant to include this piece in the PR because I know that
whitespace patches are always a touchy subject (see below for my offer
and how it relates to this line of code ...)

>
> The second part of the frag also seems a little odd.  Given that encode_int32()
> takes an int32_t as its first parameter, why do you use int64_t as the type
> for "imm" ?  In fact why have a local variable at all ?  Wouldn't it be simpler
> to just have:
>
>    if (insn->has_rmm32)
>      encode_int32 (insn->rmm32, bytes + 4);
>

Yes, absolutely. This local/64-bit difference was left over from a
piece of the code that I replaced. I had imm as a way to perform a
similar out-of-32-bit-range-check as the block above. I removed that
after revamping the way that the instruction was parsed (I originally
parsed the "register" as a signed number and had protections to check
for overflow). I think that your implementation is unquestionably
better.

>
>
> > @@ -1610,6 +1618,21 @@ md_assemble (char *str ATTRIBUTE_UNUSED)
> >                     insn.has_imm32 = 1;
> >                     p += 4;
> >                   }
> > +              else if (strncmp (p, "%r32", 4) == 0)
>
> This is not a criticism, but merely a comment.  I dislike the presence
> of "magic" numbers in code, numbers whose value may or may not be obvious
> to the reader and which may appear more than once.

Here is my offer (hinted at above): There are several parts of this
particular file that appear out of the ordinary:

1. Inconsistent tab/space usage
2. The presence of magic values
3. Spaces at the end of lines

And so on.

If I were to take a pass at "cleaning up" the code in this file, would
you be interested in having a PR? As I said, I really appreciate your
openness to my contribution and I love working with FOSS. In other
words, I would really enjoy contributing. That said, I don't want to
step in and intrude on someone else's territory. Just let me know!

>
> So for example the number 4 in the code line above is "magic" to my mind,
> and a potential source of problems.  Imagine that at some point in the
> future the name of the register was changed to "%foo32".  Most coders
> would realise that the 4 is the length of the "%r32" string and so change
> it to 5, but would they also realise that the 4 is reused later on to
> adjust the "p" pointer:
>
> > +                  p += 4;
>
> Now you are following the code style that is already present in the tc-bpf.c
> file, so there is no fault there.  But, in my opinion, the style is wrong.
> I would rather see something like this:
>
>    #define RMM32_REG_NAME "%r32"
>    #define RMM32_REG_LEN  strlen (RMM32_REG_NAME)
>
>    [...]
>    else if (strncmp (p, RMM32_REG_NAEM, RMM32_REG_LEN) == 0)
>      {
>         [...]
>         p += RMM32_REG_LEN;
>      }
>
> That way if the name is ever changed, all the other adjustments
> would happen automatically.  Plus if those defines were in a BPF
> specific header file then they could be reused elsewhere, eg in
> the disassembler.
>
> Alternatively ... there is a function called startswith() which is used
> in the assembler in lots of places, and you could use this to make the
> if-statement simpler, ie:
>
>    [...]
>    else if (startswith (p, "%r32")
>    {
>      [...]
>
> This does not resolve the use of the magic value 4 later on, but still
> makes the code look cleaner.  Just my opnion of course.
>

See above ... (smiley!)

>
>
> > diff --git a/opcodes/bpf-dis.c b/opcodes/bpf-dis.c
> > index d4020c259fb..50e714cb74b 100644
> > --- a/opcodes/bpf-dis.c
> > +++ b/opcodes/bpf-dis.c
> > @@ -251,6 +251,12 @@ print_insn_bpf (bfd_vma pc, disassemble_info *info)
> >                                                   imm32);
> >                     p += 4;
> >                   }
> > +              else if (strncmp (p, "%r32", 4) == 0)
> > +                {
> > +                  int32_t imm32 = bpf_extract_imm32 (word, endian);
> > +                  print_register (info, p, imm32);
> > +                  p += 4;
> > +                }
>
> See the comments above. :-)
>
>
>  > diff --git a/include/opcode/bpf.h b/include/opcode/bpf.h
>
>  > @@ -254,6 +254,7 @@ struct bpf_opcode
>  >       %d16 - 16-bit signed displacement (in 64-bit words minus one.)
>  >       %o16 - 16-bit signed offset (in bytes.)
>  >       %i32 - 32-bit signed immediate.
>  > +     %r32 - 32-bit signed immediate indicating a register.
>  >       %I32 - Like %i32.
>  >       %i64 - 64-bit signed immediate.
>  >       %w - expect zero or more white spaces and print a single space.
>
> Why is %r32 signed ?  Does the BPF format support negative register numbers ?
>
>

This is a great question. I struggled to decide what type to make this
format code represent. I ultimately went with 32-bit signed because
the immediate value in a BPF instruction is "typically" considered to
be signed (https://www.ietf.org/archive/id/draft-thaler-bpf-isa-00.html#section-3-6.2).
Of course, you are correct in saying that register numbers are most
likely (I am being facetious, of course!) not going to be negative
but, because of the choice to encode a register in this odd place, I
didn't think that I had much of a choice. The good news is that we
won't have to make this lesser-of-two-evils choice.

>
>
> > diff --git a/sim/bpf/bpf-sim.c b/sim/bpf/bpf-sim.c
>
> Just FYI - the simulator is part of the GDB project, not the Binutils
> project, so you will need to submit the patch to bpf-sim.c to them, not
> us.  The email address to use is: gdb-patches@sourceware.org

I had no idea! Thank you! I will make sure that any changes to the
simulator in v3 are directed to them. Sorry for the additional
overhead and thank you for the heads up!

>
> Cheers
>    Nick
>
> PS.  Please don't think that my comments are meant to be criticisms of
>    your code.  The code is good.  I am merely trying to provide some
>    suggestions to make it even better. :-)

I will say it again: I sincerely appreciate the time you took out of
your day to review this code. As an active participant in the world of
FOSS and, in particular, BPF, I hope that this contribution will be
the first of many. Thanks again!

Will


>
>
>
>