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

[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/API/SBTrace.h:31
+
+  /// Start tracing a live process using a provided configuration.
   ///
----------------



================
Comment at: lldb/include/lldb/API/SBTrace.h:37-38
   ///
-  /// \param[in] buf
-  ///     Buffer to write the trace data to.
+  ///     See the process tracing params from \a GetStartConfigurationHelp() for
+  ///     more information.
   ///
----------------



================
Comment at: lldb/include/lldb/API/SBTrace.h:44
+
+  /// Start tracing a live thread using a provided configuration.
   ///
----------------



================
Comment at: lldb/include/lldb/API/SBTrace.h:50-51
   ///
-  /// \param[in] thread_id
-  ///     Tracing could be started for the complete process or a
-  ///     single thread, in the first case the traceid obtained would
-  ///     map to all the threads existing within the process and the
-  ///     ones spawning later. The thread_id parameter can be used in
-  ///     such a scenario to select the trace data for a specific
-  ///     thread.
+  ///     See the thread tracing params from \a GetStartConfigurationHelp() for
+  ///     more information.
   ///
----------------
Modify to match the suggested process header doc mentioned above.


================
Comment at: lldb/include/lldb/API/SBTrace.h:57
 
-  /// Obtain any meta data as raw bytes for the tracing instance.
-  /// The input parameter definition is similar to the previous
-  /// function.
-  size_t GetMetaData(SBError &error, void *buf, size_t size, size_t offset = 0,
-                     lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID);
-
-  /// Stop the tracing instance. Stopping the trace will also
-  /// lead to deletion of any gathered trace data.
-  ///
-  /// \param[out] error
-  ///     An error explaining what went wrong.
+  /// Stop tracing a live process.
   ///
----------------



================
Comment at: lldb/include/lldb/API/SBTrace.h:63
 
-  /// Get the trace configuration being used for the trace instance.
-  /// The threadid in the SBTraceOptions needs to be set when the
-  /// configuration used by a specific thread is being requested.
-  ///
-  /// \param[out] options
-  ///     The trace options actually used by the trace instance
-  ///     would be filled by the API.
+  /// Stop tracing a live thread.
   ///
----------------
Can this only be used if we started tracing this specific thread? Or can we start tracing all threads in a process and then turn off tracing for individual threads? Might be good to specify this in these docs.


================
Comment at: lldb/include/lldb/API/SBTrace.h:91-92
+  /// deprecated
   lldb::ProcessWP m_opaque_wp;
+  lldb::TraceSP m_opaque_sp;
 };
----------------
Reverse these to match how they were laid out before. That way if anyone accesses m_opaque_wp it will match the type.


================
Comment at: lldb/include/lldb/Target/Target.h:1137-1141
+  llvm::Expected<lldb::TraceSP &> CreateTrace();
+
+  /// If a \a Trace object is present, this returns it, otherwise a new Trace is
+  /// created with \a Trace::CreateTrace.
   llvm::Expected<lldb::TraceSP &> GetTraceOrCreate();
----------------
Do we need both of these?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/constants.h:1
+//===-- constants.h ---------------------------------------------*- C++ -*-===//
+//
----------------
This header file name is a bit too generic and could end up causing include issues. I would rename to "TraceIntelPTConstants.h" to be safe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103500