Page MenuHomePhabricator

When there are variable errors, display an error in VS Code's local variables view.
ClosedPublic

Authored by clayborg on Sep 20 2022, 9:02 PM.

Details

Summary

After recent diffs that enable variable errors that stop variables from being correctly displayed when debugging, allow users to see these errors in the LOCALS variables in the VS Code UI. We do this by detecting when no variables are available and when there is an error to be displayed, and we add a single variable named "<error>" whose value is a string error that the user can read. This allows the user to be aware of the reason variables are not available and fix the issue. Previously if someone enabled "-gline-tables-only" or was debugging with DWARF in .o files or with .dwo files and those separate object files were missing or they were out of date, the user would see nothing in the variables view. Communicating these errors to the user is essential to a good debugging experience.

Diff Detail

Event Timeline

clayborg created this revision.Sep 20 2022, 9:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 9:02 PM
clayborg requested review of this revision.Sep 20 2022, 9:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 9:02 PM

Here is a screenshot showing this happening when a .o file is missing on Darwin for DWARF in .o file debugging.

Do we actually promise that strings returned by the SB API will live forever? That's not something *I* would want to promote. I always though that we're ConstStringifying the return values only when we don't know any better...

Do we actually promise that strings returned by the SB API will live forever? That's not something *I* would want to promote. I always though that we're ConstStringifying the return values only when we don't know any better...

Anything that does return a "const char *" should currently live forever, at least that is what we have been doing so far. We don't want anyone thinking they should free the pointer. The issue here is we were previously handing out a "Status::m_string.c_str()" pointer whose lifespan used to be tied to the SBError's lifespan, but we have a lot of APIs that return a SBError instance. So if you did code like:

value_list.GetError().GetCString()

the SBValueList::GetError() would return a SBError instance and it would immediately destruct itself and the "GetCString()" which was returning a "Status::m_string.c_str()" is now invalid. This patch in fact didn't work without fixing this issue in our public API. It would get an empty string as an error. And made our APIs less useful. So seeing as we have the SBError::GetCString() API, and we want it to work as expected for people, we really should be fixing this. Or we should add a new API like:

size_t SBError::GetErrorString(char *buffer, size_t buffer_size);

So that the temp objects can be use to safely grab the string. But seeing as we already have the GEtCString() API, I am guessing that are probably latent bugs right now due to the unexpectedly short lifespan of the string.

Our other option is to add more headerdoc to the SBError.h file and deal with this API issue by stating that the SBError object must be kept alive long enough to copy the error string returned by "const char *SBError::GetCString()". I am ok with either, I just think that it is currently safer and more consistent with all of our other APIs that return "const char *" since they all currently return a ConstString copy that will never go away,

Do we actually promise that strings returned by the SB API will live forever? That's not something *I* would want to promote. I always though that we're ConstStringifying the return values only when we don't know any better...

Anything that does return a "const char *" should currently live forever, at least that is what we have been doing so far.

I know there have been patches recently which changed some functions to return constified strings, but my impression is that things did not start out that way. From a cursory search I can find a number of (fairly old) APIs that return strings whose lifetime is less than "forever": SBBreakpoint::GetCondition, SBBreakpoint::GetThreadName, SBData::GetString, SBFrame::Disassemble, ... The constifying changes I remember were usually tied to us changing some const char *s into StringRefs internally, which mean we could no longer forward the string at the SB level (as it might not be null terminated). Constructing a temporary string was not possible because it would destruct before the function returns.

However, that's not the case here. The strings lifetime is the same as the enclosing SBError object, and I don't think it's unusual for c++ objects to be handing out pointers to parts of themselves. In this case, one could write the code in question as something like:

if (SBError err = top_scope->GetError()) {
  do_stuff(err.GetCString());
}

We don't want anyone thinking they should free the pointer.

I agree with that, and I am not aware of any situation where we do that.

The issue here is we were previously handing out a "Status::m_string.c_str()" pointer whose lifespan used to be tied to the SBError's lifespan, but we have a lot of APIs that return a SBError instance. So if you did code like:

So that the temp objects can be use to safely grab the string. But seeing as we already have the GEtCString() API, I am guessing that are probably latent bugs right now due to the unexpectedly short lifespan of the string.

One can use the a temporary object to grab the string, though it is somewhat complicated by the fact that the result can be null. You need some wrapper like:

optional<string> to_optional_string(const char *s) { if (s) return string(s); else return nullopt; }

...
auto s = to_optional_string(get_temporary_error().GetCString());

Without the null values, a plain string assignment would work fine.

I just did a search through our test sources and we use GetError().GetCString() 34 times in our test suites python files. So the SBError::GetCString() is being misused a lot like this already and is probably making some tests flaky. I would highly recommend we fix SBError::GetCString() to make sure things work correctly. If people are already doing this, or if they can do this with our API, then we should make our API as stable as possible for users even if it costs a bit more memory in our string pools.

jingham added a subscriber: jingham.EditedSep 23 2022, 2:44 PM

