[Lldb-commits] [PATCH] D122867: [trace][intel pt] Handle better tsc in the decoder

[Jakob Johnson via Phabricator via lldb-commits]

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