[Lldb-commits] [PATCH] D128477: [trace] Add a flag to the decoder to output the instruction type

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.

Thanks for working on this 🙂
Looks good overall, just left some minor comments.



================
Comment at: lldb/include/lldb/Target/TraceCursor.h:64
 /// Low level traversal:
-///   Unlike the \a TraceCursor::Next() API, which uses a given granularity and
-///   direction to advance the cursor, the \a TraceCursor::Seek() method can be
+///   Unlike the \a TraceCursor::Next() API, which direction to advance
+//    the cursor, the \a TraceCursor::Seek() method can be
----------------
For some reason I can't make inline suggestions, so I'll just put my suggested edit below:

s/which direction/which uses the current direction of the trace


================
Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:105-107
+  void PrintInstructionType();
+
   void DumpInstructionDisassembly(const InstructionSymbolInfo &insn);
----------------
Don't forget to rebase onto mainline, @wallace's recent diff (https://reviews.llvm.org/D128316) refactored some of this code, so that will effect where some of these new changes should live.


================
Comment at: lldb/source/Commands/Options.td:1132
+  def thread_trace_dump_instructions_show_kind : Option<"kind", "k">, Group<1>,
+    Desc<"For each instruction, print the instruction type">;
   def thread_trace_dump_instructions_show_tsc : Option<"tsc", "t">, Group<1>,
----------------
nit: add a period at the end of the description (:


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:46-50
     if (m_tsc_range && !m_tsc_range->InRange(m_pos))
       m_tsc_range = IsForwards() ? m_tsc_range->Next() : m_tsc_range->Prev();
 
-    if (!m_ignore_errors && IsError())
-      return true;
-    if (GetInstructionControlFlowType() & m_granularity)
-      return true;
+    return true;
   }
----------------
Apologies again bc I can't "Suggest Edits" inline, so leaving my suggestion in a code block below:

IIUC, we previously had a while loop here because the possibility of "skipping" instructions due to ignoring errors or not matching the granularity. Since this diff removes those two things, this logic can now be simplified since the trace cursor can no longer skip the instructions - below is roughly what I'm thinking:
```
bool TraceCursorIntelPT::Next() {
  auto canMoveOne = [&]() {
    if (IsForwards())
      return m_pos + 1 < GetInternalInstructionSize();
    return m_pos > 0;
  };

  if (!canMoveOne())
    return false;

  m_pos += IsForwards() ? 1 : -1;
  if (m_tsc_range && !m_tsc_range->InRange(m_pos))
    m_tsc_range = IsForwards() ? m_tsc_range->Next() : m_tsc_range->Prev();

  return true;
}
```
@wallace wdyt?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128477