[lldb] Add frame recognizer for __builtin_verbose_trap#80368
[lldb] Add frame recognizer for __builtin_verbose_trap#80368Michael137 merged 7 commits intollvm:mainfrom
Conversation
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThis patch adds a frame recognizer for Clang's The frame recognizer triggers when we encounter Example output: Full diff: https://github.com/llvm/llvm-project/pull/80368.diff 6 Files Affected:
|
|
The patch looks fine to me, but I'm a bit bugged by the fact that it is in Target. In my mind, this is part of the C LanguageRuntime, except we don't yet have a C Language Runtime... |
|
This almost seems like a compiler runtime plug-in, but C language would be fine. I would rather see this in plug-in somewhere if possible. It might be nice to have these plug-ins register one or more frame regular expression values that point to the plug-in so we can add more things. It would be nice to also add support for C assert() calls that would unwind the stack to the offinding assert function above the 3 or 4 pthread functions that occur when an assertion is hit |
|
There IS an AssertFrameRecogizer that does just what you suggest, but it is in source/Target. These really should go somewhere else, however, Target is too general for anything but the base recognizer class.
Jim
… On Feb 1, 2024, at 6:43 PM, Greg Clayton ***@***.***> wrote:
This almost seems like a compiler runtime plug-in, but C language would be fine. I would rather see this in plug-in somewhere if possible. It might be nice to have these plug-ins register one or more frame regular expression values that point to the plug-in so we can add more things. It would be nice to also add support for C assert() calls that would unwind the stack to the offinding assert function above the 3 or 4 pthread functions that occur when an assertion is hit
—
Reply to this email directly, view it on GitHub <#80368 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW3VRHN3DNFRZYNLY2TYRRHGZAVCNFSM6AAAAABCV3DNHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRSGY4TAOJVGQ>.
You are receiving this because you are on a team that was mentioned.
|
|
That one's odd, it almost belongs in Platform, after all, there's no guarantee that "assert" is implemented by pthread functions at all. It just happens to be on Unix systems.
Jim
… On Feb 1, 2024, at 7:32 PM, Jim Ingham ***@***.***> wrote:
There IS an AssertFrameRecogizer that does just what you suggest, but it is in source/Target. These really should go somewhere else, however, Target is too general for anything but the base recognizer class.
Jim
> On Feb 1, 2024, at 6:43 PM, Greg Clayton ***@***.***> wrote:
>
>
> This almost seems like a compiler runtime plug-in, but C language would be fine. I would rather see this in plug-in somewhere if possible. It might be nice to have these plug-ins register one or more frame regular expression values that point to the plug-in so we can add more things. It would be nice to also add support for C assert() calls that would unwind the stack to the offinding assert function above the 3 or 4 pthread functions that occur when an assertion is hit
>
> —
> Reply to this email directly, view it on GitHub <#80368 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW3VRHN3DNFRZYNLY2TYRRHGZAVCNFSM6AAAAABCV3DNHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRSGY4TAOJVGQ>.
> You are receiving this because you are on a team that was mentioned.
>
|
Seems reasonable to me. To clarify, do you envision a |
|
I think that makes the most sense.
Jim
… On Feb 2, 2024, at 1:45 AM, Michael Buch ***@***.***> wrote:
The patch looks fine to me, but I'm a bit bugged by the fact that it is in Target. In my mind, this is part of the C LanguageRuntime, except we don't yet have a C Language Runtime... But in any case, somewhere more specific than Target.
Seems reasonable to me. Should I create a CLanguageRuntime plugin and move the AssertRecognizedStackFrame there then? I'll open a separate PR for this then
—
Reply to this email directly, view it on GitHub <#80368 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW2EKJES7NXFOC27SJDYRSYTPAVCNFSM6AAAAABCV3DNHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRTGQ2DCMBZGA>.
You are receiving this because you are on a team that was mentioned.
|
|
What will happen in an Objective-C++ frame then? Will there be two C runtimes because we have both a C++ and an ObjC runtime? |
| void RegisterVerboseTrapFrameRecognizer(Process &process) { | ||
| RegularExpressionSP module_regex_sp = nullptr; | ||
| RegularExpressionSP symbol_regex_sp( | ||
| new RegularExpression("(__llvm_verbose_trap)")); |
There was a problem hiding this comment.
Should this be (^__llvm_verbose_trap) so my__llvm_verbose_trap doesn't match?
| # RUN: %clang_host -g -O0 %S/Inputs/verbose_trap.cpp -o %t.out | ||
| # RUN: %lldb -b -s %s %t.out | FileCheck %s | ||
| run | ||
| # CHECK: thread #{{.*}}stop reason = __llvm_verbose_trap: Function is not implemented |
There was a problem hiding this comment.
Should we hide the __llvm_verbose_trap implementation detail and display something like "Runtime Error:" instead?
|
I don't think the C++ or ObjC runtimes would vend their own C language runtimes. I don't think that would reflect a situation any system has (different C runtimes for C++ and ObjC). Rather these would just be three independent plugin instances.
Jim
… On Feb 2, 2024, at 3:57 PM, Adrian Prantl ***@***.***> wrote:
What will happen in an Objective-C++ frame then? Will there be two C runtimes because we have both a C++ and an ObjC runtime?
—
Reply to this email directly, view it on GitHub <#80368 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW7O3FSZTQIN5KZFRADYRV4OBAVCNFSM6AAAAABCV3DNHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRUHEYTQMJZHA>.
You are receiving this because you are on a team that was mentioned.
|
JDevlieghere
left a comment
There was a problem hiding this comment.
LGTM with comments addressed.
There was a problem hiding this comment.
Maybe worth hoisting Log *log = GetLog(LLDBLog::Unwind);, it should be pretty cheap?
There was a problem hiding this comment.
Rather than hard coding the name should we be getting it from a header file vended by Clang?
I mentioned this in the corresponding Clang review here.
This wouldn't need to be anything sophisticated. Just a header file under include/clang/CodeGen that is something like
#define CLANG_VERBOSE_TRAP_PREFIX "__llvm_verbose_trap"
and then from LLDB that would be something like:
new RegularExpression("^" CLANG_VERBOSE_TRAP_PREFIX ": ")
That way there is a single source of truth for the name used.
There was a problem hiding this comment.
Runtime Error: is pretty generic. Is there any reason we aren't mentioning that a trap was hit?
There was a problem hiding this comment.
How about Runtime trap: ...?
1982877 to
587ea97
Compare
This patch adds a frame recognizer for Clang's `__builtin_verbose_trap`, which behaves like a `__builtin_trap`, but emits a failure-reason string into debug-info in order for debuggers to display it to a user. The frame recognizer triggers when we encounter a frame with a function name that begins with `__llvm_verbose_trap`, which is the magic prefix Clang emits into debug-info for verbose traps. Once such frame is encountered we display the frame function name as the `Stop Reason` and display that frame to the user. Example output: ``` (lldb) run warning: a.out was compiled with optimization - stepping may behave oddly; variables may not be available. Process 35942 launched: 'a.out' (arm64) Process 35942 stopped * thread llvm#1, queue = 'com.apple.main-thread', stop reason = __llvm_verbose_trap: Function is not implemented frame llvm#1: 0x0000000100003fa4 a.out`main [inlined] Dummy::func(this=<unavailable>) at verbose_trap.cpp:3:5 [opt] 1 struct Dummy { 2 void func() { -> 3 __builtin_verbose_trap("Function is not implemented"); 4 } 5 }; 6 7 int main() { (lldb) bt * thread llvm#1, queue = 'com.apple.main-thread', stop reason = __llvm_verbose_trap: Function is not implemented frame #0: 0x0000000100003fa4 a.out`main [inlined] __llvm_verbose_trap: Function is not implemented at verbose_trap.cpp:0 [opt] * frame llvm#1: 0x0000000100003fa4 a.out`main [inlined] Dummy::func(this=<unavailable>) at verbose_trap.cpp:3:5 [opt] frame llvm#2: 0x0000000100003fa4 a.out`main at verbose_trap.cpp:8:13 [opt] frame llvm#3: 0x0000000189d518b4 dyld`start + 1988 ```
587ea97 to
4dd2c33
Compare
delcypher
left a comment
There was a problem hiding this comment.
Looks pretty good. I had a few minor comments
|
|
||
| static auto trap_regex = | ||
| llvm::Regex(llvm::formatv("^{0}\\$(.*)\\$(.*)$", ClangTrapPrefix).str()); | ||
| SmallVector<llvm::StringRef, 2> matches; |
There was a problem hiding this comment.
If we expect 3 matches shouldn't the stack allocation size be 3 instead of 2?
| ThreadSP thread_sp = frame_sp->GetThread(); | ||
| ProcessSP process_sp = thread_sp->GetProcess(); | ||
|
|
||
| StackFrameSP most_relevant_frame_sp = thread_sp->GetStackFrameAtIndex(1); |
There was a problem hiding this comment.
Is there anyway for there to not be a stack frame at index 1? If that's possible could we bail out early so we don't crash/have bad behavior?
There was a problem hiding this comment.
While I don't see it documented explicitly, GetStackFrameAtIndex handles cases where we asked for a frame index that's out-of-bounds. And according to the docs GetStackFrameCount can be expensive to check, so we probably don't want to do that here
|
@jimingham @JDevlieghere @clayborg just to confirm, are you still ok with me landing this and address the question of a common |
|
I am fine with landing as is. Maybe we should add some |
|
Sure.
Jim
… On Jul 13, 2024, at 2:20 AM, Michael Buch ***@***.***> wrote:
@jimingham <https://github.com/jimingham> @JDevlieghere <https://github.com/JDevlieghere> @clayborg <https://github.com/clayborg> just to confirm, are you still ok with me landing this and address the question of a common CLanguageRuntime plugin as a follow-up?
—
Reply to this email directly, view it on GitHub <#80368 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW55RDOYPCYBKTTE4ODZMDWPHAVCNFSM6AAAAABCV3DNHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRWHAZTINBXGU>.
You are receiving this because you were mentioned.
|
Makes sense, added one to where we register all the recognizers |
verbose_trap.test, added in llvm#80368, fails on some Windows bots. See https://lab.llvm.org/buildbot/#/builders/141/builds/808.
verbose_trap.test, added in #80368, fails on some Windows bots. See https://lab.llvm.org/buildbot/#/builders/141/builds/808.
This patch adds a frame recognizer for Clang's `__builtin_verbose_trap`, which behaves like a `__builtin_trap`, but emits a failure-reason string into debug-info in order for debuggers to display it to a user. The frame recognizer triggers when we encounter a frame with a function name that begins with `__clang_trap_msg`, which is the magic prefix Clang emits into debug-info for verbose traps. Once such frame is encountered we display the frame function name as the `Stop Reason` and display that frame to the user. Example output: ``` (lldb) run warning: a.out was compiled with optimization - stepping may behave oddly; variables may not be available. Process 35942 launched: 'a.out' (arm64) Process 35942 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = Misc.: Function is not implemented frame #1: 0x0000000100003fa4 a.out`main [inlined] Dummy::func(this=<unavailable>) at verbose_trap.cpp:3:5 [opt] 1 struct Dummy { 2 void func() { -> 3 __builtin_verbose_trap("Misc.", "Function is not implemented"); 4 } 5 }; 6 7 int main() { (lldb) bt * thread #1, queue = 'com.apple.main-thread', stop reason = Misc.: Function is not implemented frame #0: 0x0000000100003fa4 a.out`main [inlined] __clang_trap_msg$Misc.$Function is not implemented$ at verbose_trap.cpp:0 [opt] * frame #1: 0x0000000100003fa4 a.out`main [inlined] Dummy::func(this=<unavailable>) at verbose_trap.cpp:3:5 [opt] frame #2: 0x0000000100003fa4 a.out`main at verbose_trap.cpp:8:13 [opt] frame #3: 0x0000000189d518b4 dyld`start + 1988 ```
Summary: verbose_trap.test, added in #80368, fails on some Windows bots. See https://lab.llvm.org/buildbot/#/builders/141/builds/808. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250944
This patch adds a frame recognizer for Clang's
__builtin_verbose_trap, which behaves like a__builtin_trap, but emits a failure-reason string into debug-info in order for debuggers to displayit to a user.
The frame recognizer triggers when we encounter
a frame with a function name that begins with
__llvm_verbose_trap, which is the magic prefixClang emits into debug-info for verbose traps.
Once such frame is encountered we display the
frame function name as the
Stop Reasonand display that frame to the user.Example output: