[trace] Dedup different source lines when dumping instructions + refactor

[wallace]

Summary

When dumping the traced instructions in a for loop, like this one

4:  for (int a = 0; a < n; a++)  
5:    do something;

there might be multiple LineEntry objects for line 4, but with different address ranges. This was causing the dump command to dump something like this:

a.out`main + 11 at main.cpp:4
  [1] 0x0000000000400518    movl   $0x0, -0x8(%rbp)
  [2] 0x000000000040051f    jmp    0x400529                  ; <+28> at main.cpp:4
a.out`main + 28 at main.cpp:4
  [3] 0x0000000000400529    cmpl   $0x3, -0x8(%rbp)
  [4] 0x000000000040052d    jle    0x400521                  ; <+20> at main.cpp:5

which is confusing, as main.cpp:4 appears twice consecutively.

This diff fixes that issue by making the line entry comparison strictly about the line, column and file name. Before it was also comparing the address ranges, which we don't need because our output is strictly about what the user sees in the source.

Besides, I've noticed that the logic that traverses instructions and calculates symbols and disassemblies had too much coupling, and made my changes harder to implement, so I decided to decouple it. Now there are two methods for iterating over the instruction of a trace. The existing one does it on raw load addresses, but the one provides a SymbolContext and an InstructionSP, and does the calculations efficiently (not as efficient as possible for now though), so the caller doesn't need to care about these details. I think I'll be using that iterator to reconstruct the call stacks.

I was able to fix a test with this change.

[wallace]

This comment has been deleted.

[wallace]

ready for review

[clayborg]

lldb/include/lldb/Symbol/LineEntry.h

94–104 ↗

