[PATCH v2 0/1] Add BPF callx support to objdump and as

[PATCH v2 0/1] Add BPF callx support to objdump and as

[Will Hawkins]

After additional consideration and discussion with Jose and Dave,
it seems like we have determined the way that clang, gcc and binutils
need to handle the callx/callr:

1. callr remains with the register holding the target of the jump stored
in the dst_reg.
2. callx is added with the register holding the target of the jump stored
in the imm32.
3. We have to remove the pseudoc syntax because it is no longer possible
to disambiguate between versions of call by simply looking at the parameter.

Tests are added/refactored to meet the above. 

I am more than happy to resend as a separate mailing to the list but
sending first as a reply in order to keep list traffic manageable.

As I said before, I sincerely appreciate all that you are doing for
the community and how welcoming you have been to a first-time contributor.

Sincerely,
Will

Will Hawkins (1):
  objdump, as: Add callx support for BPF CPU v1

 gas/config/tc-bpf.c                           | 25 ++++++++++++++++++-
 gas/testsuite/gas/bpf/bpf.exp                 |  4 +--
 .../gas/bpf/{indcall-1.d => callr.d}          |  4 +--
 .../gas/bpf/{indcall-1.s => callr.s}          |  2 +-
 gas/testsuite/gas/bpf/indcall-1-pseudoc.d     | 23 -----------------
 gas/testsuite/gas/bpf/indcall-1-pseudoc.s     | 13 ----------
 include/opcode/bpf.h                          |  3 ++-
 opcodes/bpf-dis.c                             |  6 +++++
 opcodes/bpf-opc.c                             |  4 ++-
 sim/bpf/bpf-sim.c                             |  4 +++
 10 files changed, 44 insertions(+), 44 deletions(-)
 rename gas/testsuite/gas/bpf/{indcall-1.d => callr.d} (90%)
 rename gas/testsuite/gas/bpf/{indcall-1.s => callr.s} (90%)
 delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.d
 delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.s

-- 
2.43.0

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

[Will Hawkins]

Add support for (dis)assembling the callx instruction back to CPU v1.

gas/ChangeLog:

	* config/tc-bpf.c (struct bpf_insn): Add rmm32 for handling
	* instructions where a register is encoded in the immediate.
	(encode_insn): Encode instruction where register goes in immediate.
	(md_assemble): Assemble the callx instruction.
	* testsuite/gas/bpf/bpf.exp: Add callx test and refactor others (remove
	* pseudo-c mnemonics).
	* testsuite/gas/bpf/indcall-1.d: Moved to...
	* testsuite/gas/bpf/callr.d: ...here.
	* testsuite/gas/bpf/indcall-1.s: Moved to...
	* testsuite/gas/bpf/callr.s: ...here.
	* testsuite/gas/bpf/indcall-1-pseudoc.d: Removed.
	* testsuite/gas/bpf/indcall-1-pseudoc.s: Removed.

include/ChangeLog:

	* opcode/bpf.h (enum bpf_insn_id): Add BPF_INSN_CALLX
	(struct bpf_opcode): Add rmm32, has_rmm32.

opcodes/ChangeLog:

	* bpf-dis.c (print_insn_bpf): Support printing callx.
	* bpf-opc.c: Add callx.

ChangeLog:

	* sim/bpf/bpf-sim.c (execute): Support callx.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
---
 gas/config/tc-bpf.c                           | 25 ++++++++++++++++++-
 gas/testsuite/gas/bpf/bpf.exp                 |  4 +--
 .../gas/bpf/{indcall-1.d => callr.d}          |  4 +--
 .../gas/bpf/{indcall-1.s => callr.s}          |  2 +-
 gas/testsuite/gas/bpf/indcall-1-pseudoc.d     | 23 -----------------
 gas/testsuite/gas/bpf/indcall-1-pseudoc.s     | 13 ----------
 include/opcode/bpf.h                          |  3 ++-
 opcodes/bpf-dis.c                             |  6 +++++
 opcodes/bpf-opc.c                             |  4 ++-
 sim/bpf/bpf-sim.c                             |  4 +++
 10 files changed, 44 insertions(+), 44 deletions(-)
 rename gas/testsuite/gas/bpf/{indcall-1.d => callr.d} (90%)
 rename gas/testsuite/gas/bpf/{indcall-1.s => callr.s} (90%)
 delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.d
 delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.s

diff --git a/gas/config/tc-bpf.c b/gas/config/tc-bpf.c
index 43e098c2a86..5bc8b169819 100644
--- a/gas/config/tc-bpf.c
+++ b/gas/config/tc-bpf.c
@@ -38,6 +38,7 @@ struct bpf_insn
   bpf_insn_word opcode;
   uint8_t dst;
   uint8_t src;
+  uint8_t rmm32;
   expressionS offset16;
   expressionS imm32;
   expressionS imm64;
@@ -50,6 +51,7 @@ struct bpf_insn
   unsigned int has_disp16 : 1;
   unsigned int has_disp32 : 1;
   unsigned int has_imm32 : 1;
+  unsigned int has_rmm32 : 1;
   unsigned int has_imm64 : 1;
 
   unsigned int is_relaxable : 1;
@@ -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)
@@ -1610,6 +1618,21 @@ md_assemble (char *str ATTRIBUTE_UNUSED)
                   insn.has_imm32 = 1;
                   p += 4;
                 }
