[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D85705#2240249 <https://reviews.llvm.org/D85705#2240249>, @wallace wrote:

> Given that this feature is intended to be used by non debugger developers, I want to stick to using JSON as a default input format, as it's more ubiquitous than YAML and I prefer to make this decision based on the user experience than on the technical solution.

Alright, fair enough I guess. In that case I would prefer to split off the parser from the Trace data structure.

> I like the YAML parser though, but converting JSON to YAML for parsing might throw away relevant error messaging.

I don't think anyone suggested this.

One more thing about the parser. I really like the usage of llvm::Error/Excepted/Optional in this path btw. However, these functions take a non-const ref to classes like Process, Thread, etc. It seems like they might be left in a potentially inconsistent state when we error out. Is that something to worry about or are we guaranteed to discard them in the caller if an error occurred? Could they for instance return an `Expected<Debugger>` instead?



================
Comment at: lldb/include/lldb/Target/Trace.h:86
+  static llvm::Expected<lldb::TraceSP>
+  FindPlugin(StructuredData::Dictionary settings, const char *settings_dir);
+
----------------
StringRef


================
Comment at: lldb/include/lldb/Target/Trace.h:105
+protected:
+  Trace(StructuredData::Dictionary settings, const char *settings_dir)
+      : m_settings(std::move(settings)), m_settings_dir(settings_dir) {}
----------------
StringRef


================
Comment at: lldb/include/lldb/Target/Trace.h:127
+protected:
+  virtual const char *GetPluginSchema() = 0;
+
----------------
StringRef


================
Comment at: lldb/include/lldb/Target/Trace.h:150
+public:
+  const char *GetSchema();
+  /// \}
----------------
StringRef


================
Comment at: lldb/include/lldb/Utility/StructuredData.h:196
   private:
+    llvm::Error CreateWrongTypeError(const char *type) {
+      StreamString object_stream;
----------------
StringRef


================
Comment at: lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt:3
+
+if (NOT LLDB_BUILD_INTEL_PT)
+  return()
----------------
Move this up to where this directory is included. e.g.
```
if (LLDB_BUILD_INTEL_PT)
  add_subdirectory(intel-pt)
endif()
```
That way a change to the CMake file won't trigger a reconfig if `LLDB_BUILD_INTEL_PT` is false.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt:22
+  endif()
+  find_library(LIBIPT_LIBRARY ipt PATHS ${LIBIPT_LIBRARY_PATH})
+endif()
----------------
Why not pass this in the first place? `PATHS` will be checked *in addition* to the default locations.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt:26
+if (NOT LIBIPT_LIBRARY)
+  message (FATAL_ERROR "libipt library not found")
+endif()
----------------
If you `REQUIRED` to `find_library` CMake will take care of the error reporting. 


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:22
+
+  /// Static Functions
+  /// \{
----------------
Redundant. 


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:54
+  /// \{
+  llvm::Error
+  ParsePTCPU(const lldb_private::StructuredData::Dictionary &plugin);
----------------
I think it would be nice to separate the plugin from the parser.


================
Comment at: lldb/source/Target/Trace.cpp:27
+llvm::Expected<lldb::TraceSP> Trace::FindPlugin(StructuredData::Dictionary info,
+                                                const char *info_dir) {
+  llvm::Expected<StructuredData::Dictionary *> plugin =
----------------
StringRef


================
Comment at: lldb/source/Target/Trace.cpp:50
+
+const char *Trace::GetSchema() {
+  static std::string schema;
----------------
StringRef


================
Comment at: lldb/source/Target/Trace.cpp:146
+
+llvm::Expected<addr_t> Trace::ParseAddress(const StringRef &address_str) {
+  addr_t address;
----------------
`StringRef` should always be passed by value.


================
Comment at: lldb/test/API/commands/trace/intelpt-trace/trace.json:2
+{
+  "plugin": {
+    "type": "intel-pt",
----------------
It seems like the key here should be trace rather than "plugin"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705