Page MenuHomePhabricator

[darwin] use new crash reporter api
ClosedPublic

Authored by aralisza on Feb 16 2021, 6:55 PM.

Details

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

Diff Detail

Event Timeline

aralisza requested review of this revision.Feb 16 2021, 6:55 PM
aralisza created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2021, 6:55 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
aralisza retitled this revision from [draft] the rest of the change to [darwin] use new crash reporter api.Feb 16 2021, 7:28 PM
aralisza edited the summary of this revision. (Show Details)
delcypher added a comment.EditedFeb 16 2021, 8:06 PM

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 added inline comments.Feb 16 2021, 8:19 PM
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 added inline comments.Feb 17 2021, 12:31 PM
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 updated this revision to Diff 324392.Feb 17 2021, 12:31 PM

fix bug define

aralisza edited the summary of this revision. (Show Details)Feb 17 2021, 12:32 PM
aralisza edited the summary of this revision. (Show Details)

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.

delcypher accepted this revision.Feb 17 2021, 4:22 PM
This revision is now accepted and ready to land.Feb 17 2021, 4:22 PM
aralisza edited the summary of this revision. (Show Details)Feb 17 2021, 4:57 PM
aralisza updated this revision to Diff 324468.Feb 17 2021, 4:58 PM

remove test from stack

delcypher added inline comments.Feb 18 2021, 9:44 AM
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
798

Yep _gCRAnnotations is unmangled and __ZN11__sanitizerL22__crashreporter_info__E is mangled.

yln added inline comments.Feb 18 2021, 10:16 AM
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 edited the summary of this revision. (Show Details)Feb 18 2021, 2:09 PM
aralisza edited the summary of this revision. (Show Details)
aralisza edited the summary of this revision. (Show Details)
aralisza updated this revision to Diff 324771.Feb 18 2021, 2:13 PM

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 updated this revision to Diff 324773.Feb 18 2021, 2:15 PM

remove underscores in global names

Harbormaster completed remote builds in B89800: Diff 324773.
aralisza updated this revision to Diff 324784.Feb 18 2021, 2:59 PM

rebase main

yln added a comment.EditedFeb 18 2021, 3:57 PM

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 updated this revision to Diff 324843.Feb 18 2021, 7:13 PM

also make mutex static

aralisza updated this revision to Diff 324846.Feb 18 2021, 7:16 PM

remove inline and make static

yln added a comment.Feb 19 2021, 10:17 AM

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
67–68

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

aralisza updated this revision to Diff 325045.Feb 19 2021, 11:06 AM

squash and remove uneeded function from header

yln accepted this revision.Feb 19 2021, 11:25 AM

LGTM, thanks! :)

aralisza updated this revision to Diff 325057.Feb 19 2021, 12:11 PM

remove not useful comments

delcypher accepted this revision.Feb 22 2021, 2:27 PM

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.

This revision was automatically updated to reflect the committed changes.