[PATCH v2 2/2] PR 34558: bpf: don't mis-assemble `gotol' with signed offset
Jose E. Marchesi
jemarch@gnu.org
Fri Sep 4 12:00:58 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.
It would be nice to have test cases for these.
> 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.
What about introducing a new mark (such as %t, for tab) to explicitly
match the end of a "mnemonic" in the template:
"gotol%t%w%d32"
"goto%t%w%d32"
Where %t basically implements the flexible spacing rules, emitting an
error if the next character in the stream doesn't conform to these
rules. This way %w will stay to simply be zero or more whitespaces
regardless of the context where it appears. Note this still requires
the usual ordering of the entries by mnemonic.
>
> 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