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

[Nick Clifton <nickc@redhat.com>]

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...


> 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...:-]



> @@ -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.

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);



> @@ -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.

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.



> 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 ?




> 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

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. :-)