jj10306 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
----------------
What are some other events you could forsee adding to this enum?
================
Comment at: lldb/include/lldb/lldb-enumerations.h:1155
+ // should provide an error signalinig this data loss.
+ eTraceEventPaused = (1u << 0),
+};
----------------
Will this paused event always be due to a context switch? If so, we should
rename it as such to reduce ambiguity
================
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));
----------------
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?
================
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;
----------------
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:225
+ /// The number of instructions with associated events.
+ size_t GetInstructionsWithEventsCount() const;
+
----------------
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?
================
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.
----------------
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.
================
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:
> 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.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:68
+ /// instruction in the given \a DecodedInstruction.
+ ///
+ /// \return
----------------
consider adding docs on `insn` being an "out" parameter so it's a little more
clear
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:186
+ // were lost.
+ insn.libipt_error = -pte_overflow;
+ break;
----------------
Why have the internal buffer overflow event as an error instead of a variant of
the newly introduced Events enum?
================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInfo.py:52
+ Events:
+ Total number of instructions with events: 1
+
----------------
Will this always be 1 or is there a possibility that it could be different
depending on the number of context switches?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123982/new/
https://reviews.llvm.org/D123982
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
