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