[Lldb-commits] [PATCH] D103500: [trace][intel-pt] Create basic SB API

Mon Jun 7 14:48:59 PDT 2021

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

================
Comment at: lldb/include/lldb/API/SBProcess.h:97

+  /// Deprecated
   lldb::SBThread GetSelectedThread() const;
----------------
Remove? This is not deprecated right?

================
Comment at: lldb/include/lldb/API/SBProcess.h:305-329
+  ///     The name of the shared library that you want to load.
   ///     If image_spec is a relative path, the relative path will be
   ///     appended to the search paths.
   ///     If the image_spec is an absolute path, just the basename is used.
   ///
   /// \param[in] paths
+  ///     A list of paths to search for the library whose basename is
----------------
Revert all diffs in this file I think right?

================
Comment at: lldb/include/lldb/API/SBStructuredData.h:103
   friend class SBBreakpointName;
+  friend class SBTrace;

----------------
Is this needed?

================
Comment at: lldb/include/lldb/API/SBTarget.h:856
+  ///     An error explaining why the trace couldn't be created.
+  lldb::SBTrace GetTraceOrCreate(SBError &error);
+
----------------
Maybe just name this "lldb::SBTrace CreateTrace(SBError &error);". This would return an error if the trace was already created. If we are creating the trace we need to option to specify settings right?

================
Comment at: lldb/include/lldb/API/SBTrace.h:26-30
+  /// Start tracing a live process using a default configuration.
   ///
-  /// \param[in] buf
-  ///     Buffer to write the trace data to.
+  /// \return
+  ///     An error explaining any failures.
+  SBError StartProcess();
----------------
I would remove this and just use the next function. People can just default construct a SBStructuredData and pass it in if they don't want to set any settings.

================
Comment at: lldb/include/lldb/API/SBTrace.h:40-52
+  ///     For intel-pt:
+  ///       - int threadBufferSize (defaults to 4096 bytes):
+  ///         Trace size in bytes per thread. It must be a power of 2 greater
+  ///         than or equal to 4096 (2^12). The trace is circular keeping the
+  ///         the most recent data.
+  ///       - int processBufferSizeLimit (defaults to 500 MB):
+  ///         Maximum total trace size per process in bytes. This limit applies
----------------
It would be nice to not document specific trace settings in this header file. It should be possible to add a way to get the schema from a "SBTrace" object right? Something like "const char * SBTrace::GetSchema()" and then tell users to check that for how and what settings are available and that if none are set that good defaults will be used?

================
Comment at: lldb/include/lldb/API/SBTrace.h:56
+  ///     An error explaining any failures.
+  SBError StartProcess(const SBStructuredData &configuration);

----------------
Maybe rename to just "Start" and documentation will make it clear. We can also overload the Start below and add a SBThread argument. See comment below.

================
Comment at: lldb/include/lldb/API/SBTrace.h:62
+  ///     An error explaining any failures.
+  SBError StartThread(const SBThread &thread);

----------------
Remove this overload, and just use the one below. People can just default construct a SBStructuredData if they don't want to modify any settings.

================
Comment at: lldb/include/lldb/API/SBTrace.h:80-81
+  ///     An error explaining any failures.
+  SBError StartThread(const SBThread &thread,
+                      const SBStructuredData &configuration);

----------------
Overload the "Start" method and just add a SBThread parameter to make it clear we are tracing just a specific thread.

================
Comment at: lldb/include/lldb/API/SBTrace.h:87
+  ///     An error explaining any failures.
+  SBError StopProcess();
+
----------------
Rename to "Stop"

================
Comment at: lldb/include/lldb/API/SBTrace.h:93
+  ///     An error explaining any failures.
+  SBError StopThread(const SBThread &thread);

----------------
Rename to "Stop" and allow the overload to happen with the extra thread argument. Do we have the ability to start thread 1, go for a while, start thread 2, go for a while, stop thread 1, go for a while and then stop thread 2?

================
Comment at: lldb/include/lldb/API/SBTrace.h:102-104
+  void StopTrace(SBError &error,
+                 lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID);

----------------
Is this now deprecated?

================
Comment at: lldb/include/lldb/API/SBTrace.h:123
+protected:
+  lldb::TraceSP m_opaque_sp;
 };
----------------
Having just one argument will change the layout of the object. We need to keep "lldb::ProcessWP m_opaque_wp;" in from before so that layout of these objects doesn't change. We can mark it as deprecated, but it needs to be there.

================
Comment at: lldb/include/lldb/API/SBTraceOptions.h:16

+/// Deprecated
 class LLDB_API SBTraceOptions {
----------------
We can probably just mark the entire class as deprecated and avoid marking each method.

================
Comment at: lldb/include/lldb/Target/Trace.h:235
+  ///     \a llvm::Error otherwise.
+  virtual llvm::Error StartProcess(
+      StructuredData::ObjectSP configuration = StructuredData::ObjectSP()) = 0;
----------------
rename to "Start" and allow overload with next function based on parameters.

================
Comment at: lldb/include/lldb/Target/Trace.h:251
+  ///     \a llvm::Error otherwise.
+  virtual llvm::Error StartThreads(
+      const std::vector<lldb::tid_t> &tids,
----------------
Rename to "Start"

================
Comment at: lldb/source/Plugins/Trace/intel-pt/constants.h:8
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_CONSTANTS_H
----------------
Add include header for "size_t"

================
Comment at: lldb/source/Target/Trace.cpp:294
     return;
+
   s.Printf("    ");
----------------
Revert diffs to this file

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103500