Page MenuHomePhabricator

Show error message for optimized variables
ClosedPublic

Authored by yinghuitan on May 19 2022, 1:27 PM.

Details

Summary

This fixes an issue that optimized variable error message is not shown to end
users in lldb-vscode.

Diff Detail

Event Timeline

yinghuitan created this revision.May 19 2022, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 1:27 PM
yinghuitan requested review of this revision.May 19 2022, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 1:27 PM
clayborg accepted this revision.May 19 2022, 2:01 PM

A little background: we have people that end up debugging optimized code and when a variable's SBValue had an error, we wouldn't show anything in the variables window for the value.

This revision is now accepted and ready to land.May 19 2022, 2:01 PM
This revision was landed with ongoing or failed builds.May 23 2022, 10:05 AM
This revision was automatically updated to reflect the committed changes.
jgorbe added a subscriber: jgorbe.Oct 26 2022, 5:04 PM

Hi! I've just debugged an issue that brought me to this commit. I'll start preparing a patch tomorrow (not sure how to test it yet) but since it's a recent-ish change I figured I'd ping the commit thread to give you a heads up.

v.GetSummary() returns a char* that is backed by a member std::string m_summary_str in SBValue, and we put the result in a StringRef that just points there. I've found that in some cases the call to v.GetError() invalidates the cached summary and causes the m_summary_str member to be cleared.

So, if we have a summary provider that returns "Summary string", it ends up being displayed as "<NUL byte>ummary string", because clearing m_summary_str sets the first byte to 0 but we are still holding on to a StringRef with the old length.

Calling v.GetError() first seems to avoid the issue. Another option to fix it would be to keep the summary in a local std::string instead. Any preference?

Hi! I've just debugged an issue that brought me to this commit. I'll start preparing a patch tomorrow (not sure how to test it yet) but since it's a recent-ish change I figured I'd ping the commit thread to give you a heads up.

v.GetSummary() returns a char* that is backed by a member std::string m_summary_str in SBValue, and we put the result in a StringRef that just points there. I've found that in some cases the call to v.GetError() invalidates the cached summary and causes the m_summary_str member to be cleared.

So, if we have a summary provider that returns "Summary string", it ends up being displayed as "<NUL byte>ummary string", because clearing m_summary_str sets the first byte to 0 but we are still holding on to a StringRef with the old length.

Calling v.GetError() first seems to avoid the issue. Another option to fix it would be to keep the summary in a local std::string instead. Any preference?

I would vote that we modify our lldb::SBValue to not clear the cached m_summary_str when it is called. Doesn't make any sense for it to do this. Since we are returning "const char *" from the SBValue object methods, those strings need to live for the lifetime of the object and any methods on SBValue shouldn't be causing others to be cleared, unless someone holds onto a SBValue object and it actually gets updated due to a process resuming and then stopping.

See inlined code suggestion as a way to fix this a bit more safely.

If you want to use std::string objects you have to watch our for NULL being returned for any "const char *". If NULL is returned the std::string constructor will crash lldb-vscode.

lldb/tools/lldb-vscode/JSONUtils.cpp
136–156

Another way to fix this is to re-org the code a bit. We don't need the value, summary or typename if there is an error.

jgorbe added inline comments.Oct 27 2022, 2:56 PM
lldb/tools/lldb-vscode/JSONUtils.cpp
136–156

Going with this for now in https://reviews.llvm.org/D136890.