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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: compiler-rt/lib/hwasan/CMakeLists.txt:24
   hwasan_dynamic_shadow.h
+  hwasan_tag_mismatch_aarch64.S
   hwasan_flags.h
----------------
Sort alphabetically.


================
Comment at: compiler-rt/lib/hwasan/hwasan_linux.cc:417
 
-extern "C" SANITIZER_INTERFACE_ATTRIBUTE void __hwasan_tag_mismatch(
-    uptr addr, uptr access_info) {
+extern "C" void HwasanTagMismatch(uptr addr, uptr access_info,
+                                  uptr *registers_frame) {
----------------
The function name should be moved into the implementation namespace, i.e. prefix with `__`.


================
Comment at: compiler-rt/lib/hwasan/hwasan_report.cc:440
 
+// See the frame breakdown defined in __hwasan_fail_entry_point (from
+// hwasan_fail_entry_point_aarch64.S).
----------------
This should match the name of the function.


================
Comment at: compiler-rt/lib/hwasan/hwasan_report.cc:452
+    Printf("    x%d%s%016llx", i, (i < 10) ? kDoubleSpace : kSingleSpace,
+           frame[28 - i]);
+    Printf("  x%d%s%016llx", i + 1, (i + 1 < 10) ? kDoubleSpace : kSingleSpace,
----------------
The math here seems a little hard to reason about. Would it be better to store the registers in numerical order so that e.g. x0 is in frame[0], x1 is in frame[1] and so on?


================
Comment at: compiler-rt/lib/hwasan/hwasan_tag_mismatch_aarch64.S:94
+// We do not need executable stack.
+NO_EXEC_STACK_DIRECTIVE
+
----------------
I think this needs to be outside the `#if..#endif` to prevent the stack from becoming executable on non-aarch64 machines.


================
Comment at: compiler-rt/test/hwasan/TestCases/register-dump-read.c:28
+  // CHECK: Registers where the failure occurred
+  // CHECK-NEXT: x0{{[ ]+[0-9a-f]{16}[ ]}}x1{{[ ]+[0-9a-f]{16}[ ]}}x2{{[ ]+[0-9a-f]{16}[ ]}}x3{{[ ]+[0-9a-f]{16}$}}
+  // CHECK-NEXT: x4{{[ ]+[0-9a-f]{16}[ ]}}x5{{[ ]+[0-9a-f]{16}[ ]}}x6{{[ ]+[0-9a-f]{16}[ ]}}x7{{[ ]+[0-9a-f]{16}$}}
----------------
Is there some way to test that the correct register values are being printed? Maybe one way would be compile this translation unit with a few `-ffixed-x*` arguments (to prevent them from being clobbered by codegen; this is now possible with r353957), insert inline asm in this function that sets the fixed registers to specific values and test for those values here.


================
Comment at: llvm/utils/gn/secondary/compiler-rt/lib/hwasan/BUILD.gn:45
     "hwasan_dynamic_shadow.h",
+    "hwasan_tag_mismatch_aarch64.S",
     "hwasan_flags.h",
----------------
Sort alphabetically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58857