[PATCH v2 2/2] PR 34558: bpf: don't mis-assemble `gotol' with signed offset
Vineet Gupta <[email protected]>
| Newsgroups | gmane.comp.gcc.bpf,gmane.comp.gnu.binutils |
|---|---|
| Message-ID | <[email protected]> |
`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
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
--
2.53.0-Meta