[Clang][LLDB] Refactor trap reason demangling out of LLDB and into Clang #165996
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
delcypher requested review from Michael137, adrian-prantl, ahatanak, hnrklssn, patrykstefanski and rapidsna 
llvmbot added clang 
Member
| @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(); |
delcypher added the clang:bounds-safety delcypher force-pushed the dliew/demangle-trap-reason-refactor branch from 6aff8fc to c5ea9f5 Compare 
Michael137 previously approved these changes Nov 1, 2025
| /// 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`. | ||
| /// |
Member
There was a problem hiding this comment.
moving this into Clang makes sense to me, thanks!
Michael137 dismissed their stale review (approved by accident), left some questions
Michael137 reviewed Nov 1, 2025
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Nov 3, 2025
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
Michael137 added a commit that referenced this pull request Nov 4, 2025
…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.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Nov 4, 2025
…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.
Michael137 added a commit to swiftlang/llvm-project that referenced this pull request Nov 4, 2025
…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
delcypher force-pushed the dliew/demangle-trap-reason-refactor branch from c5ea9f5 to 57c0952 Compare Contributor Author
| @Michael137 I've rebased this patch on #166157 |
Michael137 reviewed Nov 5, 2025
| 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)) { |
Member
There was a problem hiding this comment.
LGTM
Looks like this error handling was bogus
Michael137 approved these changes Nov 5, 2025
Member
Michael137 left a comment
Michael137 left a comment There was a problem hiding this comment.
LGTM with the rebased version, thanks!
delcypher added a commit to swiftlang/llvm-project that referenced this pull request Nov 5, 2025
…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)
kavon added a commit to swiftlang/llvm-project that referenced this pull request Nov 5, 2025
…efactor-cherry-pick-stable-21.x 🍒 [Clang][LLDB] Refactor trap reason demangling out of LLDB and into Clang (llvm#165996)
Collaborator
| 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 |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
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