[PATCH] D69549: [Symbolizers] On Darwin compute function offset when possible.

[Julian Lettner via Phabricator via llvm-commits]

yln added a comment.

I have a few small comments.  Looks good overall!



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:32
   Dl_info info;
   int result = dladdr((const void *)addr, &info);
   if (!result) return false;
----------------
If we fail to dladdr, `stack->info.function_offset` will not be assigned.
What is the initial value? garbage? `0x0`, or `kUnknown = ~(uptr)0`?

What do we want here? I think `0x0` would be a good choice.
Would it be possible to construct a test case for this code path to pin down the behavior?



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:171
+      function_offset = addr - reinterpret_cast<uptr>(info.dli_saddr);
+    }
+  }
----------------
`stack->info.function_offset` is assigned unconditionally, which is inconsistent with `DlAddrSymbolizer::SymbolizePC`.

Also: `kUnknown = ~(uptr)0` so I would expect the "invalid address case" to print `0xFFFF`.
But our tests only check for `0x0`:
`// BADADDR-NOT: function_name:baz{{(\(\))?}} function_offset:0x0`

Is it possible to construct a test for the fallback not working? (Same as above.)



================
Comment at: compiler-rt/test/sanitizer_common/TestCases/Darwin/symbolizer-function-offset-atos.cpp:2
+// The no-debug case should cause atos to report the function offset so this should test that path.
+// RUN: rm -rf %t-no-debug.dSYM
+// RUN: %clangxx %s -g0 -O0 -o %t-no-debug
----------------
Another way to structure this test would be to 
1) compile only once (with `-g`) then
2) `RUN` with debug info
3) `rm *.dSYM` 
4) `RUN` same command, but now without debug info.

I think this would nicely communicate that without debug info---but with everything else being equal---offsets still work as expected.


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/Darwin/symbolizer-function-offset-atos.cpp:4
+// RUN: %clangxx %s -g0 -O0 -o %t-no-debug
+// RUN: %env_tool_opts=verbosity=2,stack_trace_format='"function_name:%f function_offset:%q"' %run %t-no-debug > %t-no-debug.output 2>&1
+// RUN: FileCheck -input-file=%t-no-debug.output %s
----------------
Is `verbosity=2` required?


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/Darwin/symbolizer-function-offset-dladdr.cpp:9
+// RUN: FileCheck -input-file=%t.output %s
+// RUN: FileCheck -check-prefix=BADADDR -input-file=%t.output %s
+#include <sanitizer/common_interface_defs.h>
----------------
Could this be replaced with `--implicit-check-not='function_offset:0x0` ?
Maybe also add `--implicit-check-not='function_offset:0xF`.
Also in above test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69549