[intel-pt] Refactor the JSON parsing

Recently https://reviews.llvm.org/D88103 introduced a nice API for
converting a JSON object into C++ types, which include nice error
messaging.

I'm using that new functioniality to perform the parsing in a much more
elegant way. As a result, the code looks simpler and more maintainable,
as we aren't parsing anymore individual fields manually.

I updated the test cases accordingly.

Differential Revision: https://reviews.llvm.org/D88264

4 files changed, 140 insertions, 283 deletions

diff --git a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp
index 2430f77..c67822d 100644
--- a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp
+++ b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp
@@ -24,45 +24,21 @@ StringRef TraceIntelPTSettingsParser::GetPluginSchema() {
   })";
 }
 
-llvm::Error TraceIntelPTSettingsParser::ParsePTCPU(const json::Object &trace) {
-  llvm::Expected<const json::Object &> pt_cpu =
-      json_helpers::GetObjectOrError(trace, "pt_cpu");
-  if (!pt_cpu)
-    return pt_cpu.takeError();
-
-  llvm::Expected<llvm::StringRef> vendor =
-      json_helpers::GetStringOrError(*pt_cpu, "vendor");
-  if (!vendor)
-    return vendor.takeError();
-
-  llvm::Expected<int64_t> family =
-      json_helpers::GetIntegerOrError(*pt_cpu, "family");
-  if (!family)
-    return family.takeError();
-
-  llvm::Expected<int64_t> model =
-      json_helpers::GetIntegerOrError(*pt_cpu, "model");
-  if (!model)
-    return model.takeError();
-
-  llvm::Expected<int64_t> stepping =
-      json_helpers::GetIntegerOrError(*pt_cpu, "stepping");
-  if (!stepping)
-    return stepping.takeError();
-
-  m_pt_cpu = {vendor->compare("intel") == 0 ? pcv_intel : pcv_unknown,
-              static_cast<uint16_t>(*family), static_cast<uint8_t>(*model),
-              static_cast<uint8_t>(*stepping)};
-  return llvm::Error::success();
+void TraceIntelPTSettingsParser::ParsePTCPU(const JSONPTCPU &pt_cpu) {
+  m_pt_cpu = {pt_cpu.vendor.compare("intel") == 0 ? pcv_intel : pcv_unknown,
+              static_cast<uint16_t>(pt_cpu.family),
+              static_cast<uint8_t>(pt_cpu.model),
+              static_cast<uint8_t>(pt_cpu.stepping)};
 }
 
-llvm::Error TraceIntelPTSettingsParser::ParsePluginSettings() {
-  llvm::Expected<const json::Object &> trace =
-      json_helpers::GetObjectOrError(m_settings, "trace");
-  if (!trace)
-    return trace.takeError();
-  if (llvm::Error err = ParsePTCPU(*trace))
-    return err;
+llvm::Error TraceIntelPTSettingsParser::ParsePluginSettings(
+    const llvm::json::Value &plugin_settings) {
+  json::Path::Root root("settings.trace");
+  JSONIntelPTSettings settings;
+  if (!json::fromJSON(plugin_settings, settings, root))
+    return CreateJSONError(root, plugin_settings);
+
+  ParsePTCPU(settings.pt_cpu);
 
   m_trace.m_pt_cpu = m_pt_cpu;
   return llvm::Error::success();
diff --git a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.h b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.h
index b8aaa10..8c9b9ea 100644
--- a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.h
+++ b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.h
@@ -18,8 +18,18 @@
 class TraceIntelPT;
 
 class TraceIntelPTSettingsParser : public lldb_private::TraceSettingsParser {
-
 public:
+  struct JSONPTCPU {
+    std::string vendor;
+    int64_t family;
+    int64_t model;
+    int64_t stepping;
+  };
+
+  struct JSONIntelPTSettings {
+    JSONPTCPU pt_cpu;
+  };
+
   TraceIntelPTSettingsParser(TraceIntelPT &trace)
       : lldb_private::TraceSettingsParser((lldb_private::Trace &)trace),
         m_trace(trace) {}
@@ -27,13 +37,37 @@ public:
 protected:
   llvm::StringRef GetPluginSchema() override;
 
-  llvm::Error ParsePluginSettings() override;
+  llvm::Error
+  ParsePluginSettings(const llvm::json::Value &plugin_settings) override;
 
 private:
-  llvm::Error ParsePTCPU(const llvm::json::Object &trace);
+  void ParsePTCPU(const JSONPTCPU &pt_cpu);
 
   TraceIntelPT &m_trace;
   pt_cpu m_pt_cpu;
 };
 
+namespace llvm {
+namespace json {
+
+inline bool fromJSON(const llvm::json::Value &value,
+                     TraceIntelPTSettingsParser::JSONPTCPU &pt_cpu,
+                     llvm::json::Path path) {
+  llvm::json::ObjectMapper o(value, path);
+  return o && o.map("vendor", pt_cpu.vendor) &&
+         o.map("family", pt_cpu.family) && o.map("model", pt_cpu.model) &&
+         o.map("stepping", pt_cpu.stepping);
+}
+
+inline bool
+fromJSON(const llvm::json::Value &value,
+         TraceIntelPTSettingsParser::JSONIntelPTSettings &intel_pt_settings,
+         llvm::json::Path path) {
+  llvm::json::ObjectMapper o(value, path);
+  return o && o.map("pt_cpu", intel_pt_settings.pt_cpu);
+}
+
+} // namespace json
+} // namespace llvm
+
 #endif // liblldb_TraceIntelPTSettingsParser_h_
diff --git a/lldb/source/Target/Trace.cpp b/lldb/source/Target/Trace.cpp
index afe4ee2..8d305c9 100644
--- a/lldb/source/Target/Trace.cpp
+++ b/lldb/source/Target/Trace.cpp
@@ -18,30 +18,48 @@ using namespace lldb;
 using namespace lldb_private;
 using namespace llvm;
 
+// Helper structs used to extract the type of a trace settings json without
+// having to parse the entire object.
+
+struct JSONSimplePluginSettings {
+  std::string type;
+};
+
+struct JSONSimpleTraceSettings {
+  JSONSimplePluginSettings trace;
+};
+
+namespace llvm {
+namespace json {
+
+bool fromJSON(const json::Value &value,
+              JSONSimplePluginSettings &plugin_settings, json::Path path) {
+  json::ObjectMapper o(value, path);
+  return o && o.map("type", plugin_settings.type);
+}
+
+bool fromJSON(const json::Value &value, JSONSimpleTraceSettings &settings,
+              json::Path path) {
+  json::ObjectMapper o(value, path);
+  return o && o.map("trace", settings.trace);
+}
+
+} // namespace json
+} // namespace llvm
+
 llvm::Expected<lldb::TraceSP> Trace::FindPlugin(Debugger &debugger,
                                                 const json::Value &settings,
                                                 StringRef info_dir) {
-  llvm::Expected<const json::Object &> settings_obj =
-      json_helpers::ToObjectOrError(settings);
-  if (!settings_obj)
-    return settings_obj.takeError();
-
-  llvm::Expected<const json::Object &> trace =
-      json_helpers::GetObjectOrError(*settings_obj, "trace");
-  if (!trace)
-    return trace.takeError();
-
-  llvm::Expected<StringRef> type =
-      json_helpers::GetStringOrError(*trace, "type");
-  if (!type)
-    return type.takeError();
-
-  ConstString plugin_name(*type);
+  JSONSimpleTraceSettings json_settings;
+  json::Path::Root root("settings");
+  if (!json::fromJSON(settings, json_settings, root))
+    return root.getError();
+
+  ConstString plugin_name(json_settings.trace.type);
   auto create_callback = PluginManager::GetTraceCreateCallback(plugin_name);
   if (create_callback) {
     TraceSP instance = create_callback();
-    if (llvm::Error err =
-            instance->ParseSettings(debugger, *settings_obj, info_dir))
+    if (llvm::Error err = instance->ParseSettings(debugger, settings, info_dir))
       return std::move(err);
     return instance;
   }
@@ -65,7 +83,7 @@ llvm::Expected<lldb::TraceSP> Trace::FindPlugin(StringRef name) {
 }
 
 llvm::Error Trace::ParseSettings(Debugger &debugger,
-                                 const llvm::json::Object &settings,
+                                 const llvm::json::Value &settings,
                                  llvm::StringRef settings_dir) {
   if (llvm::Error err =
           CreateParser()->ParseSettings(debugger, settings, settings_dir))
diff --git a/lldb/source/Target/TraceSettingsParser.cpp b/lldb/source/Target/TraceSettingsParser.cpp
index 9021d93..cc3369f 100644
--- a/lldb/source/Target/TraceSettingsParser.cpp
+++ b/lldb/source/Target/TraceSettingsParser.cpp
@@ -18,105 +18,6 @@ using namespace lldb;
 using namespace lldb_private;
 using namespace llvm;
 
-namespace json_helpers {
-
-llvm::Error CreateWrongTypeError(const json::Value &value,
-                                 llvm::StringRef type) {
-  std::string s;
-  llvm::raw_string_ostream os(s);
-  os << llvm::formatv("JSON value is expected to be \"{0}\".\nValue:\n{1:2}",
-                      type, value);
-  os.flush();
-
-  return llvm::createStringError(std::errc::invalid_argument, os.str().c_str());
-}
-
-llvm::Expected<int64_t> ToIntegerOrError(const json::Value &value) {
-  llvm::Optional<int64_t> v = value.getAsInteger();
-  if (v.hasValue())
-    return *v;
-  return CreateWrongTypeError(value, "integer");
-}
-
-llvm::Expected<StringRef> ToStringOrError(const json::Value &value) {
-  llvm::Optional<StringRef> v = value.getAsString();
-  if (v.hasValue())
-    return *v;
-  return CreateWrongTypeError(value, "string");
-}
-
-llvm::Expected<const json::Array &> ToArrayOrError(const json::Value &value) {
-  if (const json::Array *v = value.getAsArray())
-    return *v;
-  return CreateWrongTypeError(value, "array");
-}
-
-llvm::Expected<const json::Object &> ToObjectOrError(const json::Value &value) {
-  if (const json::Object *v = value.getAsObject())
-    return *v;
-  return CreateWrongTypeError(value, "object");
-}
-
-llvm::Error CreateMissingKeyError(json::Object obj, llvm::StringRef key) {
-  std::string str;
-  llvm::raw_string_ostream os(str);
-  os << llvm::formatv(
-      "JSON object is missing the \"{0}\" field.\nValue:\n{1:2}", key,
-      json::Value(std::move(obj)));
-  os.flush();
-
-  return llvm::createStringError(std::errc::invalid_argument, os.str().c_str());
-}
-
-llvm::Expected<const json::Value &> GetValueOrError(const json::Object &obj,
-                                                    StringRef key) {
-  if (const json::Value *v = obj.get(key))
-    return *v;
-  else
-    return CreateMissingKeyError(obj, key);
-}
-
-llvm::Expected<int64_t> GetIntegerOrError(const json::Object &obj,
-                                          StringRef key) {
-  if (llvm::Expected<const json::Value &> v = GetValueOrError(obj, key))
-    return ToIntegerOrError(*v);
-  else
-    return v.takeError();
-}
-
-llvm::Expected<StringRef> GetStringOrError(const json::Object &obj,
-                                           StringRef key) {
-  if (llvm::Expected<const json::Value &> v = GetValueOrError(obj, key))
-    return ToStringOrError(*v);
-  else
-    return v.takeError();
-}
-
-llvm::Expected<const json::Array &> GetArrayOrError(const json::Object &obj,
-                                                    StringRef key) {
-  if (llvm::Expected<const json::Value &> v = GetValueOrError(obj, key))
-    return ToArrayOrError(*v);
-  else
-    return v.takeError();
-}
-
-llvm::Expected<const json::Object &> GetObjectOrError(const json::Object &obj,
-                                                      StringRef key) {
-  if (llvm::Expected<const json::Value &> v = GetValueOrError(obj, key))
-    return ToObjectOrError(*v);
-  else
-    return v.takeError();
-}
-
-llvm::Expected<llvm::Optional<StringRef>>
-GetOptionalStringOrError(const json::Object &obj, StringRef key) {
-  if (const json::Value *v = obj.get(key))
-    return ToStringOrError(*v);
-  return llvm::None;
-}
-
-} // namespace json_helpers
-
 StringRef TraceSettingsParser::GetSchema() {
   static std::string schema;
   if (schema.empty()) {
@@ -156,89 +57,34 @@ void TraceSettingsParser::NormalizePath(FileSpec &file_spec) {
     file_spec.PrependPathComponent(m_settings_dir);
 }
 
-llvm::Error TraceSettingsParser::ParseThread(ProcessSP &process_sp,
-                                             const json::Object &thread) {
-  llvm::Expected<lldb::tid_t> raw_tid =
-      json_helpers::GetIntegerOrError(thread, "tid");
-  if (!raw_tid)
-    return raw_tid.takeError();
-  lldb::tid_t tid = static_cast<lldb::tid_t>(*raw_tid);
+void TraceSettingsParser::ParseThread(ProcessSP &process_sp,
+                                      const JSONThread &thread) {
+  lldb::tid_t tid = static_cast<lldb::tid_t>(thread.tid);
 
-  if (llvm::Expected<StringRef> trace_file =
-          json_helpers::GetStringOrError(thread, "traceFile")) {
-    FileSpec spec(*trace_file);
-    NormalizePath(spec);
-    m_thread_to_trace_file_map[process_sp->GetID()][tid] = spec;
-  } else
-    return trace_file.takeError();
+  FileSpec spec(thread.trace_file);
+  NormalizePath(spec);
+  m_thread_to_trace_file_map[process_sp->GetID()][tid] = spec;
 
   ThreadSP thread_sp(new HistoryThread(*process_sp, tid, /*callstack*/ {}));
   process_sp->GetThreadList().AddThread(thread_sp);
-  return llvm::Error::success();
-}
-
-llvm::Error TraceSettingsParser::ParseThreads(ProcessSP &process_sp,
-                                              const json::Object &process) {
-  llvm::Expected<const json::Array &> threads =
-      json_helpers::GetArrayOrError(process, "threads");
-  if (!threads)
-    return threads.takeError();
-
-  for (const json::Value &thread_val : *threads) {
-    llvm::Expected<const json::Object &> thread =
-        json_helpers::ToObjectOrError(thread_val);
-    if (!thread)
-      return thread.takeError();
-    if (llvm::Error err = ParseThread(process_sp, *thread))
-      return err;
-  }
-  return llvm::Error::success();
-}
-
-static llvm::Expected<addr_t> ParseAddress(StringRef address_str) {
-  addr_t address;
-  if (address_str.getAsInteger(0, address))
-    return createStringError(std::errc::invalid_argument,
-                             "\"%s\" does not represent an integer",
-                             address_str.data());
-  return address;
 }
 
 llvm::Error TraceSettingsParser::ParseModule(TargetSP &target_sp,
-                                             const json::Object &module) {
-  llvm::Expected<StringRef> load_address_str =
-      json_helpers::GetStringOrError(module, "loadAddress");
-  if (!load_address_str)
-    return load_address_str.takeError();
-  llvm::Expected<addr_t> load_address = ParseAddress(*load_address_str);
-  if (!load_address)
-    return load_address.takeError();
-
-  llvm::Expected<StringRef> system_path =
-      json_helpers::GetStringOrError(module, "systemPath");
-  if (!system_path)
-    return system_path.takeError();
-  FileSpec system_file_spec(*system_path);
+                                             const JSONModule &module) {
+  FileSpec system_file_spec(module.system_path);
   NormalizePath(system_file_spec);
 
-  llvm::Expected<llvm::Optional<StringRef>> file =
-      json_helpers::GetOptionalStringOrError(module, "file");
-  if (!file)
-    return file.takeError();
-  FileSpec local_file_spec(file->hasValue() ? **file : *system_path);
+  FileSpec local_file_spec(module.file.hasValue() ? *module.file
+                                                  : module.system_path);
   NormalizePath(local_file_spec);
 
   ModuleSpec module_spec;
   module_spec.GetFileSpec() = local_file_spec;
   module_spec.GetPlatformFileSpec() = system_file_spec;
-  module_spec.SetObjectOffset(*load_address);
+  module_spec.SetObjectOffset(module.load_address.value);
 
-  llvm::Expected<llvm::Optional<StringRef>> uuid_str =
-      json_helpers::GetOptionalStringOrError(module, "uuid");
-  if (!uuid_str)
-    return uuid_str.takeError();
-  if (uuid_str->hasValue())
-    module_spec.GetUUID().SetFromStringRef(**uuid_str);
+  if (module.uuid.hasValue())
+    module_spec.GetUUID().SetFromStringRef(*module.uuid);
 
   Status error;
   ModuleSP module_sp =
@@ -246,38 +92,12 @@ llvm::Error TraceSettingsParser::ParseModule(TargetSP &target_sp,
   return error.ToError();
 }
 
-llvm::Error TraceSettingsParser::ParseModules(TargetSP &target_sp,
-                                              const json::Object &process) {
-  llvm::Expected<const json::Array &> modules =
-      json_helpers::GetArrayOrError(process, "modules");
-  if (!modules)
-    return modules.takeError();
-
-  for (const json::Value &module_val : *modules) {
-    llvm::Expected<const json::Object &> module =
-        json_helpers::ToObjectOrError(module_val);
-    if (!module)
-      return module.takeError();
-    if (llvm::Error err = ParseModule(target_sp, *module))
-      return err;
-  }
-  return llvm::Error::success();
-}
-
 llvm::Error TraceSettingsParser::ParseProcess(Debugger &debugger,
-                                              const json::Object &process) {
-  llvm::Expected<int64_t> pid = json_helpers::GetIntegerOrError(process, "pid");
-  if (!pid)
-    return pid.takeError();
-
-  llvm::Expected<StringRef> triple =
-      json_helpers::GetStringOrError(process, "triple");
-  if (!triple)
-    return triple.takeError();
-
+                                              const JSONProcess &process) {
   TargetSP target_sp;
   Status error = debugger.GetTargetList().CreateTarget(
-      debugger, /*user_exe_path*/ llvm::StringRef(), *triple, eLoadDependentsNo,
+      debugger, /*user_exe_path*/ llvm::StringRef(), process.triple,
+      eLoadDependentsNo,
       /*platform_options*/ nullptr, target_sp);
 
   if (!target_sp)
@@ -289,55 +109,64 @@ llvm::Error TraceSettingsParser::ParseProcess(Debugger &debugger,
   ProcessSP process_sp(target_sp->CreateProcess(
       /*listener*/ nullptr, /*plugin_name*/ llvm::StringRef(),
       /*crash_file*/ nullptr));
-  process_sp->SetID(static_cast<lldb::pid_t>(*pid));
+  process_sp->SetID(static_cast<lldb::pid_t>(process.pid));
 
-  if (llvm::Error err = ParseThreads(process_sp, process))
-    return err;
+  for (const JSONThread &thread : process.threads)
+    ParseThread(process_sp, thread);
 
-  return ParseModules(target_sp, process);
+  for (const JSONModule &module : process.modules) {
+    if (llvm::Error err = ParseModule(target_sp, module))
+      return err;
+  }
+  return llvm::Error::success();
 }
 
-llvm::Error TraceSettingsParser::ParseProcesses(Debugger &debugger) {
-  llvm::Expected<const json::Array &> processes =
-      json_helpers::GetArrayOrError(m_settings, "processes");
-  if (!processes)
-    return processes.takeError();
+llvm::Error
+TraceSettingsParser::CreateJSONError(json::Path::Root &root,
+                                     const llvm::json::Value &value) {
+  std::string err;
+  raw_string_ostream os(err);
+  root.printErrorContext(value, os);
+  return createStringError(std::errc::invalid_argument,
+                           "%s\n\nContext:\n%s\n\nSchema:\n%s",
+                           llvm::toString(root.getError()).c_str(),
+                           os.str().c_str(), GetSchema().data());
+}
 
-  for (const json::Value &process_val : *processes) {
-    llvm::Expected<const json::Object &> process =
-        json_helpers::ToObjectOrError(process_val);
-    if (!process)
-      return process.takeError();
-    if (llvm::Error err = ParseProcess(debugger, *process))
+llvm::Error
+TraceSettingsParser::ParseSettingsImpl(Debugger &debugger,
+                                       const llvm::json::Value &raw_settings) {
+  json::Path::Root root("settings");
+  JSONTraceSettings settings;
+  if (!json::fromJSON(raw_settings, settings, root))
+    return CreateJSONError(root, raw_settings);
+
+  for (const JSONProcess &process : settings.processes) {
+    if (llvm::Error err = ParseProcess(debugger, process))
       return err;
   }
-  return llvm::Error::success();
-}
 
-llvm::Error TraceSettingsParser::ParseSettingsImpl(Debugger &debugger) {
-  if (llvm::Error err = ParseProcesses(debugger))
-    return err;
-  return ParsePluginSettings();
+  json::Object plugin_obj = *raw_settings.getAsObject()->getObject("trace");
+  json::Value plugin_settings(std::move(plugin_obj));
+  return ParsePluginSettings(plugin_settings);
 }
 
 llvm::Error
 TraceSettingsParser::ParseSettings(Debugger &debugger,
-                                   const llvm::json::Object &settings,
+                                   const llvm::json::Value &raw_settings,
                                    llvm::StringRef settings_dir) {
-  m_settings = settings;
   m_settings_dir = settings_dir.str();
-  if (llvm::Error err = ParseSettingsImpl(debugger)) {
+
+  if (llvm::Error err = ParseSettingsImpl(debugger, raw_settings)) {
     // We clean all the targets that were created internally, which should leave
     // the debugger unchanged
     for (auto target_sp : m_targets)
       debugger.GetTargetList().DeleteTarget(target_sp);
 
-    return createStringError(std::errc::invalid_argument, "%s\nSchema:\n%s",
-                             llvm::toString(std::move(err)).c_str(),
-                             GetSchema().data());
+    return err;
   }
 
-  m_trace.m_settings = m_settings;
+  m_trace.m_settings = *raw_settings.getAsObject();
   m_trace.m_settings_dir = m_settings_dir;
   m_trace.m_thread_to_trace_file_map = m_thread_to_trace_file_map;
   m_trace.m_targets = m_targets;