-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[lldb] Upgrade GetIndexOfChildWithName
to use llvm::Expected
#136693
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
Changes from 4 commits
f7d5ef8
db12ff1
4dda702
320301d
14b5250
86f52fa
a35fea8
8d13ebb
dcfe07b
443b930
8d14f51
8ce502e
ac7cf1f
3cf13ec
4699999
3db059d
23545b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -269,11 +269,15 @@ class VectorTypeSyntheticFrontEnd : public SyntheticChildrenFrontEnd { | |
return lldb::ChildCacheState::eRefetch; | ||
} | ||
|
||
size_t GetIndexOfChildWithName(ConstString name) override { | ||
llvm::Expected<size_t> GetIndexOfChildWithName(ConstString name) override { | ||
const char *item_name = name.GetCString(); | ||
uint32_t idx = ExtractIndexFromString(item_name); | ||
if (idx < UINT32_MAX && idx >= CalculateNumChildrenIgnoringErrors()) | ||
return UINT32_MAX; | ||
if (idx == UINT32_MAX || | ||
(idx < UINT32_MAX && idx >= CalculateNumChildrenIgnoringErrors())) | ||
return llvm::createStringError( | ||
"'SyntheticChildrenFrontEnd::VectorTypeSyntheticFrontEnd' cannot " | ||
"find index of child '%s'", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we also print the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type has no child named '%s' |
||
name.AsCString()); | ||
return idx; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -144,9 +144,12 @@ class BlockPointerSyntheticFrontEnd : public SyntheticChildrenFrontEnd { | |||||
return lldb::ChildCacheState::eRefetch; | ||||||
} | ||||||
|
||||||
size_t GetIndexOfChildWithName(ConstString name) override { | ||||||
llvm::Expected<size_t> GetIndexOfChildWithName(ConstString name) override { | ||||||
if (!m_block_struct_type.IsValid()) | ||||||
return UINT32_MAX; | ||||||
return llvm::createStringError( | ||||||
"'SyntheticChildrenFrontend::BlockPointerSyntheticFrontEnd' cannot " | ||||||
"find index of child '%s'", | ||||||
name.AsCString()); | ||||||
|
||||||
const bool omit_empty_base_classes = false; | ||||||
return m_block_struct_type.GetIndexOfChildWithName(name.AsCString(), | ||||||
|
@@ -172,8 +175,16 @@ bool lldb_private::formatters::BlockPointerSummaryProvider( | |||||
|
||||||
static const ConstString s_FuncPtr_name("__FuncPtr"); | ||||||
|
||||||
lldb::ValueObjectSP child_sp = synthetic_children->GetChildAtIndex( | ||||||
synthetic_children->GetIndexOfChildWithName(s_FuncPtr_name)); | ||||||
auto index_or_err = | ||||||
synthetic_children->GetIndexOfChildWithName(s_FuncPtr_name); | ||||||
|
||||||
if (!index_or_err) { | ||||||
LLDB_LOG_ERROR(GetLog(LLDBLog::Types), index_or_err.takeError(), "{0}"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return false; | ||||||
Michael137 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
|
||||||
lldb::ValueObjectSP child_sp = | ||||||
synthetic_children->GetChildAtIndex(*index_or_err); | ||||||
|
||||||
if (!child_sp) { | ||||||
return false; | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -309,13 +309,17 @@ lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::Update() { | |||||
return lldb::ChildCacheState::eRefetch; | ||||||
} | ||||||
|
||||||
size_t lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd:: | ||||||
llvm::Expected<size_t> | ||||||
lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd:: | ||||||
GetIndexOfChildWithName(ConstString name) { | ||||||
if (name == "__ptr_") | ||||||
return 0; | ||||||
if (name == "$$dereference$$") | ||||||
return 1; | ||||||
return UINT32_MAX; | ||||||
return llvm::createStringError( | ||||||
"'SyntheticChildrenFrontend::LibcxxSharedPtrSyntheticFrontEnd' cannot " | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
"find index of child '%s'", | ||||||
name.AsCString()); | ||||||
} | ||||||
|
||||||
lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd:: | ||||||
|
@@ -407,15 +411,19 @@ lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::Update() { | |||||
return lldb::ChildCacheState::eRefetch; | ||||||
} | ||||||
|
||||||
size_t lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd:: | ||||||
llvm::Expected<size_t> | ||||||
lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd:: | ||||||
GetIndexOfChildWithName(ConstString name) { | ||||||
if (name == "pointer") | ||||||
return 0; | ||||||
if (name == "deleter") | ||||||
return 1; | ||||||
if (name == "$$dereference$$") | ||||||
return 2; | ||||||
return UINT32_MAX; | ||||||
return llvm::createStringError( | ||||||
"'ScriptedSyntheticChildren::LibcxxUniquePtrSyntheticFrontEnd' cannot " | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
"find index of child '%s'", | ||||||
name.AsCString()); | ||||||
} | ||||||
|
||||||
bool lldb_private::formatters::LibcxxContainerSummaryProvider( | ||||||
|
@@ -456,9 +464,15 @@ ExtractLibcxxStringInfo(ValueObject &valobj) { | |||||
if (!l) | ||||||
return {}; | ||||||
|
||||||
StringLayout layout = l->GetIndexOfChildWithName("__data_") == 0 | ||||||
? StringLayout::DSC | ||||||
: StringLayout::CSD; | ||||||
auto index_or_err = l->GetIndexOfChildWithName("__data_"); | ||||||
if (!index_or_err) { | ||||||
LLDB_LOG_ERROR(GetLog(LLDBLog::DataFormatters), index_or_err.takeError(), | ||||||
"{0}"); | ||||||
return {}; | ||||||
} | ||||||
|
||||||
StringLayout layout = | ||||||
*index_or_err == 0 ? StringLayout::DSC : StringLayout::CSD; | ||||||
|
||||||
bool short_mode = false; // this means the string is in short-mode and the | ||||||
// data is stored inline | ||||||
|
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.
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.
I would suggest leaving out the function name entirely.
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.
I initially suggested it because that'd make it easier to identify where this was coming from if we ever decide to log it (or see it in user reports)
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.
@Michael137 My thinking is that these error messages are meant to be read by LLDB users, and the function name is a tool to aid in debugging LLDB itself. Users won't understand the meaning of the function name here, so we shouldn't have it in the user-facing error message. If this is useful information needed to debug LLDB, then we should log it instead.
Let me know if you think that's not a good idea!
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.
Hmm fair question. Adding logging to all these places feels a bit noisy. In my opinion its helpful to be as specific as possible, and if we wanted to present something more generic to the user eventually then we can change the error message at the point where we are about to present it. I'm not sure where these error messages will currently show up actually
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.
Currently the error is dropped silently. You wouldn't replace that with adding logging everywhere. My suggestion would be to:
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.
The error messages in the Expected return results of CompilerType are surfaced when formatting ValueObjects, for example.
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.
Hmm my main concern is that we end up getting user reports with a very generic error and then have no way to really tell where we went wrong. Then adding logging after-the-fact is a pretty slow turnaround. I guess usually these types of issues require a reproducer to debug anyway, so maybe this isn't a valid concern? Wdyt? I don't have a strong opinion on this. But it would be easier/more convenient to go from non-generic error back to generic error than the other way around if we really need to