Override CalculateFrameVariableError in SymbolFileOnDemand

[GeorgeHuyubo]

Summary

Override CalculateFrameVariableError function in SymbolFileOnDemand.
Implement it by calling the same function of m_sym_file_impl, so that the wrapped symfile's error can be exposed.
This will enable the vscode debug session (which has the symbol ondemand enabled by default) to show error in the LOCALS variable section when there is error.

This piece of code was added for symbol on demand feature(https://reviews.llvm.org/rG7b81192d462bbd8031d5c665e29cd6b4c0c6887a). And missing overridden CalculateFrameVariableError is causing the error in symfile not being able to be exposed.

Added unit test that would fail before this patch, and now is succeeding.

[kusmour]

It'd be great to put some background in the summary.

The issue is introduced when adding Symbol OnDemand feature. We added a wrapper that ignore the error. And this affected the ability to populate the error when we have this feature turned on :D

(something like this, would be nice to link back to the symbol OnDemand patch as well)

[yinghuitan]

lldb/include/lldb/Symbol/SymbolFileOnDemand.h

121

Please add a unit test which should fail in symbol on-demand mode but succeeds after your patch. Checkout Greg's original test checking frame variable error as example.

lldb/source/Symbol/SymbolFileOnDemand.cpp

277–279

You should only forward to underlying m_sym_file_impl->CalculateFrameVariableError() when m_debug_info_enabled is true otherwise it will trigger parsing of debug info. Follow the pattern in other methods of this file.

[GeorgeHuyubo]

Address comments.

[clayborg]

Just have the case when debug info is not enabled return "Status()" (a default constructed error with no error) and this will be good to go.

lldb/source/Symbol/SymbolFileOnDemand.cpp

281

This will create an infinite loop right? See code change suggestions for fix.

[clayborg]

The refactoring comment should be done as well for this diff as suggested.

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

572–574

If this function is just like the test_darwin_dwarf_missing_obj, I would suggest refactoring test_darwin_dwarf_missing_obj into a function that doesn't start with "test", and then having both test_darwin_dwarf_missing_obj and test_darwin_dwarf_missing_obj_with_symbol_ondemand_enabled call it.

So copy the "test_darwin_dwarf_missing_obj" function and name it "darwin_dwarf_missing_obj" and add a new parameter named "initCommands":

def darwin_dwarf_missing_obj(self, initCommands):

Then copy the code from this function into that function, and replace the remove the line:

initCommands = ['settings set symbols.load-on-demand true']

Then just modify the code for both tests to call the refactored function

@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.
    '''
    darwin_dwarf_missing_obj(self, None)

@no_debug_info_test
@skipUnlessDarwin
def test_darwin_dwarf_missing_obj_with_symbol_ondemand_enabled(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. Test with symbol_ondemand_enabled.
    '''
    darwin_dwarf_missing_obj(self, ['settings set symbols.load-on-demand true'])

Any method on a test suite class that doesn't start with "test" is just a help function.

[GeorgeHuyubo]

Addressed comments.

[clayborg]

Looks good!

lldb/include/lldb/Symbol/SymbolFileOnDemand.h

public:
 
   void
   ParseDeclsForContext(lldb_private::CompilerDeclContext decl_ctx) override;
 
   uint32_t ResolveSymbolContext(const lldb_private::Address &so_addr,
                                 lldb::SymbolContextItem resolve_scope,
                                 lldb_private::SymbolContext &sc) override;
 
+  lldb_private::Status
+  CalculateFrameVariableError(lldb_private::StackFrame &frame) override;
+
   uint32_t ResolveSymbolContext(
       const lldb_private::SourceLocationSpec &src_location_spec,
       lldb::SymbolContextItem resolve_scope,
       lldb_private::SymbolContextList &sc_list) override;
 
   void Dump(lldb_private::Stream &s) override;
   void DumpClangAST(lldb_private::Stream &s) override;
 

lldb/source/Symbol/SymbolFileOnDemand.cpp

SymbolFileOnDemand::ResolveSymbolContext(const Address &so_addr,
   if (!m_debug_info_enabled) {
     LLDB_LOG(GetLog(), "[{0}] {1} is skipped", GetSymbolFileName(),
              __FUNCTION__);
     return 0;
   }
   return m_sym_file_impl->ResolveSymbolContext(so_addr, resolve_scope, sc);
 }
 
+Status SymbolFileOnDemand::CalculateFrameVariableError(StackFrame &frame) {
+  if (!m_debug_info_enabled) {
+    LLDB_LOG(GetLog(), "[{0}] {1} is skipped", GetSymbolFileName(),
+             __FUNCTION__);
+    return Status();
+  }
+  return m_sym_file_impl->CalculateFrameVariableError(frame);
+}
+
 uint32_t SymbolFileOnDemand::ResolveSymbolContext(
     const SourceLocationSpec &src_location_spec,
     SymbolContextItem resolve_scope, SymbolContextList &sc_list) {
   if (!m_debug_info_enabled) {
     LLDB_LOG(GetLog(), "[{0}] {1} is skipped", GetSymbolFileName(),
              __FUNCTION__);
     return 0;
   }

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

class TestVSCode_variables(lldbvscode_testcase.VSCodeTestCaseBase):
 
     def verify_variables(self, verify_dict, variables, varref_dict=None):
         for variable in variables:
             name = variable['name']
             self.assertIn(name, verify_dict,
                           'variable "%s" in verify dictionary' % (name))
             self.verify_values(verify_dict[name], variable, varref_dict)
 
+    def darwin_dwarf_missing_obj(self, initCommands):
+        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=program,
+                    initCommands=initCommands)
+
+        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)
+
     @skipIfWindows
     @skipIfRemote
     def test_scopes_variables_setVariable_evaluate(self):
         '''
             Tests the "scopes", "variables", "setVariable", and "evaluate"
             packets.
         '''
         program = self.getBuildArtifact("a.out")
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)
+        self.darwin_dwarf_missing_obj(None)
 
-        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)
+    @no_debug_info_test
+    @skipUnlessDarwin
+    def test_darwin_dwarf_missing_obj_with_symbol_ondemand_enabled(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. Test with symbol_ondemand_enabled.
+        '''
+        initCommands = ['settings set symbols.load-on-demand true']
+        self.darwin_dwarf_missing_obj(initCommands)