+              else if (strncmp (p, "%r32", 4) == 0)
+                {
+                  uint8_t regno;
+                  char *news = parse_bpf_register (s, 'r', &regno);
+                  if (news == NULL)
+                    {
+                      PARSE_ERROR ("expected signed 32-bit immediate "
+                                   "indicating register");
+                      break;
+                    }
+                  s = news;
+                  insn.rmm32 = regno;
+                  insn.has_rmm32 = 1;
+                  p += 4;
+                }
               else if (strncmp (p, "%o16", 4) == 0)
                 {
                   while (*s == ' ' || *s == '\t')
diff --git a/gas/testsuite/gas/bpf/bpf.exp b/gas/testsuite/gas/bpf/bpf.exp
index dae8bd924d0..60c8d2ae852 100644
--- a/gas/testsuite/gas/bpf/bpf.exp
+++ b/gas/testsuite/gas/bpf/bpf.exp
@@ -41,8 +41,8 @@ if {[istarget bpf*-*-*]} {
     run_dump_test atomic-v1
     run_dump_test atomic
     run_dump_test atomic-pseudoc
-    run_dump_test indcall-1
-    run_dump_test indcall-1-pseudoc
+    run_dump_test callr
+    run_dump_test callx
 
     run_dump_test jump-relax-ja
     run_dump_test jump-relax-jump
diff --git a/gas/testsuite/gas/bpf/indcall-1.d b/gas/testsuite/gas/bpf/callr.d
similarity index 90%
rename from gas/testsuite/gas/bpf/indcall-1.d
rename to gas/testsuite/gas/bpf/callr.d
index 51103bba2a1..ba70983d0ad 100644
--- a/gas/testsuite/gas/bpf/indcall-1.d
+++ b/gas/testsuite/gas/bpf/callr.d
@@ -1,6 +1,6 @@
 #as: -EL -misa-spec=xbpf
 #objdump: -dr -M xbpf,dec
-#source: indcall-1.s
+#source: callr.s
 #name: BPF indirect call 1, normal syntax
 
 .*: +file format .*bpf.*
@@ -14,7 +14,7 @@ Disassembly of section \.text:
   18:	18 06 00 00 38 00 00 00 	lddw %r6,56
   20:	00 00 00 00 00 00 00 00[    ]*
 			18: R_BPF_64_64	.text
-  28:	8d 06 00 00 00 00 00 00 	call %r6
+  28:	8d 06 00 00 00 00 00 00 	callr %r6
   30:	95 00 00 00 00 00 00 00 	exit
 
 0000000000000038 <bar>:
diff --git a/gas/testsuite/gas/bpf/indcall-1.s b/gas/testsuite/gas/bpf/callr.s
similarity index 90%
rename from gas/testsuite/gas/bpf/indcall-1.s
rename to gas/testsuite/gas/bpf/callr.s
index 5d49e41040a..d4f8208b2d4 100644
--- a/gas/testsuite/gas/bpf/indcall-1.s
+++ b/gas/testsuite/gas/bpf/callr.s
@@ -6,7 +6,7 @@ main:
     mov %r1, 1
     mov %r2, 2
     lddw %r6, bar
-    call %r6
+    callr %r6
     exit
 
 bar:
diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d b/gas/testsuite/gas/bpf/indcall-1-pseudoc.d
deleted file mode 100644
index 7a95bad8e65..00000000000
--- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d
+++ /dev/null
@@ -1,23 +0,0 @@
-#as: -EL -mdialect=pseudoc -misa-spec=xbpf
-#objdump: -M xbpf,pseudoc,dec -dr
-#source: indcall-1-pseudoc.s
-#name: BPF indirect call 1, pseudoc syntax
-
-.*: +file format .*bpf.*
-
-Disassembly of section \.text:
-
-0000000000000000 <main>:
-   0:	b7 00 00 00 01 00 00 00 	r0=1
-   8:	b7 01 00 00 01 00 00 00 	r1=1
-  10:	b7 02 00 00 02 00 00 00 	r2=2
-  18:	18 06 00 00 38 00 00 00 	r6=56 ll
-  20:	00 00 00 00 00 00 00 00[    ]*
-			18: R_BPF_64_64	.text
-  28:	8d 06 00 00 00 00 00 00 	callx r6
-  30:	95 00 00 00 00 00 00 00 	exit
-
-0000000000000038 <bar>:
-  38:	b7 00 00 00 00 00 00 00 	r0=0
-  40:	95 00 00 00 00 00 00 00 	exit
-#pass
diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s b/gas/testsuite/gas/bpf/indcall-1-pseudoc.s
deleted file mode 100644
index 2042697f15b..00000000000
--- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s
+++ /dev/null
@@ -1,13 +0,0 @@
-
-    .text
-    .align 4
-main:
-	r0 = 1
-	r1 = 1
-	r2 = 2
-	r6 = bar ll
-	callx r6
-	exit
-bar:
-	r0 = 0
-	exit
diff --git a/include/opcode/bpf.h b/include/opcode/bpf.h
index df1e3bd0918..d5ea3c11df8 100644
--- a/include/opcode/bpf.h
+++ b/include/opcode/bpf.h
@@ -202,7 +202,7 @@ enum bpf_insn_id
   BPF_INSN_JAR, BPF_INSN_JEQR, BPF_INSN_JGTR, BPF_INSN_JSGTR,
   BPF_INSN_JGER, BPF_INSN_JSGER, BPF_INSN_JLTR, BPF_INSN_JSLTR,
   BPF_INSN_JSLER, BPF_INSN_JLER, BPF_INSN_JSETR, BPF_INSN_JNER,
-  BPF_INSN_CALLR, BPF_INSN_CALL, BPF_INSN_EXIT,
+  BPF_INSN_CALLR, BPF_INSN_CALLX, BPF_INSN_CALL, BPF_INSN_EXIT,
   /* Compare-and-jump instructions (reg OP imm.)  */
   BPF_INSN_JEQI, BPF_INSN_JGTI, BPF_INSN_JSGTI,
   BPF_INSN_JGEI, BPF_INSN_JSGEI, BPF_INSN_JLTI, BPF_INSN_JSLTI,
@@ -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.
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;
+                }
               else if (strncmp (p, "%o16", 4) == 0
                        || strncmp (p, "%d16", 4) == 0)
                 {
diff --git a/opcodes/bpf-opc.c b/opcodes/bpf-opc.c
index 19e096501a2..cbf4441c7d6 100644
--- a/opcodes/bpf-opc.c
+++ b/opcodes/bpf-opc.c
@@ -272,8 +272,10 @@ const struct bpf_opcode bpf_opcodes[] =
    BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_JSET|BPF_SRC_X},
   {BPF_INSN_JNER, "jne%W%dr , %sr , %d16", "if%w%dr != %sr%wgoto%w%d16",
    BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_JNE|BPF_SRC_X},
-  {BPF_INSN_CALLR, "call%W%dr", "callx%w%dr",
+  {BPF_INSN_CALLR, "callr%W%dr", "callr%w%dr",
    BPF_XBPF, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_CALL|BPF_SRC_X},
+  {BPF_INSN_CALLX, "callx%W%r32", "callx%w%r32",
+   BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_CALL|BPF_SRC_X},
   {BPF_INSN_CALL, "call%W%d32", "call%w%d32",
    BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_CALL|BPF_SRC_K},
   {BPF_INSN_EXIT, "exit", "exit",
diff --git a/sim/bpf/bpf-sim.c b/sim/bpf/bpf-sim.c
index c1f103823fb..a3976d0b4bf 100644
--- a/sim/bpf/bpf-sim.c
+++ b/sim/bpf/bpf-sim.c
@@ -1096,6 +1096,10 @@ execute (SIM_CPU *cpu, struct bpf_insn *insn)
       BPF_TRACE ("BPF_INSN_CALLR\n");
       bpf_call (cpu, DISP (bpf_regs[insn->dst]), insn->src);
       break;
+    case BPF_INSN_CALLX:
+      BPF_TRACE ("BPF_INSN_CALLX\n");
+      bpf_call (cpu, DISP (bpf_regs[insn->dst]), insn->src);
+      break;
     case BPF_INSN_CALL:
       BPF_TRACE ("BPF_INSN_CALL\n");
       bpf_call (cpu, insn->imm32, insn->src);
-- 
2.43.0

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

[Nick Clifton]

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

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

[Will Hawkins]

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


>
>
>
>

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

[Jose E. Marchesi]

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

Hi Will.

It seems to me that all we need binutils-wise is to enable the callx
instruction with BPF >= v1.  No other changes are necessary as far as I
can see, other than adjusting the testsuite accordingly.

Thanks!

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

[Will Hawkins]

On Wed, Feb 14, 2024 at 5:58 AM Jose E. Marchesi
<jose.marchesi@oracle.com> wrote:
>
>
> > 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.
>
> Hi Will.
>
> It seems to me that all we need binutils-wise is to enable the callx
> instruction with BPF >= v1.  No other changes are necessary as far as I
> can see, other than adjusting the testsuite accordingly.

It should be making its way across the Internet to you now!

The only thing ... it will probably cause a build error until a
corresponding patch in the simulator lands that takes into
consideration the new enum value.

I am happy to handle that however you like!

Sorry for the delay!
Will


>
> Thanks!

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

[Jose E. Marchesi]

> On Wed, Feb 14, 2024 at 5:58 AM Jose E. Marchesi
> <jose.marchesi@oracle.com> wrote:
>>
>>
>> > 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.
>>
>> Hi Will.
>>
>> It seems to me that all we need binutils-wise is to enable the callx
>> instruction with BPF >= v1.  No other changes are necessary as far as I
>> can see, other than adjusting the testsuite accordingly.
>
> It should be making its way across the Internet to you now!

Thanks :)

> The only thing ... it will probably cause a build error until a
> corresponding patch in the simulator lands that takes into
> consideration the new enum value.
>
> I am happy to handle that however you like!

No problem.  I will tackle the simulator patch in the GDB list and apply
both at almost the same time so the buildbots do not get (too) upset.

> Sorry for the delay!
> Will
>
>
>>
>> Thanks!

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

[Will Hawkins]

On Wed, Feb 14, 2024 at 11:14 AM Jose E. Marchesi
<jose.marchesi@oracle.com> wrote:
>
>
> > On Wed, Feb 14, 2024 at 5:58 AM Jose E. Marchesi
> > <jose.marchesi@oracle.com> wrote:
> >>
> >>
> >> > 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.
> >>
> >> Hi Will.
> >>
> >> It seems to me that all we need binutils-wise is to enable the callx
> >> instruction with BPF >= v1.  No other changes are necessary as far as I
> >> can see, other than adjusting the testsuite accordingly.
> >
> > It should be making its way across the Internet to you now!
>
> Thanks :)
>
> > The only thing ... it will probably cause a build error until a
> > corresponding patch in the simulator lands that takes into
> > consideration the new enum value.
> >
> > I am happy to handle that however you like!
>
> No problem.  I will tackle the simulator patch in the GDB list and apply
> both at almost the same time so the buildbots do not get (too) upset.

Thanks!! It's been fun working on this. And, as I offered to Nick last
night, I am more than happy to take on a little project to do a
cleanup of the code. Just let me know if you would be open to that!

Will

>
> > Sorry for the delay!
> > Will
> >
> >
> >>
> >> Thanks!

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

[Jose E. Marchesi]

> On Wed, Feb 14, 2024 at 11:14 AM Jose E. Marchesi
> <jose.marchesi@oracle.com> wrote:
>>
>>
>> > On Wed, Feb 14, 2024 at 5:58 AM Jose E. Marchesi
>> > <jose.marchesi@oracle.com> wrote:
>> >>
>> >>
>> >> > 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.
>> >>
>> >> Hi Will.
>> >>
>> >> It seems to me that all we need binutils-wise is to enable the callx
>> >> instruction with BPF >= v1.  No other changes are necessary as far as I
>> >> can see, other than adjusting the testsuite accordingly.
>> >
>> > It should be making its way across the Internet to you now!
>>
>> Thanks :)
>>
>> > The only thing ... it will probably cause a build error until a
>> > corresponding patch in the simulator lands that takes into
>> > consideration the new enum value.
>> >
>> > I am happy to handle that however you like!
>>
>> No problem.  I will tackle the simulator patch in the GDB list and apply
>> both at almost the same time so the buildbots do not get (too) upset.
>
> Thanks!! It's been fun working on this. And, as I offered to Nick last
> night, I am more than happy to take on a little project to do a
> cleanup of the code. Just let me know if you would be open to that!

