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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:120
+
+void DecodedThread::ReportTscError(int libipt_error_code) { m_tsc_errors++; }
+
----------------
jj10306 wrote:
> 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?
good catch. I initially wanted to show the list of actual errors happening. I'll do that now


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:236-238
+  /// Notify this class that the last added instruction or error has
+  /// an associated TSC.
+  void ReportTscForLastInstruction(uint64_t tsc);
----------------
jj10306 wrote:
> nit: When I initially read "Report" it made me think that the last TSC was being logged or reported to something else. Since all this method does is record the tsc of the last instruction, potentially "Record" is a better name. This is purely my opinion so feel free to keep it as is or change it (:
good idea


================
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) {
 
----------------
jj10306 wrote:
> This function isn't taking ownership/storing this SP so consider just passing a reference here and in the AppendError, AppendInstruction and RefreshTsc funcs
good idea :)


================
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,
----------------
jj10306 wrote:
> 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.
> Prior to this change, was there always at least one error in the instructions from the eos that occurs when tracing?

Not really. In line 128 of the original code we had:
  if (errcode == -pte_eos)
        break;
which broke the instruction decoding flow as soon as an eos is seen.

>  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?

yes, that is true. The decoding will simply finish.

> When a pt_insn_next call returns pte_eos, does that gurantee that the next call to FindNextSynchronizationPoint will return pte_eos as well?

yes

> 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.

In this case I don't want to fail fast and instead just break the innermost loop because the code is a little bit big and putting returns in the middle might cause bugs in the future when someone modifies this function. Let's suppose that you add some code right after the big loop thinking that the new code will always be reached. In this case, the early return might unexpectedly finish the execution of the function without reaching your code and you might not easily notice that. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122867/new/

https://reviews.llvm.org/D122867