Track which modules have debug info variable errors.

[clayborg]

Summary

Now that we display an error when users try to get variables, but something in the debug info is preventing variables from showing up, track this with a new bool in each module's statistic information named "debugInfoHadVariableErrors".

This patch modifies the code to track when we have variable errors in a module and adds accessors to get/set this value. This value is used in the module statistics and we added a test to verify this value gets set correctly.

[JDevlieghere]

Instead of storing a bool, would it make sense to store the error instead and then have the statistics call isFail on it to determine whether there was an issue?

[clayborg]

In D134508#3818888, @JDevlieghere wrote:

Instead of storing a bool, would it make sense to store the error instead and then have the statistics call isFail on it to determine whether there was an issue?

We would need to store a list of unique error strings if we want to go that route since we might stop N times in M compile units and currently the errors are on the compile unit boundary. We wouldn't want just one string and if we stopped 12 times in a the same function we don't want 12 copies of the same string in the stats. So if we could use a

std::set<std::string> m_var_errors;

I'd be happy to go this route if anyone feels this would be useful. I just want to know when there are errors, but I also would love to have the exact errors to help us track down.

Please chime in everyone!

[clayborg]

I can do a follow up patch that enumerates the errors in the statistics if needed.

lldb/include/lldb/Symbol/SymbolFile.h

public:
   ///   address can be used if there is not debug info due to it not being able
   ///   to be loaded, or if there is a debug info context, like a compile unit,
   ///   or function, it can be used to track down more information on why
   ///   variables are missing.
   ///
   /// \returns
   ///   An error specifying why there should have been debug info with variable
   ///   information but the variables were not able to be resolved.
-  virtual Status GetFrameVariableError(StackFrame &frame) {
+  Status GetFrameVariableError(StackFrame &frame) {
+    Status err = CalculateFrameVariableError(frame);
+    if (err.Fail())
+      SetDebugInfoHadFrameVariableErrors();
+    return err;
+  }
+
+  /// Subclasses will override this function to for GetFrameVariableError().
+  ///
+  /// This allows GetFrameVariableError() to set the member variable
+  /// m_debug_info_had_variable_errors correctly without users having to do it
+  /// manually which is error prone.
+  virtual Status CalculateFrameVariableError(StackFrame &frame) {
     return Status();
   }
   virtual uint32_t
   ResolveSymbolContext(const SourceLocationSpec &src_location_spec,
                        lldb::SymbolContextItem resolve_scope,
                        SymbolContextList &sc_list);
 
   virtual void DumpClangAST(Stream &s) {}
public:
   /// can be accessed by the statistics and emitted to help track these costs.
   /// \{
   virtual bool GetDebugInfoIndexWasLoadedFromCache() const = 0;
   virtual void SetDebugInfoIndexWasLoadedFromCache() = 0;
   virtual bool GetDebugInfoIndexWasSavedToCache() const = 0;
   virtual void SetDebugInfoIndexWasSavedToCache() = 0;
   /// \}
 
+  /// Accessors for the bool that indicates if there was debug info, but errors
+  /// stopped variables from being able to be displayed correctly. See
+  /// GetFrameVariableError() for details on what are considered errors.
+  virtual bool GetDebugInfoHadFrameVariableErrors() const = 0;
+  virtual void SetDebugInfoHadFrameVariableErrors() = 0;
+
 protected:
   void AssertModuleLock();
 
 private:
   SymbolFile(const SymbolFile &) = delete;
   const SymbolFile &operator=(const SymbolFile &) = delete;
 };
 
void SetDebugInfoIndexWasLoadedFromCache() override {
     m_index_was_loaded_from_cache = true;
   }
   bool GetDebugInfoIndexWasSavedToCache() const override {
     return m_index_was_saved_to_cache;
   }
   void SetDebugInfoIndexWasSavedToCache() override {
     m_index_was_saved_to_cache = true;
   }
+  bool GetDebugInfoHadFrameVariableErrors() const override {
+    return m_debug_info_had_variable_errors;
+  }
+  void SetDebugInfoHadFrameVariableErrors() override {
+     m_debug_info_had_variable_errors = true;
+  }
 
 protected:
   virtual uint32_t CalculateNumCompileUnits() = 0;
   virtual lldb::CompUnitSP ParseCompileUnitAtIndex(uint32_t idx) = 0;
   virtual TypeList &GetTypeList() { return m_type_list; }
   void SetCompileUnitAtIndex(uint32_t idx, const lldb::CompUnitSP &cu_sp);
 
   lldb::ObjectFileSP m_objfile_sp; // Keep a reference to the object file in
                                    // case it isn't the same as the module
                                    // object file (debug symbols in a separate
                                    // file)
   llvm::Optional<std::vector<lldb::CompUnitSP>> m_compile_units;
   TypeList m_type_list;
   Symtab *m_symtab = nullptr;
   uint32_t m_abilities = 0;
   bool m_calculated_abilities = false;
   bool m_index_was_loaded_from_cache = false;
   bool m_index_was_saved_to_cache = false;
