[PATCH v2 2/2] PR 34558: bpf: don't mis-assemble `gotol' with signed offset
Vineet Gupta
vineet.gupta@linux.dev
Fri Sep 4 05:15:22 GMT 2026
On 9/3/26 9:00 PM, Jose E. Marchesi wrote:
> Hello Vineet.
>
> Thanks for the new version of the patch, and apologies for taking so
> much time to get back at this.
No worries.
>> Simply changing the JAR template to `goto%W%d16' does not work. The
>> pseudo-C dialect deliberately supports flexible spacing, so `goto+1' and
>> `goto1' must keep assembling,
> I see.
>
>> and gas removes the whitespace next to a sign in the operand field
>> before md_assemble sees the line, which breaks the extremely common
>> `if rX > N goto +M' outright.
> I don't understand that implication.
Yet another reason the %w approach won't work.
Anyways....
> In any case, wouldn't simply reordering the opcodes table like below fix
> the existing broken issues without having to add special casing to the
> generic template-based parser? Something like this passes the new test:
>
> diff --git a/opcodes/bpf-opc.c b/opcodes/bpf-opc.c
> index 4babf6e8649..df0b90ffb90 100644
> --- a/opcodes/bpf-opc.c
> +++ b/opcodes/bpf-opc.c
> @@ -247,6 +247,10 @@ const struct bpf_opcode bpf_opcodes[] =
> {BPF_INSN_STXDWI, "stdw%W[ %dr %o16 ] , %i32", "* ( u64 * ) ( %dr %o16 ) = %i32",
> BPF_V1, BPF_CODE, BPF_CLASS_ST|BPF_SIZE_DW|BPF_MODE_MEM},
>
> + /* 32-bit jump-always. */
> + {BPF_INSN_JAL, "jal%W%d32", "gotol%w%d32",
> + BPF_V4, BPF_CODE, BPF_CLASS_JMP32|BPF_CODE_JA|BPF_SRC_K},
> +
> /* Compare-and-jump instructions (reg OP reg). */
> {BPF_INSN_JAR, "ja%W%d16", "goto%w%d16",
> BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_JA|BPF_SRC_K},
> @@ -303,10 +307,6 @@ const struct bpf_opcode bpf_opcodes[] =
> {BPF_INSN_JNEI, "jne%W%dr , %i32 , %d16", "if%w%dr != %i32%wgoto%w%d16",
> BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_JNE|BPF_SRC_K},
>
> - /* 32-bit jump-always. */
> - {BPF_INSN_JAL, "jal%W%d32", "gotol%w%d32",
> - BPF_V4, BPF_CODE, BPF_CLASS_JMP32|BPF_CODE_JA|BPF_SRC_K},
> -
> /* 32-bit compare-and-jump instructions (reg OP reg). */
> {BPF_INSN_JEQ32R, "jeq32%W%dr , %sr , %d16", "if%w%dw == %sw%wgoto%w%d16",
> BPF_V3, BPF_CODE, BPF_CLASS_JMP32|BPF_CODE_JEQ|BPF_SRC_X},
I gave this a try and indeed it fixes the issue at hand, but
mis-compiles two other cases
1. For -misa-spec=v1 build gotol +1 assembles to goto 0 + 'l' because
JAL is not available and JAR wins regardless of the order (md_assemble
skips the opcodes not covered by the isa-spec).
2. conditional jumps such as if%w%dr > %sr%wgoto%w%d16 embeds the same
goto%w prefix, so if r1 > r2 gotol +1 leads to same issue.
I understand the need to not add special casings to the parser so I'm
open to trying any other solutions to fix this. I have an orthogonal
patch which improves the diagnostics for general gotol related miscompiles.
Thx,
-Vineet
>> Instead reject a template whose literal text stops in the middle of a
>> name: if the character last matched from the template is part of a name
>> and the input continues with a name beginner, then the template has only
>> matched a prefix of a longer mnemonic and does not apply. Other templates
>> are then given a chance to match the whole mnemonic. This also covers the
>> compound conditional jumps, whose templates embed `goto%w%d16'. Testing
>> is_name_beginner rather than is_part_of_name keeps `goto1' working, a
>> digit continuing an operand rather than a mnemonic.
>>
>> Existing coverage exercised `gotol' only with a label operand, which is
>> why this went unnoticed.
>>
>> PR gas/34558
>>
>> gas/
>> * config/tc-bpf.c (md_assemble): Do not let a template match when
>> its literal text ends mid-name and the input continues with a name
>> beginner.
>> * testsuite/gas/bpf/jump-gotol-signed-pseudoc.s: New test.
>> * testsuite/gas/bpf/jump-gotol-signed-pseudoc.d: New test.
>> * testsuite/gas/bpf/bpf.exp: Run it.
>> ---
>> gas/config/tc-bpf.c | 28 ++++++++++++++++++-
>> gas/testsuite/gas/bpf/bpf.exp | 1 +
>> .../gas/bpf/jump-gotol-signed-pseudoc.d | 15 ++++++++++
>> .../gas/bpf/jump-gotol-signed-pseudoc.s | 10 +++++++
>> 4 files changed, 53 insertions(+), 1 deletion(-)
>> create mode 100644 gas/testsuite/gas/bpf/jump-gotol-signed-pseudoc.d
>> create mode 100644 gas/testsuite/gas/bpf/jump-gotol-signed-pseudoc.s
>>
>> diff --git a/gas/config/tc-bpf.c b/gas/config/tc-bpf.c
>> index 8d48b128fb90..72e97327e542 100644
>> --- a/gas/config/tc-bpf.c
>> +++ b/gas/config/tc-bpf.c
>> @@ -1506,7 +1506,33 @@ md_assemble (char *str ATTRIBUTE_UNUSED)
>> }
>> else if (*(p + 1) == 'w')
>> {
>> - /* Expect zero or more spaces. */
>> + /* Expect zero or more spaces.
>> +
>> + If the template text matched so far ends in a name
>> + character and the input continues with a letter, then
>> + the template only matched a prefix of a longer mnemonic
>> + written in the input, and this template does not apply.
>> + Rejecting it here lets a subsequent template have a go
>> + at the whole mnemonic.
>> +
>> + Without this, `gotol +1' matches the `goto%w%d16'
>> + template, with `l +1' parsed as the branch offset
>> + expression, silently assembling to JA (opcode 0x05) plus
>> + a relocation against an undefined symbol `l' instead of
>> + to JAL (opcode 0x06).
>> +
>> + is_name_beginner rather than is_part_of_name: a digit
>> + continues an operand rather than a mnemonic, and the
>> + pseudo-C dialect accepts it with no separating space, as
>> + in `goto1'. */
>> + if (!is_whitespace (*s)
>> + && p > template
>> + && is_part_of_name (*(p - 1))
>> + && is_name_beginner (*s))
>> + {
>> + PARSE_ERROR ("expected white space, got '%s'", s);
>> + break;
>> + }
>> while (is_whitespace (*s))
>> s += 1;
>> p += 2;
>> diff --git a/gas/testsuite/gas/bpf/bpf.exp b/gas/testsuite/gas/bpf/bpf.exp
>> index 74b0461f0bcb..bcedb0c98f7b 100644
>> --- a/gas/testsuite/gas/bpf/bpf.exp
>> +++ b/gas/testsuite/gas/bpf/bpf.exp
>> @@ -38,6 +38,7 @@ if {[istarget bpf*-*-*]} {
>> run_dump_test jump-pseudoc
>> run_dump_test jump32
>> run_dump_test jump32-pseudoc
>> + run_dump_test jump-gotol-signed-pseudoc
>> run_dump_test atomic-v1
>> run_dump_test atomic
>> run_dump_test atomic-pseudoc
>> diff --git a/gas/testsuite/gas/bpf/jump-gotol-signed-pseudoc.d b/gas/testsuite/gas/bpf/jump-gotol-signed-pseudoc.d
>> new file mode 100644
>> index 000000000000..ee4146acd996
>> --- /dev/null
>> +++ b/gas/testsuite/gas/bpf/jump-gotol-signed-pseudoc.d
>> @@ -0,0 +1,15 @@
>> +#as: -EL -mdialect=pseudoc
>> +#objdump: -dr -M dec,pseudoc
>> +#source: jump-gotol-signed-pseudoc.s
>> +#name: eBPF gotol with signed offsets, pseudoc syntax
>> +
>> +.*: +file format .*bpf.*
>> +
>> +Disassembly of section .text:
>> +
>> +0+ <.text>:
>> + 0: 06 00 00 00 01 00 00 00 gotol 1
>> + 8: 06 00 00 00 ff ff ff ff gotol -1
>> + 10: 06 00 00 00 01 00 00 00 gotol 1
>> + 18: 06 00 00 00 00 00 00 00 gotol 0
>> + 20: 06 00 00 00 00 00 00 00 gotol 0
>> diff --git a/gas/testsuite/gas/bpf/jump-gotol-signed-pseudoc.s b/gas/testsuite/gas/bpf/jump-gotol-signed-pseudoc.s
>> new file mode 100644
>> index 000000000000..295afd6313d5
>> --- /dev/null
>> +++ b/gas/testsuite/gas/bpf/jump-gotol-signed-pseudoc.s
>> @@ -0,0 +1,10 @@
>> + # Signed branch offsets for the pseudo-C `gotol'. PR gas/34558:
>> + # these used to match the shorter `goto' template, with the
>> + # trailing `l' parsed as the start of the offset expression.
>> + .text
>> + gotol +1
>> + gotol -1
>> + gotol 1
>> + gotol 1f
>> +1:
>> + gotol 0
More information about the Binutils
mailing list