(On Diff #339044)

I would leave the compare function alone and create a new one that just compares the file and line only. Why? The compare function is comparing a lot more: LineEntry::is_terminal_entry, LineEntry::column. So make a new function, and we don't need to return a -1, 0, or 1, but a bool:

static bool FileAndLineMatches(const LineEntry &lhs, const LineEntry &rhs);

Or just compare the file and line manually where this was being called.

lldb/include/lldb/Target/Trace.h

207–215

Does this need to be exposed in the Trace.h file? Seems like this could be declared in the Trace.cpp file

217

s/byt/but/

218–233

Can we put a static function into Trace.cpp so this isn't needed in the Trace.h file?

lldb/source/Target/Trace.cpp

115–116

Remove the "Optional<Trace::InstructionSymbolInfo>" for "prev_insn and just check it prior to calling and make params const

119

we should compare the function/symbol first, then do the line entries.

128

use != instead of ^ to make the code easier to deduce.

133

call LjneEntry::FileAndLineMatches(...) or just manually compare the file and line only.

136–140

Don't we want to check the function first before the line entry? What if the function changes but we have the same file and line, like a vector:23 from STL...

Also you can probably just make this code much much simpler

143–146

Ditto, simplify:

149

No need to do a module check as the module owns the function or symbol and they won't match.

[clayborg]

The title should be updated on this diff as well. Set the "json" right now

[wallace]

Address all comments

[wallace]

nit

[clayborg]

lldb/source/Target/Trace.cpp

163

AddressRange::ContainsFileAddress() can't be used here after looking at how it is currently implemented. It will check if the sections are the same first and return the correct answer if they are, otherwise it will just extract a file addresses and compare them.

To do this right, we need to uncomment out the currently unused:

bool AddressRange::Contains (const Address &addr) const;

And we will need to fix and use it by making sure the modules match. If the modules don't match, we don't want to extract two file addresses from two different modules and then compare those, as two modules can both have a 0x1000 file address.

The current commented out implementation of AddressRange::Contains(...) is wrong too. Fixing correctly looks like:

bool
AddressRange::Contains(const Address &addr) const
{
  SectionSP rangeSectSP = GetBaseAddress().GetSection();
  SectionSP addrSectSP = addr.GetSection();
  if (rangeSectSP) {
    if (!addrSectSP || rangeSectSP->GetModule() != addrSectSP->GetModule())
      return false; // Modules do not match.
  } else if (addrSectSP) {
    return false; // Range has no module but "addr" does because addr has a section
  }
  // Either the modules match, or both have no module, so it is ok to compare 
  // the file addresses in this case only.
  return ContainsFileAddress(addr);
}

173

Can or should we get the disassembler in calculate_symbol_context above after calling address.CalculateSymbolContext(...)?

191

Remove the " * 1" here

203

Can this function really be called without a valid load address?

210–211

Better to use:

bool Address::SetLoadAddress(lldb::addr_t load_addr, Target *target);

243–244

You can't just return if there is no symbol, you can have no symbol, but still have a function from debug info if the symbol was a local symbol since it could have been stripped from the executable at build time.

Just remove these two lines and the "else" below

255–263

304–305

Should this return a Expected<size_t>? In case there is an error we want to show? Like maybe the trace buffer was truncated, or missing when it was expected? Can we have a process that is traced, but by the time we fetch the instructions, the circular buffer was filled with other data and even though the thread was traced there is no data?

[wallace]

lldb/source/Target/Trace.cpp

173

i think it's fine as it is, as we don't always need a disassembly. I want to use this TraverseInstructionsWithSymbolInfo method for creating the call graph, and I don't need the disassembly for that

203

yes, whenever there are gaps in the trace

304–305

Any kind of errors are represented as failed instructions, which means that if the entire operation failed, there's one single instruction with an error message. If the thread is not traced at all, then this method return None.
There can be errors while getting the data or errors while decoding individual instructions. That was a suggestion by Labath and I think this makes the code simpler, as there is only one way to handle errors.

[wallace]

address comments

[clayborg]

Just fix the one issue where we use the FileSpec operator== and this is good to go!

lldb/source/Core/AddressRange.cpp

59

It might be worth scanning the code to see if anyone is using AddressRange::ContainsFileAddress() incorrectly. It is fine to use this within a module to check if an address is in the range, but not ok if the address can come from a different module.

lldb/source/Target/Trace.cpp

114

No need to compare, just use == operator

lldb/include/lldb/Core/AddressRange.h

 //===-- AddressRange.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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_CORE_ADDRESSRANGE_H
 #define LLDB_CORE_ADDRESSRANGE_H
 
 #include "lldb/Core/Address.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 
 #include <stddef.h>
 
 namespace lldb_private {
 class SectionList;
 class Stream;
public:
   /// Check if a section offset address is contained in this range.
   ///
   /// \param[in] so_addr
   ///     A section offset address object reference.
   ///
   /// \return
   ///     Returns \b true if \a so_addr is contained in this range,
   ///     \b false otherwise.
-  //    bool
-  //    Contains (const Address &so_addr) const;
+  bool Contains(const Address &so_addr) const;
 
   /// Check if a section offset address is contained in this range.
   ///
   /// \param[in] so_addr_ptr
   ///     A section offset address object pointer.
   ///
   /// \return
   ///     Returns \b true if \a so_addr is contained in this range,

lldb/include/lldb/Target/Trace.h

 //===-- Trace.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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_TARGET_TRACE_H
 #define LLDB_TARGET_TRACE_H
 
 #include <unordered_map>
 
 #include "llvm/Support/JSON.h"
 
 #include "lldb/Core/PluginInterface.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/TraceGDBRemotePackets.h"
 #include "lldb/Utility/UnimplementedError.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
public:
   /// \param[in] callback
   ///     The callback to execute on each instruction. If it returns \b false,
   ///     the iteration stops.
   virtual void TraverseInstructions(
       Thread &thread, size_t position, TraceDirection direction,
       std::function<bool(size_t index, llvm::Expected<lldb::addr_t> load_addr)>
           callback) = 0;
 
   /// Get the number of available instructions in the trace of the given thread.
   ///
   /// \param[in] thread
   ///     The thread whose trace will be inspected.
   ///
   /// \return
-  ///     The total number of instructions in the trace.
-  virtual size_t GetInstructionCount(Thread &thread) = 0;
+  ///     The total number of instructions in the trace, or \a llvm::None if the
+  ///     thread is not being traced.
+  virtual llvm::Optional<size_t> GetInstructionCount(Thread &thread) = 0;
 
   /// Check if a thread is currently traced by this object.
   ///
   /// \param[in] thread
   ///     The thread in question.
   ///
   /// \return
   ///     \b true if the thread is traced by this instance, \b false otherwise.
   virtual bool IsTraced(const Thread &thread) = 0;
 
   /// Stop tracing live threads.
   ///
   /// \param[in] tids
   ///     The threads to stop tracing on.
   ///
   /// \return
   ///     \a llvm::Error::success if the operation was successful, or
   ///     \a llvm::Error otherwise.
   llvm::Error StopThreads(const std::vector<lldb::tid_t> &tids);
 
   /// Stop tracing a live process.
   ///
   /// \param[in] request
   ///     The information determining which threads or process to stop tracing.
   ///
   /// \return

lldb/source/Core/AddressRange.cpp

 //===-- AddressRange.cpp --------------------------------------------------===//
 //
 // 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/Core/AddressRange.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/Section.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/lldb-defines.h"
 
 #include "llvm/Support/Compiler.h"
 
AddressRange::AddressRange(const lldb::SectionSP &section, addr_t offset,
                            addr_t byte_size)
     : m_base_addr(section, offset), m_byte_size(byte_size) {}
 
 AddressRange::AddressRange(const Address &so_addr, addr_t byte_size)
     : m_base_addr(so_addr), m_byte_size(byte_size) {}
 
 AddressRange::~AddressRange() {}
 
-// bool
-// AddressRange::Contains (const Address &addr) const
-//{
-//    const addr_t byte_size = GetByteSize();
-//    if (byte_size)
-//        return addr.GetSection() == m_base_addr.GetSection() &&
-//        (addr.GetOffset() - m_base_addr.GetOffset()) < byte_size;
-//}
+bool AddressRange::Contains(const Address &addr) const {
+  SectionSP range_sect_sp = GetBaseAddress().GetSection();
+  SectionSP addr_sect_sp = addr.GetSection();
+  if (range_sect_sp) {
+    if (!addr_sect_sp ||
+        range_sect_sp->GetModule() != addr_sect_sp->GetModule())
+      return false; // Modules do not match.
+  } else if (addr_sect_sp) {
+    return false; // Range has no module but "addr" does because addr has a
+                  // section
+  }
+  // Either the modules match, or both have no module, so it is ok to compare
+  // the file addresses in this case only.
+  return ContainsFileAddress(addr);
+}
+
 //
 // bool
 // AddressRange::Contains (const Address *addr) const
 //{
 //    if (addr)
 //        return Contains (*addr);
 //    return false;
 //}

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h

public:
 
   llvm::StringRef GetSchema() override;
 
   void TraverseInstructions(
       Thread &thread, size_t position, TraceDirection direction,
       std::function<bool(size_t index, llvm::Expected<lldb::addr_t> load_addr)>
           callback) override;
 
-  size_t GetInstructionCount(Thread &thread) override;
+  llvm::Optional<size_t> GetInstructionCount(Thread &thread) override;
 
   size_t GetCursorPosition(Thread &thread) override;
 
   void DoRefreshLiveProcessState(
       llvm::Expected<TraceGetStateResponse> state) override;
 
   bool IsTraced(const Thread &thread) override;
 

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

void TraceIntelPT::TraverseInstructions(
 
   ssize_t delta = direction == TraceDirection::Forwards ? 1 : -1;
   for (ssize_t i = position; i < (ssize_t)instructions.size() && i >= 0;
        i += delta)
     if (!callback(i, instructions[i].GetLoadAddress()))
       break;
 }
 
-size_t TraceIntelPT::GetInstructionCount(Thread &thread) {
+Optional<size_t> TraceIntelPT::GetInstructionCount(Thread &thread) {
   if (const DecodedThread *decoded_thread = Decode(thread))
     return decoded_thread->GetInstructions().size();
   else
-    return 0;
+    return None;
 }
 
 Expected<pt_cpu> TraceIntelPT::GetCPUInfoForLiveProcess() {
   Expected<std::vector<uint8_t>> cpu_info = GetLiveProcessBinaryData("cpuInfo");
   if (!cpu_info)
     return cpu_info.takeError();
 
   int64_t cpu_family = -1;

lldb/source/Target/Trace.cpp

 //===-- Trace.cpp ---------------------------------------------------------===//
 //
 // 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/Trace.h"
 
 #include "llvm/Support/Format.h"
 
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Symbol/Function.h"
+#include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadPostMortemTrace.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb;
 using namespace lldb_private;
Expected<StringRef> Trace::FindPluginSchema(StringRef name) {
 
   return createInvalidPlugInError(name);
 }
 
 static int GetNumberOfDigits(size_t num) {
   return num == 0 ? 1 : static_cast<int>(log10(num)) + 1;
 }
 
-/// 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.
+/// \return
+///     \b true if the provided line entries match line, column and source file.
+///     This function assumes that the line entries are valid.
+static bool FileLineAndColumnMatches(const LineEntry &a, const LineEntry &b) {
+  if (a.line != b.line)
+    return false;
+  if (a.column != b.column)
+    return false;
+
+  return FileSpec::Compare(a.file, b.file, true) == 0;
+}
+
+// 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;
+}
+
+/// Helper structure for \a TraverseInstructionsWithSymbolInfo.
+struct InstructionSymbolInfo {
+  SymbolContext sc;
+  Address address;
+  lldb::addr_t load_address;
+  lldb::DisassemblerSP disassembler;
+  lldb::InstructionSP instruction;
+  lldb_private::ExecutionContext exe_ctx;
+};
+
+/// InstructionSymbolInfo object with symbol information for the given
+/// instruction, calculated efficiently.
 ///
-/// \param[in] address
-///     The address whose symbol information will be dumped.
+/// \param[in] symbol_scope
+///     If not \b 0, then the \a InstructionSymbolInfo will have its
+///     SymbolContext calculated up to that level.
 ///
-/// \return
-///     The symbol context of the current address, which might differ from the
-///     previous one.
-static SymbolContext DumpSymbolContext(Stream &s, const SymbolContext &prev_sc,
-                                       Target &target, const Address &address) {
+/// \param[in] include_disassembler
+///     If \b true, then the \a InstructionSymbolInfo will have the
+///     \a disassembler and \a instruction objects calculated.
+static void TraverseInstructionsWithSymbolInfo(
+    Trace &trace, Thread &thread, size_t position,
+    Trace::TraceDirection direction, SymbolContextItem symbol_scope,
+    bool include_disassembler,
+    std::function<bool(size_t index, Expected<InstructionSymbolInfo> insn)>
+        callback) {
+  InstructionSymbolInfo prev_insn;
+
+  Target &target = thread.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 calculate_symbol_context = [&](const Address &address) {
     AddressRange range;
-  if (prev_sc.GetAddressRange(eSymbolContextEverything, 0,
+    if (prev_insn.sc.GetAddressRange(symbol_scope, 0,
                                      /*inline_block_range*/ false, range) &&
-      range.ContainsFileAddress(address))
-    return prev_sc;
+        range.Contains(address))
+      return prev_insn.sc;
 
     SymbolContext sc;
-  address.CalculateSymbolContext(&sc, eSymbolContextEverything);
-
-  if (!prev_sc.module_sp && !sc.module_sp)
-    return sc;
-  if (prev_sc.module_sp == sc.module_sp && !sc.function && !sc.symbol &&
-      !prev_sc.function && !prev_sc.symbol)
+    address.CalculateSymbolContext(&sc, symbol_scope);
     return sc;
+  };
 
-  s.Printf("  ");
+  // Find the disassembler for the given address reusing the previous
+  // instruction's disassembler when possible.
+  auto calculate_disass = [&](const Address &address, const SymbolContext &sc) {
+    if (prev_insn.disassembler) {
+      if (InstructionSP instruction =
+              prev_insn.disassembler->GetInstructionList()
+                  .GetInstructionAtAddress(address))
+        return std::make_tuple(prev_insn.disassembler, instruction);
+    }
 
-  if (!sc.module_sp)
-    s.Printf("(none)");
-  else if (!sc.function && !sc.symbol)
-    s.Printf("%s`(none)",
-             sc.module_sp->GetFileSpec().GetFilename().AsCString());
-  else
-    sc.DumpStopContext(&s, &target, address, /*show_fullpath*/ false,
-                       /*show_module*/ true, /*show_inlined_frames*/ false,
-                       /*show_function_arguments*/ true,
-                       /*show_function_name*/ true);
-  s.Printf("\n");
-  return sc;
+    if (sc.function) {
+      if (DisassemblerSP disassembler =
+              sc.function->GetInstructions(exe_ctx, nullptr, true)) {
+        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, /*prefer_file_cache*/ true);
+    return std::make_tuple(disassembler,
+                           disassembler ? disassembler->GetInstructionList()
+                                              .GetInstructionAtAddress(address)
+                                        : InstructionSP());
+  };
 
-/// Dump an instruction given by its address using a given disassembler, unless
-/// the instruction is not present in the disassembler.
-///
-/// \param[in] disassembler
-///     A disassembler containing a certain instruction list.
-///
-/// \param[in] address
-///     The address of the instruction to dump.
+  trace.TraverseInstructions(
+      thread, position, direction,
+      [&](size_t index, Expected<lldb::addr_t> load_address) -> bool {
+        if (!load_address)
+          return callback(index, load_address.takeError());
+
+        InstructionSymbolInfo insn;
+        insn.load_address = *load_address;
+        insn.exe_ctx = exe_ctx;
+        insn.address.SetLoadAddress(*load_address, &target);
+        if (symbol_scope != 0)
+          insn.sc = calculate_symbol_context(insn.address);
+        if (include_disassembler)
+          std::tie(insn.disassembler, insn.instruction) =
+              calculate_disass(insn.address, insn.sc);
+        prev_insn = insn;
+        return callback(index, insn);
+      });
+}
+
+/// Compare the symbol contexts of the provided \a InstructionSymbolInfo
+/// objects.
 ///
 /// \return
-///     \b true if the information could be dumped, \b false otherwise.
-static bool TryDumpInstructionInfo(Stream &s,
-                                   const DisassemblerSP &disassembler,
-                                   const ExecutionContext &exe_ctx,
-                                   const Address &address) {
-  if (!disassembler)
+///     \a true if both instructions belong to the same scope level analized
+///     in the following order:
+///       - module
+///       - symbol
+///       - function
+///       - line
+static bool
+IsSameInstructionSymbolContext(const InstructionSymbolInfo &prev_insn,
+                               const InstructionSymbolInfo &insn) {
+  // module checks
+  if (insn.sc.module_sp != prev_insn.sc.module_sp)
     return false;
 
-  if (InstructionSP instruction =
-          disassembler->GetInstructionList().GetInstructionAtAddress(address)) {
-    instruction->Dump(&s, /*show_address*/ false, /*show_bytes*/ false,
-                      /*max_opcode_byte_size*/ 0, &exe_ctx,
-                      /*sym_ctx*/ nullptr, /*prev_sym_ctx*/ nullptr,
-                      /*disassembly_addr_format*/ nullptr,
-                      /*max_address_text_size*/ 0);
-    return true;
-  }
+  // symbol checks
+  if (insn.sc.symbol != prev_insn.sc.symbol)
+    return false;
 
+  // function checks
+  if (!insn.sc.function && !prev_insn.sc.function)
+    return true;
+  else if (insn.sc.function != prev_insn.sc.function)
     return false;
+
+  // line entry checks
+  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 an instruction instruction given by its address.
+/// 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_disassembler
-///     The disassembler that was used to dump the previous instruction in the
-///     trace. It is useful to avoid recomputations.
+/// \param[in] prev_sc
+///     The symbol context of the previous instruction in the trace.
 ///
 /// \param[in] address
-///     The address of the instruction to dump.
+///     The address whose symbol information will be dumped.
 ///
 /// \return
-///     A disassembler that contains the given instruction, which might differ
-///     from the previous disassembler.
-static DisassemblerSP
-DumpInstructionInfo(Stream &s, const SymbolContext &sc,
-                    const DisassemblerSP &prev_disassembler,
-                    ExecutionContext &exe_ctx, const Address &address) {
-  // We first try to use the previous disassembler
-  if (TryDumpInstructionInfo(s, prev_disassembler, exe_ctx, address))
-    return prev_disassembler;
+///     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) {
+  if (prev_insn && IsSameInstructionSymbolContext(*prev_insn, insn))
+    return;
 
-  // Now we try using the current function's disassembler
-  if (sc.function) {
-    DisassemblerSP disassembler =
-        sc.function->GetInstructions(exe_ctx, nullptr, true);
-    if (TryDumpInstructionInfo(s, disassembler, exe_ctx, address))
-      return disassembler;
-  }
+  s.Printf("  ");
+
+  if (!insn.sc.module_sp)
+    s.Printf("(none)");
+  else if (!insn.sc.function && !insn.sc.symbol)
+    s.Printf("%s`(none)",
+             insn.sc.module_sp->GetFileSpec().GetFilename().AsCString());
+  else
+    insn.sc.DumpStopContext(&s, insn.exe_ctx.GetTargetPtr(), insn.address,
+                            /*show_fullpath*/ false,
+                            /*show_module*/ true, /*show_inlined_frames*/ false,
+                            /*show_function_arguments*/ true,
+                            /*show_function_name*/ true);
+  s.Printf("\n");
 
-  // We fallback to disassembly one instruction
-  Target &target = exe_ctx.GetTargetRef();
-  const ArchSpec &arch = target.GetArchitecture();
-  AddressRange range(address, arch.GetMaximumOpcodeByteSize() * 1);
-  DisassemblerSP disassembler = Disassembler::DisassembleRange(
-      arch, /*plugin_name*/ nullptr,
-      /*flavor*/ nullptr, target, range, /*prefer_file_cache*/ true);
-  if (TryDumpInstructionInfo(s, disassembler, exe_ctx, address))
-    return disassembler;
-  return nullptr;
+static void DumpInstructionDisassembly(Stream &s, InstructionSymbolInfo &insn) {
+  if (!insn.instruction)
+    return;
+  insn.instruction->Dump(&s, /*show_address*/ false, /*show_bytes*/ false,
+                         /*max_opcode_byte_size*/ 0, &insn.exe_ctx, &insn.sc,
+                         /*prev_sym_ctx*/ nullptr,
+                         /*disassembly_addr_format*/ nullptr,
+                         /*max_address_text_size*/ 0);
 }
 
 void Trace::DumpTraceInstructions(Thread &thread, Stream &s, size_t count,
                                   size_t end_position, bool raw) {
-  if (!IsTraced(thread)) {
+  Optional<size_t> instructions_count = GetInstructionCount(thread);
+  if (!instructions_count) {
     s.Printf("thread #%u: tid = %" PRIu64 ", not traced\n", thread.GetIndexID(),
              thread.GetID());
     return;
   }
 
-  size_t instructions_count = GetInstructionCount(thread);
   s.Printf("thread #%u: tid = %" PRIu64 ", total instructions = %zu\n",
-           thread.GetIndexID(), thread.GetID(), instructions_count);
+           thread.GetIndexID(), thread.GetID(), *instructions_count);
 
-  if (count == 0 || end_position >= instructions_count)
+  if (count == 0 || end_position >= *instructions_count)
     return;
 
+  int digits_count = GetNumberOfDigits(end_position);
   size_t start_position =
       end_position + 1 < count ? 0 : end_position + 1 - count;
-
-  int digits_count = GetNumberOfDigits(end_position);
   auto printInstructionIndex = [&](size_t index) {
     s.Printf("    [%*zu] ", digits_count, index);
   };
 
   bool was_prev_instruction_an_error = false;
-  Target &target = thread.GetProcess()->GetTarget();
+  Optional<InstructionSymbolInfo> prev_insn;
 
-  SymbolContext sc;
-  DisassemblerSP disassembler;
-  ExecutionContext exe_ctx;
-  target.CalculateExecutionContext(exe_ctx);
+  TraverseInstructionsWithSymbolInfo(
+      *this, thread, start_position, TraceDirection::Forwards,
+      eSymbolContextEverything, /*disassembler*/ true,
+      [&](size_t index, Expected<InstructionSymbolInfo> insn) -> bool {
+        if (!insn) {
+          printInstructionIndex(index);
+          s << toString(insn.takeError());
 
-  TraverseInstructions(
-      thread, start_position, TraceDirection::Forwards,
-      [&](size_t index, Expected<lldb::addr_t> load_address) -> bool {
-        if (load_address) {
-          // We print an empty line after a sequence of errors to show more
-          // clearly that there's a gap in the trace
+          prev_insn = None;
+          was_prev_instruction_an_error = true;
+        } else {
           if (was_prev_instruction_an_error)
             s.Printf("    ...missing instructions\n");
 
-          Address address;
-          if (!raw) {
-            target.GetSectionLoadList().ResolveLoadAddress(*load_address,
-                                                           address);
-
-            sc = DumpSymbolContext(s, sc, target, address);
-          }
+          if (!raw)
+            DumpInstructionSymbolContext(s, prev_insn, *insn);
 
           printInstructionIndex(index);
-          s.Printf("0x%016" PRIx64 "    ", *load_address);
+          s.Printf("0x%016" PRIx64 "    ", insn->load_address);
 
-          if (!raw) {
-            disassembler =
-                DumpInstructionInfo(s, sc, disassembler, exe_ctx, address);
-          }
+          if (!raw)
+            DumpInstructionDisassembly(s, *insn);
 
+          prev_insn = *insn;
           was_prev_instruction_an_error = false;
-        } else {
-          printInstructionIndex(index);
-          s << toString(load_address.takeError());
-          was_prev_instruction_an_error = true;
-          if (!raw)
-            sc = SymbolContext();
         }
 
         s.Printf("\n");
-
         return index < end_position;
       });
 }
 
 Error Trace::Start(const llvm::json::Value &request) {
   if (!m_live_process)
     return createStringError(inconvertibleErrorCode(),
                              "Tracing requires a live process.");

lldb/test/API/commands/trace/TestTraceStartStop.py

\[0\] {ADDRESS_REGEX}    movl'''])
         self.expect("n")
         self.expect("thread trace dump instructions", 
             patterns=[f'''thread #1: tid = .*, total instructions = 5
   a.out`main \+ 4 at main.cpp:2
     \[0\] {ADDRESS_REGEX}    movl .*
   a.out`main \+ 11 at main.cpp:4
     \[1\] {ADDRESS_REGEX}    movl .*
     \[2\] {ADDRESS_REGEX}    jmp  .* ; <\+28> at main.cpp:4
-  a.out`main \+ 28 at main.cpp:4
     \[3\] {ADDRESS_REGEX}    cmpl .*
     \[4\] {ADDRESS_REGEX}    jle  .* ; <\+20> at main.cpp:5'''])
 
         # We stop tracing
         self.expect("thread trace stop")
 
         # We can't stop twice
         self.expect("thread trace stop", error=True,