[Lldb-commits] [PATCH] D123982: [trace][intel pt] Support events

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.


================
Comment at: lldb/include/lldb/lldb-enumerations.h:1151
+// Events that might happen during a trace session.
+FLAGS_ENUM(TraceEvents){
+    // Tracing was paused. If instructions were executed after pausing
----------------
What are some other events you could forsee adding to this enum?


================
Comment at: lldb/include/lldb/lldb-enumerations.h:1155
+    // should provide an error signalinig this data loss.
+    eTraceEventPaused = (1u << 0),
+};
----------------
Will this paused event always be due to a context switch? If so, we should rename it as such to reduce ambiguity


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:137
 
-void DecodedThread::AppendError(llvm::Error &&error, uint64_t tsc) {
+void DecodedThread::SetAsFailed(llvm::Error &&error) {
   AppendError(std::move(error));
----------------
What is being "Set" and why have the extra layer of indirection with the private `AppendError(Error &&e)`? The name is a little confusing imo because all this does is call `Append` internally.
Any reason to not have `AppendError` public and call that directly?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:75
+
+  /// \return \b true iff this struct holds a libipt error.
+  explicit operator bool() const;
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:225
+  ///   The number of instructions with associated events.
+  size_t GetInstructionsWithEventsCount() const;
+
----------------
What value does this providing the total number of instructions with events add? I think it could be more useful to provide a specific event as a parameter and return the total number of instructions with that particular event. wdyt?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:258
   /// first one. Otherwise, this map will be empty.
   std::map<size_t, uint64_t> m_instruction_timestamps;
   /// This is the chronologically last TSC that has been added.
----------------
I know this isn't related to these changes, but this should be updated to be consistent with the other `instruction_index -> XXX` maps in this class.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:258
   /// first one. Otherwise, this map will be empty.
   std::map<size_t, uint64_t> m_instruction_timestamps;
   /// This is the chronologically last TSC that has been added.
----------------
jj10306 wrote:
> I know this isn't related to these changes, but this should be updated to be consistent with the other `instruction_index -> XXX` maps in this class.



================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:68
+  /// instruction in the given \a DecodedInstruction.
+  ///
+  /// \return
----------------
consider adding docs on `insn` being an "out" parameter so it's a little more clear


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:186
+        // were lost.
+        insn.libipt_error = -pte_overflow;
+        break;
----------------
Why have the internal buffer overflow event as an error instead of a variant of the newly introduced Events enum?


================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInfo.py:52
+  Events:
+    Total number of instructions with events: 1
+
----------------
Will this always be 1 or is there a possibility that it could be different depending on the number of context switches?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123982