+  /// Set to true if any variable feteching errors have been found when calling
+  /// GetFrameVariableError(). This will be emitted in the "statistics dump"
+  /// information for a module.
+  bool m_debug_info_had_variable_errors = false;
 
 private:
   SymbolFileCommon(const SymbolFileCommon &) = delete;
   const SymbolFileCommon &operator=(const SymbolFileCommon &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_SYMBOL_SYMBOLFILE_H

lldb/include/lldb/Symbol/SymbolFileOnDemand.h

void SetDebugInfoIndexWasLoadedFromCache() override {
     m_sym_file_impl->SetDebugInfoIndexWasLoadedFromCache();
   }
   bool GetDebugInfoIndexWasSavedToCache() const override {
     return m_sym_file_impl->GetDebugInfoIndexWasSavedToCache();
   }
   void SetDebugInfoIndexWasSavedToCache() override {
     m_sym_file_impl->SetDebugInfoIndexWasSavedToCache();
   }
+  bool GetDebugInfoHadFrameVariableErrors() const override {
+    return m_sym_file_impl->GetDebugInfoHadFrameVariableErrors();
+  }
+  void SetDebugInfoHadFrameVariableErrors() override {
+    return m_sym_file_impl->SetDebugInfoHadFrameVariableErrors();
+  }
 
 private:
   Log *GetLog() const { return ::lldb_private::GetLog(LLDBLog::OnDemand); }
 
   ConstString GetSymbolFileName() {
     return GetObjectFile()->GetFileSpec().GetFilename();
   }
 
 private:
   bool m_debug_info_enabled = false;
   bool m_preload_symbols = false;
   std::unique_ptr<SymbolFile> m_sym_file_impl;
 };
 } // namespace lldb_private
 
 #endif // LLDB_SYMBOL_SYMBOLFILEONDEMAND_H

lldb/include/lldb/Target/Statistics.h

struct ModuleStats {
   double debug_index_time = 0.0;
   uint64_t debug_info_size = 0;
   bool symtab_loaded_from_cache = false;
   bool symtab_saved_to_cache = false;
   bool debug_info_index_loaded_from_cache = false;
   bool debug_info_index_saved_to_cache = false;
   bool debug_info_enabled = true;
   bool symtab_stripped = false;
+  bool debug_info_had_variable_errors = false;
 };
 
 struct ConstStringStats {
   llvm::json::Value ToJSON() const;
   ConstString::MemoryStats stats = ConstString::GetMemoryStats();
 };
 
 /// A class that represents statistics for a since lldb_private::Target.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.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
-  GetFrameVariableError(lldb_private::StackFrame &frame) override;
+  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
   FindGlobalVariables(lldb_private::ConstString name,

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

 }
 
 StatsDuration::Duration SymbolFileDWARF::GetDebugInfoIndexTime() {
   if (m_index)
     return m_index->GetIndexTime();
   return {};
 }
 
-Status SymbolFileDWARF::GetFrameVariableError(StackFrame &frame) {
+Status SymbolFileDWARF::CalculateFrameVariableError(StackFrame &frame) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   CompileUnit *cu = frame.GetSymbolContext(eSymbolContextCompUnit).comp_unit;
   if (!cu)
     return Status();
 
   DWARFCompileUnit *dwarf_cu = GetDWARFCompileUnit(cu);
   if (!dwarf_cu)
     return Status();

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h

uint32_t ResolveSymbolContext(const lldb_private::Address &so_addr,
                                 lldb::SymbolContextItem resolve_scope,
                                 lldb_private::SymbolContext &sc) override;
   uint32_t ResolveSymbolContext(
       const lldb_private::SourceLocationSpec &src_location_spec,
       lldb::SymbolContextItem resolve_scope,
       lldb_private::SymbolContextList &sc_list) override;
 
   lldb_private::Status
-  GetFrameVariableError(lldb_private::StackFrame &frame) override;
+  CalculateFrameVariableError(lldb_private::StackFrame &frame) override;
 
   void
   FindGlobalVariables(lldb_private::ConstString name,
                       const lldb_private::CompilerDeclContext &parent_decl_ctx,
                       uint32_t max_matches,
                       lldb_private::VariableList &variables) override;
   void FindGlobalVariables(const lldb_private::RegularExpression &regex,
                            uint32_t max_matches,

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp

if (oso_objfile) {
       if (module_sp)
         oso_modules.Append(module_sp);
     }
     return false; // Keep iterating
   });
   return oso_modules;
 }
 
