-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[Clang][LLDB] Refactor trap reason demangling out of LLDB and into Clang #165996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Clang][LLDB] Refactor trap reason demangling out of LLDB and into Clang #165996
Conversation
|
@llvm/pr-subscribers-lldb @llvm/pr-subscribers-clang-codegen Author: Dan Liew (delcypher) ChangesThis patch refactors the trap reason demangling logic in There are two reasons for doing this:
Unit tests have been added for the new function that demonstrate how to use the new API. The function names recognized by VerboseTrapFrameRecognizer are identical to before. However, this patch isn't NFC because:
rdar://163230807 Full diff: https://github.com/llvm/llvm-project/pull/165996.diff 6 Files Affected:
diff --git a/clang/include/clang/CodeGen/ModuleBuilder.h b/clang/include/clang/CodeGen/ModuleBuilder.h
index f1b8229edd362..4298ba06c472e 100644
--- a/clang/include/clang/CodeGen/ModuleBuilder.h
+++ b/clang/include/clang/CodeGen/ModuleBuilder.h
@@ -120,6 +120,23 @@ CodeGenerator *CreateLLVMCodeGen(DiagnosticsEngine &Diags,
llvm::LLVMContext &C,
CoverageSourceInfo *CoverageInfo = nullptr);
+namespace CodeGen {
+/// Demangle the artificial function name (\param FuncName) used to encode trap
+/// reasons used in debug info for traps (e.g. __builtin_verbose_trap). See
+/// `CGDebugInfo::CreateTrapFailureMessageFor`.
+///
+/// \param FuncName - The function name to demangle.
+///
+/// \return A std::optional. If demangling succeeds the optional will contain
+/// a pair of StringRefs where the first field is the trap category and the
+/// second is the trap message. These can both be empty. If demangling fails the
+/// optional will not contain a value. Note the returned StringRefs if non-empty
+/// point into the underlying storage for \param FuncName and thus have the same
+/// lifetime.
+std::optional<std::pair<StringRef, StringRef>>
+DemangleTrapReasonInDebugInfo(StringRef FuncName);
+} // namespace CodeGen
+
} // end namespace clang
#endif
diff --git a/clang/lib/CodeGen/ModuleBuilder.cpp b/clang/lib/CodeGen/ModuleBuilder.cpp
index 96f3f6221e20f..8ec8aef311656 100644
--- a/clang/lib/CodeGen/ModuleBuilder.cpp
+++ b/clang/lib/CodeGen/ModuleBuilder.cpp
@@ -23,6 +23,7 @@
#include "llvm/IR/DataLayout.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/IR/Module.h"
+#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/VirtualFileSystem.h"
#include <memory>
@@ -378,3 +379,31 @@ clang::CreateLLVMCodeGen(DiagnosticsEngine &Diags, llvm::StringRef ModuleName,
HeaderSearchOpts, PreprocessorOpts, CGO, C,
CoverageInfo);
}
+
+namespace clang {
+namespace CodeGen {
+std::optional<std::pair<StringRef, StringRef>>
+DemangleTrapReasonInDebugInfo(StringRef FuncName) {
+ static auto TrapRegex =
+ llvm::Regex(llvm::formatv("^{0}\\$(.*)\\$(.*)$", ClangTrapPrefix).str());
+ llvm::SmallVector<llvm::StringRef, 3> Matches;
+ std::string *ErrorPtr = nullptr;
+#ifndef NDEBUG
+ std::string Error;
+ ErrorPtr = &Error;
+#endif
+ if (!TrapRegex.match(FuncName, &Matches, ErrorPtr)) {
+ assert(ErrorPtr && ErrorPtr->empty() && "Invalid regex pattern");
+ return {};
+ }
+
+ if (Matches.size() != 3) {
+ assert(0 && "Expected 3 matches from Regex::match");
+ return {};
+ }
+
+ // Returns { Trap Category, Trap Message }
+ return std::make_pair(Matches[1], Matches[2]);
+}
+} // namespace CodeGen
+} // namespace clang
diff --git a/clang/unittests/CodeGen/CMakeLists.txt b/clang/unittests/CodeGen/CMakeLists.txt
index f5bcecb0b08a3..d4efb2230a054 100644
--- a/clang/unittests/CodeGen/CMakeLists.txt
+++ b/clang/unittests/CodeGen/CMakeLists.txt
@@ -1,6 +1,7 @@
add_clang_unittest(ClangCodeGenTests
BufferSourceTest.cpp
CodeGenExternalTest.cpp
+ DemangleTrapReasonInDebugInfo.cpp
TBAAMetadataTest.cpp
CheckTargetFeaturesTest.cpp
CLANG_LIBS
diff --git a/clang/unittests/CodeGen/DemangleTrapReasonInDebugInfo.cpp b/clang/unittests/CodeGen/DemangleTrapReasonInDebugInfo.cpp
new file mode 100644
index 0000000000000..17bfe17c31d65
--- /dev/null
+++ b/clang/unittests/CodeGen/DemangleTrapReasonInDebugInfo.cpp
@@ -0,0 +1,67 @@
+//=== unittests/CodeGen/DemangleTrapReasonInDebugInfo.cpp -----------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/CodeGen/ModuleBuilder.h"
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+using namespace clang::CodeGen;
+
+void CheckValidCommon(llvm::StringRef FuncName, const char *ExpectedCategory,
+ const char *ExpectedMessage) {
+ auto MaybeTrapReason = DemangleTrapReasonInDebugInfo(FuncName);
+ ASSERT_TRUE(MaybeTrapReason.has_value());
+ auto [Category, Message] = MaybeTrapReason.value();
+ ASSERT_STREQ(Category.str().c_str(), ExpectedCategory);
+ ASSERT_STREQ(Message.str().c_str(), ExpectedMessage);
+}
+
+void CheckInvalidCommon(llvm::StringRef FuncName) {
+ auto MaybeTrapReason = DemangleTrapReasonInDebugInfo(FuncName);
+ ASSERT_TRUE(!MaybeTrapReason.has_value());
+}
+
+TEST(DemangleTrapReasonInDebugInfo, Valid) {
+ std::string FuncName(ClangTrapPrefix);
+ FuncName += "$trap category$trap message";
+ CheckValidCommon(FuncName, "trap category", "trap message");
+}
+
+TEST(DemangleTrapReasonInDebugInfo, ValidEmptyCategory) {
+ std::string FuncName(ClangTrapPrefix);
+ FuncName += "$$trap message";
+ CheckValidCommon(FuncName, "", "trap message");
+}
+
+TEST(DemangleTrapReasonInDebugInfo, ValidEmptyMessage) {
+ std::string FuncName(ClangTrapPrefix);
+ FuncName += "$trap category$";
+ CheckValidCommon(FuncName, "trap category", "");
+}
+
+TEST(DemangleTrapReasonInDebugInfo, ValidAllEmpty) {
+ // `__builtin_verbose_trap` actually allows this
+ // currently. However, we should probably disallow this in Sema because having
+ // an empty category and message completely defeats the point of using the
+ // builtin (#165981).
+ std::string FuncName(ClangTrapPrefix);
+ FuncName += "$$";
+ CheckValidCommon(FuncName, "", "");
+}
+
+TEST(DemangleTrapReasonInDebugInfo, InvalidOnlyPrefix) {
+ std::string FuncName(ClangTrapPrefix);
+ CheckInvalidCommon(FuncName);
+}
+
+TEST(DemangleTrapReasonInDebugInfo, Invalid) {
+ std::string FuncName("foo");
+ CheckInvalidCommon(FuncName);
+}
+
+TEST(DemangleTrapReasonInDebugInfo, InvalidEmpty) { CheckInvalidCommon(""); }
diff --git a/lldb/source/Target/CMakeLists.txt b/lldb/source/Target/CMakeLists.txt
index b7788e80eecac..ba8f11878899e 100644
--- a/lldb/source/Target/CMakeLists.txt
+++ b/lldb/source/Target/CMakeLists.txt
@@ -97,6 +97,9 @@ add_lldb_library(lldbTarget
lldbUtility
lldbValueObject
lldbPluginProcessUtility
+
+ CLANG_LIBS
+ clangCodeGen
)
add_dependencies(lldbTarget
diff --git a/lldb/source/Target/VerboseTrapFrameRecognizer.cpp b/lldb/source/Target/VerboseTrapFrameRecognizer.cpp
index 03ab58b8c59a9..be3769191dc9f 100644
--- a/lldb/source/Target/VerboseTrapFrameRecognizer.cpp
+++ b/lldb/source/Target/VerboseTrapFrameRecognizer.cpp
@@ -95,33 +95,14 @@ VerboseTrapFrameRecognizer::RecognizeFrame(lldb::StackFrameSP frame_sp) {
if (func_name.empty())
return {};
- static auto trap_regex =
- llvm::Regex(llvm::formatv("^{0}\\$(.*)\\$(.*)$", ClangTrapPrefix).str());
- SmallVector<llvm::StringRef, 3> matches;
- std::string regex_err_msg;
- if (!trap_regex.match(func_name, &matches, ®ex_err_msg)) {
- LLDB_LOGF(GetLog(LLDBLog::Unwind),
- "Failed to parse match trap regex for '%s': %s", func_name.data(),
- regex_err_msg.c_str());
-
- return {};
- }
-
- // For `__clang_trap_msg$category$message$` we expect 3 matches:
- // 1. entire string
- // 2. category
- // 3. message
- if (matches.size() != 3) {
- LLDB_LOGF(GetLog(LLDBLog::Unwind),
- "Unexpected function name format. Expected '<trap prefix>$<trap "
- "category>$<trap message>'$ but got: '%s'.",
- func_name.data());
-
+ auto MaybeTrapReason =
+ clang::CodeGen::DemangleTrapReasonInDebugInfo(func_name);
+ if (!MaybeTrapReason.has_value()) {
+ LLDB_LOGF(GetLog(LLDBLog::Unwind), "Failed to demangle '%s' as trap reason",
+ func_name.str().c_str());
return {};
}
-
- auto category = matches[1];
- auto message = matches[2];
+ auto [category, message] = MaybeTrapReason.value();
std::string stop_reason =
category.empty() ? "<empty category>" : category.str();
|
6aff8fc to
c5ea9f5
Compare
| /// Demangle the artificial function name (\param FuncName) used to encode trap | ||
| /// reasons used in debug info for traps (e.g. __builtin_verbose_trap). See | ||
| /// `CGDebugInfo::CreateTrapFailureMessageFor`. | ||
| /// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moving this into Clang makes sense to me, thanks!
(approved by accident), left some questions
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
…me (#166157) #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 #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.
…nguageRuntime (#166157) llvm/llvm-project#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/llvm-project#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.
…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)
This patch refactors the trap reason demangling logic in `lldb_private::VerboseTrapFrameRecognizer::RecognizeFrame` into a new public function `clang::CodeGen::DemangleTrapReasonInDebugInfo`. There are two reasons for doing this: 1. In a future patch the logic for demangling needs to be used somewhere else in LLDB and thus the logic needs refactoring to avoid duplicating code. 2. The logic for demangling shouldn't really be in LLDB anyway because it's a Clang implementation detail and thus the logic really belongs inside Clang, not LLDB. Unit tests have been added for the new function that demonstrate how to use the new API. The function names recognized by VerboseTrapFrameRecognizer are identical to before. However, this patch isn't NFC because: * The `lldbTarget` library now links against `clangCodeGen` which it didn't previously. * The LLDB logging output is a little different now. The previous code tried to log failures for an invalid regex pattern and for the `Regex::match` API not returning the correct number of matches. These failure conditions are unreachable via unit testing so they have been made assertions failures inside the `DemangleTrapReasonInDebugInfo` implementation instead of trying to log them in LLDB. rdar://163230807
c5ea9f5 to
57c0952
Compare
|
@Michael137 I've rebased this patch on #166157 |
| llvm::Regex(llvm::formatv("^{0}\\$(.*)\\$(.*)$", ClangTrapPrefix).str()); | ||
| SmallVector<llvm::StringRef, 3> matches; | ||
| std::string regex_err_msg; | ||
| if (!trap_regex.match(func_name, &matches, ®ex_err_msg)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Looks like this error handling was bogus
Michael137
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the rebased version, thanks!
…ang (llvm#165996) This patch refactors the trap reason demangling logic in `lldb_private::VerboseTrapFrameRecognizer::RecognizeFrame` into a new public function `clang::CodeGen::DemangleTrapReasonInDebugInfo`. There are two reasons for doing this: 1. In a future patch the logic for demangling needs to be used somewhere else in LLDB and thus the logic needs refactoring to avoid duplicating code. 2. The logic for demangling shouldn't really be in LLDB anyway because it's a Clang implementation detail and thus the logic really belongs inside Clang, not LLDB. Unit tests have been added for the new function that demonstrate how to use the new API. The function names recognized by VerboseTrapFrameRecognizer are identical to before. However, this patch isn't NFC because: * The `lldbTarget` library now links against `clangCodeGen` which it didn't previously. * The LLDB logging output is a little different now. The previous code tried to log failures for an invalid regex pattern and for the `Regex::match` API not returning the correct number of matches. These failure conditions are unreachable via unit testing so they have been made assertions failures inside the `DemangleTrapReasonInDebugInfo` implementation instead of trying to log them in LLDB. rdar://163230807 (cherry picked from commit 3ebed51)
…efactor-cherry-pick-stable-21.x 🍒 [Clang][LLDB] Refactor trap reason demangling out of LLDB and into Clang (llvm#165996)
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/19006 Here is the relevant piece of the build log for the reference |
This patch refactors the trap reason demangling logic in
lldb_private::VerboseTrapFrameRecognizer::RecognizeFrameinto a new public functionclang::CodeGen::DemangleTrapReasonInDebugInfo.There are two reasons for doing this:
Unit tests have been added for the new function that demonstrate how to use the new API.
The function names recognized by VerboseTrapFrameRecognizer are identical to before. However, this patch isn't NFC because:
lldbTargetlibrary now links againstclangCodeGenwhich it didn't previously.Regex::matchAPI not returning the correct number of matches. These failure conditions are unreachable via unit testing so they have been made assertions failures inside theDemangleTrapReasonInDebugInfoimplementation instead of trying to log them in LLDB.rdar://163230807