By all means, and than you.
Improvements are always very welcome :)

BTW, I would ask you to not include a separated cover letter for single
patches.  This makes it easier to reply and quote to both the
description of the patch and its contents.

>
> Will
>
>>
>> > Sorry for the delay!
>> > Will
>> >
>> >
>> >>
>> >> Thanks!

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

[Will Hawkins]

On Thu, Feb 15, 2024 at 10:32 AM Jose E. Marchesi
<jose.marchesi@oracle.com> wrote:
>
>
> > On Wed, Feb 14, 2024 at 11:14 AM Jose E. Marchesi
> > <jose.marchesi@oracle.com> wrote:
> >>
> >>
> >> > On Wed, Feb 14, 2024 at 5:58 AM Jose E. Marchesi
> >> > <jose.marchesi@oracle.com> wrote:
> >> >>
> >> >>
> >> >> > 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.
> >> >>
> >> >> Hi Will.
> >> >>
> >> >> It seems to me that all we need binutils-wise is to enable the callx
> >> >> instruction with BPF >= v1.  No other changes are necessary as far as I
> >> >> can see, other than adjusting the testsuite accordingly.
> >> >
> >> > It should be making its way across the Internet to you now!
> >>
> >> Thanks :)
> >>
> >> > The only thing ... it will probably cause a build error until a
> >> > corresponding patch in the simulator lands that takes into
> >> > consideration the new enum value.
> >> >
> >> > I am happy to handle that however you like!
> >>
> >> No problem.  I will tackle the simulator patch in the GDB list and apply
> >> both at almost the same time so the buildbots do not get (too) upset.
> >
> > Thanks!! It's been fun working on this. And, as I offered to Nick last
> > night, I am more than happy to take on a little project to do a
> > cleanup of the code. Just let me know if you would be open to that!
>
> By all means, and than you.
> Improvements are always very welcome :)
>
> BTW, I would ask you to not include a separated cover letter for single
> patches.  This makes it easier to reply and quote to both the
> description of the patch and its contents.