-Status SymbolFileDWARFDebugMap::GetFrameVariableError(StackFrame &frame) {
+Status SymbolFileDWARFDebugMap::CalculateFrameVariableError(StackFrame &frame) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
 
   // We need to make sure that our PC value from the frame matches the module
   // for this object file since we will lookup the PC file address in the debug
   // map below.
   Address pc_addr = frame.GetFrameCodeAddress();
   if (pc_addr.GetModule() == m_objfile_sp->GetModule()) {
     Symtab *symtab = m_objfile_sp->GetSymtab();

lldb/source/Target/Statistics.cpp

json::Value ModuleStats::ToJSON() const {
   module.try_emplace("debugInfoParseTime", debug_parse_time);
   module.try_emplace("debugInfoIndexTime", debug_index_time);
   module.try_emplace("debugInfoByteSize", (int64_t)debug_info_size);
   module.try_emplace("debugInfoIndexLoadedFromCache",
                      debug_info_index_loaded_from_cache);
   module.try_emplace("debugInfoIndexSavedToCache",
                      debug_info_index_saved_to_cache);
   module.try_emplace("debugInfoEnabled", debug_info_enabled);
+  module.try_emplace("debugInfoHadVariableErrors",
+                     debug_info_had_variable_errors);
   module.try_emplace("symbolTableStripped", symtab_stripped);
   if (!symfile_path.empty())
     module.try_emplace("symbolFilePath", symfile_path);
 
   if (!symfile_modules.empty()) {
     json::Array symfile_ids;
     for (const auto symfile_id: symfile_modules)
       symfile_ids.emplace_back(symfile_id);
if (sym_file) {
       ModuleList symbol_modules = sym_file->GetDebugInfoModules();
       for (const auto &symbol_module: symbol_modules.Modules())
         module_stat.symfile_modules.push_back((intptr_t)symbol_module.get());
       module_stat.symtab_stripped = module->GetObjectFile()->IsStripped();
       if (module_stat.symtab_stripped)
         ++num_stripped_modules;
       module_stat.debug_info_enabled = sym_file->GetLoadDebugInfoEnabled() &&
                                        module_stat.debug_info_size > 0;
+      module_stat.debug_info_had_variable_errors =
+          sym_file->GetDebugInfoHadFrameVariableErrors();
       if (module_stat.debug_info_enabled)
         ++num_debug_info_enabled_modules;
       if (module_stat.debug_info_size > 0)
         ++num_modules_has_debug_info;
     }
     symtab_parse_time += module_stat.symtab_parse_time;
     symtab_index_time += module_stat.symtab_index_time;
     debug_parse_time += module_stat.debug_parse_time;

lldb/test/API/commands/statistics/basic/TestStats.py

def test_no_dsym_binary_has_symfile_identifiers_in_stats(self):
         self.assertIn('symbolFileModuleIdentifiers', exe_stats)
 
         symfileIDs = exe_stats['symbolFileModuleIdentifiers']
         for symfileID in symfileIDs:
             o_module = self.find_module_by_id_in_metrics(symfileID, debug_stats)
             self.assertNotEqual(o_module, None)
             # Make sure each .o file has some debug info bytes.
             self.assertGreater(o_module['debugInfoByteSize'], 0)
+
+    @skipUnlessDarwin
+    @no_debug_info_test
+    def test_had_frame_variable_errors(self):
+        """
+            Test that if we have frame variable errors that we see this in the
+            statistics for the module that had issues.
+        """
+        self.build(debug_info="dwarf")
+        exe_name = 'a.out'
+        exe = self.getBuildArtifact(exe_name)
+        dsym = self.getBuildArtifact(exe_name + ".dSYM")
+        main_obj = self.getBuildArtifact('main.o')
+        print("carp: dsym = '%s'" % (dsym))
+        # Make sure the executable file exists after building.
+        self.assertEqual(os.path.exists(exe), True)
+        # Make sure the dSYM file doesn't exist after building.
+        self.assertEqual(os.path.isdir(dsym), False)
+        # Make sure the main.o object file exists after building.
+        self.assertEqual(os.path.exists(main_obj), True)
+
+        # 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)
+
+        (target, process, thread, bkpt) = lldbutil.run_to_name_breakpoint(self, 'main')
+
+        # Get stats and verify we had errors.
+        exe_stats = self.find_module_in_metrics(exe, self.get_stats())
+        self.assertTrue(exe_stats is not None)
+
+        # Make sure we have "debugInfoHadVariableErrors" variable that is set to
+        # false before failing to get local variables due to missing .o file.
+        self.assertEqual(exe_stats['debugInfoHadVariableErrors'], False)
+
+        # Try and fail to get variables
+        vars = thread.GetFrameAtIndex(0).GetVariables(True, True, False, True)
+
+        # Make sure we got an error back that indicates that variables were not
+        # available
+        self.assertTrue(vars.GetError().Fail())
+
+        # Get stats and verify we had errors.
+        exe_stats = self.find_module_in_metrics(exe, self.get_stats())
+        self.assertTrue(exe_stats is not None)
+
+        # Make sure we have "hadFrameVariableErrors" variable that is set to
+        # true after failing to get local variables due to missing .o file.
+        self.assertEqual(exe_stats['debugInfoHadVariableErrors'], True)