[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 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

review- part 1



================
Comment at: lldb/include/lldb/Target/Trace.h:171
+  ///     information failed to load, an \a llvm::Error is returned.
+  virtual llvm::Expected<lldb::TraceCursorUP>
+  CreateNewCursor(Thread &thread) = 0;
----------------
Do we want to keep the cursor as a UP or take this refactor before creating the public bindings to make it a SP? IMO a UP might make sense because I can't really think of many cases where you would want multiple users of the same cursor.
wdyt?


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:44-46
+///     The cursor can also point at events in the trace, which aren't errors
+///     nor instructions. An example of an event could be a context switch in
+///     between two instructions.
----------------
do you want to include pointers to the methods you can use to check/get events similar to what you did for errors above?


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:232-246
+  virtual const char *GetError() const = 0;
 
-  /// Get the corresponding error message if the cursor points to an error in
-  /// the trace.
-  ///
   /// \return
-  ///     \b nullptr if the cursor is not pointing to an error in
-  ///     the trace. Otherwise return the actual error message.
-  virtual const char *GetError() = 0;
+  ///     Whether the cursor points to an event or not.
+  virtual bool IsEvent() const = 0;
 
   /// \return
----------------
For the getters, what are your thoughts on returning an optional instead of using designated value/variants (ie nullptr, LLDB_INVALID_INSTRUCTION, eTraceEventNone`) to indicate that the current item does not match what `Get` method is being used?

Along the same lines, but a slightly bigger change:
With the introduction of `Events` as first class citizens of the trace cursor API, it may make sense to introduce the notion of of a trace "item" or something similar that encapsulates (instructions, events, errors, etc). I'm thinking of a tagged union type structure (ie a Rust enum)  that enforces the correctness of the access to the underlying item.
wdyt?


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:258
+  virtual llvm::Optional<uint64_t>
+  GetCounter(lldb::TraceCounter counter_type) const = 0;
 
----------------
Now that we are introducing the notion of an `Event`? wdyt about combining Events and Counters since a counter value in a trace really is just a type of event? In this case, `Counter` could just become a variant of `TraceEvent`. This may make more sense because even in the case of TSCs in an IntelPT trace because, strictly speaking, these aren't associated with instructions, correct? Instead the TSC values are emitted with PSBs and then we "attribute" these values to the nearest instruction, right?


================
Comment at: lldb/include/lldb/Target/TraceDumper.h:59
+  /// Helper struct that holds all the information we know about a trace item
+  struct TraceItem {
     lldb::user_id_t id;
----------------
oh looks like you already introduced a `TraceItem` structure!
Similar to my comment above related to introducing a trace item type structure for the trace cursor, would it make sense to have a separate type for each of the different "items". With the way the TraceItem struct is currently, a lot of this data is mutually exclusive, hence the many optional fields, correct?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:145-147
   /// \return
-  ///     The control flow categories, or \b 0 if the instruction is an error.
+  ///     The control flow categories, or an undefined vlaue if the item is not
+  ///     an instruction.
----------------
Why not use an optional here instead?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:236-239
+  enum class ItemKind : uint64_t {
+    Instruction = 0,
+    Error = LLDB_INVALID_ADDRESS - 1,
+    Event = LLDB_INVALID_ADDRESS - 2,
----------------
Continuing the theme of my comments related to the notion of a `TraceItem` - would it be possible to unite the notion of a trace item? currently there is an Item enum here, a item struct in the Dumper and I feel there is a need for a Item at the trace cursor level as well.
wdyt?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:241
+  };
+  ItemKind LowestNonInstructionItemKind = ItemKind::Event;
+
----------------
why is this needed?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:253-258
+  /// The low level storage of all instruction addresses, error and events. Each
+  /// trace item has an index in this vector and this index is used throughout
+  /// the code. As an storage optimization, values of \b ItemKind::Error
+  /// indicate that this item is an error, \b ItemKind::Event if the item is an
+  /// event, and otherwise the value is an instruction address.
+  std::vector<uint64_t> m_trace_items;
----------------
I found this a bit confusing. Are the values of the vectors the indexes into the DenseMaps below (m_errors and m_events) or are these the actual error/event values?
How does this vector relate to the two vectors below (instruction_size/classes since now this is items and not just instructions?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128576