Thank you! That's great feedback! I will definitely do that from now on!

Will


>
> >
> > Will
> >
> >>
> >> > Sorry for the delay!
> >> > Will
> >> >
> >> >
> >> >>
> >> >> Thanks!

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

[Nick Clifton]

Hi Will,

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

No, that is the right way.  I was only checking, because as a new contributor
(to the GNU Binutils at least) I wanted to be certain that you were aware of
the significance of the sign-off.


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

Really ?  Personally I have no problem with whitespace cleanup patches.
My only real worry is when they appear as part of a patch that is doing
something else.  Generally speaking it is always best to keep patches
as simple as possible, so a patch that changes code, cleans up whitespace
and corrects spelling mistakes all in one is going to be frowned upon (by
me at least) whereas as three separate patches would be welcome.


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

Yes.

Sorry - I was confused when I first read this as "PR" to me normally
means "Problem Report", as in a bug filed in the binutils bugzilla system.
These are assigned PR numbers and can often be seen referred to in comments
in the code and the changelogs.

I assume however that you mean "Pull Request" here, which makes sense in
context, except that the binutils project being old and stuffy we normally
go by patch submissions rather than pull requests...


One other thing I should also mention.  Whenever you do submit a patch it
always helps if you mention how it was tested.  Noting that it was tested
with a binutils build configured for bpf-elf and that there were no
regressions in the gas, binutils or ld testsuites is reassuring and helps
to make the job of reviewing the patch easier.  This applies even to code
cleanup patches, where of course no test regressions are expected, but you
never know...


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

