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