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

[clayborg]

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.

[clayborg]

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

[labath]

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...

[clayborg]

In D134333#3805945, @labath wrote:

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.

[clayborg]

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,

[labath]

In D134333#3807511, @clayborg wrote:

In D134333#3805945, @labath wrote:

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.

[clayborg]

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]

In D134333#3812448, @clayborg wrote:

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.

[clayborg]

In D134333#3812653, @jingham wrote:

In D134333#3812448, @clayborg wrote:

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]

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

[labath]

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

In D134333#3812653, @jingham wrote:

In D134333#3812448, @clayborg wrote:

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]

LGTM pending Pavel's suggestion.

[thakis]

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.

[thakis]

Reverted in dcb94010eb8f0110c593a41e90b01365b45795e6 for now.

[clayborg]

reopening to fix simple namespace issue...

[clayborg]

Added needed lldb:: namespace on the SBError.

lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py

 """
 Test lldb-vscode setBreakpoints request
 """
 
 import vscode
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 import lldbvscode_testcase
-
+import os
 
 def make_buffer_verify_dict(start_idx, count, offset=0):
     verify_dict = {}
     for i in range(start_idx, start_idx + count):
         verify_dict['[%i]' % (i)] = {'type': 'int', 'value': str(i+offset)}
     return verify_dict
 
 
def verify_values(self, verify_dict, actual, varref_dict=None, expression=None):
                 verify_value = verify[key]
                 actual_value = actual[key]
                 startswith = actual_value.startswith(verify_value)
                 self.assertTrue(startswith,
                                 ('"%s" value "%s" doesn\'t start with'
                                  ' "%s")') % (
                                     key, actual_value,
                                     verify_value))
+        if 'contains' in verify_dict:
+            verify = verify_dict['contains']
+            for key in verify:
+                contains_array = verify[key]
+                actual_value = actual[key]
+                self.assertTrue(isinstance(contains_array, list))
+                for verify_value in contains_array:
+                    self.assertIn(verify_value, actual_value)
         if 'missing' in verify_dict:
             missing = verify_dict['missing']
             for key in missing:
                 self.assertTrue(key not in actual,
                                 'key "%s" is not expected in %s' % (key, actual))
         hasVariablesReference = 'variablesReference' in actual
         varRef = None
         if hasVariablesReference:
             # Remember variable references in case we want to test further
             # by using the evaluate name.
             varRef = actual["variablesReference"]
             if varRef != 0 and varref_dict is not None:
                 if expression is None:
def test_registers(self):
                 varRef = reg_set['variablesReference']
                 regs = self.vscode.request_variables(varRef)['body']['variables']
                 for reg in regs:
                     if reg['name'] == pc_name:
                         value = reg['value']
                         self.assertTrue(value.startswith('0x'))
                         self.assertTrue('a.out`main + ' in value)
                         self.assertTrue('at main.cpp:' in value)
+
+    @no_debug_info_test
+    @skipUnlessDarwin
+    def test_darwin_dwarf_missing_obj(self):
+        '''
+            Test that if we build a binary with DWARF in .o files and we remove
+            the .o file for main.cpp, that we get a variable named "<error>"
+            whose value matches the appriopriate error. Errors when getting
+            variables are returned in the LLDB API when the user should be
+            notified of issues that can easily be solved by rebuilding or
+            changing compiler options and are designed to give better feedback
+            to the user.
+        '''
+        self.build(debug_info="dwarf")
+        program = self.getBuildArtifact("a.out")
+        main_obj = self.getBuildArtifact("main.o")
+        self.assertTrue(os.path.exists(main_obj))
+        # Delete the main.o file that contains the debug info so we force an
+        # error when we run to main and try to get variables
+        os.unlink(main_obj)
+
+        self.create_debug_adaptor()
+        self.assertTrue(os.path.exists(program), 'executable must exist')
+        self.launch(program)
+
+        functions = ['main']
+        breakpoint_ids = self.set_function_breakpoints(functions)
+        self.assertEquals(len(breakpoint_ids), len(functions), "expect one breakpoint")
+        self.continue_to_breakpoints(breakpoint_ids)
+
+        locals = self.vscode.get_local_variables()
+
+        verify_locals = {
+            '<error>': {
+                'equals': {'type': 'const char *'},
+                'contains': { 'value': [
+                    'debug map object file ',
+                    'main.o" containing debug info does not exist, debug info will not be loaded']
+                }
+            },
+        }
+        varref_dict = {}
+        self.verify_variables(verify_locals, locals, varref_dict)

lldb/tools/lldb-vscode/lldb-vscode.cpp

if (variablesReference == VARREF_REGS) {
         if (format == lldb::eFormatDefault || format == lldb::eFormatHex) {
           if (reg.GetByteSize() == addr_size)
             reg.SetFormat(lldb::eFormatAddressInfo);
         }
       }
     }
 
     num_children = top_scope->GetSize();
+    if (num_children == 0 && variablesReference == VARREF_LOCALS) {
+      // Check for an error in the SBValueList that might explain why we don't
+      // have locals. If we have an error display it as the sole value in the
+      // the locals.
+
+      // "error" owns the error string so we must keep it alive as long as we
+      // want to use the returns "const char *"
+      lldb::SBError error = top_scope->GetError();
+      const char *var_err = error.GetCString();
+      if (var_err) {
+        // Create a fake variable named "error" to explain why variables were
+        // not available. This new error will help let users know when there was
+        // a problem that kept variables from being available for display and
+        // allow users to fix this issue instead of seeing no variables. The
+        // errors are only set when there is a problem that the user could
+        // fix, so no error will show up when you have no debug info, only when
+        // we do have debug info and something that is fixable can be done.
+        llvm::json::Object object;
+        EmplaceSafeString(object, "name", "<error>");
+        EmplaceSafeString(object, "type", "const char *");
+        EmplaceSafeString(object, "value", var_err);
+        object.try_emplace("variablesReference", (int64_t)0);
+        variables.emplace_back(std::move(object));
+      }
+    }
     const int64_t end_idx = start_idx + ((count == 0) ? num_children : count);
 
     // We first find out which variable names are duplicated
     std::map<std::string, int> variable_name_counts;
     for (auto i = start_idx; i < end_idx; ++i) {
       lldb::SBValue variable = top_scope->GetValueAtIndex(i);
       if (!variable.IsValid())
         break;