[Lldb-commits] [PATCH] D88841: [intel pt] Refactor parsing

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/include/lldb/Target/Trace.h:77
   ///
-  /// \param[in] settings
-  ///     JSON object describing a trace.
+  /// \param[in] trace_session_file
+  ///     The contents of the trace session file describing the trace session.
----------------
Is this the top level object from the JSON file? I think the old name of "settings" was better?


================
Comment at: lldb/include/lldb/Target/Trace.h:95
   ///
-  /// \param[in] debugger
-  ///   The debugger instance where the targets are created.
-  ///
-  /// \param[in] settings
-  ///     JSON object describing a trace.
+  /// It should include at least these basic fields
   ///
----------------
Maybe some more comments here saying something about the fact that there are common settings required by all plug-ins and where to find this info, and also where this plug-in specific schema must appear within the JSON trace file? Maybe also mention that each trace plug-in can defined their own needed settings and that this information will appear in "bla bla" command if you type it so you can see the information required by each plug-in?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.h:51
+  lldb_private::FileSpec m_trace_file;
+  pt_cpu m_pt_cpu;
+};
----------------
Can we have different "pt_cpu" values for each thread? Or are we just storing this here because we don't control the Process class that we will use when doing IntelPT when we have a live process? Do we need this in the thread? Could we not just fetch the trace from the target and cast it to TraceIntelPT and call an accessor if we need the pt_cpu?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:40
+  ///
+  /// \param[in] trace_session_file
+  ///     The contents of the trace session file. See \a Trace::FindPlugin.
----------------
I still think "settings" is a better word for this argument.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:58
+  static lldb::TraceSP
+  CreateInstance(const llvm::json::Value *trace_session_file,
+                 llvm::StringRef *session_file_dir,
----------------
"const llvm::json::Value &" instead of a pointer? This can't ever be NULL and it would be nice to not each plug-in think it might need to check for NULL?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionParser.cpp:31-50
+  "processes": [
+    {
+      "pid": integer,
+      "triple": string, // llvm-triple
+      "threads": [
+        {
+          "tid": integer,
----------------
This common information should still be parsed by code in Trace.cpp.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionParser.h:21-42
+  struct JSONAddress {
+    lldb::addr_t value;
+  };
+
+  struct JSONModule {
+    std::string system_path;
+    llvm::Optional<std::string> file;
----------------
Seems like we should still parse this in Trace.cpp. Any common information shouldn't be duplicated or represented differently in different trace plug-ins IMHO. 


================
Comment at: lldb/source/Target/Trace.cpp:80-82
+    TraceSP instance = create_callback(/*trace_session_file*/ nullptr,
+                                       /*session_file_dir*/ nullptr,
+                                       /*debugger*/ nullptr, error);
----------------
Why have the create_callback take an of these arguments if they are always null?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88841/new/

https://reviews.llvm.org/D88841