[Lldb-commits] [PATCH] D128576: [trace] Make events first class items in the trace cursor and rework errors

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

This is awesome work - the code is much more understandable, so thanks for doing this!

lgtm overall, just left a couple final minor comments. I think we can continue to iterate/improve this design, so we can discuss those improvements for future diffs. 
Also, let me know what you think about how to support the two different types of disable events - this doesn't need to be done in this diff but it would be extremely useful as we begin to explore kernel tracing since this will most likely require us to understand any differences between user only, kernel only, or user + kernel tracing.



================
Comment at: lldb/include/lldb/lldb-enumerations.h:1162-1167
+// Enum used to identify which kind of item a \a TraceCursor is pointing at
+enum TraceItemKind {
+  eTraceItemKindError = 0,
+  eTraceItemKindEvent,
+  eTraceItemKindInstruction,
 };
----------------
nice


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:51-52
 
-lldb::addr_t DecodedThread::GetInstructionLoadAddress(size_t insn_index) const {
-  return m_instruction_ips[insn_index];
+lldb::addr_t DecodedThread::GetInstructionLoadAddress(size_t item_index) const {
+  return m_items[item_index].data.instruction.load_address;
 }
----------------
with this current implementation, it's the caller's responsibility to check the Item's type before calling this otherwise they will get a garbage load address if the item is actually an event for example.
Would it be better to enforce the validity of the Get call at this lowest level of storage by checking that the type matches what is expected and return None or something like LLDB_INVALID_ADDRESS if there's a mismatch?
same idea applies below for the other getters


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:84-86
+  m_items.emplace_back();
+  size_t index = m_items.size() - 1;
+  m_items[index].kind = kind;
----------------
nit: consider using `emplace` since this returns a ref to the item. or just create a constructor for the TraceItemKind that you can pass the kind to so you can set the kind in the emplace call and then there's no need for a reference to the newly inserted object

another suggestion would be to just make this method return a reference to the newly created TraceItemStorage since all the callers end up using the index to get access to the newly added item, so returning the ref would remove the need for those "redundant" lookups


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:245
+    /// The TraceItemKind for each trace item encoded as uint8_t.
+    uint8_t kind;
+    union {
----------------
why not use the enum as the type?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:264
+
+  /// Most of the trace data is stored here.
+  std::vector<TraceItemStorage> m_items;
----------------
why is this "most"?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:265
+  /// Most of the trace data is stored here.
+  std::vector<TraceItemStorage> m_items;
 
----------------
nice, this is much more clear now (:


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:159-165
       case ptev_disabled:
         // The CPU paused tracing the program, e.g. due to ip filtering.
       case ptev_async_disabled:
         // The kernel or user code paused tracing the program, e.g.
         // a breakpoint or a ioctl invocation pausing the trace, or a
         // context switch happened.
+        m_decoded_thread.AppendEvent(lldb::eTraceEventDisabled);
----------------
oooh, having a separate event for these two disabled cases would be provide great insight for us when trying to understand how tracing is enabled/disabled for user vs kernel mode (ie is it disabled by the hardware due to CPL filtering (ptev_disabled) or by the kernel/user (ptev_asyn_disabled)) - this is super cool and will be very useful!

not sure how we could cleanly expose this though since these different types of disables are pretty intelpt specific, but the notion of the traceevent is exposed at the top-level trace-agnostic tracecursor, right?
wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128576