[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/test/sanitizer_common/TestCases/Darwin/symbolizer-function-offset-atos.cpp:8
+
+// The no-debug info case should cause atos to report the function offset so this should test that path.
+// RUN: rm -rf %t-with-debug.dSYM
----------------
yln wrote:
> Are the comments accidentally switched?
> I thought:
> debug -> atos works
> no debug -> dladdr fallback
> 
No the comments are the right way round.

It there's debug info atos will report a source location without a function offset. In that case we'll fallback to dladdr.
If there's no debug info atos will not report a source location and it will report the function offset. In that case we don't fallback to dladdr.


================
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:
> delcypher wrote:
> > delcypher wrote:
> > > 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`.
> > I tried using `--implicit-check-not=`. It doesn't work for this use case. This flag adds implicit CHECK-NOT in between patterns, it won't apply to lines that match the CHECK patterns. The existing patterns are the same lines we want to apply CHECK-NOT on but that doesn't work here. Initially I thought it was working but that's because the `CHECK-NOT` was firing on the `__sanitizer_print_stack_trace` on the top of the stacktrace.
> > 
> Ah, got it, this is the explicit negative case.  Could we make the regex for the positive case smarter so it doesn't match `0x0`?  This would remove the need for the separate `BADADDR` FileCheck run and the temporary output file, etc.
Regexes are good for explicitly stating what you want to match. They are (IMHO) terrible if you want to explicitly not match a pattern unless the regex language variant has explicit support for it (e.g. `(?!...)` in Python's re module). LLVM's regex implementation is "POSIX extended regular expression (ERE)" which I don't think has `(?!...)`. We could probably come up with a complicated regex that does what we want but it would be difficult to understand.

So let's just go with what we have here. The approach here is **a lot easier to understand** than a complicated regex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69549