Include target settings in "statistics dump" output.

[clayborg]

Summary

Many target settings can be set that can affect how breakpoints are found during a debug session. Having these settings along with the statistics are important for being able to reproduce issues clients are having.

A new "settings" key/value pair is added to each target dictionary that contains settings:

"settings": {
  "target.arg0": "/tmp/a.out",
  "target.clang-module-search-paths": [
    "/tmp",
    "/var"
  ],
  "target.debug-file-search-paths": [
    "/tmp",
    "/var"
  ],
  "target.exec-search-paths": [
    "/tmp",
    "/var"
  ],
  "target.inline-breakpoint-strategy": "headers",
  "target.preload-symbols": true,
  "target.run-args": [
    "a",
    "b",
    "c"
  ],
  "target.skip-prologue": true,
  "target.source-map": [
    [
      "/tmp",
      "/var"
    ]
  ]
},

[JDevlieghere]

I understand the need for something like this to make some of the statistics more meaningful, but this is stretching the notion of statistics. Conceptually, this is approaching something like a dump of the debugger for issue/performance analysis. I think that idea is really exciting, and from that perspective there's a lot more useful information we could add to it. Long term, I can see this output be something that we ask users to include in every bug report.

TL;DR: Can we make this more generic and keep the separation between statistics/metrics and a more general concept that includes these target settings?

[clayborg]

In D112691#3093889, @JDevlieghere wrote:

I understand the need for something like this to make some of the statistics more meaningful, but this is stretching the notion of statistics. Conceptually, this is approaching something like a dump of the debugger for issue/performance analysis. I think that idea is really exciting, and from that perspective there's a lot more useful information we could add to it. Long term, I can see this output be something that we ask users to include in every bug report.

That is kind of what I am going for with this command. One main issue is when breakpoints fail to resolve we often don't know why, and knowing if we a) have debug info enabled and b) have the right settings to remap things really will help. I was thinking the same thing though: this info is not really statistics.

Would adding a "--target-state" option that would only include target stuff if requested make more sense? See my comment below as well

TL;DR: Can we make this more generic and keep the separation between statistics/metrics and a more general concept that includes these target settings?

I am open to ideas on how to do this kind of thing. We can always gather this information on our own or with a separate command.

We could do a "target read" and "target write" just like we do for breakpoints? The idea would be to serialize all settings needed for a target into JSON and be able to load a target from JSON. This could be really handy for bug reports. If we have a command that does this, we could easily add an option to the "statistics dump" command to include this state with --target-state so we can still get it all in one place, but the user must ask for it? The description for --target-state could be something like:

-t (--target-state)
    Include the serialization of the target in this information to help with being able to reproduce the conditions that led to the statistics

Lemme know what you think.

[jingham]

Do you care about the history of these settings? After all, the problem might arise because someone set a setting then unset it. Your statistics approach wouldn't catch that. If you are really trying to build an architecture where we can track this sort of problem down, then you might need more of a history approach, where the settings and certain other changes in the state of the debugger mark epochs, and you aggregate data into those epochs?

[clayborg]

In D112691#3095010, @jingham wrote:

Do you care about the history of these settings? After all, the problem might arise because someone set a setting then unset it. Your statistics approach wouldn't catch that. If you are really trying to build an architecture where we can track this sort of problem down, then you might need more of a history approach, where the settings and certain other changes in the state of the debugger mark epochs, and you aggregate data into those epochs?

Personally I find most people set the important settings once and them leave them alone for the debug session. History of settings and timings could be nice, but we have no infrastructure to associate timestamps with events in the debug session right now, that being said it could be added.

If we don't want this in the statistics dump I can fully understand, though I do like a one stop command people can run when they want to report issues that may involve performance or other things going wrong with the debug session.

[clayborg]

In D112691#3095010, @jingham wrote:
I can see wanting to dump statistics at various points in the running of a process, maybe triggered by breakpoints, for instance. In that case I wouldn't want to dump the settings data - if it is indeed redundant (see above) every time. Having the settings as a separate emission would make that possible. And just like we add gdb-remote as a convenience, it would be fine to have some low level commands that you can reassemble and then a portmanteau command that generates a "good for most purposes" report.

