[PATCH] D58857: [HWASan] Save + print registers when tag mismatch occurs in AArch64.

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: compiler-rt/lib/hwasan/hwasan_tag_mismatch_aarch64.S:47
+.section .text
+.file "hwasan_fail_entry_point_aarch64.S"
+.global __hwasan_tag_mismatch
----------------
eugenis wrote:
> This does not match the actual file name. Intentional?
Nope, fixed.


================
Comment at: compiler-rt/lib/hwasan/hwasan_tag_mismatch_aarch64.S:62
+  CFI_OFFSET(w30, -8)
+  CFI_OFFSET(w29, -16)
+
----------------
eugenis wrote:
> This does not seem to match stack frame description in the comment above, or the IR test. It looks like x29 offset should be 16, and x30 offset should be 24.
My interpretation is the following layout is the current one:

```
// |              ...                |
// |              ...                |
// | Previous stack frames...        |
// +=================================+ <-- [CFA (x29 + 16)]
// | Return address (x30) for caller |
// | of __hwasan_check_*.            |
// +---------------------------------+ <-- [&x30 == CFA - 8]
// | Frame address (x29) for caller  |
// | of __hwasan_check_*             |
// +---------------------------------+ <-- [x29 / FP]
// | Saved x1, as __hwasan_check_*   |     [&x29 == CFA - 16]
// | clobbers it.                    |
// +---------------------------------+
// | Saved x0, likewise above.       |
// +---------------------------------+ <-- [x30 / SP]
```

Just stepped through GDB and it looks like the frame locates the saved registers (x29/x30) in the address they're correctly saved in. Do you see a different layout than what I'm seeing?


================
Comment at: compiler-rt/lib/hwasan/hwasan_tag_mismatch_aarch64.S:87
+
+  bl HwasanTagMismatch
+  CFI_ENDPROC
----------------
eugenis wrote:
> Could this use "b" instruction, so that the reporting code does not need to remove a stack frame?
We could, but unfortunately it still sets up a frame for __hwasan_tag_mismatch, but misattributes it to the __hwasan_check (i.e. we have two entries for __hwasan_check at the bottom of the stack trace). 

We could change it to a direct branch and avoid the extra frame by never adjusting the frame pointer, but this means we also lose any debug-ability in __hwasan_tag_mismatch. This would also nullify the comment above regarding CFA correctness.

If you'd like me to make this tradeoff, LMK.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58857/new/

https://reviews.llvm.org/D58857