* [PATCH v3 0/1] Move callx (nee callr) to v1 ISA variant @ 2024-02-14 16:02 Will Hawkins 2024-02-14 16:03 ` [PATCH v3 1/1] objdump, as: Add callx support for BPF CPU v1 Will Hawkins 0 siblings, 1 reply; 5+ messages in thread From: Will Hawkins @ 2024-02-14 16:02 UTC (permalink / raw) To: binutils; +Cc: Will Hawkins, yonghong.song Thank you for all the feedback on v1 and v2! Thanks to the willingness of the clang/llvm developers to compromise on their encoding of the callx instruction, this version of the patch is much more straightforward than v2. In other words, a special thanks to Yonghong Song! The only "controversial" piece of this patch is the reworking of the testsuite code. I refactored the test code slightly to (visually) match the test code from the clang/llvm test code for the callx instruction. It made it easier for me to visually confirm that the objdump output matches. I am more than happy to revert that piece if what I did goes too far. Of course, I am also happy to change anything about this patch to make sure that it meets the community's guidelines. Thank you, again, for all the help! Will Will Hawkins (1): objdump, as: Add callx support for BPF CPU v1 gas/testsuite/gas/bpf/indcall-1-pseudoc.d | 8 ++++---- gas/testsuite/gas/bpf/indcall-1-pseudoc.s | 6 +++--- gas/testsuite/gas/bpf/indcall-1.d | 10 +++++----- gas/testsuite/gas/bpf/indcall-1.s | 6 +++--- include/opcode/bpf.h | 2 +- opcodes/bpf-opc.c | 4 ++-- 6 files changed, 18 insertions(+), 18 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 1/1] objdump, as: Add callx support for BPF CPU v1 2024-02-14 16:02 [PATCH v3 0/1] Move callx (nee callr) to v1 ISA variant Will Hawkins @ 2024-02-14 16:03 ` Will Hawkins 2024-02-14 16:30 ` Jose E. Marchesi 0 siblings, 1 reply; 5+ messages in thread From: Will Hawkins @ 2024-02-14 16:03 UTC (permalink / raw) To: binutils; +Cc: Will Hawkins, yonghong.song Add support for (dis)assembling the callx instruction back to CPU v1. gas/ChangeLog: * testsuite/gas/bpf/indcall-1-pseudoc.d: Refactor tests ... * testsuite/gas/bpf/indcall-1-pseudoc.s: ... to visually match ... * testsuite/gas/bpf/indcall-1.d: ... equivalent test in ... * testsuite/gas/bpf/indcall-1.s: ... clang/llvm. include/ChangeLog: * opcode/bpf.h (enum bpf_insn_id): BPF_INSN_CALLR to BPF_INSN_CALLX * (for consistency) and add it to the v1 ISA variant. opcodes/ChangeLog: * bpf-opc.c: Use BPF_INSN_CALLX instead of BPF_INSN_CALLR. Signed-off-by: Will Hawkins <hawkinsw@obs.cr> --- gas/testsuite/gas/bpf/indcall-1-pseudoc.d | 8 ++++---- gas/testsuite/gas/bpf/indcall-1-pseudoc.s | 6 +++--- gas/testsuite/gas/bpf/indcall-1.d | 10 +++++----- gas/testsuite/gas/bpf/indcall-1.s | 6 +++--- include/opcode/bpf.h | 2 +- opcodes/bpf-opc.c | 4 ++-- 6 files changed, 18 insertions(+), 18 deletions(-) diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d b/gas/testsuite/gas/bpf/indcall-1-pseudoc.d index 7a95bad8e65..12f9d6a9d49 100644 --- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d +++ b/gas/testsuite/gas/bpf/indcall-1-pseudoc.d @@ -1,4 +1,4 @@ -#as: -EL -mdialect=pseudoc -misa-spec=xbpf +#as: -EL -mdialect=pseudoc -misa-spec=v1 #objdump: -M xbpf,pseudoc,dec -dr #source: indcall-1-pseudoc.s #name: BPF indirect call 1, pseudoc syntax @@ -10,11 +10,11 @@ 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 + 10: b7 03 00 00 03 00 00 00 r3=3 + 18: 18 02 00 00 38 00 00 00 r2=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 + 28: 8d 02 00 00 00 00 00 00 callx r2 30: 95 00 00 00 00 00 00 00 exit 0000000000000038 <bar>: diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s b/gas/testsuite/gas/bpf/indcall-1-pseudoc.s index 2042697f15b..5639e288869 100644 --- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s +++ b/gas/testsuite/gas/bpf/indcall-1-pseudoc.s @@ -4,9 +4,9 @@ main: r0 = 1 r1 = 1 - r2 = 2 - r6 = bar ll - callx r6 + r3 = 3 + r2 = bar ll + callx r2 exit bar: r0 = 0 diff --git a/gas/testsuite/gas/bpf/indcall-1.d b/gas/testsuite/gas/bpf/indcall-1.d index 51103bba2a1..1a2c36999b1 100644 --- a/gas/testsuite/gas/bpf/indcall-1.d +++ b/gas/testsuite/gas/bpf/indcall-1.d @@ -1,5 +1,5 @@ -#as: -EL -misa-spec=xbpf -#objdump: -dr -M xbpf,dec +#as: -EL -misa-spec=v1 +#objdump: -dr -M v1,dec #source: indcall-1.s #name: BPF indirect call 1, normal syntax @@ -10,11 +10,11 @@ Disassembly of section \.text: 0000000000000000 <main>: 0: b7 00 00 00 01 00 00 00 mov %r0,1 8: b7 01 00 00 01 00 00 00 mov %r1,1 - 10: b7 02 00 00 02 00 00 00 mov %r2,2 - 18: 18 06 00 00 38 00 00 00 lddw %r6,56 + 10: b7 03 00 00 03 00 00 00 mov %r3,3 + 18: 18 02 00 00 38 00 00 00 lddw %r2,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 02 00 00 00 00 00 00 call %r2 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/indcall-1.s index 5d49e41040a..7fbeeeb9a38 100644 --- a/gas/testsuite/gas/bpf/indcall-1.s +++ b/gas/testsuite/gas/bpf/indcall-1.s @@ -4,9 +4,9 @@ main: mov %r0, 1 mov %r1, 1 - mov %r2, 2 - lddw %r6, bar - call %r6 + mov %r3, 3 + lddw %r2, bar + call %r2 exit bar: diff --git a/include/opcode/bpf.h b/include/opcode/bpf.h index df1e3bd0918..97e25053175 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_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, diff --git a/opcodes/bpf-opc.c b/opcodes/bpf-opc.c index 19e096501a2..23473fc0cd9 100644 --- a/opcodes/bpf-opc.c +++ b/opcodes/bpf-opc.c @@ -272,8 +272,8 @@ 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_XBPF, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_CALL|BPF_SRC_X}, + {BPF_INSN_CALLX, "call%W%dr", "callx%w%dr", + 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", -- 2.43.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/1] objdump, as: Add callx support for BPF CPU v1 2024-02-14 16:03 ` [PATCH v3 1/1] objdump, as: Add callx support for BPF CPU v1 Will Hawkins @ 2024-02-14 16:30 ` Jose E. Marchesi 2024-02-14 16:36 ` Will Hawkins 0 siblings, 1 reply; 5+ messages in thread From: Jose E. Marchesi @ 2024-02-14 16:30 UTC (permalink / raw) To: Will Hawkins; +Cc: binutils, yonghong.song Hi Will. Thanks for the patch. Please see some comments below. > Add support for (dis)assembling the callx instruction back to CPU v1. > > gas/ChangeLog: > > * testsuite/gas/bpf/indcall-1-pseudoc.d: Refactor tests ... > * testsuite/gas/bpf/indcall-1-pseudoc.s: ... to visually match ... > * testsuite/gas/bpf/indcall-1.d: ... equivalent test in ... > * testsuite/gas/bpf/indcall-1.s: ... clang/llvm. > > include/ChangeLog: > > * opcode/bpf.h (enum bpf_insn_id): BPF_INSN_CALLR to BPF_INSN_CALLX > * (for consistency) and add it to the v1 ISA variant. > > opcodes/ChangeLog: > > * bpf-opc.c: Use BPF_INSN_CALLX instead of BPF_INSN_CALLR. > > Signed-off-by: Will Hawkins <hawkinsw@obs.cr> > --- > gas/testsuite/gas/bpf/indcall-1-pseudoc.d | 8 ++++---- > gas/testsuite/gas/bpf/indcall-1-pseudoc.s | 6 +++--- > gas/testsuite/gas/bpf/indcall-1.d | 10 +++++----- > gas/testsuite/gas/bpf/indcall-1.s | 6 +++--- > include/opcode/bpf.h | 2 +- > opcodes/bpf-opc.c | 4 ++-- > 6 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d b/gas/testsuite/gas/bpf/indcall-1-pseudoc.d > index 7a95bad8e65..12f9d6a9d49 100644 > --- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d > +++ b/gas/testsuite/gas/bpf/indcall-1-pseudoc.d > @@ -1,4 +1,4 @@ > -#as: -EL -mdialect=pseudoc -misa-spec=xbpf > +#as: -EL -mdialect=pseudoc -misa-spec=v1 I think this test can now omit -misa-spec entirely. > #objdump: -M xbpf,pseudoc,dec -dr > #source: indcall-1-pseudoc.s > #name: BPF indirect call 1, pseudoc syntax > @@ -10,11 +10,11 @@ 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 > + 10: b7 03 00 00 03 00 00 00 r3=3 > + 18: 18 02 00 00 38 00 00 00 r2=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 > + 28: 8d 02 00 00 00 00 00 00 callx r2 > 30: 95 00 00 00 00 00 00 00 exit I don't see any reason for changing the test bodies like this. If it is about matching some llvm test, IMO consistency between binutils and llvm testsuites is not a general goal we can (or should) aim to. Same applies to the other tests changed in the thunk below. > 0000000000000038 <bar>: > diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s b/gas/testsuite/gas/bpf/indcall-1-pseudoc.s > index 2042697f15b..5639e288869 100644 > --- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s > +++ b/gas/testsuite/gas/bpf/indcall-1-pseudoc.s > @@ -4,9 +4,9 @@ > main: > r0 = 1 > r1 = 1 > - r2 = 2 > - r6 = bar ll > - callx r6 > + r3 = 3 > + r2 = bar ll > + callx r2 > exit > bar: > r0 = 0 > diff --git a/gas/testsuite/gas/bpf/indcall-1.d b/gas/testsuite/gas/bpf/indcall-1.d > index 51103bba2a1..1a2c36999b1 100644 > --- a/gas/testsuite/gas/bpf/indcall-1.d > +++ b/gas/testsuite/gas/bpf/indcall-1.d > @@ -1,5 +1,5 @@ > -#as: -EL -misa-spec=xbpf > -#objdump: -dr -M xbpf,dec > +#as: -EL -misa-spec=v1 > +#objdump: -dr -M v1,dec Likewise, I think this test can just omit the -M v1 and -ima-spec=v1 arguments now. > #source: indcall-1.s > #name: BPF indirect call 1, normal syntax > > @@ -10,11 +10,11 @@ Disassembly of section \.text: > 0000000000000000 <main>: > 0: b7 00 00 00 01 00 00 00 mov %r0,1 > 8: b7 01 00 00 01 00 00 00 mov %r1,1 > - 10: b7 02 00 00 02 00 00 00 mov %r2,2 > - 18: 18 06 00 00 38 00 00 00 lddw %r6,56 > + 10: b7 03 00 00 03 00 00 00 mov %r3,3 > + 18: 18 02 00 00 38 00 00 00 lddw %r2,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 02 00 00 00 00 00 00 call %r2 > 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/indcall-1.s > index 5d49e41040a..7fbeeeb9a38 100644 > --- a/gas/testsuite/gas/bpf/indcall-1.s > +++ b/gas/testsuite/gas/bpf/indcall-1.s > @@ -4,9 +4,9 @@ > main: > mov %r0, 1 > mov %r1, 1 > - mov %r2, 2 > - lddw %r6, bar > - call %r6 > + mov %r3, 3 > + lddw %r2, bar > + call %r2 > exit > > bar: > diff --git a/include/opcode/bpf.h b/include/opcode/bpf.h > index df1e3bd0918..97e25053175 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_CALLX, BPF_INSN_CALL, BPF_INSN_EXIT, I don't think it is a good idea to rename BPF_INSN_CALLR to BPF_INSN_CALLX. At the moment the assembler uses the `callx' mnemonic for it and GCC uses the same name because that is what clang does, but if/when the corresponding instruction gets incorporated into BPF, I indend to push for a better name, certainly not something as unmemorable and indescriptive as `callx'. > /* 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, > diff --git a/opcodes/bpf-opc.c b/opcodes/bpf-opc.c > index 19e096501a2..23473fc0cd9 100644 > --- a/opcodes/bpf-opc.c > +++ b/opcodes/bpf-opc.c > @@ -272,8 +272,8 @@ 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_XBPF, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_CALL|BPF_SRC_X}, > + {BPF_INSN_CALLX, "call%W%dr", "callx%w%dr", > + 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", ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/1] objdump, as: Add callx support for BPF CPU v1 2024-02-14 16:30 ` Jose E. Marchesi @ 2024-02-14 16:36 ` Will Hawkins 2024-02-14 22:13 ` Will Hawkins 0 siblings, 1 reply; 5+ messages in thread From: Will Hawkins @ 2024-02-14 16:36 UTC (permalink / raw) To: Jose E. Marchesi; +Cc: binutils, yonghong.song On Wed, Feb 14, 2024 at 11:30 AM Jose E. Marchesi <jose.marchesi@oracle.com> wrote: > > > Hi Will. > Thanks for the patch. Please see some comments below. > > > Add support for (dis)assembling the callx instruction back to CPU v1. > > > > gas/ChangeLog: > > > > * testsuite/gas/bpf/indcall-1-pseudoc.d: Refactor tests ... > > * testsuite/gas/bpf/indcall-1-pseudoc.s: ... to visually match ... > > * testsuite/gas/bpf/indcall-1.d: ... equivalent test in ... > > * testsuite/gas/bpf/indcall-1.s: ... clang/llvm. > > > > include/ChangeLog: > > > > * opcode/bpf.h (enum bpf_insn_id): BPF_INSN_CALLR to BPF_INSN_CALLX > > * (for consistency) and add it to the v1 ISA variant. > > > > opcodes/ChangeLog: > > > > * bpf-opc.c: Use BPF_INSN_CALLX instead of BPF_INSN_CALLR. > > > > Signed-off-by: Will Hawkins <hawkinsw@obs.cr> > > --- > > gas/testsuite/gas/bpf/indcall-1-pseudoc.d | 8 ++++---- > > gas/testsuite/gas/bpf/indcall-1-pseudoc.s | 6 +++--- > > gas/testsuite/gas/bpf/indcall-1.d | 10 +++++----- > > gas/testsuite/gas/bpf/indcall-1.s | 6 +++--- > > include/opcode/bpf.h | 2 +- > > opcodes/bpf-opc.c | 4 ++-- > > 6 files changed, 18 insertions(+), 18 deletions(-) > > > > diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d b/gas/testsuite/gas/bpf/indcall-1-pseudoc.d > > index 7a95bad8e65..12f9d6a9d49 100644 > > --- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d > > +++ b/gas/testsuite/gas/bpf/indcall-1-pseudoc.d > > @@ -1,4 +1,4 @@ > > -#as: -EL -mdialect=pseudoc -misa-spec=xbpf > > +#as: -EL -mdialect=pseudoc -misa-spec=v1 > > I think this test can now omit -misa-spec entirely. > > > #objdump: -M xbpf,pseudoc,dec -dr > > #source: indcall-1-pseudoc.s > > #name: BPF indirect call 1, pseudoc syntax > > @@ -10,11 +10,11 @@ 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 > > + 10: b7 03 00 00 03 00 00 00 r3=3 > > + 18: 18 02 00 00 38 00 00 00 r2=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 > > + 28: 8d 02 00 00 00 00 00 00 callx r2 > > 30: 95 00 00 00 00 00 00 00 exit > > I don't see any reason for changing the test bodies like this. If it is > about matching some llvm test, IMO consistency between binutils and llvm > testsuites is not a general goal we can (or should) aim to. > > Same applies to the other tests changed in the thunk below. As I said, I am happy to do whatever you like! I did it just so that I could visually check that the two matched and then I left it. In v4 I will revert this particular piece! > > > 0000000000000038 <bar>: > > diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s b/gas/testsuite/gas/bpf/indcall-1-pseudoc.s > > index 2042697f15b..5639e288869 100644 > > --- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s > > +++ b/gas/testsuite/gas/bpf/indcall-1-pseudoc.s > > @@ -4,9 +4,9 @@ > > main: > > r0 = 1 > > r1 = 1 > > - r2 = 2 > > - r6 = bar ll > > - callx r6 > > + r3 = 3 > > + r2 = bar ll > > + callx r2 > > exit > > bar: > > r0 = 0 > > diff --git a/gas/testsuite/gas/bpf/indcall-1.d b/gas/testsuite/gas/bpf/indcall-1.d > > index 51103bba2a1..1a2c36999b1 100644 > > --- a/gas/testsuite/gas/bpf/indcall-1.d > > +++ b/gas/testsuite/gas/bpf/indcall-1.d > > @@ -1,5 +1,5 @@ > > -#as: -EL -misa-spec=xbpf > > -#objdump: -dr -M xbpf,dec > > +#as: -EL -misa-spec=v1 > > +#objdump: -dr -M v1,dec > > Likewise, I think this test can just omit the -M v1 and -ima-spec=v1 > arguments now. > Ack! > > #source: indcall-1.s > > #name: BPF indirect call 1, normal syntax > > > > @@ -10,11 +10,11 @@ Disassembly of section \.text: > > 0000000000000000 <main>: > > 0: b7 00 00 00 01 00 00 00 mov %r0,1 > > 8: b7 01 00 00 01 00 00 00 mov %r1,1 > > - 10: b7 02 00 00 02 00 00 00 mov %r2,2 > > - 18: 18 06 00 00 38 00 00 00 lddw %r6,56 > > + 10: b7 03 00 00 03 00 00 00 mov %r3,3 > > + 18: 18 02 00 00 38 00 00 00 lddw %r2,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 02 00 00 00 00 00 00 call %r2 > > 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/indcall-1.s > > index 5d49e41040a..7fbeeeb9a38 100644 > > --- a/gas/testsuite/gas/bpf/indcall-1.s > > +++ b/gas/testsuite/gas/bpf/indcall-1.s > > @@ -4,9 +4,9 @@ > > main: > > mov %r0, 1 > > mov %r1, 1 > > - mov %r2, 2 > > - lddw %r6, bar > > - call %r6 > > + mov %r3, 3 > > + lddw %r2, bar > > + call %r2 > > exit > > > > bar: > > diff --git a/include/opcode/bpf.h b/include/opcode/bpf.h > > index df1e3bd0918..97e25053175 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_CALLX, BPF_INSN_CALL, BPF_INSN_EXIT, > > I don't think it is a good idea to rename BPF_INSN_CALLR to > BPF_INSN_CALLX. > > At the moment the assembler uses the `callx' mnemonic for it and GCC > uses the same name because that is what clang does, but if/when the > corresponding instruction gets incorporated into BPF, I indend to push > for a better name, certainly not something as unmemorable and > indescriptive as `callx'. I will drop this change, as well! I know that Dave is working on this naming now on the standardization mailing list. I look forward to watching that argument! (I am definitely going to get butter with the popcorn!). I will have a v4 later today -- the day job calls for the next few hours! Sorry! Will > > > /* 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, > > diff --git a/opcodes/bpf-opc.c b/opcodes/bpf-opc.c > > index 19e096501a2..23473fc0cd9 100644 > > --- a/opcodes/bpf-opc.c > > +++ b/opcodes/bpf-opc.c > > @@ -272,8 +272,8 @@ 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_XBPF, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_CALL|BPF_SRC_X}, > > + {BPF_INSN_CALLX, "call%W%dr", "callx%w%dr", > > + 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", ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/1] objdump, as: Add callx support for BPF CPU v1 2024-02-14 16:36 ` Will Hawkins @ 2024-02-14 22:13 ` Will Hawkins 0 siblings, 0 replies; 5+ messages in thread From: Will Hawkins @ 2024-02-14 22:13 UTC (permalink / raw) To: Jose E. Marchesi; +Cc: binutils, yonghong.song On Wed, Feb 14, 2024 at 11:36 AM Will Hawkins <hawkinsw@obs.cr> wrote: > > On Wed, Feb 14, 2024 at 11:30 AM Jose E. Marchesi > <jose.marchesi@oracle.com> wrote: > > > > > > Hi Will. > > Thanks for the patch. Please see some comments below. > > > > > Add support for (dis)assembling the callx instruction back to CPU v1. > > > > > > gas/ChangeLog: > > > > > > * testsuite/gas/bpf/indcall-1-pseudoc.d: Refactor tests ... > > > * testsuite/gas/bpf/indcall-1-pseudoc.s: ... to visually match ... > > > * testsuite/gas/bpf/indcall-1.d: ... equivalent test in ... > > > * testsuite/gas/bpf/indcall-1.s: ... clang/llvm. > > > > > > include/ChangeLog: > > > > > > * opcode/bpf.h (enum bpf_insn_id): BPF_INSN_CALLR to BPF_INSN_CALLX > > > * (for consistency) and add it to the v1 ISA variant. > > > > > > opcodes/ChangeLog: > > > > > > * bpf-opc.c: Use BPF_INSN_CALLX instead of BPF_INSN_CALLR. > > > > > > Signed-off-by: Will Hawkins <hawkinsw@obs.cr> > > > --- > > > gas/testsuite/gas/bpf/indcall-1-pseudoc.d | 8 ++++---- > > > gas/testsuite/gas/bpf/indcall-1-pseudoc.s | 6 +++--- > > > gas/testsuite/gas/bpf/indcall-1.d | 10 +++++----- > > > gas/testsuite/gas/bpf/indcall-1.s | 6 +++--- > > > include/opcode/bpf.h | 2 +- > > > opcodes/bpf-opc.c | 4 ++-- > > > 6 files changed, 18 insertions(+), 18 deletions(-) > > > > > > diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d b/gas/testsuite/gas/bpf/indcall-1-pseudoc.d > > > index 7a95bad8e65..12f9d6a9d49 100644 > > > --- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.d > > > +++ b/gas/testsuite/gas/bpf/indcall-1-pseudoc.d > > > @@ -1,4 +1,4 @@ > > > -#as: -EL -mdialect=pseudoc -misa-spec=xbpf > > > +#as: -EL -mdialect=pseudoc -misa-spec=v1 > > > > I think this test can now omit -misa-spec entirely. > > > > > #objdump: -M xbpf,pseudoc,dec -dr > > > #source: indcall-1-pseudoc.s > > > #name: BPF indirect call 1, pseudoc syntax > > > @@ -10,11 +10,11 @@ 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 > > > + 10: b7 03 00 00 03 00 00 00 r3=3 > > > + 18: 18 02 00 00 38 00 00 00 r2=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 > > > + 28: 8d 02 00 00 00 00 00 00 callx r2 > > > 30: 95 00 00 00 00 00 00 00 exit > > > > I don't see any reason for changing the test bodies like this. If it is > > about matching some llvm test, IMO consistency between binutils and llvm > > testsuites is not a general goal we can (or should) aim to. > > > > Same applies to the other tests changed in the thunk below. > > As I said, I am happy to do whatever you like! I did it just so that I > could visually check that the two matched and then I left it. In v4 I > will revert this particular piece! > > > > > > 0000000000000038 <bar>: > > > diff --git a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s b/gas/testsuite/gas/bpf/indcall-1-pseudoc.s > > > index 2042697f15b..5639e288869 100644 > > > --- a/gas/testsuite/gas/bpf/indcall-1-pseudoc.s > > > +++ b/gas/testsuite/gas/bpf/indcall-1-pseudoc.s > > > @@ -4,9 +4,9 @@ > > > main: > > > r0 = 1 > > > r1 = 1 > > > - r2 = 2 > > > - r6 = bar ll > > > - callx r6 > > > + r3 = 3 > > > + r2 = bar ll > > > + callx r2 > > > exit > > > bar: > > > r0 = 0 > > > diff --git a/gas/testsuite/gas/bpf/indcall-1.d b/gas/testsuite/gas/bpf/indcall-1.d > > > index 51103bba2a1..1a2c36999b1 100644 > > > --- a/gas/testsuite/gas/bpf/indcall-1.d > > > +++ b/gas/testsuite/gas/bpf/indcall-1.d > > > @@ -1,5 +1,5 @@ > > > -#as: -EL -misa-spec=xbpf > > > -#objdump: -dr -M xbpf,dec > > > +#as: -EL -misa-spec=v1 > > > +#objdump: -dr -M v1,dec > > > > Likewise, I think this test can just omit the -M v1 and -ima-spec=v1 > > arguments now. > > > > Ack! > > > > #source: indcall-1.s > > > #name: BPF indirect call 1, normal syntax > > > > > > @@ -10,11 +10,11 @@ Disassembly of section \.text: > > > 0000000000000000 <main>: > > > 0: b7 00 00 00 01 00 00 00 mov %r0,1 > > > 8: b7 01 00 00 01 00 00 00 mov %r1,1 > > > - 10: b7 02 00 00 02 00 00 00 mov %r2,2 > > > - 18: 18 06 00 00 38 00 00 00 lddw %r6,56 > > > + 10: b7 03 00 00 03 00 00 00 mov %r3,3 > > > + 18: 18 02 00 00 38 00 00 00 lddw %r2,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 02 00 00 00 00 00 00 call %r2 > > > 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/indcall-1.s > > > index 5d49e41040a..7fbeeeb9a38 100644 > > > --- a/gas/testsuite/gas/bpf/indcall-1.s > > > +++ b/gas/testsuite/gas/bpf/indcall-1.s > > > @@ -4,9 +4,9 @@ > > > main: > > > mov %r0, 1 > > > mov %r1, 1 > > > - mov %r2, 2 > > > - lddw %r6, bar > > > - call %r6 > > > + mov %r3, 3 > > > + lddw %r2, bar > > > + call %r2 > > > exit > > > > > > bar: > > > diff --git a/include/opcode/bpf.h b/include/opcode/bpf.h > > > index df1e3bd0918..97e25053175 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_CALLX, BPF_INSN_CALL, BPF_INSN_EXIT, > > > > I don't think it is a good idea to rename BPF_INSN_CALLR to > > BPF_INSN_CALLX. > > > > At the moment the assembler uses the `callx' mnemonic for it and GCC > > uses the same name because that is what clang does, but if/when the > > corresponding instruction gets incorporated into BPF, I indend to push > > for a better name, certainly not something as unmemorable and > > indescriptive as `callx'. > > I will drop this change, as well! I know that Dave is working on this > naming now on the standardization mailing list. I look forward to > watching that argument! (I am definitely going to get butter with the > popcorn!). > > I will have a v4 later today -- the day job calls for the next few hours! Sorry! > Will Sent! Sorry for the delay! Will > > > > > > > /* 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, > > > diff --git a/opcodes/bpf-opc.c b/opcodes/bpf-opc.c > > > index 19e096501a2..23473fc0cd9 100644 > > > --- a/opcodes/bpf-opc.c > > > +++ b/opcodes/bpf-opc.c > > > @@ -272,8 +272,8 @@ 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_XBPF, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_CALL|BPF_SRC_X}, > > > + {BPF_INSN_CALLX, "call%W%dr", "callx%w%dr", > > > + 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", ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-02-14 22:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-14 16:02 [PATCH v3 0/1] Move callx (nee callr) to v1 ISA variant Will Hawkins 2024-02-14 16:03 ` [PATCH v3 1/1] objdump, as: Add callx support for BPF CPU v1 Will Hawkins 2024-02-14 16:30 ` Jose E. Marchesi 2024-02-14 16:36 ` Will Hawkins 2024-02-14 22:13 ` Will Hawkins
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).