[lldb] Add/change options in statistics dump to control what sections are dumped

[royitaqi]

Added/changed options

The following options are added to the statistics dump command:

• --targets=bool: Boolean. Dumps the targets section.
• --modules=bool: Boolean. Dumps the modules section.
  When both options are given, the field moduleIdentifiers will be dumped for each target in the targets section.

The following options are changed:

• --transcript=bool: Changed to a boolean. Dumps the transcript section.

Behavior of statistics dump with various options

The behavior is backward compatible:

• When no options are provided, statistics dump dumps all sections.
• When --summary is provided, only dumps the summary info.

New behavior:

• --targets=bool, --modules=bool, --transcript=bool overrides the above "default".

For example:

• statistics dump --modules=false dumps summary + targets + transcript. No modules.
• statistics dump --summary --targets=true --transcript=true dumps summary + targets (in summary mode) + transcript.

Added options into public API

In SBStatisticsOptions, add:

• Set/GetIncludeTargets
• Set/GetIncludeModules
• Set/GetIncludeTranscript

Alternative considered: Thought about adding Set/GetIncludeSections(string sections_spec), which receives a comma-separated list of section names to be included ("targets", "modules", "transcript"). The benefit of this approach is that the API is more future-proof when it comes to possible adding/changing of section names. However, I feel the section names are likely to remain unchanged for a while - it's not like we plan to make big changes to the output of statistics dump any time soon. The downsides of this approach are: 1\ the readability of the API is worse (requires reading doc to understand what string can be accepted), 2\ string input are more prone to human error (e.g. typo "target" instead of expected "targets").

Tests

bin/llvm-lit -sv ../external/llvm-project/lldb/test/API/commands/statistics/basic/TestStats.py

./tools/lldb/unittests/Interpreter/InterpreterTests

New test cases have been added to verify:

• Different sections are dumped/not dumped when different StatisticsOptions are given through command line (CLI or HandleCommand; see test_sections_existence_through_command) or API (see test_sections_existence_through_api).
• The order in which the options are given in command line does not matter (see test_order_of_options_do_not_matter).

Build Bot

https://lab.llvm.org/buildbot/#/changes/928

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (royitaqi)

Changes

Add options

The following options are added to the statistics dump command:

• --targets: Dumps the targets section.
• --modules: Dumps the modules section.

When both options are given, the field moduleIdentifiers will be dumped for each target in the targets section.

Split options into groups

We are splitting the section-selecting options into the following groups.

• Group 1: --summary. This dumps only the summary.
• Group 2: --targets, --modules, --transcript. This dumps both the summary and any of the specified sections.

There is also an implicit Group 0, i.e. when none of the above options are given by the user. This dumps all sections.

Other horizontal options (--all-targets and --load-all-debug-info) are accessible from all groups.

Tests

bin/llvm-lit -sv ../external/llvm-project/lldb/test/API/commands/statistics/basic/TestStats.py

A new test case test_sections_existence is added to verify that different sections are dumped/not dumped when different options are given.

---

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

5 Files Affected:

• (modified) lldb/include/lldb/Target/Statistics.h (+2)
• (modified) lldb/source/Commands/CommandObjectStats.cpp (+6)
• (modified) lldb/source/Commands/Options.td (+13-3)
• (modified) lldb/source/Target/Statistics.cpp (+23-10)
• (modified) lldb/test/API/commands/statistics/basic/TestStats.py (+96-6)

