[NFC][trace] simplify the instruction dumper

[wallace]

Summary

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.

[jj10306]

two very minor comments, but looks good - I'm sure you already have, but be sure to run the dumper unittest test to ensure the output didn't change unexpectedly as a result some minor formatting mistake in this diff 🙂

lldb/include/lldb/Target/TraceInstructionDumper.h

26

It shouldn't be an issue now because this struct is never stored any where, but in the future we should be aware that if this struct is ever stored "long term" (ie in another class), we should instead use ExececutionContextRef
From the documentation in ExecutionContext.h

/// exist during a function that requires the objects. ExecutionContext
/// objects should NOT be used for long term storage since they will keep
/// objects alive with extra shared pointer references to these  objects

lldb/source/Target/TraceInstructionDumper.cpp

199

should this be a static function?

[wallace]

lldb/include/lldb/Target/TraceInstructionDumper.h

26

Yes, you are right :)

lldb/source/Target/TraceInstructionDumper.cpp

199

goot catch

[wallace]

address comments

lldb/include/lldb/Target/TraceInstructionDumper.h

 //===-- TraceInstructionDumper.h --------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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 {
   /// If \b true, the cursor will be iterated forwards starting from the
   /// oldest instruction. Otherwise, the iteration starts from the most
   /// recent instruction.
   bool forwards = false;
   /// Dump only instruction addresses without disassembly nor symbol
private:
   /// Move the cursor one step.
   ///
   /// \return
   ///     \b true if the cursor moved.
   bool TryMoveOneStep();
 
   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;
   /// If \b true, all the instructions have been traversed.
   bool m_no_more_data = false;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_TRACE_INSTRUCTION_DUMPER_H

lldb/source/Target/TraceInstructionDumper.cpp

if (m_cursor_up->Seek((m_options.forwards ? 1 : -1) * to_skip,
                           TraceCursor::SeekType::Current) < to_skip) {
       // This happens when the skip value was more than the number of
       // available instructions.
       SetNoMoreData();
     }
   }
 }
 
-/// \return
-///     Return \b true if the cursor could move one step.
 bool TraceInstructionDumper::TryMoveOneStep() {
   if (!m_cursor_up->Next()) {
     SetNoMoreData();
     return false;
   }
   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.
 static bool IsLineEntryValid(const LineEntry &line_entry) {
   return line_entry.IsValid() && line_entry.line > 0;
 }
 
 /// \return
IsSameInstructionSymbolContext(const InstructionSymbolInfo &prev_insn,
   const bool curr_line_valid = IsLineEntryValid(insn.sc.line_entry);
   const bool prev_line_valid = IsLineEntryValid(prev_insn.sc.line_entry);
   if (curr_line_valid && prev_line_valid)
     return FileLineAndColumnMatches(insn.sc.line_entry,
                                     prev_insn.sc.line_entry);
   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)",
+    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,
                          /*max_address_text_size=*/0);
 }
 
 void TraceInstructionDumper::SetNoMoreData() { m_no_more_data = true; }
 
 bool TraceInstructionDumper::HasMoreData() { return !m_no_more_data; }
 
-void TraceInstructionDumper::PrintEvents() {
-  if (!m_options.show_events)
-    return;
-
-  trace_event_utils::ForEachEvent(
-      m_cursor_up->GetEvents(), [&](TraceEvents event) {
-        m_s.Format("  [{0}]\n", trace_event_utils::EventToDisplayString(event));
-      });
-}
-
-Optional<lldb::tid_t> TraceInstructionDumper::DumpInstructions(size_t count) {
-  ThreadSP thread_sp = m_cursor_up->GetExecutionContextRef().GetThreadSP();
-  if (!thread_sp) {
-    m_s.Printf("invalid thread");
-    return None;
-  }
+void TraceInstructionDumper::PrintMissingInstructionsMessage() {
+  m_s << "    ...missing instructions\n";
 
-  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);
+void TraceInstructionDumper::PrintInstructionHeader() {
+  m_s.Format("    {0}: ", m_cursor_up->GetId());
 
-    if (m_options.show_tsc) {
-      m_s.Printf("[tsc=");
+    m_s << "[tsc=";
 
-      if (Optional<uint64_t> timestamp = m_cursor_up->GetCounter(lldb::eTraceCounterTSC))
-        m_s.Printf("0x%016" PRIx64, *timestamp);
-      else
-        m_s.Printf("unavailable");
+    if (Optional<uint64_t> timestamp =
+            m_cursor_up->GetCounter(lldb::eTraceCounterTSC))
+      m_s.Format("{0:x+16}", *timestamp);
+      m_s << "unavailable";
 
-      m_s.Printf("] ");
-    }
-  };
+    m_s << "] ";
+  }
 
-  InstructionSymbolInfo prev_insn_info;
+void TraceInstructionDumper::PrintEvents() {
+  if (!m_options.show_events)
+    return;
 
-  Target &target = thread_sp->GetProcess()->GetTarget();
-  ExecutionContext exe_ctx;
-  target.CalculateExecutionContext(exe_ctx);
-  const ArchSpec &arch = target.GetArchitecture();
+  trace_event_utils::ForEachEvent(
+      m_cursor_up->GetEvents(), [&](TraceEvents event) {
+        m_s.Format("  [{0}]\n", trace_event_utils::EventToDisplayString(event));
+      });
+}
 
-  // 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;
+/// 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) {
+                                        /*inline_block_range*/ false, range) &&
 
-    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);
-    }
+/// 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) {
+                .GetInstructionAtAddress(insn_info.address))
 
-    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());
-  };
+  if (insn_info.sc.function) {
+            insn_info.sc.function->GetInstructions(exe_ctx, nullptr)) {
+                  insn_info.address))
+  Target &target = exe_ctx.GetTargetRef();
+  const ArchSpec arch = target.GetArchitecture();
+  AddressRange range(insn_info.address, arch.GetMaximumOpcodeByteSize());
+  return std::make_tuple(
+      disassembler,
+      disassembler ? disassembler->GetInstructionList().GetInstructionAtAddress(
+                         insn_info.address)
+}
+
+Optional<lldb::user_id_t>
+TraceInstructionDumper::DumpInstructions(size_t count) {
+  ThreadSP thread_sp = m_cursor_up->GetExecutionContextRef().GetThreadSP();
+  if (!thread_sp) {
+    m_s << "invalid thread";
+    return None;
+  }
 
+  bool was_prev_instruction_an_error = false;
+  InstructionSymbolInfo prev_insn_info;
   Optional<lldb::user_id_t> last_id;
+
+  ExecutionContext exe_ctx;
+  thread_sp->GetProcess()->GetTarget().CalculateExecutionContext(exe_ctx);
+
   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();
     if (m_options.forwards) {
       // When moving forwards, we first print the event before printing
       // the actual instruction.
       PrintEvents();
     }
 
     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;
 
       InstructionSymbolInfo insn_info;
 
       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
       // the actual instruction so that reading chronologically
       // makes sense.
       PrintEvents();
     }
     TryMoveOneStep();
   }
   return last_id;
 }