[Lldb-commits] [PATCH] D122603: [intelpt] Refactor timestamps out of IntelPTInstruction

[Alisamar Husain via Phabricator via lldb-commits]

zrthxn added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:45-52
+    if (!m_current_tsc)
+      m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);
+    else if (!m_current_tsc->InRange(m_pos)) {
+      if (m_pos > m_current_tsc->GetEnd())
+        m_current_tsc = m_current_tsc->Next();
+      if (m_pos < m_current_tsc->GetStart())
+        m_current_tsc = m_current_tsc->Prev();
----------------
wallace wrote:
> No need to do `m_current_tsc = m_decoded_thread_sp->CalculateTscRange(m_pos);` because its value has already been calculated in the constructor. We can simplify this as well
It is possible that when TraceCursorIntelPT is created the m_current_tsc is None, for example when just started the trace and tried to dump instructions... But then if a tsc is emitted later, this would cause it to remain None since we don't re-calculate it if it was initially None


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:102-103
 
-Optional<uint64_t> TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+Optional<uint64_t>
+TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
+  if (!m_current_tsc)
----------------
wallace wrote:
> are you using git clang-format? I'm curious why this line changed
Yes  I am. I think its because its longer than 80 chars.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:104-110
+  if (!m_current_tsc)
+    return None;
+
   switch (counter_type) {
-    case lldb::eTraceCounterTSC:
-      return m_decoded_thread_sp->GetInstructions()[m_pos].GetTimestampCounter();
+  case lldb::eTraceCounterTSC:
+    return m_current_tsc->GetTsc();
   }
----------------
wallace wrote:
> 
m_current_tsc is already checked at the beginning of this function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122603