[darwin] use new crash reporter api

[aralisza]

Summary

Add support for the new crash reporter api if the headers are available. Falls back to the old API if they are not available. This change was based on /llvm/lib/Support/PrettyStackTrace.cpp

There is a lit for this behavior here: https://reviews.llvm.org/D96737 but is not included in this diff because it is potentially flaky.

rdar://69767688

[aralisza]

rebase

[delcypher]

It's probably worth mentioning in the description and commit message

• That the code here is based on llvm/lib/Support/PrettyStackTrace.cpp
• A radar number at the end of the message.

Other than that LGTM.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

798

Hmm I just had a thought. I'm not actually sure what happens when you use extern "C" inside a C++ namespace. I would hope the declared symbols would not be mangled and would use the unmangled C name. Can you check with nm if the gCRAnnotations symbol is mangled?

If we have doubts about this perhaps we should move this declaration outside of the __sanitizer namespace?

[aralisza]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

798

Just to check if I'm doing this right, I am running nm projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.osx.dir/sanitizer_mac.cpp.o

In the output, I see 0000000000006d48 S _gCRAnnotations, this would mean it is *not* mangled right?

If I try the old crash reporter api, I see that the crash reporter info. shows up as 0000000000006158 d __ZN11__sanitizerL22__crashreporter_info__E. This *is* mangled?

[aralisza]

fix bug define

[delcypher]

LGTM

[delcypher]

Once we've done the necessary testing to confirm that the info shows up in ASan, TSan, and UBSan then I think this is good to go.

[aralisza]

remove test from stack

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

798

Yep _gCRAnnotations is unmangled and __ZN11__sanitizerL22__crashreporter_info__E is mangled.

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

47

Can you extend the comment to mention which macOS SDK version introduced this?

792

My understand is that gCRAnnotations and ___crashreporter_info__ are "magic names" that we rely on, but the above are not.

It took me a while to understand that the above are just implementation details because their names look magic as well. ;)
Maybe make these static and remove underscores to communicate this better. (I see that these names were already there before the change, but I think it would be a good opportunity to clean things up.)

813

Should we always emit both so our runtimes work on new and old OSes? Or are the approaches mutually exclusive?

825

Tiny, pedantic nit: fill contents of buffer before announcing it seems like the better logical order (although it doesn't matter).

[aralisza]

reordering append/set buffer

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

47

how would I look this up?

792

I had made these _not_ static because of dan's concerns here: https://reviews.llvm.org/D96737?id=323843#inline-907262

Which globals should I make static?

813

Not sure if this has to be mutually exclusive, but I copied this logic from PrettyStackTrace.cpp https://github.com/llvm/llvm-project/blob/0164d546d2691c439fc06c8fff126224276c2d02/llvm/lib/Support/PrettyStackTrace.cpp#L111

I think it makes sense for it to be mutually exclusive since if the crash reporter header exists, it should be the one that is used, not the old one, right?

[aralisza]

remove underscores in global names

[aralisza]

static

[aralisza]

rebase main

[yln]

I think the current diff does not contain all parts of this change.

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

789–830

Drop inline, mark static as Dan requested.

790

792

C++ is confusing! :(

static at file scope means: module private, this global/function can only be used within this translation unit.
static variable within a function is a "function private" variable with an extended (global) life-time (as compared to a variable living on the stack) that will be initialized the first time "execution passes through".

To accomplish initialization exactly once the first time "execution passes through" in a multithreaded environment:

func() {
  // synchronization via ___cxa_guard_acquire()
  static x = bar();
  // synchronization via ___cxa_guard_release()
}

These synchronization functions live in libc++ and in sanitizer runtimes we wan't to avoid a dependency on it so we can't use the "function private static" C++ language feature.

See the blog post Dan linked: https://iamroman.org/blog/2017/04/cpp11-static-init/

As a general rule it's good to mark implementation details (globals and functions) as static. It make sure those are not exported/cannot be used from outside the translation unit and enables the compiler to be more aggressive when optimizing because it can "see all uses".

813

There is an important distinction here:

• what's available in the SDK at build time
• what's available on the OS when we run

One of our goals is to make the sanitizer runtimes work even on older OSes (with a limit on how far we go back and how much effort it is worth).

On never OSes we want to use the new way, but our runtime should still work on older OSes, which makes me believe that we should emit both mechanisms, if they don't interfere with each other.
Or maybe the old mechanism is only relevant on OS versions so old that we can drop it altogether?

@delcypher : can you clarify?

[aralisza]

also make mutex static

[aralisza]

remove inline and make static

[yln]

The diff still looks incomplete.

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

813

Dan clarified offline, I understand now.

compiler-rt/lib/sanitizer_common/sanitizer_mac.h

66–67

Do we still need this in the header? Can we remove it?

[aralisza]

squash and remove uneeded function from header

[yln]

LGTM, thanks! :)

[aralisza]

remove not useful comments

[aralisza]

squash

[delcypher]

LGTM

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

813

This isn't actually an API. CRSetCrashLogMessage() is a macro that updates a field in the struct crashreporter_annotations_t gCRAnnotations global.

So in both cases we're not actually calling into any external code which means the actions this code takes will always be available at runtime. It doesn't necessarily mean it will actually work though. That depends on when CrashReporter started inspecting the gCRAnnotations global.

compiler-rt/lib/sanitizer_common/sanitizer_mac.h

 MacosVersion GetMacosAlignedVersion();
 DarwinKernelVersion GetDarwinKernelVersion();
 
 char **GetEnviron();
 
 void RestrictMemoryToMaxAddress(uptr max_address);
 
 }  // namespace __sanitizer
 
