jj10306 added a comment.
The changes to the decoder look sound, just left two questions related to it
and a couple other nits.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:120
+
+void DecodedThread::ReportTscError(int libipt_error_code) { m_tsc_errors++; }
+
----------------
The parameter is unused, is there a reason to keep this or should it be removed
since the method is simply incrementing the tsc error count?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:221
+ /// Return he number of TSC decoding errors that happened. A TSC error
+ /// is not a fatal error and doesn't create gaps in the trace. Instead
----------------
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:106
+void RefreshTscInfo(TscInfo &tsc_info, pt_insn_decoder &decoder,
+ DecodedThreadSP decoded_thread_sp) {
+ if (tsc_info.has_tsc == eLazyBoolNo)
----------------
see comment on `DecodeInstructions`
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:130
+
+static void AppendError(DecodedThreadSP &decoded_thread_sp, Error &&error,
+ TscInfo &tsc_info) {
----------------
see comment on `DecodeInstructions`
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:138
+
+static void AppendInstruction(DecodedThreadSP &decoded_thread_sp,
+ const pt_insn &insn, TscInfo &tsc_info) {
----------------
see comment on `DecodeInstructions`
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:159
static void DecodeInstructions(pt_insn_decoder &decoder,
- DecodedThreadSP &decoded_thread_sp) {
- while (true) {
- int errcode = FindNextSynchronizationPoint(decoder);
- if (errcode == -pte_eos)
- break;
+ DecodedThreadSP decoded_thread_sp) {
----------------
This function isn't taking ownership/storing this SP so consider just passing a
reference here and in the AppendError, AppendInstruction and RefreshTsc funcs
================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:192
+ // We signal a gap only if it's not "end of stream"
+ if (errcode != -pte_eos)
+ AppendError(decoded_thread_sp,
----------------
This makes sense to not include the errors if you are at the end of the stream,
I have two questions related to this:
1. Prior to this change, was there always at least one error in the
instructions from the eos that occurs when tracing? My understanding is that
eos as a result of pt_insn_next is an indication that you are at the end of the
buffer and thus the decoding is done, is that correct?
2. When a pt_insn_next call returns pte_eos, does that gurantee that the next
call to FindNextSynchronizationPoint will return pte_eos as well? If so, could
this code be changed to immediately return if pte_eos is returned here since
currently it will break from the inner loop, go back to the top of the outer
loop, call FindNextSynchronizationPoint which will ultimately return pte_eos
which causes a break from the outer loop and finally the implicit return from
the function? Seems like we could fail fast by immediately returning from the
function here if pt_insn_next returns pte_eos.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122867/new/
https://reviews.llvm.org/D122867
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
