Re: [PATCH] PR 34558: bpf: don't mis-assemble `gotol' with signed offset

Hi Vineet, thanks for the patch.

>   `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.
>
> The fix is to 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 another name character, 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'.

There are no "mnemonics" in pseudo-C syntax (not my idea).

BPF should remove the syntactic ambiguity (one of many) from the syntax
by mandating a whitespace after "goto" and "gotol" in these
instructions.  Then you can use the existing tag %W in the templates.

>
> 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
> 	character.
> 	* 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                           | 23 ++++++++++++++++++-
>  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, 48 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..9b207c2e2389 100644
> --- a/gas/config/tc-bpf.c
> +++ b/gas/config/tc-bpf.c
> @@ -1506,7 +1506,28 @@ 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 another name
> +                     character, 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).  */
> +                  if (!is_whitespace (*s)
> +                      && p > template
> +                      && is_part_of_name (*(p - 1))
> +                      && is_part_of_name (*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 f19322122527..efd1eab08f72 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