[lldb][Runtime] Move VerboseTrapFrameRecognizer into CPPLanguageRuntime #166157
Conversation
llvm#165996 is adding a Clang dependency to Target because we're moving some of the functionality of the VerboseTrapFrameRecognizer into libClang. To avoid adding this dependency this patch moves VerboseTrapFrameRecognizer into the CPPLanguageRuntime. Most of the frame recognizers already live in the various runtime plugins. An alternative discussed was to create a common `CLanguageRuntime` whose currentl sole responsibility was to register the `VerboseTrapFrameRecognizer` and `AssertStackFrameRecognizer`. However, making this plugin a fully-fledged runtime felt wrong because all the `LanguageRuntime` functionality would live in the derived C++/ObjC runtime plugins. We also have this awkward problem that frame recognizers aren't uniqued in the target. Currently this only manifests when re-running a target, which re-triggers all the recognizer registration (added a test with a FIXME for this). If we had a common `CLanguageRuntime` that `CPPLanguageRuntime` and `ObjCLanguageRuntime` inherited from, I didn't find a great way to avoid registering the recognizer multiple times. In order to unblock llvm#165996 simply move the `VerboseTrapFrameRecognizer` for now
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) Changes#165996 is adding a Clang dependency to Target because we're moving some of the functionality of the VerboseTrapFrameRecognizer into libClang. To avoid adding this dependency this patch moves VerboseTrapFrameRecognizer into the CPPLanguageRuntime. Most of the frame recognizers already live in the various runtime plugins. An alternative discussed was to create a common In order to unblock #165996 simply move the Full diff: https://github.com/llvm/llvm-project/pull/166157.diff 9 Files Affected:
diff --git a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CMakeLists.txt b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CMakeLists.txt
index 1717b0a896669..a27bceffe2e3a 100644
--- a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CMakeLists.txt
+++ b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CMakeLists.txt
@@ -1,5 +1,6 @@
add_lldb_library(lldbPluginCPPRuntime
CPPLanguageRuntime.cpp
+ VerboseTrapFrameRecognizer.cpp
LINK_LIBS
lldbCore
diff --git a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
index 21a5ebe53073a..913678b629f2f 100644
--- a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
+++ b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
@@ -12,6 +12,7 @@
#include <memory>
#include "CPPLanguageRuntime.h"
+#include "VerboseTrapFrameRecognizer.h"
#include "llvm/ADT/StringRef.h"
@@ -107,12 +108,15 @@ class LibCXXFrameRecognizer : public StackFrameRecognizer {
CPPLanguageRuntime::CPPLanguageRuntime(Process *process)
: LanguageRuntime(process) {
- if (process)
+ if (process) {
process->GetTarget().GetFrameRecognizerManager().AddRecognizer(
StackFrameRecognizerSP(new LibCXXFrameRecognizer()), {},
std::make_shared<RegularExpression>("^std::__[^:]*::"),
/*mangling_preference=*/Mangled::ePreferDemangledWithoutArguments,
/*first_instruction_only=*/false);
+
+ RegisterVerboseTrapFrameRecognizer(*process);
+ }
}
bool CPPLanguageRuntime::IsAllowedRuntimeValue(ConstString name) {
diff --git a/lldb/source/Target/VerboseTrapFrameRecognizer.cpp b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/VerboseTrapFrameRecognizer.cpp
similarity index 98%
rename from lldb/source/Target/VerboseTrapFrameRecognizer.cpp
rename to lldb/source/Plugins/LanguageRuntime/CPlusPlus/VerboseTrapFrameRecognizer.cpp
index 03ab58b8c59a9..730aba5b42a3e 100644
--- a/lldb/source/Target/VerboseTrapFrameRecognizer.cpp
+++ b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/VerboseTrapFrameRecognizer.cpp
@@ -1,4 +1,4 @@
-#include "lldb/Target/VerboseTrapFrameRecognizer.h"
+#include "VerboseTrapFrameRecognizer.h"
#include "lldb/Core/Module.h"
#include "lldb/Symbol/Function.h"
diff --git a/lldb/include/lldb/Target/VerboseTrapFrameRecognizer.h b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/VerboseTrapFrameRecognizer.h
similarity index 63%
rename from lldb/include/lldb/Target/VerboseTrapFrameRecognizer.h
rename to lldb/source/Plugins/LanguageRuntime/CPlusPlus/VerboseTrapFrameRecognizer.h
index 7e045760a28be..7d7020f63c8d2 100644
--- a/lldb/include/lldb/Target/VerboseTrapFrameRecognizer.h
+++ b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/VerboseTrapFrameRecognizer.h
@@ -1,5 +1,13 @@
-#ifndef LLDB_TARGET_VERBOSETRAPFRAMERECOGNIZER_H
-#define LLDB_TARGET_VERBOSETRAPFRAMERECOGNIZER_H
+//===-- VerboseTrapFrameRecognizer.h --------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_SOURCE_PLUGINS_LANGUAGERUNTIME_C_PLUS_PLUS_VERBOSETRAPFRAMERECOGNIZER_H
+#define LLDB_SOURCE_PLUGINS_LANGUAGERUNTIME_C_PLUS_PLUS_VERBOSETRAPFRAMERECOGNIZER_H
#include "lldb/Target/StackFrameRecognizer.h"
@@ -36,4 +44,4 @@ class VerboseTrapFrameRecognizer : public StackFrameRecognizer {
} // namespace lldb_private
-#endif // LLDB_TARGET_VERBOSETRAPFRAMERECOGNIZER_H
+#endif // LLDB_SOURCE_PLUGINS_LANGUAGERUNTIME_C_PLUS_PLUS_VERBOSETRAPFRAMERECOGNIZER_H
diff --git a/lldb/source/Target/CMakeLists.txt b/lldb/source/Target/CMakeLists.txt
index b7788e80eecac..8e6d51efad1f3 100644
--- a/lldb/source/Target/CMakeLists.txt
+++ b/lldb/source/Target/CMakeLists.txt
@@ -80,7 +80,6 @@ add_lldb_library(lldbTarget
UnixSignals.cpp
UnwindAssembly.cpp
UnwindLLDB.cpp
- VerboseTrapFrameRecognizer.cpp
ADDITIONAL_HEADER_DIRS
${LLDB_INCLUDE_DIR}/lldb/Target
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index fb9e7eb5ed1bd..42ce198a283da 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -65,7 +65,6 @@
#include "lldb/Target/ThreadPlanCallFunction.h"
#include "lldb/Target/ThreadPlanStack.h"
#include "lldb/Target/UnixSignals.h"
-#include "lldb/Target/VerboseTrapFrameRecognizer.h"
#include "lldb/Utility/AddressableBits.h"
#include "lldb/Utility/Event.h"
#include "lldb/Utility/LLDBLog.h"
@@ -513,7 +512,6 @@ Process::Process(lldb::TargetSP target_sp, ListenerSP listener_sp,
// We should have a plugin do the registration instead, for example, a
// common C LanguageRuntime plugin.
RegisterAssertFrameRecognizer(this);
- RegisterVerboseTrapFrameRecognizer(*this);
}
Process::~Process() {
diff --git a/lldb/test/Shell/Recognizer/Inputs/verbose_trap.m b/lldb/test/Shell/Recognizer/Inputs/verbose_trap.m
new file mode 100644
index 0000000000000..83a829a8c2fdd
--- /dev/null
+++ b/lldb/test/Shell/Recognizer/Inputs/verbose_trap.m
@@ -0,0 +1,4 @@
+int main() {
+ __builtin_verbose_trap("Foo", "Bar");
+ return 0;
+}
diff --git a/lldb/test/Shell/Recognizer/registration-unique.test b/lldb/test/Shell/Recognizer/registration-unique.test
new file mode 100644
index 0000000000000..cd4dd8f02d352
--- /dev/null
+++ b/lldb/test/Shell/Recognizer/registration-unique.test
@@ -0,0 +1,53 @@
+# Checks that the recognizers that should work across language runtimes
+# are only registered once with the target.
+
+# RUN: split-file %s %t
+
+# RUN: %clang_host %t/main.cpp -g -o %t/cpp.out
+# RUN: %lldb -b -s %t/commands.input %t/cpp.out | FileCheck %s
+
+# RUN: %clang_host -x objective-c++ %t/main.mm -g -o %t/objcxx.out
+# RUN: %lldb -b -s %t/commands.input %t/objcxx.out | FileCheck %s
+
+# RUN: %clang_host %t/main.c -g -o %t/c.out
+# RUN: %lldb -b -s %t/commands.input %t/c.out | FileCheck %s
+
+# RUN: %clang_host -x objective-c %t/main.m -g -o %t/objc.out
+# RUN: %lldb -b -s %t/commands.input %t/objc.out | FileCheck %s
+
+#--- main.m
+int main() {}
+
+#--- main.c
+int main() {}
+
+#--- main.mm
+int main() {}
+
+#--- main.cpp
+int main() {}
+
+#--- commands.input
+
+b main
+frame recognizer list
+run
+frame recognizer list
+run
+frame recognizer list
+
+# CHECK: frame recognizer list
+# CHECK-NEXT: no matching results found.
+
+# CHECK: frame recognizer list
+# CHECK: Verbose Trap StackFrame Recognizer
+# CHECK: Assert StackFrame Recognizer
+# CHECK-NOT: Verbose Trap StackFrame Recognizer
+# CHECK-NOT: Assert StackFrame Recognizer
+
+# FIXME: avoid duplicate frame recognizers in the target.
+# CHECK: frame recognizer list
+# CHECK: Verbose Trap StackFrame Recognizer
+# CHECK: Assert StackFrame Recognizer
+# CHECK: Verbose Trap StackFrame Recognizer
+# CHECK: Assert StackFrame Recognizer
diff --git a/lldb/test/Shell/Recognizer/verbose_trap-objc.test b/lldb/test/Shell/Recognizer/verbose_trap-objc.test
new file mode 100644
index 0000000000000..789e79c9542c5
--- /dev/null
+++ b/lldb/test/Shell/Recognizer/verbose_trap-objc.test
@@ -0,0 +1,12 @@
+# REQUIRES: system-darwin
+#
+# RUN: %clangxx_host -x objective-c -g %S/Inputs/verbose_trap.m -o %t.out
+# RUN: %lldb -b -s %s %t.out | FileCheck %s
+
+run
+# CHECK: thread #{{.*}}stop reason = Foo: Bar
+frame info
+# CHECK: frame #{{.*}}`main at verbose_trap.m
+frame recognizer info 0
+# CHECK: frame 0 is recognized by Verbose Trap StackFrame Recognizer
+q
|
I don't quite follow this. Presumably you'd have What about that design implies you'd have to put the recognizer inside |
Is this a typo? |
delcypher
left a comment
delcypher
left a comment
There was a problem hiding this comment.
Provided CPPLanguateRuntime is something that's always loaded when debugging an application that contains C/C++/objective-c/objective-c++ then this seems reasonable to me. Your test case implies that this is the case.
| /*mangling_preference=*/Mangled::ePreferDemangledWithoutArguments, | ||
| /*first_instruction_only=*/false); | ||
|
|
||
| RegisterVerboseTrapFrameRecognizer(*process); |
There was a problem hiding this comment.
Is CPPLanguageRuntime guaranteed to be loaded even if the process being debugged was built with C only?
There was a problem hiding this comment.
I believe so yes. I don't know of a case where the C++ language plugin registration wouldn't trigger. IIUC, this would only happen if someone chose to compile the plugin out of LLDB when building it. I don't think we have a mechanism in CMake to do that yet? @jimingham @JDevlieghere Is my understanding correct here?
There was a problem hiding this comment.
You can compile out plugins but it's not guaranteed to be safe. There are many "plugins" that LLDB can't work without (or plugins that depend on other plugins) so although it's possible, it's not encouraged or easy to do. I don't think we should account for that in our design.
Yea I didn't describe that very well. I meant that it felt off because the There may very well be solutions I haven't considered, but all the things I've tried so far I wasn't very happy with. So in the end I just moved this to the C++ runtime for now. AFAICT the C++ language runtime is always available (even for C targets) if the C++ language plugin gets available. Which it should also be unless someone is using an LLDB with the C++ plugin compiled out? But at that point numerous things wouldn't work when even debugging just C. |
delcypher
left a comment
delcypher
left a comment
There was a problem hiding this comment.
Thanks for the explanation. A few suggestions:
- File an issue about the duplicate recognizer bugs and refer to it from the test case
- Improve the commit description to include your clearer explaination.
I'll approve now under the assumption you'll fix those. It's probably best you get approval from another LLDB maintainer though as this isn't my area of expertise.
| # CHECK-NOT: Verbose Trap StackFrame Recognizer | ||
| # CHECK-NOT: Assert StackFrame Recognizer | ||
|
|
||
| # FIXME: avoid duplicate frame recognizers in the target. |
There was a problem hiding this comment.
Maybe file an issue and refer to it here so that we don't lose track of this bug?
adrian-prantl
left a comment
adrian-prantl
left a comment
There was a problem hiding this comment.
It's a bit awkward to have this in the C++ runtime, but otherwise LGTM.
Updated description and filed issue (#166341)! Thanks for the review |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/211/builds/3468 Here is the relevant piece of the build log for the reference |
Looking |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/12749 Here is the relevant piece of the build log for the reference |
…me (llvm#166157) llvm#165996 is adding a Clang dependency to Target because we're moving some of the functionality of the VerboseTrapFrameRecognizer into libClang. To avoid adding this dependency this patch moves VerboseTrapFrameRecognizer into the CPPLanguageRuntime. Most of the frame recognizers already live in the various runtime plugins. An alternative discussed was to create a common `CLanguageRuntime` whose currently sole responsibility was to register the `VerboseTrapFrameRecognizer` and `AssertStackFrameRecognizer`. The main issue I ran into here was frame recognizers aren't uniqued in the target. Currently this only manifests when re-running a target, which re-triggers all the recognizer registration (added a test with a FIXME for this). If we had a common `CLanguageRuntime` that `CPPLanguageRuntime` and `ObjCLanguageRuntime` inherited from, I didn't find a great way to avoid registering the recognizer multiple times. We can't just call_once on it because we do want the recognisers to be re-registered for new targets in the same debugger session. If the recognisers were stored in something like a UniqueVector in the Target, then we wouldn't have that issue. But currently that's not the case, and it would take a bit of refactoring to de-dupe the recognisers. There may very well be solutions I haven't considered, but all the things I've tried so far I wasn't very happy with. So in the end I just moved this to the C++ runtime for now in order to unblock llvm#165996. The C++ language runtime is always available (even for C targets) if the C++ language plugin is available. Which it should also be unless someone is using an LLDB with the C++ plugin compiled out. But at that point numerous things wouldn't work when even debugging just C. (cherry picked from commit bb4ed55)
6 participants
#165996 is adding a Clang dependency to Target because we're moving some of the functionality of the VerboseTrapFrameRecognizer into libClang. To avoid adding this dependency this patch moves VerboseTrapFrameRecognizer into the CPPLanguageRuntime. Most of the frame recognizers already live in the various runtime plugins.
An alternative discussed was to create a common
CLanguageRuntimewhose currently sole responsibility was to register theVerboseTrapFrameRecognizerandAssertStackFrameRecognizer. The main issue I ran into here was frame recognizers aren't uniqued in the target. Currently this only manifests when re-running a target, which re-triggers all the recognizer registration (added a test with a FIXME for this). If we had a commonCLanguageRuntimethatCPPLanguageRuntimeandObjCLanguageRuntimeinherited from, I didn't find a great way to avoid registering the recognizer multiple times. We can't just call_once on it because we do want the recognisers to be re-registered for new targets in the same debugger session. If the recognisers were stored in something like a UniqueVector in the Target, then we wouldn't have that issue. But currently that's not the case, and it would take a bit of refactoring to de-dupe the recognisers.There may very well be solutions I haven't considered, but all the things I've tried so far I wasn't very happy with. So in the end I just moved this to the C++ runtime for now in order to unblock #165996.
The C++ language runtime is always available (even for C targets) if the C++ language plugin is available. Which it should also be unless someone is using an LLDB with the C++ plugin compiled out. But at that point numerous things wouldn't work when even debugging just C.