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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
