[Lldb-commits] [PATCH] D136610: [trace][intelpt] Fix multi CPU decoding TSC assertion error

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

pretty good! I just left cosmetic requests



================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:285-312
+      if (event.has_tsc) {
+        if (m_tsc_upper_bound && event.tsc > m_tsc_upper_bound.value()) {
+          // This event and all the remaining events of this PSB have a TSC
+          // outside the range of the "owning" ThreadContinuousExecution. For
+          // now we drop all of these events/instructions, future work can
+          // improve upon this by determining the "owning"
+          // ThreadContinuousExecution of the remaining PSB data.
----------------
I suggest moving all of this to another function because it's a bit long


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:286
+      if (event.has_tsc) {
+        if (m_tsc_upper_bound && event.tsc > m_tsc_upper_bound.value()) {
+          // This event and all the remaining events of this PSB have a TSC
----------------
use * instead of value(). value() is rarely used in LLVM code. Also use >= instead of >


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:294-295
+              formatv(
+                  "TSC {0} exceeds maximum TSC value {1}. Will skip decoding"
+                  " the remining the remaining data of the PSB.",
+                  event.tsc, m_tsc_upper_bound.value())
----------------
add a prefix message for easier regexp search
  "decoding truncated: TSC packet {0} exceeds maximum TSC value {1} for this PSB block . Will resume decoding with the next PSB."

Use a better prefix if you can, and btw, you wrote `the remining the remaining ` here. 


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:296
+                  " the remining the remaining data of the PSB.",
+                  event.tsc, m_tsc_upper_bound.value())
+                  .str();
----------------
ditto


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:299-307
+          uint64_t offset;
+          int status = pt_insn_get_offset(m_decoder_up.get(), &offset);
+          if (!IsLibiptError(status)) {
+            err_msg =
+                formatv("At byte offset {0} of PSB with size {1} bytes, {2}",
+                        offset, m_psb_block.size, err_msg)
+                    .str();
----------------
I think let's better not include this to keep the error a bit smaller. In any case, you can do `thread trace dump instructions <thread> -E` and then look for the error prefix when debugging.

but if you insist, the byte offset message should come after the textual description of the error


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:308
+          m_decoded_thread.AppendCustomError(err_msg);
+          return -1;
+        } else {
----------------
return -pte_internal;


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:345
   DecodedThread &m_decoded_thread;
+  // Maximum allowed value of TSCs decoded from m_psb_block.
+  llvm::Optional<DecodedThread::TSC> m_tsc_upper_bound;
----------------
move this to the constructor so that it's highlighted by IDEs and appears in the public documentation

  Maximum allowed value of TSCs decoded from this psb block. If this value is hit, then decoding for this block is stopped and an error is appended to the trace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136610