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

[walter erquinigo via Phabricator via lldb-commits]

wallace 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
----------------
jj10306 wrote:
> What are some other events you could forsee adding to this enum?
paging, execution mode change (from 64 to 32 bit mode or viceversa), power state change, core bus ratio change (i.e. frequency change). Some of these might include an additional payload, which should be gotten using other APIs.


================
Comment at: lldb/include/lldb/lldb-enumerations.h:1155
+    // should provide an error signalinig this data loss.
+    eTraceEventPaused = (1u << 0),
+};
----------------
jj10306 wrote:
> Will this paused event always be due to a context switch? If so, we should rename it as such to reduce ambiguity
not really. Two examples:
- context switch
- ip filtering -> in this case tracing stopped at CPU level because an unwanted memory region was executed. In this case the process is still running


================
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));
----------------
jj10306 wrote:
> 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?
The reason is that I want all callers to invoke Append() except for exceptional cases. The benefit of Append is that can provide an error with events and timing information. SetAsFailed is receiving only an error without additional data to be used only in extreme situations where there's no event or timing data available, which happens if the decoding setup fails.


================
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;
----------------
jj10306 wrote:
> 
oh, i wanted to mean if and only if. Or is that too mathematical and less colloquial? 


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:225
+  ///   The number of instructions with associated events.
+  size_t GetInstructionsWithEventsCount() const;
+
----------------
jj10306 wrote:
> 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?
good idea


================
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:
> 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.
> 
in this case we can't do that, because when doing random accesses in the trace, we need to quickly find the most recent TSC for the new instruction, which is done with binary search using a map. This is impossible in a hash map like DenseMap


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


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:186
+        // were lost.
+        insn.libipt_error = -pte_overflow;
+        break;
----------------
jj10306 wrote:
> Why have the internal buffer overflow event as an error instead of a variant of the newly introduced Events enum?
that's because an overflow means that we lost data, and therefore we should show that as an error in the trace. Remember that errors mean that instructions were lost, which is exactly what happens in an overflow case. This is not just an event that could be ignored.


================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInfo.py:52
+  Events:
+    Total number of instructions with events: 1
+
----------------
jj10306 wrote:
> Will this always be 1 or is there a possibility that it could be different depending on the number of context switches?
This is using trace load, which uses an offline trace, so it will never change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123982