diff --git a/lldb/include/lldb/Target/Statistics.h b/lldb/include/lldb/Target/Statistics.h
index c04d529290fff..b24520b72a95d 100644
--- a/lldb/include/lldb/Target/Statistics.h
+++ b/lldb/include/lldb/Target/Statistics.h
@@ -133,6 +133,8 @@ struct ConstStringStats {
 struct StatisticsOptions {
   bool summary_only = false;
   bool load_all_debug_info = false;
+  bool include_targets = false;
+  bool include_modules = false;
   bool include_transcript = false;
 };
 
diff --git a/lldb/source/Commands/CommandObjectStats.cpp b/lldb/source/Commands/CommandObjectStats.cpp
index 1935b0fdfadfb..1347b1ba25b1d 100644
--- a/lldb/source/Commands/CommandObjectStats.cpp
+++ b/lldb/source/Commands/CommandObjectStats.cpp
@@ -81,6 +81,12 @@ class CommandObjectStatsDump : public CommandObjectParsed {
       case 'f':
         m_stats_options.load_all_debug_info = true;
         break;
+      case 'r':
+        m_stats_options.include_targets = true;
+        break;
+      case 'm':
+        m_stats_options.include_modules = true;
+        break;
       case 't':
         m_stats_options.include_transcript = true;
         break;
diff --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index cee5a81d3796b..45f46334d5c73 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -1416,16 +1416,26 @@ let Command = "trace schema" in {
 }
 
 let Command = "statistics dump" in {
-  def statistics_dump_all: Option<"all-targets", "a">, Group<1>,
+  def statistics_dump_all: Option<"all-targets", "a">, GroupRange<1, 2>,
     Desc<"Include statistics for all targets.">;
   def statistics_dump_summary: Option<"summary", "s">, Group<1>,
     Desc<"Dump only high-level summary statistics. "
          "Exclude targets, modules, breakpoints etc... details.">;
-  def statistics_dump_force: Option<"load-all-debug-info", "f">, Group<1>,
+  def statistics_dump_force: Option<"load-all-debug-info", "f">, GroupRange<1, 2>,
     Desc<"Dump the total possible debug info statistics. "
     "Force loading all the debug information if not yet loaded, and collect "
     "statistics with those.">;
-  def statistics_dump_transcript: Option<"transcript", "t">, Group<1>,
+  def statistics_dump_targets: Option<"targets", "r">, Group<2>,
+    Desc<"Dump statistics for the targets, including breakpoints, expression "
+    "evaluations, frame variables, etc. "
+    "If both the '--targets' and the '--modules' options are specified, a "
+    "list of module identifiers will be added to the 'targets' section.">;
+  def statistics_dump_modules: Option<"modules", "m">, Group<2>,
+    Desc<"Dump statistics for the modules, including time and size of various "
+    "aspects of the module and debug information, type system, path, etc. "
+    "If both the '--targets' and the '--modules' options are specified, a "
+    "list of module identifiers will be added to the 'targets' section.">;
+  def statistics_dump_transcript: Option<"transcript", "t">, Group<2>,
     Desc<"If the setting interpreter.save-transcript is enabled and this "
     "option is specified, include a JSON array with all commands the user and/"
     "or scripts executed during a debug session.">;
diff --git a/lldb/source/Target/Statistics.cpp b/lldb/source/Target/Statistics.cpp
index 2a5300012511a..15c7dfe07ac87 100644
--- a/lldb/source/Target/Statistics.cpp
+++ b/lldb/source/Target/Statistics.cpp
@@ -108,6 +108,10 @@ TargetStats::ToJSON(Target &target,
   json::Object target_metrics_json;
   ProcessSP process_sp = target.GetProcessSP();
   const bool summary_only = options.summary_only;
+  const bool include_targets = options.include_targets;
+  const bool include_modules = options.include_modules;
+  const bool include_transcript = options.include_transcript;
+  const bool include_default_sections = !summary_only && !include_targets && !include_modules && !include_transcript;
   if (!summary_only) {
     CollectStats(target);
 
@@ -117,8 +121,9 @@ TargetStats::ToJSON(Target &target,
 
     target_metrics_json.try_emplace(m_expr_eval.name, m_expr_eval.ToJSON());
     target_metrics_json.try_emplace(m_frame_var.name, m_frame_var.ToJSON());
-    target_metrics_json.try_emplace("moduleIdentifiers",
-                                    std::move(json_module_uuid_array));
+    if (include_default_sections || include_modules)
+      target_metrics_json.try_emplace("moduleIdentifiers",
+                                      std::move(json_module_uuid_array));
 
     if (m_launch_or_attach_time && m_first_private_stop_time) {
       double elapsed_time =
@@ -226,7 +231,10 @@ llvm::json::Value DebuggerStats::ReportStatistics(
 
   const bool summary_only = options.summary_only;
   const bool load_all_debug_info = options.load_all_debug_info;
+  const bool include_targets = options.include_targets;
+  const bool include_modules = options.include_modules;
   const bool include_transcript = options.include_transcript;
+  const bool include_default_sections = !summary_only && !include_targets && !include_modules && !include_transcript;
 
   json::Array json_targets;
   json::Array json_modules;
@@ -314,7 +322,7 @@ llvm::json::Value DebuggerStats::ReportStatistics(
     if (module_stat.debug_info_had_incomplete_types)
       ++num_modules_with_incomplete_types;
 
-    if (!summary_only) {
+    if (include_default_sections || include_modules) {
       module_stat.identifier = (intptr_t)module;
       module_stat.path = module->GetFileSpec().GetPath();
       if (ConstString object_name = module->GetObjectName()) {
@@ -347,13 +355,15 @@ llvm::json::Value DebuggerStats::ReportStatistics(
       {"totalSymbolTableStripped", num_stripped_modules},
   };
 
-  if (target) {
-    json_targets.emplace_back(target->ReportStatistics(options));
-  } else {
-    for (const auto &target : debugger.GetTargetList().Targets())
+  if (include_default_sections || include_targets) {
+    if (target) {
       json_targets.emplace_back(target->ReportStatistics(options));
+    } else {
+      for (const auto &target : debugger.GetTargetList().Targets())
+        json_targets.emplace_back(target->ReportStatistics(options));
+    }
+    global_stats.try_emplace("targets", std::move(json_targets));
   }
-  global_stats.try_emplace("targets", std::move(json_targets));
 
   ConstStringStats const_string_stats;
   json::Object json_memory{
@@ -362,11 +372,14 @@ llvm::json::Value DebuggerStats::ReportStatistics(
   global_stats.try_emplace("memory", std::move(json_memory));
   if (!summary_only) {
     json::Value cmd_stats = debugger.GetCommandInterpreter().GetStatistics();
-    global_stats.try_emplace("modules", std::move(json_modules));
     global_stats.try_emplace("commands", std::move(cmd_stats));
   }
 
-  if (include_transcript) {
+  if (include_default_sections || include_modules) {
+    global_stats.try_emplace("modules", std::move(json_modules));
+  }
+
+  if (include_default_sections || include_transcript) {
     // When transcript is available, add it to the to-be-returned statistics.
     //
     // NOTE:
diff --git a/lldb/test/API/commands/statistics/basic/TestStats.py b/lldb/test/API/commands/statistics/basic/TestStats.py
index ebe6166eb45e5..2132d2df03b4b 100644
--- a/lldb/test/API/commands/statistics/basic/TestStats.py
+++ b/lldb/test/API/commands/statistics/basic/TestStats.py
@@ -657,16 +657,106 @@ def test_transcript_happy_path(self):
         # The second "statistics dump" in the transcript should have no output
         self.assertNotIn("output", transcript[2])
 
-    def test_transcript_should_not_exist_when_not_asked_for(self):
+    def test_sections_existence(self):
         """
-        Test "statistics dump" and the transcript information.
+        Test "statistics dump" and the existence of sections when different
+        options are given.
         """
         self.build()
         exe = self.getBuildArtifact("a.out")
         target = self.createTestTarget(file_path=exe)
         self.runCmd("settings set interpreter.save-transcript true")
-        self.runCmd("version")
 
-        # Verify the output of a first "statistics dump"
-        debug_stats = self.get_stats()  # Not with "--transcript"
-        self.assertNotIn("transcript", debug_stats)
+        test_cases = [
+            {   # statistics dump
+                "options": "",
+                "expect": {
+                    "commands": True,
+                    "targets": True,
+                    "targets.moduleIdentifiers": True,
+                    "targets.breakpoints": True,
+                    "modules": True,
+                    "transcript": True,
+                },
+            },
+            {   # statistics dump --summary
+                "options": " --summary",
+                "expect": {
+                    "commands": False,
+                    "targets": False,
+                    "targets.moduleIdentifiers": False,
+                    "targets.breakpoints": False,
+                    "modules": False,
+                    "transcript": False,
+                },
+            },
+            {   # statistics dump --targets
+                "options": " --targets",
+                "expect": {
+                    "commands": True,
+                    "targets": True,
+                    "targets.moduleIdentifiers": False,
+                    "targets.breakpoints": True,
+                    "modules": False,
+                    "transcript": False,
+                },
+            },
+            {   # statistics dump --modules
+                "options": " --modules",
+                "expect": {
+                    "commands": True,
+                    "targets": False,
+                    "targets.moduleIdentifiers": False,
+                    "targets.breakpoints": False,
+                    "modules": True,
+                    "transcript": False,
+                },
+            },
+            {   # statistics dump --targets --modules
+                "options": " --targets --modules",
+                "expect": {
+                    "commands": True,
+                    "targets": True,
+                    "targets.moduleIdentifiers": True,
+                    "targets.breakpoints": True,
+                    "modules": True,
+                    "transcript": False,
+                },
+            },
+            {   # statistics dump --transcript
+                "options": " --transcript",
+                "expect": {
+                    "commands": True,
+                    "targets": False,
+                    "targets.moduleIdentifiers": False,
+                    "targets.breakpoints": False,
+                    "modules": False,
+                    "transcript": True,
+                },
+            },
+        ]
+
+        for test_case in test_cases:
+            options = test_case["options"]
+            debug_stats = self.get_stats(options)
+            # The following fields should always exist
+            self.assertIn("totalDebugInfoEnabled", debug_stats, "Global stats should always exist")
+            self.assertIn("memory", debug_stats, "'memory' should always exist")
+            # The following fields should exist/not exist depending on the test case
+            for field_name in test_case["expect"]:
+                idx = field_name.find(".")
+                if idx == -1:
+                    # `field` is a top-level field
+                    exists = field_name in debug_stats
+                    should_exist = test_case["expect"][field_name]
+                    should_exist_string = "" if should_exist else "not "
+                    self.assertEqual(exists, should_exist, f"'{field_name}' should {should_exist_string}exist for 'statistics dump{options}'")
+                else:
+                    # `field` is a string of "<top-level field>.<second-level field>"
+                    top_level_field_name = field_name[0:idx]
+                    second_level_field_name = field_name[idx+1:]
+                    for top_level_field in debug_stats[top_level_field_name] if top_level_field_name in debug_stats else []:
+                        exists = second_level_field_name in top_level_field
+                        should_exist = test_case["expect"][field_name]
+                        should_exist_string = "" if should_exist else "not "
+                        self.assertEqual(exists, should_exist, f"'{field_name}' should {should_exist_string}exist for 'statistics dump{options}'")

[royitaqi]

cc @kusmour, @clayborg , @jeffreytan81

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[clayborg]

With these new options the default statistics dump changes the output from what is was before. I might suggest that we set the defaults so it does what it used to do by default, and then let users opt out of the targets and modules in each output. This means that the --targets and --modules would need to take a bool variable, or be switched to --no-targets and --no-modules.

[hawkinsw]

Thank you for doing this work! I hope that these little bits help!