Skip to content

Commit

Permalink
Fix handling of 64-bit SP updates in 2 space-separated chunks.
Browse files Browse the repository at this point in the history
I heard recently that at least one CPU's Tarmac generator will emit
AArch64 core register updates in the form 'R X0 01234567 89abcdef',
with the 64-bit register contents divided into two tokens.

I hadn't seen that before for core registers, but I had for vector
registers - we already have a test case that updates Q0 in four 32-bit
chunks. So this worked already for X0-X30. But it failed for SP,
because there's a different code path for the special-case handling in
which we wait to see whether we see 32 or 64 bits of data in order to
decide whether it's AArch32 SP (aka r13) or AArch64 XSP. An SP update
in that form was being misinterpreted as a 32-bit update to r13,
because that code path hadn't noticed that the following token
contained another 32 bits of data.

While fixing that, I had to make sure not to break an edge case of
CPSR updates (which also use that special code path, for a different
reason). I haven't added a regression test for that edge case, because
by great luck we already had one, on line 146 of parsertest.txt (as of
this commit), reading "R cpsr 20000000 __C_".
  • Loading branch information
statham-arm committed May 31, 2024
1 parent 3305c32 commit b906ecc
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 1 deletion.
11 changes: 10 additions & 1 deletion lib/parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,14 @@ class TarmacLineParserImpl {
//
// In all cases of this so far encountered, it's enough to read
// a single contiguous token of register contents, plus a
// second one if a ':' follows it.
// second one if a ':' or a space follows it.
//
// However, for CPSR, we're a bit stricter about a second token
// after a space, because some trace generators suffix a
// human-readable flags representation in a form like __C_ or
// NZ_V, and it would be a shame if __C_ were mistaken for a
// register value just because it contains valid hex digits and
// underscores only. So in this case we expect real hex.
if (!tok.isregvalue())
parse_error(tok, _("expected register contents"));
consume_register_contents(tok);
Expand All @@ -837,6 +844,8 @@ class TarmacLineParserImpl {
parse_error(tok, _("expected additional register "
"contents after ':'"));
consume_register_contents(tok);
} else if (tok.isregvalue() && !(is_cpsr && !tok.ishex())) {
consume_register_contents(tok);
}

if (is_sp) {
Expand Down
4 changes: 4 additions & 0 deletions tests/parsertest.ref
Original file line number Diff line number Diff line change
Expand Up @@ -278,3 +278,7 @@ Parse warning: unsupported system operation 'AT'
* MemoryEvent time=3981 read=true known=true addr=401c size=4 contents=3e000a73
--- Tarmac line: 8677000000 ps R cpsr 1220023cd
* RegisterEvent time=8677000000 reg=psr offset=0 bytes=22:00:23:cd
--- Tarmac line: R X0 01234567 89abcdef
* RegisterEvent time=8677000000 reg=x0 offset=0 bytes=01:23:45:67:89:ab:cd:ef
--- Tarmac line: R SP_EL2 fedcba98 76543210
* RegisterEvent time=8677000000 reg=xsp offset=0 bytes=fe:dc:ba:98:76:54:32:10
5 changes: 5 additions & 0 deletions tests/parsertest.txt
Original file line number Diff line number Diff line change
Expand Up @@ -274,3 +274,8 @@ R SP 0123456789abcdef
# sense, including some additional status bits stored elsewhere. So
# an update that apparently writes more than 32 bits to CPSR is possible.
8677000000 ps R cpsr 1220023cd

# Seen in the output of an unnamed CPU's hardware Tarmac module: the
# data for a 64-bit register update can be split by a space.
R X0 01234567 89abcdef
R SP_EL2 fedcba98 76543210

0 comments on commit b906ecc

Please sign in to comment.