Oh no, please do step in.  No-one has a "its all mine" territory.  We do
have contributors who have volunteered to act as maintainers for certain
architectures and/or parts of the binutils, and they will often be the
ones who review patch submissions in their areas.  But everyone is welcome
to submit patches to any and all parts of the binutils.

Cheers  Nick

Re: [PATCH v2 0/1] Add BPF callx support to objdump and as

[Jose E. Marchesi]

Hi Will.

[Adding Yonghong and Eduard in CC]

> After additional consideration and discussion with Jose and Dave,
> it seems like we have determined the way that clang, gcc and binutils
> need to handle the callx/callr:
>
> 1. callr remains with the register holding the target of the jump stored
> in the dst_reg.
> 2. callx is added with the register holding the target of the jump stored
> in the imm32.
> 3. We have to remove the pseudoc syntax because it is no longer possible
> to disambiguate between versions of call by simply looking at the
> parameter.

I don't recall reaching any agreement on the above.  What is the point
of having both callr and callx?

The existing callr is generated by GCC in -mxbpf mode.  It is an
experimental extension that we use in order to be able to run more of
the GCC testsuite, so it is always possible to change it to use imm32
instead of dst_reg.

I wouldn't personally welcome that change and would much prefer if clang
starts using either reg_src or reg_dst, because compromising/reserving
endian-dependent 32 whole bits for a register number that only requires
4 bits seems like a waste of insn space that will complicate future ISA
extensions.

In either case, if we all use the same encoding for the indirect call
instruction (I fail to see any reason for not doing so) then point
3. becomes moot.

>
> Tests are added/refactored to meet the above. 
>
> I am more than happy to resend as a separate mailing to the list but
> sending first as a reply in order to keep list traffic manageable.
>
> As I said before, I sincerely appreciate all that you are doing for
> the community and how welcoming you have been to a first-time contributor.
>
> Sincerely,
> Will
>
> Will Hawkins (1):
>   objdump, as: Add callx support for BPF CPU v1
>
>  gas/config/tc-bpf.c                           | 25 ++++++++++++++++++-
>  gas/testsuite/gas/bpf/bpf.exp                 |  4 +--
>  .../gas/bpf/{indcall-1.d => callr.d}          |  4 +--
>  .../gas/bpf/{indcall-1.s => callr.s}          |  2 +-
>  gas/testsuite/gas/bpf/indcall-1-pseudoc.d     | 23 -----------------
>  gas/testsuite/gas/bpf/indcall-1-pseudoc.s     | 13 ----------
>  include/opcode/bpf.h                          |  3 ++-
>  opcodes/bpf-dis.c                             |  6 +++++
>  opcodes/bpf-opc.c                             |  4 ++-
>  sim/bpf/bpf-sim.c                             |  4 +++
>  10 files changed, 44 insertions(+), 44 deletions(-)
>  rename gas/testsuite/gas/bpf/{indcall-1.d => callr.d} (90%)
>  rename gas/testsuite/gas/bpf/{indcall-1.s => callr.s} (90%)
>  delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.d
>  delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.s

Re: [PATCH v2 0/1] Add BPF callx support to objdump and as

[Will Hawkins]

Hello!

First, thank you for the response!

On Mon, Feb 12, 2024 at 1:39 PM Jose E. Marchesi
<jose.marchesi@oracle.com> wrote:
>
>
> Hi Will.
>
> [Adding Yonghong and Eduard in CC]
>
> > After additional consideration and discussion with Jose and Dave,
> > it seems like we have determined the way that clang, gcc and binutils
> > need to handle the callx/callr:
> >
> > 1. callr remains with the register holding the target of the jump stored
> > in the dst_reg.
> > 2. callx is added with the register holding the target of the jump stored
> > in the imm32.
> > 3. We have to remove the pseudoc syntax because it is no longer possible
> > to disambiguate between versions of call by simply looking at the
> > parameter.
>
> I don't recall reaching any agreement on the above.  What is the point
> of having both callr and callx?

Sorry! I was being slightly loose in terms of agreement -- I was
reading into your comments in the email between you, me and Dave from
earlier this weekend!

The only point in having both callr and callx was to allow the gcc
encoding to continue to exist in its current form. I assumed that
there was a compelling reason and certainly did not want to do
anything to interfere with the great work that you are doing!

>
> The existing callr is generated by GCC in -mxbpf mode.  It is an
> experimental extension that we use in order to be able to run more of
> the GCC testsuite, so it is always possible to change it to use imm32
> instead of dst_reg.
>
> I wouldn't personally welcome that change and would much prefer if clang
> starts using either reg_src or reg_dst, because compromising/reserving
> endian-dependent 32 whole bits for a register number that only requires
> 4 bits seems like a waste of insn space that will complicate future ISA
> extensions.

I 100% agree that it is less than ideal. However, it seems like the
cat is out of the bag. I am adding Dave who is leading the ISA
standardization effort. He and I (and others) have discussed this as
recently as this morning. I will let him weigh in on whether or not we
have the "power" to push back on clang's choice of how to encode the
instructions.

>
> In either case, if we all use the same encoding for the indirect call
> instruction (I fail to see any reason for not doing so) then point
> 3. becomes moot.

I agree and I really would like that to be the outcome. However, see
above (insert smiley face here!)

Thank you for responding!

Will

