[NFC][trace] simplify the instruction dumper

TraceInstructionDumper::DumpInstructions was becoming too big, so I'm
refactoring it into smaller functions. I also made some static methods proper
instance methods to simplify calls. Other minor improvements are also done.

Differential Revision: https://reviews.llvm.org/D124064

Author: Walter Erquinigo

lldb/include/lldb/Target/TraceInstructionDumper.h

@@ -8,11 +8,24 @@
 
 #include "lldb/Target/TraceCursor.h"
 
+#include "lldb/Symbol/SymbolContext.h"
+
 #ifndef LLDB_TARGET_TRACE_INSTRUCTION_DUMPER_H
 #define LLDB_TARGET_TRACE_INSTRUCTION_DUMPER_H
 
 namespace lldb_private {
 
+/// Helper struct that holds symbol, disassembly and address information of an
+/// instruction.
+struct InstructionSymbolInfo {
+  SymbolContext sc;
+  Address address;
+  lldb::addr_t load_address;
+  lldb::DisassemblerSP disassembler;
+  lldb::InstructionSP instruction;
+  lldb_private::ExecutionContext exe_ctx;
+};
+
 /// Class that holds the configuration used by \a TraceInstructionDumper for
 /// traversing and dumping instructions.
 struct TraceInstructionDumperOptions {
@@ -83,6 +96,28 @@ class TraceInstructionDumper {
 
   void PrintEvents();
 
+  void PrintMissingInstructionsMessage();
+
+  void PrintInstructionHeader();
+
+  void DumpInstructionDisassembly(const InstructionSymbolInfo &insn);
+
+  /// Dump the symbol context of the given instruction address if it's different
+  /// from the symbol context of the previous instruction in the trace.
+  ///
+  /// \param[in] prev_sc
+  ///     The symbol context of the previous instruction in the trace.
+  ///
+  /// \param[in] address
+  ///     The address whose symbol information will be dumped.
+  ///
+  /// \return
+  ///     The symbol context of the current address, which might differ from the
+  ///     previous one.
+  void DumpInstructionSymbolContext(
+      const llvm::Optional<InstructionSymbolInfo> &prev_insn,
+      const InstructionSymbolInfo &insn);
+
   lldb::TraceCursorUP m_cursor_up;
   TraceInstructionDumperOptions m_options;
   Stream &m_s;

lldb/source/Target/TraceInstructionDumper.cpp

@@ -47,8 +47,6 @@ TraceInstructionDumper::TraceInstructionDumper(
   }
 }
 
-/// \return
-///     Return \b true if the cursor could move one step.
 bool TraceInstructionDumper::TryMoveOneStep() {
   if (!m_cursor_up->Next()) {
     SetNoMoreData();
@@ -57,17 +55,6 @@ bool TraceInstructionDumper::TryMoveOneStep() {
   return true;
 }
 
-/// Helper struct that holds symbol, disassembly and address information of an
-/// instruction.
-struct InstructionSymbolInfo {
-  SymbolContext sc;
-  Address address;
-  lldb::addr_t load_address;
-  lldb::DisassemblerSP disassembler;
-  lldb::InstructionSP instruction;
-  lldb_private::ExecutionContext exe_ctx;
-};
-
 // This custom LineEntry validator is neded because some line_entries have
 // 0 as line, which is meaningless. Notice that LineEntry::IsValid only
 // checks that line is not LLDB_INVALID_LINE_NUMBER, i.e. UINT32_MAX.
@@ -122,46 +109,35 @@ IsSameInstructionSymbolContext(const InstructionSymbolInfo &prev_insn,
   return curr_line_valid == prev_line_valid;
 }
 
-/// Dump the symbol context of the given instruction address if it's different
-/// from the symbol context of the previous instruction in the trace.
-///
-/// \param[in] prev_sc
-///     The symbol context of the previous instruction in the trace.
-///
-/// \param[in] address
-///     The address whose symbol information will be dumped.
-///
-/// \return
-///     The symbol context of the current address, which might differ from the
-///     previous one.
-static void
-DumpInstructionSymbolContext(Stream &s,
-                             Optional<InstructionSymbolInfo> prev_insn,
-                             InstructionSymbolInfo &insn) {
+void TraceInstructionDumper::DumpInstructionSymbolContext(
+    const Optional<InstructionSymbolInfo> &prev_insn,
+    const InstructionSymbolInfo &insn) {
   if (prev_insn && IsSameInstructionSymbolContext(*prev_insn, insn))
     return;
 
-  s.Printf("  ");
+  m_s << "  ";
 
   if (!insn.sc.module_sp)
-    s.Printf("(none)");
+    m_s << "(none)";
   else if (!insn.sc.function && !insn.sc.symbol)
-    s.Printf("%s`(none)",
-             insn.sc.module_sp->GetFileSpec().GetFilename().AsCString());
+    m_s.Format("{0}`(none)",
+               insn.sc.module_sp->GetFileSpec().GetFilename().AsCString());
   else
-    insn.sc.DumpStopContext(&s, insn.exe_ctx.GetTargetPtr(), insn.address,
+    insn.sc.DumpStopContext(&m_s, insn.exe_ctx.GetTargetPtr(), insn.address,
                             /*show_fullpaths=*/false,
                             /*show_module=*/true, /*show_inlined_frames=*/false,
                             /*show_function_arguments=*/true,
                             /*show_function_name=*/true);
-  s.Printf("\n");
+  m_s << "\n";
 }
 
-static void DumpInstructionDisassembly(Stream &s, InstructionSymbolInfo &insn) {
+void TraceInstructionDumper::DumpInstructionDisassembly(
+    const InstructionSymbolInfo &insn) {
   if (!insn.instruction)
     return;
-  s.Printf("    ");
-  insn.instruction->Dump(&s, /*max_opcode_byte_size=*/0, /*show_address=*/false,
+  m_s << "    ";
+  insn.instruction->Dump(&m_s, /*max_opcode_byte_size=*/0,
+                         /*show_address=*/false,
                          /*show_bytes=*/false, &insn.exe_ctx, &insn.sc,
                          /*prev_sym_ctx=*/nullptr,
                          /*disassembly_addr_format=*/nullptr,
@@ -172,6 +148,26 @@ void TraceInstructionDumper::SetNoMoreData() { m_no_more_data = true; }
 
 bool TraceInstructionDumper::HasMoreData() { return !m_no_more_data; }
 
+void TraceInstructionDumper::PrintMissingInstructionsMessage() {
+  m_s << "    ...missing instructions\n";
+}
+
+void TraceInstructionDumper::PrintInstructionHeader() {
+  m_s.Format("    {0}: ", m_cursor_up->GetId());
+
+  if (m_options.show_tsc) {
+    m_s << "[tsc=";
+
+    if (Optional<uint64_t> timestamp =
+            m_cursor_up->GetCounter(lldb::eTraceCounterTSC))
+      m_s.Format("{0:x+16}", *timestamp);
+    else
+      m_s << "unavailable";
+
+    m_s << "] ";
+  }
+}
+
 void TraceInstructionDumper::PrintEvents() {
   if (!m_options.show_events)
     return;
@@ -182,90 +178,76 @@ void TraceInstructionDumper::PrintEvents() {
       });
 }
 
-Optional<lldb::tid_t> TraceInstructionDumper::DumpInstructions(size_t count) {
+/// Find the symbol context for the given address reusing the previous
+/// instruction's symbol context when possible.
+static SymbolContext
+CalculateSymbolContext(const Address &address,
+                       const InstructionSymbolInfo &prev_insn_info) {
+  AddressRange range;
+  if (prev_insn_info.sc.GetAddressRange(eSymbolContextEverything, 0,
+                                        /*inline_block_range*/ false, range) &&
+      range.Contains(address))
+    return prev_insn_info.sc;
+
+  SymbolContext sc;
+  address.CalculateSymbolContext(&sc, eSymbolContextEverything);
+  return sc;
+}
+
+/// Find the disassembler for the given address reusing the previous
+/// instruction's disassembler when possible.
+static std::tuple<DisassemblerSP, InstructionSP>
+CalculateDisass(const InstructionSymbolInfo &insn_info,
+                const InstructionSymbolInfo &prev_insn_info,
+                const ExecutionContext &exe_ctx) {
+  if (prev_insn_info.disassembler) {
+    if (InstructionSP instruction =
+            prev_insn_info.disassembler->GetInstructionList()
+                .GetInstructionAtAddress(insn_info.address))
+      return std::make_tuple(prev_insn_info.disassembler, instruction);
+  }
+
+  if (insn_info.sc.function) {
+    if (DisassemblerSP disassembler =
+            insn_info.sc.function->GetInstructions(exe_ctx, nullptr)) {
+      if (InstructionSP instruction =
+              disassembler->GetInstructionList().GetInstructionAtAddress(
+                  insn_info.address))
+        return std::make_tuple(disassembler, instruction);
+    }
+  }
+  // We fallback to a single instruction disassembler
+  Target &target = exe_ctx.GetTargetRef();
+  const ArchSpec arch = target.GetArchitecture();
+  AddressRange range(insn_info.address, arch.GetMaximumOpcodeByteSize());
+  DisassemblerSP disassembler =
+      Disassembler::DisassembleRange(arch, /*plugin_name*/ nullptr,
+                                     /*flavor*/ nullptr, target, range);
+  return std::make_tuple(
+      disassembler,
+      disassembler ? disassembler->GetInstructionList().GetInstructionAtAddress(
+                         insn_info.address)
+                   : InstructionSP());
+}
+
+Optional<lldb::user_id_t>
+TraceInstructionDumper::DumpInstructions(size_t count) {
   ThreadSP thread_sp = m_cursor_up->GetExecutionContextRef().GetThreadSP();
   if (!thread_sp) {
-    m_s.Printf("invalid thread");
+    m_s << "invalid thread";
     return None;
   }
 
   bool was_prev_instruction_an_error = false;
-
-  auto printMissingInstructionsMessage = [&]() {
-    m_s.Printf("    ...missing instructions\n");
-  };
-
-  auto printInstructionHeader = [&](uint64_t id) {
-    m_s.Printf("    %" PRIu64 ": ", id);
-
-    if (m_options.show_tsc) {
-      m_s.Printf("[tsc=");
-
-      if (Optional<uint64_t> timestamp = m_cursor_up->GetCounter(lldb::eTraceCounterTSC))
-        m_s.Printf("0x%016" PRIx64, *timestamp);
-      else
-        m_s.Printf("unavailable");
-
-      m_s.Printf("] ");
-    }
-  };
-
   InstructionSymbolInfo prev_insn_info;
+  Optional<lldb::user_id_t> last_id;
 
-  Target &target = thread_sp->GetProcess()->GetTarget();
   ExecutionContext exe_ctx;
-  target.CalculateExecutionContext(exe_ctx);
-  const ArchSpec &arch = target.GetArchitecture();
-
-  // Find the symbol context for the given address reusing the previous
-  // instruction's symbol context when possible.
-  auto calculateSymbolContext = [&](const Address &address) {
-    AddressRange range;
-    if (prev_insn_info.sc.GetAddressRange(eSymbolContextEverything, 0,
-                                          /*inline_block_range*/ false,
-                                          range) &&
-        range.Contains(address))
-      return prev_insn_info.sc;
-
-    SymbolContext sc;
-    address.CalculateSymbolContext(&sc, eSymbolContextEverything);
-    return sc;
-  };
-
-  // Find the disassembler for the given address reusing the previous
-  // instruction's disassembler when possible.
-  auto calculateDisass = [&](const Address &address, const SymbolContext &sc) {
-    if (prev_insn_info.disassembler) {
-      if (InstructionSP instruction =
-              prev_insn_info.disassembler->GetInstructionList()
-                  .GetInstructionAtAddress(address))
-        return std::make_tuple(prev_insn_info.disassembler, instruction);
-    }
+  thread_sp->GetProcess()->GetTarget().CalculateExecutionContext(exe_ctx);
 
-    if (sc.function) {
-      if (DisassemblerSP disassembler =
-              sc.function->GetInstructions(exe_ctx, nullptr)) {
-        if (InstructionSP instruction =
-                disassembler->GetInstructionList().GetInstructionAtAddress(
-                    address))
-          return std::make_tuple(disassembler, instruction);
-      }
-    }
-    // We fallback to a single instruction disassembler
-    AddressRange range(address, arch.GetMaximumOpcodeByteSize());
-    DisassemblerSP disassembler =
-        Disassembler::DisassembleRange(arch, /*plugin_name*/ nullptr,
-                                       /*flavor*/ nullptr, target, range);
-    return std::make_tuple(disassembler,
-                           disassembler ? disassembler->GetInstructionList()
-                                              .GetInstructionAtAddress(address)
-                                        : InstructionSP());
-  };
-
-  Optional<lldb::user_id_t> last_id;
   for (size_t i = 0; i < count; i++) {
     if (!HasMoreData()) {
-      m_s.Printf("    no more data\n");
+      m_s << "    no more data\n";
       break;
     }
     last_id = m_cursor_up->GetId();
@@ -277,15 +259,15 @@ Optional<lldb::tid_t> TraceInstructionDumper::DumpInstructions(size_t count) {
 
     if (const char *err = m_cursor_up->GetError()) {
       if (!m_cursor_up->IsForwards() && !was_prev_instruction_an_error)
-        printMissingInstructionsMessage();
+        PrintMissingInstructionsMessage();
 
       was_prev_instruction_an_error = true;
 
-      printInstructionHeader(m_cursor_up->GetId());
+      PrintInstructionHeader();
       m_s << err;
     } else {
       if (m_cursor_up->IsForwards() && was_prev_instruction_an_error)
-        printMissingInstructionsMessage();
+        PrintMissingInstructionsMessage();
 
       was_prev_instruction_an_error = false;
 
@@ -294,24 +276,26 @@ Optional<lldb::tid_t> TraceInstructionDumper::DumpInstructions(size_t count) {
       if (!m_options.raw) {
         insn_info.load_address = m_cursor_up->GetLoadAddress();
         insn_info.exe_ctx = exe_ctx;
-        insn_info.address.SetLoadAddress(insn_info.load_address, &target);
-        insn_info.sc = calculateSymbolContext(insn_info.address);
+        insn_info.address.SetLoadAddress(insn_info.load_address,
+                                         exe_ctx.GetTargetPtr());
+        insn_info.sc =
+            CalculateSymbolContext(insn_info.address, prev_insn_info);
         std::tie(insn_info.disassembler, insn_info.instruction) =
-            calculateDisass(insn_info.address, insn_info.sc);
+            CalculateDisass(insn_info, prev_insn_info, exe_ctx);
 
-        DumpInstructionSymbolContext(m_s, prev_insn_info, insn_info);
+        DumpInstructionSymbolContext(prev_insn_info, insn_info);
       }
 
-      printInstructionHeader(m_cursor_up->GetId());
-      m_s.Printf("0x%016" PRIx64, m_cursor_up->GetLoadAddress());
+      PrintInstructionHeader();
+      m_s.Format("{0:x+16}", m_cursor_up->GetLoadAddress());
 
       if (!m_options.raw)
-        DumpInstructionDisassembly(m_s, insn_info);
+        DumpInstructionDisassembly(insn_info);
 
       prev_insn_info = insn_info;
     }
 
-    m_s.Printf("\n");
+    m_s << "\n";
 
     if (!m_options.forwards) {
       // If we move backwards, we print the events after printing