Sounds good. We can do this with a separate command and keep the "statistics dump" cleaner. And yes, we do want to write this data out at various points in the process' lifetime to see where and when delays are introduced, so keeping this to just stats is a smart.

Also, we already have "settings read" and "settings write" so adding another way to dump them seems redundant. You are dumping a subset, but the "settings write" command can do that as well. If the format's not one you like, I think we should be able to change that as the successful round trip is the main thing.

I just tried "settings write -f /tmp/a" after loading up lldb with "lldb a.out 1 2 3", but the output doesn't seem to contain any of the right settings? target.arg0 and target.run-args are not saved at all and have no value? Is this command tested? I will opt for "settings show" and saving the entire output out to a file instead.

lldb/include/lldb/Core/FileSpecList.h

 //===-- FileSpecList.h ------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_CORE_FILESPECLIST_H
 #define LLDB_CORE_FILESPECLIST_H
 #if defined(__cplusplus)
 
 #include "lldb/Utility/FileSpec.h"
+#include "llvm/Support/JSON.h"
 
 #include <vector>
 
 #include <cstddef>
 
 namespace lldb_private {
 class Stream;
 
public:
   }
 
   static size_t GetFilesMatchingPartialPath(const char *path, bool dir_okay,
                                             FileSpecList &matches);
 
   const_iterator begin() const { return m_files.begin(); }
   const_iterator end() const { return m_files.end(); }
 
+  llvm::json::Value ToJSON() const;
+
 protected:
   collection m_files; ///< A collection of FileSpec objects.
 };
 
 } // namespace lldb_private
 
 #endif // #if defined(__cplusplus)
 #endif // LLDB_CORE_FILESPECLIST_H

lldb/include/lldb/Target/PathMappingList.h

 //===-- PathMappingList.h ---------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_TARGET_PATHMAPPINGLIST_H
 #define LLDB_TARGET_PATHMAPPINGLIST_H
 
