[PATCH] D63469: hwasan: Teach the runtime to identify the local variable being accessed in UAR reports.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: compiler-rt/lib/hwasan/hwasan_report.cpp:255
+        uptr fp = (record >> 48) << 4;
+        uptr pc_mask = (1ULL << 48) - 1;
+        uptr pc = record & pc_mask;
----------------
Let's give names to the magic numbers - they are ABI details after all.


================
Comment at: compiler-rt/lib/hwasan/hwasan_report.cpp:266
+              continue;
+            uptr obj_offset =
+                (untagged_addr - fp - local.frame_offset) & 0xfffff;
----------------
This is a bit tricky. Leave a comment that underflow modulo 2^20 is expected when the address is expected when the address is below the local variable?


================
Comment at: compiler-rt/lib/hwasan/hwasan_report.cpp:281
+
+      if (found_local)
+        return;
----------------
I think it might be useful to iterate until the end of the buffer to find more candidates; if anything, that would avoid losing the information that's already there.

This is good enough for now. Some time in the future I think we should refactor this to collect all the candidates (including ex. potential buffer overflows with larger offsets, across multiple small allocation) and sort them by likelihood.



================
Comment at: compiler-rt/lib/hwasan/hwasan_report.cpp:282
+      if (found_local)
+        return;
+
----------------
This function is way too big. Do you mind splitting it?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer.h:87
+
+  bool has_frame_offset = false;
+  sptr frame_offset;
----------------
group the bools together to reduce the gaps


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_internal.h:18
 #include "sanitizer_file.h"
+#include "sanitizer_vector.h"
 
----------------
is this include necessary here?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc:424
+  const char *buf = FormatAndSendCommand(
+      "FRAME ", info->module, info->module_offset, info->module_arch);
+  if (buf) {
----------------
remove the space from "FRAME ", it can be added in the callee


================
Comment at: compiler-rt/test/hwasan/TestCases/stack-uar.c:36
   // NOSYM: Previously allocated frames:
-  // NOSYM-NEXT: sp: 0x{{.*}} #0 0x{{.*}} ({{.*}}/stack-uar.c.tmp+0x{{.*}}){{$}}
-  // NOSYM-NEXT: 16 CCC;
+  // NOSYM-NEXT: record_addr:0x{{.*}} record:0x{{.*}} ({{.*}}/stack-uar.c.tmp+0x{{.*}}){{$}}
+  // NOSYM-NEXT: record_addr:0x{{.*}} record:0x{{.*}} ({{.*}}/stack-uar.c.tmp+0x{{.*}}){{$}}
----------------
The advantage of printing decoded records, as before, is that they can be parsed with all the old symbolization scripts. Explicitly listing the last digits of FP is also sometimes useful to match against register values in the report.

Could we change the format to

[tag], fp:0x12345 #0 PC (module+offset)  

#0 is also for compatibility with symbolization scripts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63469