Skip to content

Conversation

@thetruestblue
Copy link
Contributor

@thetruestblue thetruestblue commented Jan 22, 2025

An initializer for the Crash Reporter Annotatoins struct was added in version 5. For the simplicity of not needing to always update the struct in subsequent versions, this patchs checks for the initializer before attempting to redefine the struct on its own.

Note -- we have an existing test for this that is disabled by default, it is inherently flakey due to the nature of crash reporter. But we can run that when making crash reporter related changes.

rdar://136156203

…ter Annotations struct

An initializer for the Crash Reporter Annotatoins struct was added in version 5. For the simplicity of not needing to always update the struct in subsequent versions, this patchs checks for the initializer before attempting to redefine the struct on its own.

rdar://136156203
@llvmbot
Copy link
Member

llvmbot commented Jan 22, 2025

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (thetruestblue)

Changes

An initializer for the Crash Reporter Annotatoins struct was added in version 5. For the simplicity of not needing to always update the struct in subsequent versions, this patchs checks for the initializer before attempting to redefine the struct on its own.

rdar://136156203


Full diff: https://github.com/llvm/llvm-project/pull/123978.diff

1 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp (+13-8)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
index c8a0afccb254e5..ead3d7123e8a18 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
@@ -45,7 +45,7 @@ extern char **environ;
 #    define SANITIZER_OS_TRACE 0
 #  endif
 
-// import new crash reporting api
+// Integrate with CrashReporter library if available
 #  if defined(__has_include) && __has_include(<CrashReporterClient.h>)
 #    define HAVE_CRASHREPORTERCLIENT_H 1
 #    include <CrashReporterClient.h>
@@ -796,8 +796,13 @@ static char crashreporter_info_buff[__sanitizer::kErrorMessageBufferSize] = {};
 static Mutex crashreporter_info_mutex;
 
 extern "C" {
-// Integrate with crash reporter libraries.
+
 #if HAVE_CRASHREPORTERCLIENT_H
+// Available in CRASHREPORTER_ANNOTATIONS_VERSION 5+
+#    ifdef CRASHREPORTER_ANNOTATIONS_INITIALIZER
+CRASHREPORTER_ANNOTATIONS_INITIALIZER()
+#    else
+// Support for older CrashRerporter annotiations
 CRASH_REPORTER_CLIENT_HIDDEN
 struct crashreporter_annotations_t gCRAnnotations
     __attribute__((section("__DATA," CRASHREPORTER_ANNOTATIONS_SECTION))) = {
@@ -808,17 +813,17 @@ struct crashreporter_annotations_t gCRAnnotations
         0,
         0,
         0,
-#if CRASHREPORTER_ANNOTATIONS_VERSION > 4
+#      if CRASHREPORTER_ANNOTATIONS_VERSION > 4
         0,
-#endif
+#      endif
 };
-
-#else
-// fall back to old crashreporter api
+#    endif
+#  else
+// Revert to previous crash reporter API if client is not available
 static const char *__crashreporter_info__ __attribute__((__used__)) =
     &crashreporter_info_buff[0];
 asm(".desc ___crashreporter_info__, 0x10");
-#endif
+#endif  // HAVE_CRASHREPORTERCLIENT_H
 
 }  // extern "C"
 

@github-actions
Copy link

github-actions bot commented Jan 22, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 7a831eb924e34e9c5e62f3b5a8e0db0278284f84 2154683801e079efeb4fc1fc969bb576dc33d3f8 --extensions cpp -- compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
View the diff from clang-format here.
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
index d15f30c61b..a312fe6b2b 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
@@ -823,7 +823,7 @@ struct crashreporter_annotations_t gCRAnnotations
 static const char *__crashreporter_info__ __attribute__((__used__)) =
     &crashreporter_info_buff[0];
 asm(".desc ___crashreporter_info__, 0x10");
-#endif  // HAVE_CRASHREPORTERCLIENT_H
+#  endif  // HAVE_CRASHREPORTERCLIENT_H
 
 }  // extern "C"
 

@thetruestblue
Copy link
Contributor Author

thetruestblue commented Jan 22, 2025

I don't understand the clang-format failure -- I think it's because the corresponding #if is not within the diff. I am going to ignore it, unless someone can explain why the ending #endif should be indented?

@wrotki
Copy link
Contributor

wrotki commented Jan 23, 2025

Wouldn't it be enough to undo this change (from clang-format above here), to make clang-format happy?:

-#endif // HAVE_CRASHREPORTERCLIENT_H
+# endif // HAVE_CRASHREPORTERCLIENT_H

@thetruestblue
Copy link
Contributor Author

thetruestblue commented Jan 23, 2025

Wouldn't it be enough to undo this change (from clang-format above here), to make clang-format happy?:

-#endif // HAVE_CRASHREPORTERCLIENT_H +# endif // HAVE_CRASHREPORTERCLIENT_H

this is the diff suggested by clang format, not the diff of my change. My change on this line is adding a comment only (it wasn't there before). Because the entire conditional isn't in the diff, I think clang-format is confused.

Copy link
Contributor

@wrotki wrotki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@thetruestblue thetruestblue merged commit 0b7cbd2 into llvm:main Jan 23, 2025
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants