Add SBDebugger::GetSetting() public API

[yinghuitan]

Summary

This patch adds new SBDebugger::GetSetting() API which
enables client to access settings as SBStructuredData.

Implementation wise, a new ToJSON() virtual function is added to OptionValue
class so that each concrete child class can override and provides its
own JSON representation. This patch aims to define the APIs and implement
a common set of OptionValue child classes, leaving the remaining for
future patches.

This patch is used later by auto deduce source map from source line breakpoint
feature for testing generated source map entries.

[yinghuitan]

Re-diff with clang-format

[jingham]

I'm a little sad that we don't yet have a way to read the current value of a setting into an SBStructuredData, so we do this piecemeal instead. But that's a bigger project, so if you need this now, it doesn't seem fair to block you on that. We should really name the output SBStructuredData keys more instructively, however.

lldb/include/lldb/API/SBTarget.h

91

You have to say what the structure is so people will know how to fetch the elements.

lldb/source/API/SBTarget.cpp

222

It seems a little round-about to convert the source map to JSON, then from JSON to an SBStructuredData. You should be able to write the elements directly. I'm not sure how much that matters, however.

lldb/source/Target/PathMappingList.cpp

138

Can we call these something more instructive than "first" and "second"? These are the "original" path and the "substitution" path, maybe those would be good keys?

[clayborg]

lldb/bindings/interface/SBTarget.i

969

Do we want to actually have something more like this:

lldb::SBStructuredData GetSetting(const char *setting);

That allows us to get any setting as structured data? We have a lot of settings and I would rather not have an API for each one of these added to the public API as we need them.

Eventually we could also have:

void SetSetting(lldb::SBStructuredData &data);

This would allow the output to contain multiple settings if needed. Hopefully the output would be in the format of:

{ "target.source-map": [["<orig-path>", "<replace-path>"], ...] }

This would allow us to export all settings, and then import them.

lldb/source/Target/PathMappingList.cpp

138

We can either do this as a key/value pair thing or just as a array of array path pairs in JSON?

[yinghuitan]

Address review feedback.

[clayborg]

lldb/bindings/interface/SBDebugger.i

228 ↗

