[v2,2/2] PR 34558: bpf: don't mis-assemble `gotol' with signed offset

  `gotol +1' is assembled as if it were `goto l +1'

`gotol +imm' is the canonical form in the BPF instruction set
documentation, and it is what LLVM emits and accepts, so it needs to work.

The reason is the asm templates for the two unconditional jumps:

  BPF_INSN_JAR   "goto%w%d16"
  BPF_INSN_JAL   "gotol%w%d32"

where %w matches zero or more whitespace characters.

JAR sorts before JAL in the opcode table so it tries to match first and
succeeds as `goto' and `l +1', with `l' being an undefined symbol.  This
ends up as JA (opcode 0x05, with the displacement in the 16-bit `off'
field) plus an R_BPF_GNU_64_16 relocation against an undefined symbol `l',
rather than as JAL (opcode 0x06, displacement in the 32-bit `imm' field).
No diagnostic is emitted.  The signed form -1 is similarly affected.  For
non-signed forms, `gotol 1' or `gotol 1f', the remainder does not parse as
a single expression, the JAR template fails, and JAL is reached and
matched correctly.  The normal dialect is not affected either, as
`ja%W%d16' requires at least one whitespace character after the mnemonic.

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, 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.

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
  

Hello Vineet.

Thanks for the new version of the patch, and apologies for taking so
much time to get back at this.

>   `gotol +1' is assembled as if it were `goto l +1'
>
> `gotol +imm' is the canonical form in the BPF instruction set
> documentation, and it is what LLVM emits and accepts, so it needs to work.
>
> The reason is the asm templates for the two unconditional jumps:
>
>   BPF_INSN_JAR   "goto%w%d16"
>   BPF_INSN_JAL   "gotol%w%d32"
>
> where %w matches zero or more whitespace characters.
>
> JAR sorts before JAL in the opcode table so it tries to match first and
> succeeds as `goto' and `l +1', with `l' being an undefined symbol.  This
> ends up as JA (opcode 0x05, with the displacement in the 16-bit `off'
> field) plus an R_BPF_GNU_64_16 relocation against an undefined symbol `l',
> rather than as JAL (opcode 0x06, displacement in the 32-bit `imm' field).
> No diagnostic is emitted.  The signed form -1 is similarly affected.  For
> non-signed forms, `gotol 1' or `gotol 1f', the remainder does not parse as
> a single expression, the JAR template fails, and JAL is reached and
> matched correctly.  The normal dialect is not affected either, as
> `ja%W%d16' requires at least one whitespace character after the mnemonic.
>
> 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.

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},


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

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
  

> 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
  

On 9/4/26 17:30, Jose E. Marchesi wrote:
>>> 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.

Good point, will do.

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

Sound like a nice idea indeed. Let me give that a spin.

Thx,
-Vineet