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

[Dan Liew via Phabricator via llvm-commits]

delcypher marked 2 inline comments as done.
delcypher added inline comments.


================
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;
----------------
yln wrote:
> 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?
> 
The initial value is `kUnknown`. See `sanitizer_symbolizer.cpp`.

```lang=c++
AddressInfo::AddressInfo() {
  internal_memset(this, 0, sizeof(AddressInfo));
  function_offset = kUnknown;
}
```

I don't know a way of writing a test case such that `dladdr()` fails. Perhaps stripping the binary would do it?


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

No it's not. We assign `AddressInfo::kUnknown` which is the default value of the function offset.

> Also: kUnknown = ~(uptr)0 so I would expect the "invalid address case" to print 0xFFFF

The stacktrace printer prints `0x0` for `kUnknown`. See `lib/sanitizer_common/sanitizer_stacktrace_printer.cpp`.

> But our tests only check for 0x0

Given my point above, this isn't relevant.

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

We have a test case for falling back to dlsym (when debug information is missing) but we don't have a test case for when dlsym fails.


================
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
----------------
yln wrote:
> 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.
This might work. I'll give it a try.


================
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
----------------
yln wrote:
> Is `verbosity=2` required?
This is for knowing which symbolizer is being used (e.g. `CHECK: Using atos found at`)


================
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>
----------------
yln wrote:
> Could this be replaced with `--implicit-check-not='function_offset:0x0` ?
> Maybe also add `--implicit-check-not='function_offset:0xF`.
> Also in above test.
Maybe. I've never used this feature. I had to do the check as two separate FileCheck invocations because otherwise FileCheck failed to detect `function_offset:0x0`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69549