Show error message for optimized variables

[yinghuitan]

Summary

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

[clayborg]

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.

[jgorbe]

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?

[clayborg]

In D126014#3886996, @jgorbe wrote:

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]

lldb/tools/lldb-vscode/JSONUtils.cpp

136–156

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

lldb/test/API/tools/lldb-vscode/optimized/TestVSCode_optimized.py

def test_stack_frame_name(self):
         breakpoint_ids = self.set_source_breakpoints(source, lines)
         self.assertEqual(len(breakpoint_ids), len(lines),
                         "expect correct number of breakpoints")
         self.continue_to_breakpoints(breakpoint_ids)
         leaf_frame = self.vscode.get_stackFrame(frameIndex=0)
         self.assertTrue(leaf_frame['name'].endswith(' [opt]'))
         parent_frame = self.vscode.get_stackFrame(frameIndex=1)
         self.assertTrue(parent_frame['name'].endswith(' [opt]'))
+
+    @skipIfWindows
+    @skipIfRemote
+    def test_optimized_variable(self):
+        ''' Test optimized variable value contains error.
+        '''
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        source = 'main.cpp'
+        breakpoint_line = line_number(source, '// breakpoint 2')
+        lines = [breakpoint_line]
+        # Set breakpoint in the thread function so we can step the threads
+        breakpoint_ids = self.set_source_breakpoints(source, lines)
+        self.assertEqual(len(breakpoint_ids), len(lines),
+                        "expect correct number of breakpoints")
+        self.continue_to_breakpoints(breakpoint_ids)
+        optimized_variable = self.vscode.get_local_variable('optimized')
+
+        self.assertTrue(optimized_variable['value'].startswith('<error:'))

lldb/tools/lldb-vscode/JSONUtils.cpp

for (const auto &value : *json_array) {
     }
   }
   return strs;
 }
 
 void SetValueForKey(lldb::SBValue &v, llvm::json::Object &object,
                     llvm::StringRef key) {
 
   llvm::StringRef value = v.GetValue();
   llvm::StringRef summary = v.GetSummary();
   llvm::StringRef type_name = v.GetType().GetDisplayTypeName();
+  lldb::SBError error = v.GetError();
 
   std::string result;
   llvm::raw_string_ostream strm(result);
-  if (!value.empty()) {
+  if (!error.Success()) {
+    strm << "<error: " << error.GetCString() << ">";
+  } else if (!value.empty()) {
     strm << value;
     if (!summary.empty())
       strm << ' ' << summary;
   } else if (!summary.empty()) {
     strm << ' ' << summary;
   } else if (!type_name.empty()) {
     strm << type_name;
     lldb::addr_t address = v.GetLoadAddress();
     if (address != LLDB_INVALID_ADDRESS)
       strm << " @ " << llvm::format_hex(address, 0);
   }
   strm.flush();
   EmplaceSafeString(object, key, result);
 }
 
 void FillResponse(const llvm::json::Object &request,
                   llvm::json::Object &response) {
   // Fill in all of the needed response fields to a "request" and set "success"
   // to true by default.