[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/test/sanitizer_common/TestCases/Darwin/symbolizer-function-offset-atos.cpp:3
+
+// The debug info case should cause atos to not report the function offset so this should test the dladdr() fallback path.
+// RUN: %env_tool_opts=verbosity=2,stack_trace_format='"function_name:%f function_offset:%q"' %run %t-with-debug > %t-with-debug.output 2>&1
----------------
> // With debug info atos reports the source location, but no function offset.  We fallback to dladdr() to retrieve the function offset.


================
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
----------------
delcypher wrote:
> 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.
Got it.  Suggesting comment:
> // Without debug info atos already reports the function offset.  No fallback 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>
----------------
delcypher wrote:
> delcypher wrote:
> > 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.
> @yln Just be clear. If you have a suggested regex then I'll consider it but I don't see how to write a comprehensible regex that does what we want it to do
How about any number of 0s, followed by at least one non-0?
`0*[1-9a-f]`
https://regexr.com/4ou65


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69549