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

[Julian Lettner via Phabricator via llvm-commits]

yln 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;
----------------
delcypher wrote:
> 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?
Got it, thanks for explaining.  Let's ignore this test case for this change.


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

> Do you have a suggestion to make it more consistent. Maybe in DlAddrSymbolizer::SymbolizePC we should always assign to function_offset too?
I would flip it: only assign to it here, if we were able to compute something.

```
uptr start_address = AddressInfo::kUnknown;
// Attempt 1: fill via ParseCommandOutput

// Attempt 2/fallback: fill via dladdr
if (start_address == AddressInfo::kUnknown) {
  Dl_info info;
    int result = dladdr((const void *)addr, &info);
    if (result)
      start_address = reinterpret_cast<uptr>(info.dli_saddr);
}

// Only assign if we were able to compute start_address
if (start_address != AddressInfo::kUnknown) {
  CHECK(addr >= start_address);
  stack->info.function_offset = addr - start_address;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69549