>
> >
> > Tests are added/refactored to meet the above.
> >
> > I am more than happy to resend as a separate mailing to the list but
> > sending first as a reply in order to keep list traffic manageable.
> >
> > As I said before, I sincerely appreciate all that you are doing for
> > the community and how welcoming you have been to a first-time contributor.
> >
> > Sincerely,
> > Will
> >
> > Will Hawkins (1):
> >   objdump, as: Add callx support for BPF CPU v1
> >
> >  gas/config/tc-bpf.c                           | 25 ++++++++++++++++++-
> >  gas/testsuite/gas/bpf/bpf.exp                 |  4 +--
> >  .../gas/bpf/{indcall-1.d => callr.d}          |  4 +--
> >  .../gas/bpf/{indcall-1.s => callr.s}          |  2 +-
> >  gas/testsuite/gas/bpf/indcall-1-pseudoc.d     | 23 -----------------
> >  gas/testsuite/gas/bpf/indcall-1-pseudoc.s     | 13 ----------
> >  include/opcode/bpf.h                          |  3 ++-
> >  opcodes/bpf-dis.c                             |  6 +++++
> >  opcodes/bpf-opc.c                             |  4 ++-
> >  sim/bpf/bpf-sim.c                             |  4 +++
> >  10 files changed, 44 insertions(+), 44 deletions(-)
> >  rename gas/testsuite/gas/bpf/{indcall-1.d => callr.d} (90%)
> >  rename gas/testsuite/gas/bpf/{indcall-1.s => callr.s} (90%)
> >  delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.d
> >  delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.s

Re: [PATCH v2 0/1] Add BPF callx support to objdump and as

[Will Hawkins]

On Mon, Feb 12, 2024 at 5:25 PM Will Hawkins <hawkinsw@obs.cr> wrote:
>
> Hello!
>
> First, thank you for the response!
>
> On Mon, Feb 12, 2024 at 1:39 PM Jose E. Marchesi
> <jose.marchesi@oracle.com> wrote:
> >
> >
> > Hi Will.
> >
> > [Adding Yonghong and Eduard in CC]
> >
> > > After additional consideration and discussion with Jose and Dave,
> > > it seems like we have determined the way that clang, gcc and binutils
> > > need to handle the callx/callr:
> > >
> > > 1. callr remains with the register holding the target of the jump stored
> > > in the dst_reg.
> > > 2. callx is added with the register holding the target of the jump stored
> > > in the imm32.
> > > 3. We have to remove the pseudoc syntax because it is no longer possible
> > > to disambiguate between versions of call by simply looking at the
> > > parameter.
> >
> > I don't recall reaching any agreement on the above.  What is the point
> > of having both callr and callx?
>
> Sorry! I was being slightly loose in terms of agreement -- I was
> reading into your comments in the email between you, me and Dave from
> earlier this weekend!
>
> The only point in having both callr and callx was to allow the gcc
> encoding to continue to exist in its current form. I assumed that
> there was a compelling reason and certainly did not want to do
> anything to interfere with the great work that you are doing!
>
> >
> > The existing callr is generated by GCC in -mxbpf mode.  It is an
> > experimental extension that we use in order to be able to run more of
> > the GCC testsuite, so it is always possible to change it to use imm32
> > instead of dst_reg.
> >
> > I wouldn't personally welcome that change and would much prefer if clang
> > starts using either reg_src or reg_dst, because compromising/reserving
> > endian-dependent 32 whole bits for a register number that only requires
> > 4 bits seems like a waste of insn space that will complicate future ISA
> > extensions.
>
> I 100% agree that it is less than ideal. However, it seems like the
> cat is out of the bag. I am adding Dave who is leading the ISA
> standardization effort. He and I (and others) have discussed this as
> recently as this morning. I will let him weigh in on whether or not we
> have the "power" to push back on clang's choice of how to encode the
> instructions.
>
> >
> > In either case, if we all use the same encoding for the indirect call
> > instruction (I fail to see any reason for not doing so) then point
> > 3. becomes moot.
>
> I agree and I really would like that to be the outcome. However, see
> above (insert smiley face here!)
>

I just reviewed some mailing traffic from another list and it looks
like the folks at clang/llvm are going to change the way that they
encode the callx instruction! Great news!

I will make a (simpler) updated patch to binutils once those changes
are in llvm and we can verify them.

Thank you again for your response, Jose!
Will


> Thank you for responding!
>
> Will
>
> >
> > >
> > > Tests are added/refactored to meet the above.
> > >
> > > I am more than happy to resend as a separate mailing to the list but
> > > sending first as a reply in order to keep list traffic manageable.
> > >
> > > As I said before, I sincerely appreciate all that you are doing for
> > > the community and how welcoming you have been to a first-time contributor.
> > >
> > > Sincerely,
> > > Will
> > >
> > > Will Hawkins (1):
> > >   objdump, as: Add callx support for BPF CPU v1
> > >
> > >  gas/config/tc-bpf.c                           | 25 ++++++++++++++++++-
> > >  gas/testsuite/gas/bpf/bpf.exp                 |  4 +--
> > >  .../gas/bpf/{indcall-1.d => callr.d}          |  4 +--
> > >  .../gas/bpf/{indcall-1.s => callr.s}          |  2 +-
> > >  gas/testsuite/gas/bpf/indcall-1-pseudoc.d     | 23 -----------------
> > >  gas/testsuite/gas/bpf/indcall-1-pseudoc.s     | 13 ----------
> > >  include/opcode/bpf.h                          |  3 ++-
> > >  opcodes/bpf-dis.c                             |  6 +++++
> > >  opcodes/bpf-opc.c                             |  4 ++-
> > >  sim/bpf/bpf-sim.c                             |  4 +++
> > >  10 files changed, 44 insertions(+), 44 deletions(-)
> > >  rename gas/testsuite/gas/bpf/{indcall-1.d => callr.d} (90%)
> > >  rename gas/testsuite/gas/bpf/{indcall-1.s => callr.s} (90%)
> > >  delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.d
> > >  delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.s

