[compiler-rt] Remove -fomit-frame-pointer from sanitizers CFLAGS

[kubamracek]

Summary

Currently, ASan (and other sanitizers) is built with -fomit-frame-pointer, which doesn't go well in combination with stacktrace snapshots that ASan takes. Basically whenever the snapshot contains a no-fp frame, the caller of such a function is missing from the backtrace. I propose to remove this build settings.

[earthdok]

Stack traces are not supposed to contain sanitizer functions. Can you give an example of when this causes problems?

[glider]

Do you have an example where this really leads to an incorrect stack trace?
Sergey and I asked Kostya about this a couple of times and his point
was that -fomit-frame-pointer produces faster calls, but doesn't
worsen the reports, because the functions from ASan runtime library
are always on top of the stack.

[kubamracek]

I am having this problem with libdispatch functions. ASan has wrappers around functions such as dispatch_async, which do appear on the stack trace when using GCD. Unfortunately, when such a no-fp wrapper appears on the stack, it's not that wrapper frame that's missing (that would be probably fine), but it's the caller of it – i.e. the most important frame.

Also in my specific case it's actually another library that's recording stack traces (Xcode's GCD debugging library) and the issue is the same. The backtrace is taken when a dispatch_async is called, and that trace contains the no-fp wrapper function.

[kcc]

[sorry for delay, I was OOO]
I am strongly against any such change as it may lead to performance drop.
Sadly, we don't have a performance bot yet and we will simply not see a performance drop until it is too late.

For a particular interceptor or a set of interceptors the problem can be solved by adding something like "volatile uptr x = GET_CALLER_PC();"

[eugenis]

For a particular interceptor or a set of interceptors the problem can be solved by adding something like "volatile uptr x = GET_CALLER_PC();"

This sounds like a very good idea. GET_CALLER_PC, at least in the current implementation, enables frame pointer in the calling function. There are only a few interceptors that really need it.

[kcc]

Of course, hide it in a macro, e.g.

#define ENABLE_FRAME_POINTERS do {volatile uptr enable_fp = GET_CALLER_PC();"} while (0)

[dvyukov]

What is the performance impact on tsan?
Does it add any frame pointers to __tsan_read/write/func_entry/exit functions?
If yes, please add explicit -fomit-frame-pointer to tsan runtime.

[kubamracek]

Patch that enables frame pointer only for the selected GCD functions (block-based ones). In all the other, frame pointer is already enabled implicitly by using GET_STACK_TRACE_THREAD.

In the macro I didn't use GET_CALLER_PC as suggested, because it seems this one doesn't enable the frame pointer. Instead I used GET_CURRENT_FRAME, which does the trick.

[samsonov]

I'm fine with this change as long as you move ENABLE_FRAME_POINTERS to the only file that uses it (asan_mac.cc) - not sure it would be generally useful.

[kubamracek]

Moving the macro to the only file that uses it.

[earthdok]

This is bikeshedding, but I think "ENABLE_FRAME_POINTER" makes more sense since there's only one per function call.

[kubamracek]

Fix macro name.

[samsonov]

Landed in r212664, thanks!

lib/asan/asan_mac.cc

     work(); \
   }
 
+// forces the compiler to generate a frame pointer in the calling function
+#define ENABLE_FRAME_POINTER                                       \
+  do {                                                             \
+    volatile uptr enable_fp;                                       \
+    enable_fp = GET_CURRENT_FRAME();                               \
+  } while (0)
+
 INTERCEPTOR(void, dispatch_async,
             dispatch_queue_t dq, void(^work)(void)) {
+  ENABLE_FRAME_POINTER;
   GET_ASAN_BLOCK(work);
   REAL(dispatch_async)(dq, asan_block);
 }
 
 INTERCEPTOR(void, dispatch_group_async,
             dispatch_group_t dg, dispatch_queue_t dq, void(^work)(void)) {
+  ENABLE_FRAME_POINTER;
   GET_ASAN_BLOCK(work);
   REAL(dispatch_group_async)(dg, dq, asan_block);
 }
 
 INTERCEPTOR(void, dispatch_after,
             dispatch_time_t when, dispatch_queue_t queue, void(^work)(void)) {
+  ENABLE_FRAME_POINTER;
   GET_ASAN_BLOCK(work);
   REAL(dispatch_after)(when, queue, asan_block);
 }
 
 INTERCEPTOR(void, dispatch_source_set_cancel_handler,
             dispatch_source_t ds, void(^work)(void)) {
+  ENABLE_FRAME_POINTER;
   GET_ASAN_BLOCK(work);
   REAL(dispatch_source_set_cancel_handler)(ds, asan_block);
 }
 
 INTERCEPTOR(void, dispatch_source_set_event_handler,
             dispatch_source_t ds, void(^work)(void)) {
+  ENABLE_FRAME_POINTER;
   GET_ASAN_BLOCK(work);
   REAL(dispatch_source_set_event_handler)(ds, asan_block);
 }