Revert "[lldb] Add count for errors of DWO files in statistics and combine DWO file count functions"

[dmpots]

Reverts #155023

The PR tests passed, but it failed in the CI. Reverting to give time to investigate.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: David Peixotto (dmpots)

Changes

Reverts llvm/llvm-project#155023

---

Full diff: https://github.com/llvm/llvm-project/pull/156777.diff

8 Files Affected:

• (modified) lldb/include/lldb/Symbol/SymbolFile.h (+5-8)
• (modified) lldb/include/lldb/Target/Statistics.h (+2-14)
• (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+6-10)
• (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h (+4-5)
• (modified) lldb/source/Target/Statistics.cpp (+10-10)
• (modified) lldb/test/API/commands/statistics/basic/TestStats.py (-115)
• (removed) lldb/test/API/commands/statistics/basic/dwo_error_foo.cpp (-10)
• (removed) lldb/test/API/commands/statistics/basic/dwo_error_main.cpp (-5)

diff --git a/lldb/include/lldb/Symbol/SymbolFile.h b/lldb/include/lldb/Symbol/SymbolFile.h
index ad35b1defb5c9..bbc615d9fdc38 100644
--- a/lldb/include/lldb/Symbol/SymbolFile.h
+++ b/lldb/include/lldb/Symbol/SymbolFile.h
@@ -488,16 +488,13 @@ class SymbolFile : public PluginInterface {
     return false;
   };
 
-  /// Retrieves statistics about DWO files associated with this symbol file.
-  /// This function returns a DWOStats struct containing:
-  ///   - The number of successfully loaded/parsed DWO files.
-  ///   - The total number of DWO files encountered.
-  ///   - The number of DWO CUs that failed to load due to errors.
-  /// If this symbol file does not support DWO files, all counts will be zero.
+  /// Get number of loaded/parsed DWO files. This is emitted in "statistics
+  /// dump"
   ///
   /// \returns
-  ///   A DWOStats struct with loaded, total, and error counts for DWO files.
-  virtual DWOStats GetDwoStats() { return {}; }
+  ///     A pair containing (loaded_dwo_count, total_dwo_count). If this
+  ///     symbol file doesn't support DWO files, both counts will be 0.
+  virtual std::pair<uint32_t, uint32_t> GetDwoFileCounts() { return {0, 0}; }
 
   virtual lldb::TypeSP
   MakeType(lldb::user_id_t uid, ConstString name,
diff --git a/lldb/include/lldb/Target/Statistics.h b/lldb/include/lldb/Target/Statistics.h
index c288917edbc6a..55dff8861a9ab 100644
--- a/lldb/include/lldb/Target/Statistics.h
+++ b/lldb/include/lldb/Target/Statistics.h
@@ -123,19 +123,6 @@ struct StatsSuccessFail {
   uint32_t failures = 0;
 };
 
-/// Holds statistics about DWO (Debug With Object) files.
-struct DWOStats {
-  uint32_t loaded_dwo_file_count = 0;
-  uint32_t dwo_file_count = 0;
-  uint32_t dwo_error_count = 0;
-
-  DWOStats operator+(const DWOStats &rhs) const {
-    return DWOStats{loaded_dwo_file_count + rhs.loaded_dwo_file_count,
-                    dwo_file_count + rhs.dwo_file_count,
-                    dwo_error_count + rhs.dwo_error_count};
-  }
-};
-
 /// A class that represents statistics for a since lldb_private::Module.
 struct ModuleStats {
   llvm::json::Value ToJSON() const;
@@ -166,7 +153,8 @@ struct ModuleStats {
   bool symtab_stripped = false;
   bool debug_info_had_variable_errors = false;
   bool debug_info_had_incomplete_types = false;
-  DWOStats dwo_stats;
+  uint32_t dwo_file_count = 0;
+  uint32_t loaded_dwo_file_count = 0;
 };
 
 struct ConstStringStats {
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 7108ac72eb030..b15e0c15fedb8 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4502,8 +4502,9 @@ void SymbolFileDWARF::GetCompileOptions(
   }
 }
 
-DWOStats SymbolFileDWARF::GetDwoStats() {
-  DWOStats stats;
+std::pair<uint32_t, uint32_t> SymbolFileDWARF::GetDwoFileCounts() {
+  uint32_t total_dwo_count = 0;
+  uint32_t loaded_dwo_count = 0;
 
   DWARFDebugInfo &info = DebugInfo();
   const size_t num_cus = info.GetNumUnits();
@@ -4516,21 +4517,16 @@ DWOStats SymbolFileDWARF::GetDwoStats() {
     if (!dwarf_cu->GetDWOId().has_value())
       continue;
 
-    stats.dwo_file_count++;
+    total_dwo_count++;
 
     // If we have a DWO symbol file, that means we were able to successfully
     // load it.
     SymbolFile *dwo_symfile =
         dwarf_cu->GetDwoSymbolFile(/*load_all_debug_info=*/false);
     if (dwo_symfile) {
-      stats.loaded_dwo_file_count++;
+      loaded_dwo_count++;
     }
-
-    // Check if this unit has a DWO load error, false by default.
-    const Status &dwo_error = dwarf_cu->GetDwoError();
-    if (dwo_error.Fail())
-      stats.dwo_error_count++;
   }
 
-  return stats;
+  return {loaded_dwo_count, total_dwo_count};
 }
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
index 8b8cb5859e8a0..d7db8a3c0869f 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -283,11 +283,10 @@ class SymbolFileDWARF : public SymbolFileCommon {
   bool GetSeparateDebugInfo(StructuredData::Dictionary &d, bool errors_only,
                             bool load_all_debug_info = false) override;
 
-  /// Gets statistics about dwo files associated with this symbol file.
-  /// For split-dwarf files, this reports the counts for successfully loaded DWO
-  /// CUs, total DWO CUs, and the number of DWO CUs with loading errors.
-  /// For non-split-dwarf files, this reports 0 for all.
-  DWOStats GetDwoStats() override;
+  // Gets a pair of loaded and total dwo file counts.
+  // For split-dwarf files, this reports the counts for successfully loaded DWO
+  // CUs and total DWO CUs. For non-split-dwarf files, this reports 0 for both.
+  std::pair<uint32_t, uint32_t> GetDwoFileCounts() override;
 
   DWARFContext &GetDWARFContext() { return m_context; }
 
diff --git a/lldb/source/Target/Statistics.cpp b/lldb/source/Target/Statistics.cpp
index 3c16df5440f25..909f335687b21 100644
--- a/lldb/source/Target/Statistics.cpp
+++ b/lldb/source/Target/Statistics.cpp
@@ -73,9 +73,8 @@ json::Value ModuleStats::ToJSON() const {
                      debug_info_had_incomplete_types);
   module.try_emplace("symbolTableStripped", symtab_stripped);
   module.try_emplace("symbolTableSymbolCount", symtab_symbol_count);
-  module.try_emplace("dwoFileCount", dwo_stats.dwo_file_count);
-  module.try_emplace("loadedDwoFileCount", dwo_stats.loaded_dwo_file_count);
-  module.try_emplace("dwoErrorCount", dwo_stats.dwo_error_count);
+  module.try_emplace("dwoFileCount", dwo_file_count);
+  module.try_emplace("loadedDwoFileCount", loaded_dwo_file_count);
 
   if (!symbol_locator_time.map.empty()) {
     json::Object obj;
@@ -325,7 +324,8 @@ llvm::json::Value DebuggerStats::ReportStatistics(
   uint32_t num_modules_with_incomplete_types = 0;
   uint32_t num_stripped_modules = 0;
   uint32_t symtab_symbol_count = 0;
-  DWOStats total_dwo_stats;
+  uint32_t total_loaded_dwo_file_count = 0;
+  uint32_t total_dwo_file_count = 0;
   for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
     Module *module = target != nullptr
                          ? target->GetImages().GetModuleAtIndex(image_idx).get()
@@ -357,9 +357,10 @@ llvm::json::Value DebuggerStats::ReportStatistics(
         for (const auto &symbol_module : symbol_modules.Modules())
           module_stat.symfile_modules.push_back((intptr_t)symbol_module.get());
       }
-      DWOStats current_dwo_stats = sym_file->GetDwoStats();
-      module_stat.dwo_stats = module_stat.dwo_stats + current_dwo_stats;
-      total_dwo_stats = total_dwo_stats + current_dwo_stats;
+      std::tie(module_stat.loaded_dwo_file_count, module_stat.dwo_file_count) =
+          sym_file->GetDwoFileCounts();
+      total_dwo_file_count += module_stat.dwo_file_count;
+      total_loaded_dwo_file_count += module_stat.loaded_dwo_file_count;
       module_stat.debug_info_index_loaded_from_cache =
           sym_file->GetDebugInfoIndexWasLoadedFromCache();
       if (module_stat.debug_info_index_loaded_from_cache)
@@ -434,9 +435,8 @@ llvm::json::Value DebuggerStats::ReportStatistics(
       {"totalDebugInfoEnabled", num_debug_info_enabled_modules},
       {"totalSymbolTableStripped", num_stripped_modules},
       {"totalSymbolTableSymbolCount", symtab_symbol_count},
-      {"totalLoadedDwoFileCount", total_dwo_stats.loaded_dwo_file_count},
-      {"totalDwoFileCount", total_dwo_stats.dwo_file_count},
-      {"totalDwoErrorCount", total_dwo_stats.dwo_error_count},
+      {"totalLoadedDwoFileCount", total_loaded_dwo_file_count},
+      {"totalDwoFileCount", total_dwo_file_count},
   };
 
   if (include_targets) {
diff --git a/lldb/test/API/commands/statistics/basic/TestStats.py b/lldb/test/API/commands/statistics/basic/TestStats.py
index 670c675cbd648..e9ee8b8661e5a 100644
--- a/lldb/test/API/commands/statistics/basic/TestStats.py
+++ b/lldb/test/API/commands/statistics/basic/TestStats.py
@@ -1,8 +1,6 @@
-import glob
 import json
 import os
 import re
-import shutil
 
 import lldb
 from lldbsuite.test.decorators import *
@@ -181,7 +179,6 @@ def test_default_no_run(self):
             "totalDebugInfoParseTime",
             "totalDwoFileCount",
             "totalLoadedDwoFileCount",
-            "totalDwoErrorCount",
         ]
         self.verify_keys(debug_stats, '"debug_stats"', debug_stat_keys, None)
         if self.getPlatform() != "windows":
@@ -294,7 +291,6 @@ def test_default_with_run(self):
             "totalDebugInfoParseTime",
             "totalDwoFileCount",
             "totalLoadedDwoFileCount",
-            "totalDwoErrorCount",
         ]
         self.verify_keys(debug_stats, '"debug_stats"', debug_stat_keys, None)
         stats = debug_stats["targets"][0]
@@ -335,7 +331,6 @@ def test_memory(self):
             "totalDebugInfoByteSize",
             "totalDwoFileCount",
             "totalLoadedDwoFileCount",
-            "totalDwoErrorCount",
         ]
         self.verify_keys(debug_stats, '"debug_stats"', debug_stat_keys, None)
 
@@ -390,7 +385,6 @@ def test_modules(self):
             "totalDebugInfoByteSize",
             "totalDwoFileCount",
             "totalLoadedDwoFileCount",
-            "totalDwoErrorCount",
         ]
         self.verify_keys(debug_stats, '"debug_stats"', debug_stat_keys, None)
         stats = debug_stats["targets"][0]
@@ -413,7 +407,6 @@ def test_modules(self):
             "symbolTableSavedToCache",
             "dwoFileCount",
             "loadedDwoFileCount",
-            "dwoErrorCount",
             "triple",
             "uuid",
         ]
@@ -504,7 +497,6 @@ def test_breakpoints(self):
             "totalDebugInfoByteSize",
             "totalDwoFileCount",
             "totalLoadedDwoFileCount",
-            "totalDwoErrorCount",
         ]
         self.verify_keys(debug_stats, '"debug_stats"', debug_stat_keys, None)
         target_stats = debug_stats["targets"][0]
@@ -663,113 +655,6 @@ def test_dwp_dwo_file_count(self):
         self.assertEqual(debug_stats["totalDwoFileCount"], 2)
         self.assertEqual(debug_stats["totalLoadedDwoFileCount"], 2)
 
-    @add_test_categories(["dwo"])
-    def test_dwo_missing_error_stats(self):
-        """
-        Test that DWO missing errors are reported correctly in statistics.
-        This test:
-        1) Builds a program with split DWARF (.dwo files)
-        2) Delete all two .dwo files
-        3) Verify that 2 DWO errors are reported in statistics
-        """
-        da = {
-            "CXX_SOURCES": "dwo_error_main.cpp dwo_error_foo.cpp",
-            "EXE": self.getBuildArtifact("a.out"),
-        }
-        # -gsplit-dwarf creates separate .dwo files,
-        # Expected output: dwo_error_main.dwo (contains main) and dwo_error_foo.dwo (contains foo struct/function)
-        self.build(dictionary=da, debug_info="dwo")
-        self.addTearDownCleanup(dictionary=da)
-        exe = self.getBuildArtifact("a.out")
-
-        # Remove the two .dwo files to trigger a DWO load error
-        dwo_files = glob.glob(self.getBuildArtifact("*.dwo"))
-        for dwo_file in dwo_files:
-            os.rename(dwo_file, dwo_file + ".bak")
-
-        target = self.createTestTarget(file_path=exe)
-        debug_stats = self.get_stats()
-
-        # Check DWO load error statistics are reported
-        self.assertIn("totalDwoErrorCount", debug_stats)
-        self.assertEqual(debug_stats["totalDwoErrorCount"], 2)
-
-        # Since there's only one module, module stats should have the same count as total count
-        self.assertIn("dwoErrorCount", debug_stats["modules"][0])
-        self.assertEqual(debug_stats["modules"][0]["dwoErrorCount"], 2)
-
-        # Restore the original .dwo file
-        for dwo_file in dwo_files:
-            os.rename(dwo_file + ".bak", dwo_file)
-
-    @add_test_categories(["dwo"])
-    def test_dwo_id_mismatch_error_stats(self):
-        """
-        Test that DWO ID mismatch errors are reported correctly in statistics.
-        This test:
-        1) Builds a program with split DWARF (.dwo files)
-        2) Change one of the source file content and rebuild
-        3) Replace the new .dwo file with the original one to create a DWO ID mismatch
-        4) Verifies that a DWO error is reported in statistics
-        5) Restores the original source file
-        """
-        da = {
-            "CXX_SOURCES": "dwo_error_main.cpp dwo_error_foo.cpp",
-            "EXE": self.getBuildArtifact("a.out"),
-        }
-        # -gsplit-dwarf creates separate .dwo files,
-        # Expected output: dwo_error_main.dwo (contains main) and dwo_error_foo.dwo (contains foo struct/function)
-        self.build(dictionary=da, debug_info="dwo")
-        self.addTearDownCleanup(dictionary=da)
-        exe = self.getBuildArtifact("a.out")
-
-        # Find and make a backup of the original .dwo file
-        dwo_files = glob.glob(self.getBuildArtifact("*.dwo"))
-
-        original_dwo_file = dwo_files[1]
-        original_dwo_backup = original_dwo_file + ".bak"
-        shutil.copy2(original_dwo_file, original_dwo_backup)
-
-        target = self.createTestTarget(file_path=exe)
-        initial_stats = self.get_stats()
-        self.assertIn("totalDwoErrorCount", initial_stats)
-        self.assertEqual(initial_stats["totalDwoErrorCount"], 0)
-        self.dbg.DeleteTarget(target)
-
-        # Get the original file size before modification
-        source_file_path = self.getSourcePath("dwo_error_foo.cpp")
-        original_size = os.path.getsize(source_file_path)
-
-        try:
-            # Modify the source code  and rebuild
-            with open(source_file_path, "a") as f:
-                f.write("\n void additional_foo(){}\n")
-
-            # Rebuild and replace the new .dwo file with the original one
-            self.build(dictionary=da, debug_info="dwo")
-            shutil.copy2(original_dwo_backup, original_dwo_file)
-
-            # Create a new target and run to a breakpoint to force DWO file loading
-            target = self.createTestTarget(file_path=exe)
-            debug_stats = self.get_stats()
-
-            # Check that DWO load error statistics are reported
-            self.assertIn("totalDwoErrorCount", debug_stats)
-            self.assertEqual(debug_stats["totalDwoErrorCount"], 1)
-
-            # Since there's only one module, module stats should have the same count as total count
-            self.assertIn("dwoErrorCount", debug_stats["modules"][0])
-            self.assertEqual(debug_stats["modules"][0]["dwoErrorCount"], 1)
-
-        finally:
-            # Remove the appended content
-            with open(source_file_path, "a") as f:
-                f.truncate(original_size)
-
-            # Restore the original .dwo file
-            if os.path.exists(original_dwo_backup):
-                os.unlink(original_dwo_backup)
-
     @skipUnlessDarwin
     @no_debug_info_test
     def test_dsym_binary_has_symfile_in_stats(self):
diff --git a/lldb/test/API/commands/statistics/basic/dwo_error_foo.cpp b/lldb/test/API/commands/statistics/basic/dwo_error_foo.cpp
deleted file mode 100644
index 41618bdcee958..0000000000000
--- a/lldb/test/API/commands/statistics/basic/dwo_error_foo.cpp
+++ /dev/null
@@ -1,10 +0,0 @@
-struct foo {
-  int x;
-  bool y;
-};
-
-void dwo_error_foo() {
-  foo f;
-  f.x = 1;
-  f.y = true;
-}
diff --git a/lldb/test/API/commands/statistics/basic/dwo_error_main.cpp b/lldb/test/API/commands/statistics/basic/dwo_error_main.cpp
deleted file mode 100644
index 4f09bd74e1fd6..0000000000000
--- a/lldb/test/API/commands/statistics/basic/dwo_error_main.cpp
+++ /dev/null
@@ -1,5 +0,0 @@
-void dwo_error_foo();
-int main() {
-  dwo_error_foo();
-  return 0;
-}

[zhyty]

Thanks for reverting! @zw3917 and I will have to revisit the test to try to catch the possible race condition.