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

[Will Hawkins <hawkinsw@obs.cr>]

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