Re: [PATCH v2 0/1] Add BPF callx support to objdump and as

[Yonghong Song]

On 2/12/24 2:38 PM, Will Hawkins wrote:
> On Mon, Feb 12, 2024 at 5:25 PM Will Hawkins <hawkinsw@obs.cr> wrote:
>> Hello!
>>
>> First, thank you for the response!
>>
>> On Mon, Feb 12, 2024 at 1:39 PM Jose E. Marchesi
>> <jose.marchesi@oracle.com> wrote:
>>>
>>> Hi Will.
>>>
>>> [Adding Yonghong and Eduard in CC]
>>>
>>>> After additional consideration and discussion with Jose and Dave,
>>>> it seems like we have determined the way that clang, gcc and binutils
>>>> need to handle the callx/callr:
>>>>
>>>> 1. callr remains with the register holding the target of the jump stored
>>>> in the dst_reg.
>>>> 2. callx is added with the register holding the target of the jump stored
>>>> in the imm32.
>>>> 3. We have to remove the pseudoc syntax because it is no longer possible
>>>> to disambiguate between versions of call by simply looking at the
>>>> parameter.
>>> I don't recall reaching any agreement on the above.  What is the point
>>> of having both callr and callx?
>> Sorry! I was being slightly loose in terms of agreement -- I was
>> reading into your comments in the email between you, me and Dave from
>> earlier this weekend!
>>
>> The only point in having both callr and callx was to allow the gcc
>> encoding to continue to exist in its current form. I assumed that
>> there was a compelling reason and certainly did not want to do
>> anything to interfere with the great work that you are doing!
>>
>>> The existing callr is generated by GCC in -mxbpf mode.  It is an
>>> experimental extension that we use in order to be able to run more of
>>> the GCC testsuite, so it is always possible to change it to use imm32
>>> instead of dst_reg.
>>>
>>> I wouldn't personally welcome that change and would much prefer if clang
>>> starts using either reg_src or reg_dst, because compromising/reserving
>>> endian-dependent 32 whole bits for a register number that only requires
>>> 4 bits seems like a waste of insn space that will complicate future ISA
>>> extensions.
>> I 100% agree that it is less than ideal. However, it seems like the
>> cat is out of the bag. I am adding Dave who is leading the ISA
>> standardization effort. He and I (and others) have discussed this as
>> recently as this morning. I will let him weigh in on whether or not we
>> have the "power" to push back on clang's choice of how to encode the
>> instructions.
>>
>>> In either case, if we all use the same encoding for the indirect call
>>> instruction (I fail to see any reason for not doing so) then point
>>> 3. becomes moot.
>> I agree and I really would like that to be the outcome. However, see
>> above (insert smiley face here!)
>>
> I just reviewed some mailing traffic from another list and it looks
> like the folks at clang/llvm are going to change the way that they
> encode the callx instruction! Great news!
>
> I will make a (simpler) updated patch to binutils once those changes
> are in llvm and we can verify them.

the llvm patch:
    https://github.com/llvm/llvm-project/pull/81546
Could you help double check encoding is the same as gcc?

Thanks!

>
> Thank you again for your response, Jose!
> Will
>
>
>> Thank you for responding!
>>
>> Will
>>
>>>> Tests are added/refactored to meet the above.
>>>>
>>>> I am more than happy to resend as a separate mailing to the list but
>>>> sending first as a reply in order to keep list traffic manageable.
>>>>
>>>> As I said before, I sincerely appreciate all that you are doing for
>>>> the community and how welcoming you have been to a first-time contributor.
>>>>
>>>> Sincerely,
>>>> Will
>>>>
>>>> Will Hawkins (1):
>>>>    objdump, as: Add callx support for BPF CPU v1
>>>>
>>>>   gas/config/tc-bpf.c                           | 25 ++++++++++++++++++-
>>>>   gas/testsuite/gas/bpf/bpf.exp                 |  4 +--
>>>>   .../gas/bpf/{indcall-1.d => callr.d}          |  4 +--
>>>>   .../gas/bpf/{indcall-1.s => callr.s}          |  2 +-
>>>>   gas/testsuite/gas/bpf/indcall-1-pseudoc.d     | 23 -----------------
>>>>   gas/testsuite/gas/bpf/indcall-1-pseudoc.s     | 13 ----------
>>>>   include/opcode/bpf.h                          |  3 ++-
>>>>   opcodes/bpf-dis.c                             |  6 +++++
>>>>   opcodes/bpf-opc.c                             |  4 ++-
>>>>   sim/bpf/bpf-sim.c                             |  4 +++
>>>>   10 files changed, 44 insertions(+), 44 deletions(-)
>>>>   rename gas/testsuite/gas/bpf/{indcall-1.d => callr.d} (90%)
>>>>   rename gas/testsuite/gas/bpf/{indcall-1.s => callr.s} (90%)
>>>>   delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.d
>>>>   delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.s

Re: [PATCH v2 0/1] Add BPF callx support to objdump and as

[Will Hawkins]