-extern "C" {
-static char __crashreporter_info_buff__[__sanitizer::kErrorMessageBufferSize] =
-  {};
-static const char *__crashreporter_info__ __attribute__((__used__)) =
-  &__crashreporter_info_buff__[0];
-asm(".desc ___crashreporter_info__, 0x10");
-} // extern "C"
-
-namespace __sanitizer {
-static BlockingMutex crashreporter_info_mutex(LINKER_INITIALIZED);
-
-inline void CRAppendCrashLogMessage(const char *msg) {
-  BlockingMutexLock l(&crashreporter_info_mutex);
-  internal_strlcat(__crashreporter_info_buff__, msg,
-                   sizeof(__crashreporter_info_buff__)); }
-}  // namespace __sanitizer
-
 #endif  // SANITIZER_MAC
 #endif  // SANITIZER_MAC_H

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

 
 #if defined(__has_include) && __has_include(<os/trace.h>)
 #define SANITIZER_OS_TRACE 1
 #include <os/trace.h>
 #else
 #define SANITIZER_OS_TRACE 0
 #endif
 
+// import new crash reporting api
+#if defined(__has_include) && __has_include(<CrashReporterClient.h>)
+#define HAVE_CRASHREPORTERCLIENT_H 1
+#include <CrashReporterClient.h>
+#else
+#define HAVE_CRASHREPORTERCLIENT_H 0
+#endif
+
 #if !SANITIZER_IOS
 #include <crt_externs.h>  // for _NSGetArgv and _NSGetEnviron
 #else
 extern "C" {
   extern char ***_NSGetArgv(void);
 }
 #endif
 
#if !SANITIZER_GO
   syslog_lock.CheckLocked();
   if (GetMacosAlignedVersion() >= MacosVersion(10, 12)) {
     os_log_error(OS_LOG_DEFAULT, "%{public}s", s);
   } else {
     asl_log(nullptr, nullptr, ASL_LEVEL_ERR, "%s", s);
   }
 #endif
 }
 
+// buffer to store crash report application information
+static char crashreporter_info_buff[__sanitizer::kErrorMessageBufferSize] = {};
+static BlockingMutex crashreporter_info_mutex(LINKER_INITIALIZED);
+
+extern "C" {
+// Integrate with crash reporter libraries.
+#if HAVE_CRASHREPORTERCLIENT_H
+CRASH_REPORTER_CLIENT_HIDDEN
+struct crashreporter_annotations_t gCRAnnotations
+    __attribute__((section("__DATA," CRASHREPORTER_ANNOTATIONS_SECTION))) = {
+        CRASHREPORTER_ANNOTATIONS_VERSION,
+        0,
+        0,
+        0,
+        0,
+        0,
+        0,
+#if CRASHREPORTER_ANNOTATIONS_VERSION > 4
+        0,
+#endif
+};
+
+#else
+// fall back to old crashreporter api
+static const char *__crashreporter_info__ __attribute__((__used__)) =
+    &crashreporter_info_buff[0];
+asm(".desc ___crashreporter_info__, 0x10");
+#endif
+
+}  // extern "C"
+
+static void CRAppendCrashLogMessage(const char *msg) {
+  BlockingMutexLock l(&crashreporter_info_mutex);
+  internal_strlcat(crashreporter_info_buff, msg,
+                   sizeof(crashreporter_info_buff));
+#if HAVE_CRASHREPORTERCLIENT_H
+  (void)CRSetCrashLogMessage(crashreporter_info_buff);
+#endif
+}
+
 void LogMessageOnPrintf(const char *str) {
   // Log all printf output to CrashLog.
   if (common_flags()->abort_on_error)
     CRAppendCrashLogMessage(str);
 }
 
 void LogFullErrorReport(const char *buffer) {
 #if !SANITIZER_GO
   // Log with os_trace. This will make it into the crash log.