This is an archive of the discontinued LLVM Phabricator instance.

Prepare PrettyStackTrace for LLDB adoption
ClosedPublic

Authored by spyffe on Dec 12 2016, 11:52 AM.

Details

Summary

This patch fixes the linkage for __crashtracer_info__ making it have the proper mangling (extern "C") and linkage (private extern).
It also adds a new PrettyStackTrace type, allowing LLDB to adopt this instead of Host::SetCrashDescriptionWithFormat().

Without this patch, CrashTracer on macOS won't pick up pretty stack traces from any LLVM client. Also, this patch needs to be synced with an LLDB patch that adopts it.

Diff Detail

Repository
rL LLVM

Event Timeline

spyffe updated this revision to Diff 81120.Dec 12 2016, 11:52 AM
spyffe retitled this revision from to Fix the linkage for __crashtracer_info__.
spyffe updated this object.
spyffe set the repository for this revision to rL LLVM.
spyffe edited subscribers, added: cfe-commits; removed: jgosnell.
spyffe added a subscriber: jgosnell.
clayborg accepted this revision.Dec 12 2016, 1:06 PM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Dec 12 2016, 1:06 PM
spyffe updated this revision to Diff 81304.Dec 13 2016, 2:38 PM
spyffe retitled this revision from Fix the linkage for __crashtracer_info__ to Prepare PrettyStackTrace for LLDB adoption.
spyffe updated this object.
spyffe edited edge metadata.
beanz accepted this revision.Dec 13 2016, 9:34 PM
beanz edited edge metadata.

LGTM other than a few style comments. Please clang-format before committing. Some of the formatting looks off (although that might just be my phone).

lib/Support/PrettyStackTrace.cpp
152

Your variable names are very LLDB-esque. Please follow the LLVM style guide, which is CamelCase.

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.May 17 2021, 8:28 AM
thakis added inline comments.
llvm/trunk/lib/Support/PrettyStackTrace.cpp
93 ↗(On Diff #81426)

As far as I can tell, making this a hidden symbol makes the .desc ___crashreporter_info__, 0x10 (ie REFERENCED_DYNAMICALLY) not have any effect:

% cat crashref.cc
extern "C" const char *__crashreporter_info__  __attribute__((visibility("hidden"))) = 0;
asm(".desc ___crashreporter_info__, 0x10");
int main() {}
% clang crashref.cc
% nm -m a.out
0000000100004000 (__DATA,__common) non-external (was a private external) ___crashreporter_info__
0000000100000000 (__TEXT,__text) [referenced dynamically] external __mh_execute_header
0000000100003fb0 (__TEXT,__text) external _main
                 (undefined) external dyld_stub_binder (from libSystem)
% strip -r a.out
% nm -m a.out
0000000100000000 (__TEXT,__text) [referenced dynamically] external __mh_execute_header

It does have an effect without it:

% cat crashref.cc
extern "C" const char *__crashreporter_info__ = 0;
asm(".desc ___crashreporter_info__, 0x10");
int main() {}
% clang crashref.cc
% nm -m a.out
0000000100004000 (__DATA,__common) [referenced dynamically] external ___crashreporter_info__
0000000100000000 (__TEXT,__text) [referenced dynamically] external __mh_execute_header
0000000100003fb0 (__TEXT,__text) external _main
                 (undefined) external dyld_stub_binder (from libSystem)
% strip -r a.out
% nm -m a.out
0000000100004000 (__DATA,__common) [referenced dynamically] external ___crashreporter_info__
0000000100000000 (__TEXT,__text) [referenced dynamically] external __mh_execute_header

Is that intentional? Should we just remove the .desc line?

Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2021, 8:28 AM
clayborg added inline comments.May 17 2021, 7:48 PM
llvm/trunk/lib/Support/PrettyStackTrace.cpp
92–93 ↗(On Diff #81426)

lol, if we still need the ___crashreporter_info__ symbol to be around, then it should be visible so that ReportCrash can find it...