[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