wallace 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
----------------
jj10306 wrote:
> What are some other events you could forsee adding to this enum?
paging, execution mode change (from 64 to 32 bit mode or viceversa), power
state change, core bus ratio change (i.e. frequency change). Some of these
might include an additional payload, which should be gotten using other APIs.
================
Comment at: lldb/include/lldb/lldb-enumerations.h:1155
+ // should provide an error signalinig this data loss.
+ eTraceEventPaused = (1u << 0),
+};
----------------
jj10306 wrote:
> Will this paused event always be due to a context switch? If so, we should
> rename it as such to reduce ambiguity
not really. Two examples:
- context switch
- ip filtering -> in this case tracing stopped at CPU level because an unwanted
memory region was executed. In this case the process is still running
================
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));
----------------
jj10306 wrote:
> 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?
The reason is that I want all callers to invoke Append() except for exceptional
cases. The benefit of Append is that can provide an error with events and
timing information. SetAsFailed is receiving only an error without additional
data to be used only in extreme situations where there's no event or timing
data available, which happens if the decoding setup fails.
================
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;
----------------
jj10306 wrote:
>
oh, i wanted to mean if and only if. Or is that too mathematical and less
colloquial?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:225
+ /// The number of instructions with associated events.
+ size_t GetInstructionsWithEventsCount() const;
+
----------------
jj10306 wrote:
> 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?
good idea
================
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:
> 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.
>
in this case we can't do that, because when doing random accesses in the trace,
we need to quickly find the most recent TSC for the new instruction, which is
done with binary search using a map. This is impossible in a hash map like
DenseMap
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:68
+ /// instruction in the given \a DecodedInstruction.
+ ///
+ /// \return
----------------
jj10306 wrote:
> consider adding docs on `insn` being an "out" parameter so it's a little more
> clear
good catch
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:186
+ // were lost.
+ insn.libipt_error = -pte_overflow;
+ break;
----------------
jj10306 wrote:
> Why have the internal buffer overflow event as an error instead of a variant
> of the newly introduced Events enum?
that's because an overflow means that we lost data, and therefore we should
show that as an error in the trace. Remember that errors mean that instructions
were lost, which is exactly what happens in an overflow case. This is not just
an event that could be ignored.
================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInfo.py:52
+ Events:
+ Total number of instructions with events: 1
+
----------------
jj10306 wrote:
> Will this always be 1 or is there a possibility that it could be different
> depending on the number of context switches?
This is using trace load, which uses an offline trace, so it will never change.
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