On Mon, Feb 12, 2024 at 5:50 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 2/12/24 2:38 PM, Will Hawkins wrote:
> > On Mon, Feb 12, 2024 at 5:25 PM Will Hawkins <hawkinsw@obs.cr> wrote:
> >> Hello!
> >>
> >> First, thank you for the response!
> >>
> >> On Mon, Feb 12, 2024 at 1:39 PM Jose E. Marchesi
> >> <jose.marchesi@oracle.com> wrote:
> >>>
> >>> Hi Will.
> >>>
> >>> [Adding Yonghong and Eduard in CC]
> >>>
> >>>> After additional consideration and discussion with Jose and Dave,
> >>>> it seems like we have determined the way that clang, gcc and binutils
> >>>> need to handle the callx/callr:
> >>>>
> >>>> 1. callr remains with the register holding the target of the jump stored
> >>>> in the dst_reg.
> >>>> 2. callx is added with the register holding the target of the jump stored
> >>>> in the imm32.
> >>>> 3. We have to remove the pseudoc syntax because it is no longer possible
> >>>> to disambiguate between versions of call by simply looking at the
> >>>> parameter.
> >>> I don't recall reaching any agreement on the above.  What is the point
> >>> of having both callr and callx?
> >> Sorry! I was being slightly loose in terms of agreement -- I was
> >> reading into your comments in the email between you, me and Dave from
> >> earlier this weekend!
> >>
> >> The only point in having both callr and callx was to allow the gcc
> >> encoding to continue to exist in its current form. I assumed that
> >> there was a compelling reason and certainly did not want to do
> >> anything to interfere with the great work that you are doing!
> >>
> >>> The existing callr is generated by GCC in -mxbpf mode.  It is an
> >>> experimental extension that we use in order to be able to run more of
> >>> the GCC testsuite, so it is always possible to change it to use imm32
> >>> instead of dst_reg.
> >>>
> >>> I wouldn't personally welcome that change and would much prefer if clang
> >>> starts using either reg_src or reg_dst, because compromising/reserving
> >>> endian-dependent 32 whole bits for a register number that only requires
> >>> 4 bits seems like a waste of insn space that will complicate future ISA
> >>> extensions.
> >> I 100% agree that it is less than ideal. However, it seems like the
> >> cat is out of the bag. I am adding Dave who is leading the ISA
> >> standardization effort. He and I (and others) have discussed this as
> >> recently as this morning. I will let him weigh in on whether or not we
> >> have the "power" to push back on clang's choice of how to encode the
> >> instructions.
> >>
> >>> In either case, if we all use the same encoding for the indirect call
> >>> instruction (I fail to see any reason for not doing so) then point
> >>> 3. becomes moot.
> >> I agree and I really would like that to be the outcome. However, see
> >> above (insert smiley face here!)
> >>
> > I just reviewed some mailing traffic from another list and it looks
> > like the folks at clang/llvm are going to change the way that they
> > encode the callx instruction! Great news!
> >
> > I will make a (simpler) updated patch to binutils once those changes
> > are in llvm and we can verify them.
>
> the llvm patch:
>     https://github.com/llvm/llvm-project/pull/81546
> Could you help double check encoding is the same as gcc?


I would be more than happy to do so! It will be a few hours, but I
will absolutely look at it ASAP!

Better yet, I will pull that patch, build an LLVM and give it a try to
double check.

Thank you for working so quickly, Yonghong!
Will

>
> Thanks!
>
> >
> > Thank you again for your response, Jose!
> > Will
> >
> >
> >> Thank you for responding!
> >>
> >> Will
> >>
> >>>> Tests are added/refactored to meet the above.
> >>>>
> >>>> I am more than happy to resend as a separate mailing to the list but
> >>>> sending first as a reply in order to keep list traffic manageable.
> >>>>
> >>>> As I said before, I sincerely appreciate all that you are doing for
> >>>> the community and how welcoming you have been to a first-time contributor.
> >>>>
> >>>> Sincerely,
> >>>> Will
> >>>>
> >>>> Will Hawkins (1):
> >>>>    objdump, as: Add callx support for BPF CPU v1
> >>>>
> >>>>   gas/config/tc-bpf.c                           | 25 ++++++++++++++++++-
> >>>>   gas/testsuite/gas/bpf/bpf.exp                 |  4 +--
> >>>>   .../gas/bpf/{indcall-1.d => callr.d}          |  4 +--
> >>>>   .../gas/bpf/{indcall-1.s => callr.s}          |  2 +-
> >>>>   gas/testsuite/gas/bpf/indcall-1-pseudoc.d     | 23 -----------------
> >>>>   gas/testsuite/gas/bpf/indcall-1-pseudoc.s     | 13 ----------
> >>>>   include/opcode/bpf.h                          |  3 ++-
> >>>>   opcodes/bpf-dis.c                             |  6 +++++
> >>>>   opcodes/bpf-opc.c                             |  4 ++-
> >>>>   sim/bpf/bpf-sim.c                             |  4 +++
> >>>>   10 files changed, 44 insertions(+), 44 deletions(-)
> >>>>   rename gas/testsuite/gas/bpf/{indcall-1.d => callr.d} (90%)
> >>>>   rename gas/testsuite/gas/bpf/{indcall-1.s => callr.s} (90%)
> >>>>   delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.d
> >>>>   delete mode 100644 gas/testsuite/gas/bpf/indcall-1-pseudoc.s