[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.

[Ravitheja Addepally via Phabricator via lldb-commits]

ravitheja added inline comments.


================
Comment at: include/lldb/API/SBProcess.h:269
+  lldb::user_id_t StartTrace(SBTraceOptions &sboptions, lldb::SBError &sberror,
+                             lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID);
+
----------------
clayborg wrote:
> Should the thread be in the SBTraceOptions? What if you want to trace N threads, but not all? Seems like this would be better represented in the SBTraceOptions.
We wanted to keep some symmetry among the APIs , the threadid could go in the TraceOptions as well.


================
Comment at: include/lldb/API/SBProcess.h:281-288
+  /// @param[in] buf
+  ///     Buffer to write the trace data to.
+  ///
+  /// @param[in] size
+  ///     The size of the buffer used to read the data. This is
+  ///     also the size of the data intended to read. It is also
+  ///     possible to partially read the trace data for some trace
----------------
clayborg wrote:
> Should we make a SBTraceData object instead of getting a buffer of bytes? I haven't looked past this code to the code below, so I am not sure how this data will be used. Just thinking out loud here.
So the idea we have is that we don't want to interpret the trace data inside lldb. Now one could go and make a class SBTraceData but it would just be a buffer of trace data, which is why we left it as a buffer of bytes.


================
Comment at: include/lldb/API/SBProcess.h:290-291
+  ///
+  /// @param[in] offset
+  ///     The start offset to begin reading the trace data.
+  ///
----------------
clayborg wrote:
> Should the trace data not be a stream? Can you elaborate on why you think you need the offset?
For processor Trace implementation buffer was more suitable for decoding purposes. The reason for offset was that it gives the plugin the chance to request partial segments of the trace data which it could decode.


https://reviews.llvm.org/D29581