[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: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
----------------
Are the comments accidentally switched?
I thought:
debug -> atos works
no debug -> dladdr fallback



================
Comment at: compiler-rt/test/sanitizer_common/TestCases/Darwin/symbolizer-function-offset-atos.cpp:12
+// RUN: FileCheck -input-file=%t-no-debug.output %s
+// RUN: FileCheck -check-prefix=BADADDR -input-file=%t-no-debug.output %s
+
----------------
Same as below.  Refined positive regex would allow for removal of `BADADDR`, second FileCheck. invocation, temporary file, etc.


================
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:
> > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69549