-#include <map>
-#include <vector>
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Status.h"
+#include "llvm/Support/JSON.h"
+#include <map>
+#include <vector>
 
 namespace lldb_private {
 
 class PathMappingList {
 public:
   typedef void (*ChangedCallback)(const PathMappingList &path_list,
                                   void *baton);
 
public:
   /// \return
   ///     The newly remapped filespec that is guaranteed to exist.
   llvm::Optional<FileSpec> FindFile(const FileSpec &orig_spec) const;
 
   uint32_t FindIndexForPath(ConstString path) const;
 
   uint32_t GetModificationID() const { return m_mod_id; }
 
+  /// Return the path mappings as a JSON array.
+  ///
+  /// The array contains arrays of path mapping paths as strings.
+  llvm::json::Value ToJSON() const;
+
 protected:
   typedef std::pair<ConstString, ConstString> pair;
   typedef std::vector<pair> collection;
   typedef collection::iterator iterator;
   typedef collection::const_iterator const_iterator;
 
   iterator FindIteratorForPath(ConstString path);
 

lldb/source/Core/FileSpecList.cpp

 // Return the number of files in the file spec list.
 size_t FileSpecList::GetSize() const { return m_files.size(); }
 
 size_t FileSpecList::GetFilesMatchingPartialPath(const char *path,
                                                  bool dir_okay,
                                                  FileSpecList &matches) {
   return 0;
 }
+
+llvm::json::Value FileSpecList::ToJSON() const {
+  llvm::json::Array files;
+  for (const auto &file : m_files)
+    files.emplace_back(file.GetPath());
+  return llvm::json::Value(std::move(files));
+}

lldb/source/Target/PathMappingList.cpp

uint32_t PathMappingList::FindIndexForPath(ConstString orig_path) const {
   const_iterator end = m_pairs.end();
 
   for (pos = begin; pos != end; ++pos) {
     if (pos->first == path)
       return std::distance(begin, pos);
   }
   return UINT32_MAX;
 }
+
+llvm::json::Value PathMappingList::ToJSON() const {
+  llvm::json::Array mappings;
+  for (const auto &pair : m_pairs) {
+    llvm::json::Array mapping;
+    mapping.emplace_back(pair.first.GetStringRef());
+    mapping.emplace_back(pair.second.GetStringRef());
+    mappings.emplace_back(std::move(mapping));
+  }
+  return llvm::json::Value(std::move(mappings));
+}

lldb/source/Target/Statistics.cpp

json::Value TargetStats::ToJSON(Target &target) {
 
   ProcessSP process_sp = target.GetProcessSP();
   if (process_sp) {
     UnixSignalsSP unix_signals_sp = process_sp->GetUnixSignals();
     if (unix_signals_sp)
       target_metrics_json.try_emplace("signals",
                                       unix_signals_sp->GetHitCountStatistics());
   }
+
+  json::Object settings;
+  settings.try_emplace("target.arg0", target.GetArg0());
+  Args target_args;
+  json::Array run_args;
+  target.GetRunArguments(target_args);
+  for (const auto &arg : target_args.entries())
+    run_args.emplace_back(std::string(arg.c_str()));
+  settings.try_emplace("target.run-args", std::move(run_args));
+  settings.try_emplace("target.source-map", target.GetSourcePathMap().ToJSON());
+  settings.try_emplace("target.exec-search-paths",
+                       target.GetExecutableSearchPaths().ToJSON());
+  settings.try_emplace("target.debug-file-search-paths",
+                       target.GetDebugFileSearchPaths().ToJSON());
+  settings.try_emplace("target.clang-module-search-paths",
+                       target.GetClangModuleSearchPaths().ToJSON());
+  settings.try_emplace("target.preload-symbols", target.GetPreloadSymbols());
+  const char *strategy = nullptr;
+  switch (target.GetInlineStrategy()) {
+  case eInlineBreakpointsNever:
+    strategy = "never";
+    break;
+  case eInlineBreakpointsHeaders:
+    strategy = "headers";
+    break;
+  case eInlineBreakpointsAlways:
+    strategy = "always";
+    break;
+  }
+  settings.try_emplace("target.inline-breakpoint-strategy", strategy);
+  settings.try_emplace("target.skip-prologue", target.GetSkipPrologue());
+
+  target_metrics_json.try_emplace("settings", std::move(settings));
+
   target_metrics_json.try_emplace("breakpoints", std::move(breakpoints_array));
   target_metrics_json.try_emplace("totalBreakpointResolveTime",
                                   totalBreakpointResolveTime);
 
   return target_metrics_json;
 }
 
 void TargetStats::SetLaunchOrAttachTime() {

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

 import lldb
 import json
+import os
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 class TestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
def test_default_no_run(self):
           "totalDebugInfoParseTime": 8.2121400240000071,
           "totalSymbolTableParseTime": 0.123,
           "totalSymbolTableIndexTime": 0.234,
         }
         """
         target = self.createTestTarget()
         debug_stats = self.get_stats()
         debug_stat_keys = [
-            'modules', 
+            'modules',
             'targets',
             'totalSymbolTableParseTime',
             'totalSymbolTableIndexTime',
             'totalDebugInfoByteSize',
             'totalDebugInfoIndexTime',
             'totalDebugInfoParseTime',
         ]
         self.verify_keys(debug_stats, '"debug_stats"', debug_stat_keys, None)
def test_default_with_run(self):
         }
 
         """
         target = self.createTestTarget()
         lldbutil.run_to_source_breakpoint(self, "// break here",
                                           lldb.SBFileSpec("main.c"))
         debug_stats = self.get_stats()
         debug_stat_keys = [
-            'modules', 
+            'modules',
             'targets',
             'totalSymbolTableParseTime',
             'totalSymbolTableIndexTime',
             'totalDebugInfoByteSize',
             'totalDebugInfoIndexTime',
             'totalDebugInfoParseTime',
         ]
         self.verify_keys(debug_stats, '"debug_stats"', debug_stat_keys, None)
class TestCase(TestBase):
     def test_modules(self):
         """
             Test "statistics dump" and the module information.
         """
         exe = self.getBuildArtifact("a.out")
         target = self.createTestTarget(file_path=exe)
         debug_stats = self.get_stats()
         debug_stat_keys = [
-            'modules', 
+            'modules',
             'targets',
             'totalSymbolTableParseTime',
             'totalSymbolTableIndexTime',
             'totalDebugInfoParseTime',
             'totalDebugInfoIndexTime',
             'totalDebugInfoByteSize'
         ]
         self.verify_keys(debug_stats, '"debug_stats"', debug_stat_keys, None)
def test_breakpoints(self):
                             "details": {...},
                             "id": 1,
                             "resolveTime": 2.65438675
                         },
                         {
                             "details": {...},
                             "id": 2,
                             "resolveTime": 4.3632581669999997
-                        }                        
+                        }
                     ]
                 }
             ],
             "totalDebugInfoByteSize": 182522234,
             "totalDebugInfoIndexTime": 2.33343,
             "totalDebugInfoParseTime": 8.2121400240000071,
             "totalSymbolTableParseTime": 0.123,
             "totalSymbolTableIndexTime": 0.234,
             "totalBreakpointResolveTime": 7.0176449170000001
         }
 
         """
         target = self.createTestTarget()
         self.runCmd("b main.cpp:7")
         self.runCmd("b a_function")
         debug_stats = self.get_stats()
         debug_stat_keys = [
-            'modules', 
+            'modules',
             'targets',
             'totalSymbolTableParseTime',
             'totalSymbolTableIndexTime',
             'totalDebugInfoParseTime',
             'totalDebugInfoIndexTime',
             'totalDebugInfoByteSize',
         ]
         self.verify_keys(debug_stats, '"debug_stats"', debug_stat_keys, None)
def test_breakpoints(self):
             'details',
             'id',
             'internal',
             'numLocations',
             'numResolvedLocations',
             'resolveTime'
         ]
         for breakpoint in breakpoints:
-            self.verify_keys(breakpoint, 'target_stats["breakpoints"]', 
+            self.verify_keys(breakpoint, 'target_stats["breakpoints"]',
                              bp_keys_exist, None)
+
+    def test_settings(self):
+        """Test "statistics dump --settings"
+
+        Output expected to be something like:
+
+        (lldb) statistics dump --settings
+        {
+          "targetCreateTime": 0.26566899599999999,
+          "totalBreakpointResolveTime": 0.0031409419999999999,
+          "totalDebugInfoIndexTime": 0,
+          "totalDebugInfoParseTime": 0.00037288300000000001,
+          "totalDebugInfoSize": 2193,
+          "totalSymbolTableIndexTime": 0.056834488000000009,
+          "totalSymbolTableParseTime": 0.093979421999999979
+          "settings": {
+            "target.arg0": "/.../commands/target/metrics/TestStats.test_settings/a.out",
+            "target.clang-module-search-paths": [],
+            "target.debug-file-search-paths": [],
+            "target.exec-search-paths": [
+              "/.../commands/target/metrics/TestStats.test_settings"
+            ],
+            "target.inline-breakpoint-strategy": "always",
+            "target.preload-symbols": true,
+            "target.run-args": [],
+            "target.skip-prologue": true,
+            "target.source-map": []
+            },
+          }
+
+        """
+        exe = self.getBuildArtifact("a.out")
+        exe_dir = os.path.dirname(exe)
+        path1 = self.getSourceDir()
+        path2 = exe_dir
+        target = self.createTestTarget(file_path=exe)
+        debug_stats = self.get_stats(log_path="/tmp/a")
+        debug_stat_keys = [
+            'modules',
+            'targets',
+            'totalSymbolTableParseTime',
+            'totalSymbolTableIndexTime',
+            'totalDebugInfoParseTime',
+            'totalDebugInfoIndexTime',
+            'totalDebugInfoByteSize',
+        ]
+        self.verify_keys(debug_stats, '"debug_stats"', debug_stat_keys, None)
+        target_stats = debug_stats['targets'][0]
+        keys_exist = [
+            'settings'
+        ]
+        self.verify_keys(target_stats, '"stats"', keys_exist, None)
+        settings = target_stats['settings']
+        settings_keys_exist = [
+            "target.arg0",
+            "target.clang-module-search-paths",
+            "target.debug-file-search-paths",
+            "target.exec-search-paths",
+            "target.inline-breakpoint-strategy",
+            "target.preload-symbols",
+            "target.run-args",
+            "target.skip-prologue",
+            "target.source-map",
+        ]
+        self.verify_keys(settings, 'stats["settings"]', settings_keys_exist,
+                         None)
+
+        self.assertEqual(settings['target.arg0'], exe,
+                         'verify "target.arg0" setting is "%s"' % (exe))
+        self.assertEqual(settings['target.clang-module-search-paths'], [],
+                         'verify "target.clang-module-search-paths" setting is []')
+        self.assertEqual(settings['target.debug-file-search-paths'], [],
+                         'verify "target.debug-file-search-paths" setting is []')
+        self.assertEqual(settings['target.exec-search-paths'], [exe_dir],
+                         'verify "target.exec-search-paths" setting is ["%s"]' % (exe_dir))
+        self.assertEqual(settings['target.inline-breakpoint-strategy'], 'always',
+                         'verify "target.inline-breakpoint-strategy" setting is "always"')
+        self.assertEqual(settings['target.preload-symbols'], True,
+                         'verify "target.preload-symbols" setting is True')
+        self.assertEqual(settings['target.run-args'], [],
+                         'verify "target.run-args" setting is []')
+        self.assertEqual(settings['target.skip-prologue'], True,
+                         'verify "target.skip-prologue" setting is True')
+        self.assertEqual(settings['target.source-map'], [],
+                         'verify "target.source-map" setting is []')
+
+        self.runCmd("settings set target.arg0 hello")
+        self.runCmd('settings set target.clang-module-search-paths "%s" "%s"' % (path1, path2))
+        self.runCmd('settings set target.debug-file-search-paths "%s" "%s"' % (path1, path2))
+        self.runCmd('settings set target.exec-search-paths "%s" "%s"' % (path1, path2))
+        self.runCmd('settings set target.inline-breakpoint-strategy never')
+        self.runCmd('settings set target.preload-symbols false')
+        self.runCmd('settings set target.run-args a b c')
+        self.runCmd('settings set target.skip-prologue false')
+        self.runCmd('settings set target.source-map "%s" "%s"' % (path1, path2))
+
+        debug_stats = self.get_stats(log_path="/tmp/b")
+        target_stats = debug_stats['targets'][0]
+        settings = target_stats['settings']
+        self.verify_keys(settings, 'stats["settings"]', settings_keys_exist,
+                         None)
+
+        self.assertEqual(settings['target.arg0'], 'hello',
+                         'verify "target.arg0" setting is "hello"')
+        self.assertEqual(settings['target.clang-module-search-paths'], [path1, path2],
+                         'verify "target.clang-module-search-paths" setting is ["%s", "%s"]' % (path1, path2))
+        self.assertEqual(settings['target.debug-file-search-paths'], [path1, path2],
+                         'verify "target.debug-file-search-paths" setting is ["%s", "%s"]' % (path1, path2))
+        self.assertEqual(settings['target.exec-search-paths'], [path1, path2],
+                         'verify "target.exec-search-paths" setting is ["%s", "%s"]' % (path1, path2))
+        self.assertEqual(settings['target.inline-breakpoint-strategy'], 'never',
+                         'verify "target.inline-breakpoint-strategy" setting is "never"')
+        self.assertEqual(settings['target.preload-symbols'], False,
+                         'verify "target.preload-symbols" setting is False')
+        self.assertEqual(settings['target.run-args'], ["a", "b", "c"],
+                         'verify "target.run-args" setting is ["a", "b", "c"]')
+        self.assertEqual(settings['target.skip-prologue'], False,
+                         'verify "target.skip-prologue" setting is False')
+        self.assertEqual(settings['target.source-map'], [[path1, path2]],
+                         'verify "target.source-map" setting is [["%s", "%s"]]' % (path1, path2))
+
+        self.runCmd('settings set target.inline-breakpoint-strategy headers')
+        debug_stats = self.get_stats(log_path="/tmp/b")
+        target_stats = debug_stats['targets'][0]
+        settings = target_stats['settings']
+        self.verify_keys(settings, 'stats["settings"]', settings_keys_exist,
+                         None)
+        self.assertEqual(settings['target.inline-breakpoint-strategy'], 'headers',
+                         'verify "target.inline-breakpoint-strategy" setting is "headers"')