I just did a search through our test sources and we use GetError().GetCString() 34 times in our test suites python files. So the SBError::GetCString() is being misused a lot like this already and is probably making some tests flaky. I would highly recommend we fix SBError::GetCString() to make sure things work correctly. If people are already doing this, or if they can do this with our API, then we should make our API as stable as possible for users even if it costs a bit more memory in our string pools.

When we return Python strings from the SWIG'ed version of GetError().GetCString() does the Python string we make & return copy the string data, or is it just holding onto the pointer? In C/C++ if someone returns you a char * unless explicitly specified you assume that string is only going to live as long as the object that handed it out. But a python string is generally self-contained, so you wouldn't expect it to lose its data when the generating object goes away. I haven't checked yet, but if this is how SWIG handles making python strings from C strings, then this wouldn't be an error in the testsuite, only in C++ code. If that's not how it works, that might be a good way to finesse the issue.

I just did a search through our test sources and we use GetError().GetCString() 34 times in our test suites python files. So the SBError::GetCString() is being misused a lot like this already and is probably making some tests flaky. I would highly recommend we fix SBError::GetCString() to make sure things work correctly. If people are already doing this, or if they can do this with our API, then we should make our API as stable as possible for users even if it costs a bit more memory in our string pools.

When we return Python strings from the SWIG'ed version of GetError().GetCString() does the Python string we make & return copy the string data, or is it just holding onto the pointer? In C/C++ if someone returns you a char * unless explicitly specified you assume that string is only going to live as long as the object that handed it out. But a python string is generally self-contained, so you wouldn't expect it to lose its data when the generating object goes away. I haven't checked yet, but if this is how SWIG handles making python strings from C strings, then this wouldn't be an error in the testsuite, only in C++ code. If that's not how it works, that might be a good way to finesse the issue.

The main issue is people are already using this API this way. We can fix this by adding documentation and saying "don't do that", but that will lead to bugs and our API appearing flaky. Many strings we return have no lifetime issues due to string constification. We can't really even stop people from doing this by making sure the string is emptied when the object destructs because another object could come and live in its place quickly on the stack and we could be using a bogus data as the error string.

All that said, I am fine going with what the majority of people want on this. I will remove this issue from this patch so it doesn't hold up this patch.

clayborg updated this revision to Diff 463102.Sep 26 2022, 11:44 PM

Don't constify SBError::GetCString anymore. We can fix this in another patch.

The vscode change seems fine to me, but I don't consider myself an authority on that.

I just did a search through our test sources and we use GetError().GetCString() 34 times in our test suites python files. So the SBError::GetCString() is being misused a lot like this already and is probably making some tests flaky. I would highly recommend we fix SBError::GetCString() to make sure things work correctly. If people are already doing this, or if they can do this with our API, then we should make our API as stable as possible for users even if it costs a bit more memory in our string pools.

When we return Python strings from the SWIG'ed version of GetError().GetCString() does the Python string we make & return copy the string data, or is it just holding onto the pointer? In C/C++ if someone returns you a char * unless explicitly specified you assume that string is only going to live as long as the object that handed it out. But a python string is generally self-contained, so you wouldn't expect it to lose its data when the generating object goes away. I haven't checked yet, but if this is how SWIG handles making python strings from C strings, then this wouldn't be an error in the testsuite, only in C++ code. If that's not how it works, that might be a good way to finesse the issue.

The python version is fine because swig SBError::GetCString wrapper gets essentially a pointer to the SBError objects (wrapped as PyObject), and returns another PyObject with the (copied) string (also a PyObject). All the lifetime management happens outside of that function.

lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
48–53
yinghuitan accepted this revision.Sep 27 2022, 11:53 AM

LGTM pending Pavel's suggestion.

This revision is now accepted and ready to land.Sep 27 2022, 11:53 AM
thakis added a subscriber: thakis.Sep 28 2022, 4:07 PM

This breaks building everywhere:
http://45.33.8.238/linux/87654/step_4.txt
http://45.33.8.238/macm1/45381/step_4.txt
http://45.33.8.238/win/67047/step_4.txt

../../lldb/tools/lldb-vscode/lldb-vscode.cpp(2963,7): error: unknown type name 'SBError'; did you mean 'lldb::SBError'?
      SBError error = top_scope->GetError();
      ^~~~~~~
      lldb::SBError
../../lldb/include\lldb/API/SBError.h(20,16): note: 'lldb::SBError' declared here
class LLDB_API SBError {
               ^
1 error generated.

Please take a look, and revert for now if it takes a while to fix.

clayborg reopened this revision.Sep 28 2022, 5:09 PM

reopening to fix simple namespace issue...

This revision is now accepted and ready to land.Sep 28 2022, 5:09 PM
clayborg updated this revision to Diff 463724.Sep 28 2022, 5:23 PM

Added needed lldb:: namespace on the SBError.

yinghuitan accepted this revision.Sep 28 2022, 9:37 PM