(On Diff #458809)

remove this and document the function below such that "setting" can be NULL or empty for to get all settings. Maybe also document that anything that you can type as an argument for "settings show" should work here. See the output of "settings show target" for an example.

Examples that should work here are:

lldb::SBStructuredData settings = debugger.GetSetting(nullptr); // Get all settings
lldb::SBStructuredData settings = debugger.GetSetting(""); // Get all settings
lldb::SBStructuredData settings = debugger.GetSetting("target.arg0"); // Get 1 specific setting
lldb::SBStructuredData settings = debugger.GetSetting("target.arg0 target.language"); // Get 2 specific settings
lldb::SBStructuredData settings = debugger.GetSetting("target"); // Get all target specific settings

So basically anything you can type into "settings show" should work here.

lldb/include/lldb/API/SBDebugger.h

118 ↗

(On Diff #458809)

remove

lldb/include/lldb/Interpreter/OptionValue.h

88 ↗

(On Diff #458809)

89 ↗

(On Diff #458809)

lldb/source/API/SBDebugger.cpp

439–455 ↗

(On Diff #458809)

Remove this function. We should have only one that takes a "const char *settings"

464 ↗

(On Diff #458809)

No need to create a shared pointer here.

467 ↗

(On Diff #458809)

471 ↗

(On Diff #458809)

lldb/source/Interpreter/OptionValueArray.cpp

81–83 ↗

(On Diff #458809)

omit braces for single line for statement per llvm guidelines

lldb/source/Interpreter/OptionValueDictionary.cpp

89–90 ↗

(On Diff #458809)

Use modern C++ iterator style:

for (const auto &value: m_values) {

92–93 ↗

(On Diff #458809)

This is a dictionary, we already have a key/value to use

lldb/test/API/commands/settings/TestSettings.py

806 ↗

(On Diff #458809)

we should test values that have strings that contain reserved JSON characters. I believe '\' can be in a string and will need to be double desensitized. Also string values that contain the string quote characters like a string "hello \"world\""

[hawkinsw]

lldb/test/API/functionalities/source-map/TestTargetSourceMap.py

25

Sorry if this comment is not helpful, but I was wondering ...

Could we use source_map_setting_path here? That would make future changes easier?

[yinghuitan]

Address review comments

[yinghuitan]

This comment has been deleted.

lldb/test/API/functionalities/source-map/TestTargetSourceMap.py

25

@hawkinsw, sorry, almost missed your comment. There is no single value for source map path but a pair of original/replacement. In this case, original is "." while the replacement is "src_dir" so I think src_dir makes sense here.

[hawkinsw]

lldb/test/API/functionalities/source-map/TestTargetSourceMap.py

25

No problem! I am sorry I miscommunicated! I just meant, could we write something like

self.runCmd('settings set %s . "%s"' % (source_map_setting_path, src_dir))

[clayborg]

lldb/include/lldb/API/SBDebugger.h

119 ↗

(On Diff #458932)

We still don't need two actual APIs in SBDebugger.h. Remove this.

136 ↗

(On Diff #458932)

We can default "settings = nullptr" here so users can call without anything, but we should have only 1 API function for getting the settings.

lldb/include/lldb/Interpreter/OptionValue.h

89 ↗

(On Diff #458932)

Add a comment here. Suggested comment added.

lldb/include/lldb/Interpreter/OptionValueChar.h

34–37 ↗

(On Diff #458932)

Since this is actually a character, it should store what ever the character value is. So if the character is zero, it should store zero, not a string "(null)"

lldb/source/API/SBDebugger.cpp

439–443 ↗

(On Diff #458932)

Remove this and only keep the one that takes a 'const char *setting"'

lldb/source/Interpreter/OptionValueDictionary.cpp

92 ↗

(On Diff #458932)

I think this move can be removed? Watch for warnings here. Some buildbots have warnings as errors which could cause the submission to be reverted.

lldb/test/API/commands/settings/TestSettings.py

799 ↗

(On Diff #458932)

We need to set a few more settings for anything we support the ToJSON for to ensure they function as expected.

lldb/test/API/functionalities/source-map/TestTargetSourceMap.py

25

Good idea!

[yinghuitan]

lldb/include/lldb/Interpreter/OptionValueChar.h

34–37 ↗

(On Diff #458932)

I am not sure about this. This mimics what OptionValueChar::DumpValue() does. Do we want to keep consistence here?

[yinghuitan]

Address review comments

[clayborg]

Just a comment fix and allow the "char" option value to be saved as zero and this will be good to go.

lldb/include/lldb/API/SBDebugger.h

120–121 ↗

(On Diff #459120)

This comment is no longer true right? Any setting args that are accepted by settings show not longer is true right? It is just a single setting value you can either specify or not right?

lldb/include/lldb/Interpreter/OptionValueChar.h

34–37 ↗

(On Diff #458932)

if we want to set/restore this value, it should have the correct value. We aren't trying to mimic dump value here. So we should allow it to be set to zero.

34–37 ↗

(On Diff #459120)

[yinghuitan]

Address review comments

[JDevlieghere]

lldb/include/lldb/Core/UserSettingsController.h

62–64 ↗

(On Diff #459396)

A flag doesn't scale if we ever decide we need another format. An enum would solve that and would eliminate the need for a comment /*is_json=*/. I think the two values could be Plain` (or Text) and JSON.

lldb/include/lldb/Interpreter/OptionValue.h

86–87 ↗

(On Diff #459396)

Is this done?

lldb/source/Core/UserSettingsController.cpp

63 ↗

(On Diff #459396)

Earlier you said that ToJSON should never return NULL. We should enforce that with an assert here.

lldb/source/Interpreter/OptionValueArray.cpp

80–81 ↗

(On Diff #459396)

lldb/source/Interpreter/OptionValueDictionary.cpp

89–91 ↗

(On Diff #459396)

[yinghuitan]

lldb/include/lldb/Interpreter/OptionValue.h

86–87 ↗

(On Diff #459396)

No. My goal is simply adding a public API for testing auto deduce source map feature, this patch is much larger than I wanted to achieve so it focuses adding skeleton of the API and implements a subset of OptionValue child classes leaving the remaining for future patches (the implementation is not hard but adding test coverage takes some time).

lldb/bindings/interface/SBTarget.i

#endif
     SetLaunchInfo (const lldb::SBLaunchInfo &launch_info);
 
     void SetCollectingStats(bool v);
 
     bool GetCollectingStats();
 
     lldb::SBStructuredData GetStatistics();
 
+    lldb::SBStructuredData GetSourceMap();
+
     bool
     operator == (const lldb::SBTarget &rhs) const;
 
     bool
     operator != (const lldb::SBTarget &rhs) const;
 
     lldb::SBValue
     EvaluateExpression (const char *expr);

lldb/include/lldb/API/SBTarget.h

 //===-- SBTarget.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
 //
 //===----------------------------------------------------------------------===//
 
public:
   bool GetCollectingStats();
 
   /// Returns a dump of the collected statistics.
   ///
   /// \return
   ///     A SBStructuredData with the statistics collected.
   lldb::SBStructuredData GetStatistics();
 
+  /// Returns a dump of the source map settings.
+  ///
+  /// \return
+  ///     A SBStructuredData with the source map entries collected.
+  lldb::SBStructuredData GetSourceMap();
+
   /// Return the platform object associated with the target.
   ///
   /// After return, the platform object should be checked for
   /// validity.
   ///
   /// \return
   ///     A platform object.
   lldb::SBPlatform GetPlatform();
public:
   lldb::SBSymbolContextList
   FindCompileUnits(const lldb::SBFileSpec &sb_file_spec);
 
   lldb::ByteOrder GetByteOrder();
 
   uint32_t GetAddressByteSize();
 
   const char *GetTriple();
-  
   const char *GetABIName();
 
   /// Architecture data byte width accessor
   ///
   /// \return
   /// The size in 8-bit (host) bytes of a minimum addressable
   /// unit from the Architecture's data bus
   uint32_t GetDataByteSize();

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"
 
 namespace lldb_private {
 
 class PathMappingList {
 public:
   typedef void (*ChangedCallback)(const PathMappingList &path_list,
                                   void *baton);
 
public:
 
   void Append(const PathMappingList &rhs, bool notify);
 
   void Clear(bool notify);
 
   // By default, dump all pairs.
   void Dump(Stream *s, int pair_index = -1);
 
+  llvm::json::Value ToJSON();
+
   bool IsEmpty() const { return m_pairs.empty(); }
 
   size_t GetSize() const { return m_pairs.size(); }
 
   bool GetPathsAtIndex(uint32_t idx, ConstString &path,
                        ConstString &new_path) const;
 
   void Insert(llvm::StringRef path, llvm::StringRef replacement,

lldb/source/API/SBTarget.cpp

 //===-- SBTarget.cpp ------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
SBStructuredData SBTarget::GetStatistics() {
   std::string json_str =
       llvm::formatv("{0:2}",
           DebuggerStats::ReportStatistics(target_sp->GetDebugger(),
                                           target_sp.get())).str();
   data.m_impl_up->SetObjectSP(StructuredData::ParseJSON(json_str));
   return data;
 }
 
+SBStructuredData SBTarget::GetSourceMap() {
+  LLDB_INSTRUMENT_VA(this);
+
+  SBStructuredData data;
+  TargetSP target_sp(GetSP());
+  if (!target_sp)
+    return data;
+
+  std::string json_str =
+      llvm::formatv("{0:2}",
+          target_sp->GetSourcePathMap().ToJSON()).str();
+  data.m_impl_up->SetObjectSP(StructuredData::ParseJSON(json_str));
+  return data;
+}
+
 void SBTarget::SetCollectingStats(bool v) {
   LLDB_INSTRUMENT_VA(this, v);
 
   TargetSP target_sp(GetSP());
   if (!target_sp)
     return;
   return DebuggerStats::SetCollectingStats(v);
 }
if (target_sp) {
     ConstString const_triple(triple.c_str());
     return const_triple.GetCString();
   }
   return nullptr;
 }
 
 const char *SBTarget::GetABIName() {
   LLDB_INSTRUMENT_VA(this);
-  
   TargetSP target_sp(GetSP());
   if (target_sp) {
     std::string abi_name(target_sp->GetABIName().str());
     ConstString const_name(abi_name.c_str());
     return const_name.GetCString();
   }
   return nullptr;
 }

lldb/source/Target/PathMappingList.cpp

 //===-- PathMappingList.cpp -----------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
for (index = 0; index < numPairs; ++index)
                 m_pairs[index].second.GetCString());
   } else {
     if (static_cast<unsigned int>(pair_index) < numPairs)
       s->Printf("%s -> %s", m_pairs[pair_index].first.GetCString(),
                 m_pairs[pair_index].second.GetCString());
   }
 }
 
+llvm::json::Value PathMappingList::ToJSON() {
+  llvm::json::Array entries;
+  for (const auto &pair : m_pairs) {
+    llvm::json::Object entry;
+    entry.try_emplace("first", pair.first.GetStringRef().str());
+    entry.try_emplace("second", pair.second.GetStringRef().str());
+    entries.emplace_back(std::move(entry));
+  }
+  return std::move(entries);
+}
+
 void PathMappingList::Clear(bool notify) {
   if (!m_pairs.empty())
     ++m_mod_id;
   m_pairs.clear();
   if (notify && m_callback)
     m_callback(*this, m_callback_baton);
 }
 

lldb/test/API/functionalities/source-map/TestTargetSourceMap.py

 import lldb
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test.decorators import *
+import json
 import os
 
 
 class TestTargetSourceMap(TestBase):
 
     @no_debug_info_test
+    def test_source_map_target_api(self):
+        """
+            Test that ensures SBTarget::GetSourceMap() API can correctly fetch
+            source mapping entries.
+        """
+        # Set the target soure map to map "./" to the current test directory
+        src_dir = self.getSourceDir()
+        src_path = os.path.join(src_dir, "main.c")
+        yaml_path = os.path.join(src_dir, "a.yaml")
+        yaml_base, ext = os.path.splitext(yaml_path)
+        obj_path = self.getBuildArtifact("main.o")
+        self.yaml2obj(yaml_path, obj_path)
+
+        # Create a target with the object file we just created from YAML
+        target = self.dbg.CreateTarget(obj_path)
+
+        initial_source_map = target.GetSourceMap()
+        self.assertEquals(initial_source_map.GetSize(), 0,
+            "Initial source map should be empty")
+
+        src_dir = self.getSourceDir()
+        self.runCmd('settings append target.source-map . "%s"' % src_dir)
+
+        source_map = target.GetSourceMap()
+        self.assertEquals(source_map.GetSize(), 1,
+            "source map should be have one appended entry")
+
+        stream = lldb.SBStream()
+        source_map.GetAsJSON(stream)
+        serialized_source_map = json.loads(stream.GetData())
+        self.assertEquals(serialized_source_map[0]["first"], ".",
+            "source map entry 'first' does not match")
+        self.assertEquals(serialized_source_map[0]["second"], src_dir,
+            "source map entry 'second' does not match")
+
+
+    @no_debug_info_test
     def test_source_map(self):
         """Test target.source-map' functionality."""
 
         def assertBreakpointWithSourceMap(src_path):
             # Set a breakpoint after we remap source and verify that it succeeds
             bp = target.BreakpointCreateByLocation(src_path, 2)
             self.assertEquals(bp.GetNumLocations(), 1,
                             "make sure breakpoint was resolved with map")