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

[Dan Liew via Phabricator via llvm-commits]

delcypher marked an inline comment as done.
delcypher added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp:171
+      function_offset = addr - reinterpret_cast<uptr>(info.dli_saddr);
+    }
+  }
----------------
delcypher wrote:
> 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.
> No it's not. We assign AddressInfo::kUnknown which is the default value of the function offset.

Just to elaborate. It looks inconsistent but it actually isn't given how `function_offset` is initialized. Do you have a suggestion to make it more consistent. Maybe in `DlAddrSymbolizer::SymbolizePC` we should always assign to `function_offset` too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69549