+142 −155
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
@llvm/pr-subscribers-lldb Author: Charles Zablit (charles-zablit) ChangesUpgrade the callees of This also bundles the logic of validating the demangled name and information into a single reusable function to reduce code duplication. Full diff: https://.com/llvm/llvm-project/pull/144731.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
index 0f18abb47591d..1810c07652a2b 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -236,199 +236,140 @@ static bool PrettyPrintFunctionNameWithArgs(Stream &out_stream,
return true;
}
-static std::optional<llvm::StringRef>
-GetDemangledBasename(const SymbolContext &sc) {
+static llvm::Expected<std::pair<llvm::StringRef, DemangledNameInfo>>
+GetAndValidateInfo(const SymbolContext &sc) {
Mangled mangled = sc.GetPossiblyInlinedFunctionName();
if (!mangled)
- return std::nullopt;
+ return llvm::createStringError("Function does not have a mangled name.");
auto demangled_name = mangled.GetDemangledName().GetStringRef();
if (demangled_name.empty())
- return std::nullopt;
+ return llvm::createStringError("Function does not have a demangled name.");
const std::optional<DemangledNameInfo> &info = mangled.GetDemangledInfo();
if (!info)
- return std::nullopt;
+ return llvm::createStringError("Function does not have demangled info.");
// Function without a basename is nonsense.
if (!info->hasBasename())
- return std::nullopt;
+ return llvm::createStringError("Info do not have basename range.");
- return demangled_name.slice(info->BasenameRange.first,
- info->BasenameRange.second);
+ return std::make_pair(demangled_name, *info);
}
-static std::optional<llvm::StringRef>
-GetDemangledTemplateArguments(const SymbolContext &sc) {
- Mangled mangled = sc.GetPossiblyInlinedFunctionName();
- if (!mangled)
- return std::nullopt;
+static llvm::Expected<llvm::StringRef>
+GetDemangledBasename(const SymbolContext &sc) {
+ auto info_or_err = GetAndValidateInfo(sc);
+ if (!info_or_err)
+ return info_or_err.takeError();
- auto demangled_name = mangled.GetDemangledName().GetStringRef();
- if (demangled_name.empty())
- return std::nullopt;
+ auto [demangled_name, info] = *info_or_err;
- const std::optional<DemangledNameInfo> &info = mangled.GetDemangledInfo();
- if (!info)
- return std::nullopt;
+ return demangled_name.slice(info.BasenameRange.first,
+ info.BasenameRange.second);
+}
- // Function without a basename is nonsense.
- if (!info->hasBasename())
- return std::nullopt;
+static llvm::Expected<llvm::StringRef>
+GetDemangledTemplateArguments(const SymbolContext &sc) {
+ auto info_or_err = GetAndValidateInfo(sc);
+ if (!info_or_err)
+ return info_or_err.takeError();
+
+ auto [demangled_name, info] = *info_or_err;
- if (info->ArgumentsRange.first < info->BasenameRange.second)
- return std::nullopt;
+ if (info.ArgumentsRange.first < info.BasenameRange.second)
+ return llvm::createStringError("Arguments in info are invalid.");
- return demangled_name.slice(info->BasenameRange.second,
- info->ArgumentsRange.first);
+ return demangled_name.slice(info.BasenameRange.second,
+ info.ArgumentsRange.first);
}
-static std::optional<llvm::StringRef>
+static llvm::Expected<llvm::StringRef>
GetDemangledReturnTypeLHS(const SymbolContext &sc) {
- Mangled mangled = sc.GetPossiblyInlinedFunctionName();
- if (!mangled)
- return std::nullopt;
+ auto info_or_err = GetAndValidateInfo(sc);
+ if (!info_or_err)
+ return info_or_err.takeError();
- auto demangled_name = mangled.GetDemangledName().GetStringRef();
- if (demangled_name.empty())
- return std::nullopt;
+ auto [demangled_name, info] = *info_or_err;
- const std::optional<DemangledNameInfo> &info = mangled.GetDemangledInfo();
- if (!info)
- return std::nullopt;
+ if (info.ScopeRange.first >= demangled_name.size())
+ return llvm::createStringError("Scope range is invalid.");
- // Function without a basename is nonsense.
- if (!info->hasBasename())
- return std::nullopt;
-
- if (info->ScopeRange.first >= demangled_name.size())
- return std::nullopt;
-
- return demangled_name.substr(0, info->ScopeRange.first);
+ return demangled_name.substr(0, info.ScopeRange.first);
}
-static std::optional<llvm::StringRef>
+static llvm::Expected<llvm::StringRef>
GetDemangledFunctionQualifiers(const SymbolContext &sc) {
- Mangled mangled = sc.GetPossiblyInlinedFunctionName();
- if (!mangled)
- return std::nullopt;
+ auto info_or_err = GetAndValidateInfo(sc);
+ if (!info_or_err)
+ return info_or_err.takeError();
- auto demangled_name = mangled.GetDemangledName().GetStringRef();
- if (demangled_name.empty())
- return std::nullopt;
+ auto [demangled_name, info] = *info_or_err;
- const std::optional<DemangledNameInfo> &info = mangled.GetDemangledInfo();
- if (!info)
- return std::nullopt;
-
- // Function without a basename is nonsense.
- if (!info->hasBasename())
- return std::nullopt;
+ if (info.QualifiersRange.second < info.QualifiersRange.first)
+ return llvm::createStringError("Qualifiers range is invalid.");
- if (info->QualifiersRange.second < info->QualifiersRange.first)
- return std::nullopt;
-
- return demangled_name.slice(info->QualifiersRange.first,
- info->QualifiersRange.second);
+ return demangled_name.slice(info.QualifiersRange.first,
+ info.QualifiersRange.second);
}
-static std::optional<llvm::StringRef>
+static llvm::Expected<llvm::StringRef>
GetDemangledReturnTypeRHS(const SymbolContext &sc) {
- Mangled mangled = sc.GetPossiblyInlinedFunctionName();
- if (!mangled)
- return std::nullopt;
-
- auto demangled_name = mangled.GetDemangledName().GetStringRef();
- if (demangled_name.empty())
- return std::nullopt;
+ auto info_or_err = GetAndValidateInfo(sc);
+ if (!info_or_err)
+ return info_or_err.takeError();
- const std::optional<DemangledNameInfo> &info = mangled.GetDemangledInfo();
- if (!info)
- return std::nullopt;
-
- // Function without a basename is nonsense.
- if (!info->hasBasename())
- return std::nullopt;
+ auto [demangled_name, info] = *info_or_err;
- if (info->QualifiersRange.first < info->ArgumentsRange.second)
- return std::nullopt;
+ if (info.QualifiersRange.first < info.ArgumentsRange.second)
+ return llvm::createStringError("Qualifiers range is invalid.");
- return demangled_name.slice(info->ArgumentsRange.second,
- info->QualifiersRange.first);
+ return demangled_name.slice(info.ArgumentsRange.second,
+ info.QualifiersRange.first);
}
-static std::optional<llvm::StringRef>
+static llvm::Expected<llvm::StringRef>
GetDemangledScope(const SymbolContext &sc) {
- Mangled mangled = sc.GetPossiblyInlinedFunctionName();
- if (!mangled)
- return std::nullopt;
-
- auto demangled_name = mangled.GetDemangledName().GetStringRef();
- if (demangled_name.empty())
- return std::nullopt;
+ auto info_or_err = GetAndValidateInfo(sc);
+ if (!info_or_err)
+ return info_or_err.takeError();
- const std::optional<DemangledNameInfo> &info = mangled.GetDemangledInfo();
- if (!info)
- return std::nullopt;
-
- // Function without a basename is nonsense.
- if (!info->hasBasename())
- return std::nullopt;
+ auto [demangled_name, info] = *info_or_err;
- if (info->ScopeRange.second < info->ScopeRange.first)
- return std::nullopt;
+ if (info.ScopeRange.second < info.ScopeRange.first)
+ return llvm::createStringError("Info do not have basename range.");
- return demangled_name.slice(info->ScopeRange.first, info->ScopeRange.second);
+ return demangled_name.slice(info.ScopeRange.first, info.ScopeRange.second);
}
/// Handles anything printed after the FunctionEncoding ItaniumDemangle
/// node. Most notably the DotSUffix node.
-static std::optional<llvm::StringRef>
+static llvm::Expected<llvm::StringRef>
GetDemangledFunctionSuffix(const SymbolContext &sc) {
- Mangled mangled = sc.GetPossiblyInlinedFunctionName();
- if (!mangled)
- return std::nullopt;
+ auto info_or_err = GetAndValidateInfo(sc);
+ if (!info_or_err)
+ return info_or_err.takeError();
- auto demangled_name = mangled.GetDemangledName().GetStringRef();
- if (demangled_name.empty())
- return std::nullopt;
+ auto [demangled_name, info] = *info_or_err;
- const std::optional<DemangledNameInfo> &info = mangled.GetDemangledInfo();
- if (!info)
- return std::nullopt;
-
- // Function without a basename is nonsense.
- if (!info->hasBasename())
- return std::nullopt;
-
- return demangled_name.slice(info->SuffixRange.first,
- info->SuffixRange.second);
+ return demangled_name.slice(info.SuffixRange.first, info.SuffixRange.second);
}
static bool PrintDemangledArgumentList(Stream &s, const SymbolContext &sc) {
assert(sc.symbol);
- Mangled mangled = sc.GetPossiblyInlinedFunctionName();
- if (!mangled)
- return false;
-
- auto demangled_name = mangled.GetDemangledName().GetStringRef();
- if (demangled_name.empty())
- return false;
-
- const std::optional<DemangledNameInfo> &info = mangled.GetDemangledInfo();
- if (!info)
- return false;
-
- // Function without a basename is nonsense.
- if (!info->hasBasename())
+ auto info_or_err = GetAndValidateInfo(sc);
+ if (!info_or_err) {
+ info_or_err.takeError();
return false;
+ }
+ auto [demangled_name, info] = *info_or_err;
- if (info->ArgumentsRange.second < info->ArgumentsRange.first)
+ if (info.ArgumentsRange.second < info.ArgumentsRange.first)
return false;
- s << demangled_name.slice(info->ArgumentsRange.first,
- info->ArgumentsRange.second);
+ s << demangled_name.slice(info.ArgumentsRange.first,
+ info.ArgumentsRange.second);
return true;
}
@@ -1954,32 +1895,37 @@ bool CPlusPlusLanguage::HandleFrameFormatVariable(
FormatEntity::Entry::Type type, Stream &s) {
switch (type) {
case FormatEntity::Entry::Type::FunctionScope: {
- std::optional<llvm::StringRef> scope = GetDemangledScope(sc);
- if (!scope)
+ auto scope_or_err = GetDemangledScope(sc);
+ if (!scope_or_err) {
+ llvm::consumeError(scope_or_err.takeError());
return false;
+ }
- s << *scope;
+ s << *scope_or_err;
return true;
}
case FormatEntity::Entry::Type::FunctionBasename: {
- std::optional<llvm::StringRef> name = GetDemangledBasename(sc);
- if (!name)
+ auto name_or_err = GetDemangledBasename(sc);
+ if (!name_or_err) {
+ llvm::consumeError(name_or_err.takeError());
return false;
+ }
- s << *name;
+ s << *name_or_err;
return true;
}
case FormatEntity::Entry::Type::FunctionTemplateArguments: {
- std::optional<llvm::StringRef> template_args =
- GetDemangledTemplateArguments(sc);
- if (!template_args)
+ auto template_args_or_err = GetDemangledTemplateArguments(sc);
+ if (!template_args_or_err) {
+ llvm::consumeError(template_args_or_err.takeError());
return false;
+ }
- s << *template_args;
+ s << *template_args_or_err;
return true;
}
@@ -2008,38 +1954,46 @@ bool CPlusPlusLanguage::HandleFrameFormatVariable(
return true;
}
case FormatEntity::Entry::Type::FunctionReturnRight: {
- std::optional<llvm::StringRef> return_rhs = GetDemangledReturnTypeRHS(sc);
- if (!return_rhs)
+ auto return_rhs_or_err = GetDemangledReturnTypeRHS(sc);
+ if (!return_rhs_or_err) {
+ llvm::consumeError(return_rhs_or_err.takeError());
return false;
+ }
- s << *return_rhs;
+ s << *return_rhs_or_err;
return true;
}
case FormatEntity::Entry::Type::FunctionReturnLeft: {
- std::optional<llvm::StringRef> return_lhs = GetDemangledReturnTypeLHS(sc);
- if (!return_lhs)
+ auto return_lhs_or_err = GetDemangledReturnTypeLHS(sc);
+ if (!return_lhs_or_err) {
+ llvm::consumeError(return_lhs_or_err.takeError());
return false;
+ }
- s << *return_lhs;
+ s << *return_lhs_or_err;
return true;
}
case FormatEntity::Entry::Type::FunctionQualifiers: {
- std::optional<llvm::StringRef> quals = GetDemangledFunctionQualifiers(sc);
- if (!quals)
+ auto quals_or_err = GetDemangledFunctionQualifiers(sc);
+ if (!quals_or_err) {
+ llvm::consumeError(quals_or_err.takeError());
return false;
+ }
- s << *quals;
+ s << *quals_or_err;
return true;
}
case FormatEntity::Entry::Type::FunctionSuffix: {
- std::optional<llvm::StringRef> suffix = GetDemangledFunctionSuffix(sc);
- if (!suffix)
+ auto suffix_or_err = GetDemangledFunctionSuffix(sc);
+ if (!suffix_or_err) {
+ llvm::consumeError(suffix_or_err.takeError());
return false;
+ }
- s << *suffix;
+ s << *suffix_or_err;
return true;
}
|
| if (!info->hasBasename()) | ||
| auto info_or_err = GetAndValidateInfo(sc); | ||
| if (!info_or_err) { | ||
| llvm::consumeError(info_or_err.takeError()); |
There was a problem hiding this comment.
Can we log here instead of consuming the error?
Something like:
Suggested change
| llvm::consumeError(info_or_err.takeError()); | |
| LLDB_LOG_ERROR(GetLog(LLDBLog::Language), info_or_err.takeError(), "Failed to retrieve demangled info: {0}" |
Might need to downgrade it to LLDB_LOG_ERRORV if we find that this is a common/unimportant error path
There was a problem hiding this comment.
Same applies to all the other places where we consumeError
There was a problem hiding this comment.
Fixed, thanks! LLDB_LOG_ERROR seems to be reasonable for now, I did not encounter an error message while testing on a small C++ program.
Uh oh!
There was an error while loading. Please reload this page.
| auto demangled_name = mangled.GetDemangledName().GetStringRef(); | ||
| if (demangled_name.empty()) | ||
| return std::nullopt; | ||
| return llvm::createStringError("Function does not have a demangled name."); |
There was a problem hiding this comment.
Suggested change
| return llvm::createStringError("Function does not have a demangled name."); | |
| return llvm::createStringError("Function '%s' does not have a demangled name.", mangled.GetMangledName().AsCString("")); |
There was a problem hiding this comment.
Added better messages for all the createStringError.
| const std::optional<DemangledNameInfo> &info = mangled.GetDemangledInfo(); | ||
| if (!info) | ||
| return std::nullopt; | ||
| return llvm::createStringError("Function does not have demangled info."); |
There was a problem hiding this comment.
Suggested change
| return llvm::createStringError("Function does not have demangled info."); | |
| return llvm::createStringError("Function '%s' does not have demangled info.", demangled_name.data()); |
| // Function without a basename is nonsense. | ||
| if (!info->hasBasename()) | ||
| return std::nullopt; | ||
| return llvm::createStringError("Info do not have basename range."); |
There was a problem hiding this comment.
Suggested change
| return llvm::createStringError("Info do not have basename range."); | |
| return llvm::createStringError("DemangledInfo for '%s' do not have basename range.", demangled_name.data()); |
| if (!info) | ||
| return std::nullopt; | ||
| if (info.ScopeRange.first >= demangled_name.size()) | ||
| return llvm::createStringError("Scope range is invalid."); |
There was a problem hiding this comment.
Suggested change
| return llvm::createStringError("Scope range is invalid."); | |
| return llvm::createStringError("Scope range for '%s' is invalid.", demangled_name.data()); |
There was a problem hiding this comment.
Can you similarly include the demangled_name in the error message elsewhere too?
| auto name_or_err = GetDemangledBasename(sc); | ||
| if (!name_or_err) { | ||
| LLDB_LOG_ERROR(GetLog(LLDBLog::Language), name_or_err.takeError(), | ||
| "Failed to retrieve basename: {0}"); |
There was a problem hiding this comment.
Suggested change
| "Failed to retrieve basename: {0}"); | |
| "Failed to handle ${function.basename} frame-format variable: {0}"); |
And do this for the other messages in this switch too?
There was a problem hiding this comment.
Added better messages for all the LLDB_LOG_ERROR.
I think we should wait before the following PR gets merged before merging, in order to use the new |
1af4c9e to 6d8e8c0 Compare
Done ✅ |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Michael137 approved these changes Jun 19, 2025
| if (!quals_or_err) { | ||
| LLDB_LOG_ERROR( | ||
| GetLog(LLDBLog::Language), quals_or_err.takeError(), | ||
| "Failed to handle ${function.qualifiers} frame-format variable: {0}"); |
There was a problem hiding this comment.
Apologies, I think you need to escape the { here. Since that's the syntax for specifying the format index. I think {{ is the way to do that
There was a problem hiding this comment.
No problem, thanks!
✅ With the latest revision this PR passed the C/C++ code formatter. |
| if (!info->hasBasename()) | ||
| return std::nullopt; | ||
| if (!info.hasQualifiers()) | ||
| return llvm::createStringError("Qualifiers range for '%s' is invalid.", |
There was a problem hiding this comment.
Oh, not having qualifiers on a demangled name doesn't necessarily mean this is fatal. This explains the latest linux failures. Some methods just don't have qualifiers. I guess we need to return "" in that case. Should hasQualifiers also accept cases where QualifiersRange.start == QualifiersRange.end? Which just means they don't exist, but didn't fail to parse. I'm also happy to just remove this check. I think that's why i used to only check for hasBasename.
Sign up for free to join this conversation on . Already have an account? Sign in to comment
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.
Upgrade the callees of
HandleFrameFormatVariable(GetDemangledTemplateArguments, etc), to return allvm::Expectedinstead of anstd::optional.This also bundles the logic of validating the demangled name and information into a single reusable function to reduce code duplication.