[lldb] Reduce duplication in the Disassembler class

[labath]

Summary

The class has two pairs of functions whose functionalities differ in
only how one specifies how much he wants to disasseble. One limits the
process by the size of the input memory region. The other based on the
total amount of instructions disassembled. They also differ in various
features (like error reporting) that were only added to one of the
versions.

There are various ways in which this could be addressed. This patch
does it by introducing a helper struct called "Limit", which is
effectively a pair specifying the value that you want to limit, and the
actual limit itself.

[labath]

lldb/source/Commands/CommandObjectDisassemble.cpp

462–469

The idea is that further refactorings will change ranges from a vector<AddressRange> to a vector<pair<Address, Limit>> and this will become a simple ranged for loop.

[JDevlieghere]

Neat!

lldb/include/lldb/Core/Disassembler.h

public:
   FindPlugin(const ArchSpec &arch, const char *flavor, const char *plugin_name);
 
   // This version will use the value in the Target settings if flavor is NULL;
   static lldb::DisassemblerSP FindPluginForTarget(const Target &target,
                                                   const ArchSpec &arch,
                                                   const char *flavor,
                                                   const char *plugin_name);
 
+  struct Limit {
+    enum { Bytes, Instructions } kind;
+    lldb::addr_t value;
+  };
+
   static lldb::DisassemblerSP
   DisassembleRange(const ArchSpec &arch, const char *plugin_name,
                    const char *flavor, Target &target,
                    const AddressRange &disasm_range, bool prefer_file_cache);
 
   static lldb::DisassemblerSP
   DisassembleBytes(const ArchSpec &arch, const char *plugin_name,
                    const char *flavor, const Address &start, const void *bytes,
                    size_t length, uint32_t max_num_instructions,
                    bool data_from_file);
 
   static bool Disassemble(Debugger &debugger, const ArchSpec &arch,
                           const char *plugin_name, const char *flavor,
-                          const ExecutionContext &exe_ctx,
-                          const AddressRange &range, uint32_t num_instructions,
-                          bool mixed_source_and_assembly,
-                          uint32_t num_mixed_context_lines, uint32_t options,
-                          Stream &strm);
-
-  static bool Disassemble(Debugger &debugger, const ArchSpec &arch,
-                          const char *plugin_name, const char *flavor,
                           const ExecutionContext &exe_ctx, const Address &start,
-                          uint32_t num_instructions,
-                          bool mixed_source_and_assembly,
+                          Limit limit, bool mixed_source_and_assembly,
                           uint32_t num_mixed_context_lines, uint32_t options,
                           Stream &strm);
 
   static bool
   Disassemble(Debugger &debugger, const ArchSpec &arch, const char *plugin_name,
               const char *flavor, const ExecutionContext &exe_ctx,
               uint32_t num_instructions, bool mixed_source_and_assembly,
               uint32_t num_mixed_context_lines, uint32_t options, Stream &strm);
 
   // Constructors and Destructors
   Disassembler(const ArchSpec &arch, const char *flavor);
   ~Disassembler() override;
 
   void PrintInstructions(Debugger &debugger, const ArchSpec &arch,
                          const ExecutionContext &exe_ctx,
-                         uint32_t num_instructions,
                          bool mixed_source_and_assembly,
                          uint32_t num_mixed_context_lines, uint32_t options,
                          Stream &strm);
 
-  size_t ParseInstructions(Target &target, AddressRange range,
+  size_t ParseInstructions(Target &target, Address address, Limit limit,
                            Stream *error_strm_ptr, bool prefer_file_cache);
 
-  size_t ParseInstructions(Target &target, Address address,
-                           uint32_t num_instructions, bool prefer_file_cache);
-
   virtual size_t DecodeInstructions(const Address &base_addr,
                                     const DataExtractor &data,
                                     lldb::offset_t data_offset,
                                     size_t num_instructions, bool append,
                                     bool data_from_file) = 0;
 
   InstructionList &GetInstructionList();
 

lldb/source/Commands/CommandObjectDisassemble.cpp

if (ranges.empty()) {
         return false;
       }
       ranges.push_back(range);
     }
   }
 
   bool print_sc_header = ranges.size() > 1;
   for (AddressRange cur_range : ranges) {
-    bool success;
-    if (m_options.num_instructions != 0) {
-      success = Disassembler::Disassemble(
-          GetDebugger(), m_options.arch, plugin_name, flavor_string, m_exe_ctx,
-          cur_range.GetBaseAddress(), m_options.num_instructions,
-          m_options.show_mixed,
-          m_options.show_mixed ? m_options.num_lines_context : 0, options,
-          result.GetOutputStream());
+    Disassembler::Limit limit;
+    if (m_options.num_instructions == 0) {
+      limit = {Disassembler::Limit::Bytes, cur_range.GetByteSize()};
+      if (limit.value == 0)
+        limit.value = DEFAULT_DISASM_BYTE_SIZE;
     } else {
-      if (cur_range.GetByteSize() == 0)
-        cur_range.SetByteSize(DEFAULT_DISASM_BYTE_SIZE);
-
-      success = Disassembler::Disassemble(
-          GetDebugger(), m_options.arch, plugin_name, flavor_string, m_exe_ctx,
-          cur_range, m_options.num_instructions, m_options.show_mixed,
-          m_options.show_mixed ? m_options.num_lines_context : 0, options,
-          result.GetOutputStream());
+      limit = {Disassembler::Limit::Instructions, m_options.num_instructions};
     }
-    if (success) {
+    if (Disassembler::Disassemble(
+            GetDebugger(), m_options.arch, plugin_name, flavor_string,
+            m_exe_ctx, cur_range.GetBaseAddress(), limit, m_options.show_mixed,
+            m_options.show_mixed ? m_options.num_lines_context : 0, options,
+            result.GetOutputStream())) {
       result.SetStatus(eReturnStatusSuccessFinishResult);
     } else {
       if (m_options.symbol_containing_addr != LLDB_INVALID_ADDRESS) {
         result.AppendErrorWithFormat(
             "Failed to disassemble memory in function at 0x%8.8" PRIx64 ".\n",
             m_options.symbol_containing_addr);
       } else {
         result.AppendErrorWithFormat(

lldb/source/Core/Disassembler.cpp

if (!range.GetBaseAddress().IsValid())
     return {};
 
   lldb::DisassemblerSP disasm_sp =
       Disassembler::FindPluginForTarget(target, arch, flavor, plugin_name);
 
   if (!disasm_sp)
     return {};
 
-  const size_t bytes_disassembled =
-      disasm_sp->ParseInstructions(target, range, nullptr, prefer_file_cache);
+  const size_t bytes_disassembled = disasm_sp->ParseInstructions(
+      target, range.GetBaseAddress(), {Limit::Bytes, range.GetByteSize()},
+      nullptr, prefer_file_cache);
   if (bytes_disassembled == 0)
     return {};
 
   return disasm_sp;
 }
 
 lldb::DisassemblerSP
 Disassembler::DisassembleBytes(const ArchSpec &arch, const char *plugin_name,
Disassembler::DisassembleBytes(const ArchSpec &arch, const char *plugin_name,
   (void)disasm_sp->DecodeInstructions(start, data, 0, num_instructions, false,
                                       data_from_file);
   return disasm_sp;
 }
 
 bool Disassembler::Disassemble(Debugger &debugger, const ArchSpec &arch,
                                const char *plugin_name, const char *flavor,
                                const ExecutionContext &exe_ctx,
-                               const AddressRange &range,
-                               uint32_t num_instructions,
+                               const Address &address, Limit limit,
                                bool mixed_source_and_assembly,
                                uint32_t num_mixed_context_lines,
                                uint32_t options, Stream &strm) {
-  if (!range.GetByteSize() || !exe_ctx.GetTargetPtr())
-    return false;
-
-  lldb::DisassemblerSP disasm_sp(Disassembler::FindPluginForTarget(
-      exe_ctx.GetTargetRef(), arch, flavor, plugin_name));
-
-  if (!disasm_sp)
-    return false;
-
-  const bool prefer_file_cache = false;
-  size_t bytes_disassembled = disasm_sp->ParseInstructions(
-      exe_ctx.GetTargetRef(), range, &strm, prefer_file_cache);
-  if (bytes_disassembled == 0)
-    return false;
-
-  disasm_sp->PrintInstructions(debugger, arch, exe_ctx, num_instructions,
-                               mixed_source_and_assembly,
-                               num_mixed_context_lines, options, strm);
-  return true;
-}
-
-bool Disassembler::Disassemble(
-    Debugger &debugger, const ArchSpec &arch, const char *plugin_name,
-    const char *flavor, const ExecutionContext &exe_ctx, const Address &address,
-    uint32_t num_instructions, bool mixed_source_and_assembly,
-    uint32_t num_mixed_context_lines, uint32_t options, Stream &strm) {
-  if (num_instructions == 0 || !exe_ctx.GetTargetPtr())
+  if (!exe_ctx.GetTargetPtr())
     return false;
 
   lldb::DisassemblerSP disasm_sp(Disassembler::FindPluginForTarget(
       exe_ctx.GetTargetRef(), arch, flavor, plugin_name));
   if (!disasm_sp)
     return false;
 
   const bool prefer_file_cache = false;
   size_t bytes_disassembled = disasm_sp->ParseInstructions(
-      exe_ctx.GetTargetRef(), address, num_instructions, prefer_file_cache);
+      exe_ctx.GetTargetRef(), address, limit, &strm, prefer_file_cache);
   if (bytes_disassembled == 0)
     return false;
 
-  disasm_sp->PrintInstructions(debugger, arch, exe_ctx, num_instructions,
+  disasm_sp->PrintInstructions(debugger, arch, exe_ctx,
                                mixed_source_and_assembly,
                                num_mixed_context_lines, options, strm);
   return true;
 }
 
 Disassembler::SourceLine
 Disassembler::GetFunctionDeclLineEntry(const SymbolContext &sc) {
   if (!sc.function)
if (avoid_regex && sc.symbol != nullptr) {
     }
   }
   // don't skip this source line
   return false;
 }
 
 void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
                                      const ExecutionContext &exe_ctx,
-                                     uint32_t num_instructions,
                                      bool mixed_source_and_assembly,
                                      uint32_t num_mixed_context_lines,
                                      uint32_t options, Stream &strm) {
   // We got some things disassembled...
   size_t num_instructions_found = GetInstructionList().GetSize();
 
-  if (num_instructions > 0 && num_instructions < num_instructions_found)
-    num_instructions_found = num_instructions;
-
   const uint32_t max_opcode_byte_size =
       GetInstructionList().GetMaxOpcocdeByteSize();
   SymbolContext sc;
   SymbolContext prev_sc;
   AddressRange current_source_line_range;
   const Address *pc_addr_ptr = nullptr;
   StackFrame *frame = exe_ctx.GetFramePtr();
 
if (frame) {
     } else {
       range.GetBaseAddress() = frame->GetFrameCodeAddress();
     }
 
     if (range.GetBaseAddress().IsValid() && range.GetByteSize() == 0)
       range.SetByteSize(DEFAULT_DISASM_BYTE_SIZE);
   }
 
-  return Disassemble(debugger, arch, plugin_name, flavor, exe_ctx, range,
-                     num_instructions, mixed_source_and_assembly,
-                     num_mixed_context_lines, options, strm);
+  return Disassemble(
+      debugger, arch, plugin_name, flavor, exe_ctx, range.GetBaseAddress(),
+      {Limit::Instructions, num_instructions}, mixed_source_and_assembly,
 }
 
 Instruction::Instruction(const Address &address, AddressClass addr_class)
     : m_address(address), m_address_class(addr_class), m_opcode(),
       m_calculated_strings(false) {}
 
 Instruction::~Instruction() = default;
 

 uint32_t
 InstructionList::GetIndexOfInstructionAtLoadAddress(lldb::addr_t load_addr,
                                                     Target &target) {
   Address address;
   address.SetLoadAddress(load_addr, &target);
   return GetIndexOfInstructionAtAddress(address);
 }
 
-size_t Disassembler::ParseInstructions(Target &target, AddressRange range,
-                                       Stream *error_strm_ptr,
-                                       bool prefer_file_cache) {
-  const addr_t byte_size = range.GetByteSize();
-  if (byte_size == 0 || !range.GetBaseAddress().IsValid())
-    return 0;
-
-  range.GetBaseAddress() = ResolveAddress(target, range.GetBaseAddress());
-
-  auto data_sp = std::make_shared<DataBufferHeap>(byte_size, '\0');
-
-  Status error;
-  lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
-  const size_t bytes_read = target.ReadMemory(
-      range.GetBaseAddress(), prefer_file_cache, data_sp->GetBytes(),
-      data_sp->GetByteSize(), error, &load_addr);
-
-  if (bytes_read > 0) {
-    if (bytes_read != data_sp->GetByteSize())
-      data_sp->SetByteSize(bytes_read);
-    DataExtractor data(data_sp, m_arch.GetByteOrder(),
-                       m_arch.GetAddressByteSize());
-    const bool data_from_file = load_addr == LLDB_INVALID_ADDRESS;
-    return DecodeInstructions(range.GetBaseAddress(), data, 0, UINT32_MAX,
-                              false, data_from_file);
-  } else if (error_strm_ptr) {
-    const char *error_cstr = error.AsCString();
-    if (error_cstr) {
-      error_strm_ptr->Printf("error: %s\n", error_cstr);
-    }
-  }
-  return 0;
-}
-
 size_t Disassembler::ParseInstructions(Target &target, Address start,
-                                       uint32_t num_instructions,
+                                       Limit limit, Stream *error_strm_ptr,
                                        bool prefer_file_cache) {
   m_instruction_list.Clear();
 
-  if (num_instructions == 0 || !start.IsValid())
+  if (!start.IsValid())
     return 0;
 
   start = ResolveAddress(target, start);
 
-  // Calculate the max buffer size we will need in order to disassemble
-  const addr_t byte_size = num_instructions * m_arch.GetMaximumOpcodeByteSize();
-
-  if (byte_size == 0)
-    return 0;
-
-  DataBufferHeap *heap_buffer = new DataBufferHeap(byte_size, '\0');
-  DataBufferSP data_sp(heap_buffer);
+  addr_t byte_size = limit.value;
+  if (limit.kind == Limit::Instructions)
+    byte_size *= m_arch.GetMaximumOpcodeByteSize();
+  auto data_sp = std::make_shared<DataBufferHeap>(byte_size, '\0');
 
   Status error;
   lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
   const size_t bytes_read =
-      target.ReadMemory(start, prefer_file_cache, heap_buffer->GetBytes(),
-                        byte_size, error, &load_addr);
-
+      target.ReadMemory(start, prefer_file_cache, data_sp->GetBytes(),
+                        data_sp->GetByteSize(), error, &load_addr);
   const bool data_from_file = load_addr == LLDB_INVALID_ADDRESS;
 
-  if (bytes_read == 0)
+  if (bytes_read == 0) {
+    if (error_strm_ptr) {
+      if (const char *error_cstr = error.AsCString())
+        error_strm_ptr->Printf("error: %s\n", error_cstr);
+    }
     return 0;
+  }
+
+  if (bytes_read != data_sp->GetByteSize())
+    data_sp->SetByteSize(bytes_read);
   DataExtractor data(data_sp, m_arch.GetByteOrder(),
                      m_arch.GetAddressByteSize());
-
-  const bool append_instructions = true;
-  DecodeInstructions(start, data, 0, num_instructions, append_instructions,
-                     data_from_file);
-
-  return m_instruction_list.GetSize();
+  return DecodeInstructions(start, data, 0,
+                            limit.kind == Limit::Instructions ? limit.value
+                                                              : UINT32_MAX,
+                            false, data_from_file);
 }
 
 // Disassembler copy constructor
 Disassembler::Disassembler(const ArchSpec &arch, const char *flavor)
     : m_arch(arch), m_instruction_list(), m_base_addr(LLDB_INVALID_ADDRESS),
       m_flavor() {
   if (flavor == nullptr)
     m_flavor.assign("default");

lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp

Instruction *ArchitectureMips::GetInstructionAtAddress(
   bool prefer_file_cache = true; // Read from file
   uint32_t inst_to_choose = 0;
 
   Address addr = resolved_addr;
 
   for (uint32_t i = 1; i <= loop_count; i++) {
     // Adjust the address to read from.
     addr.Slide(-2);
-    AddressRange range(addr, i * 2);
     uint32_t insn_size = 0;
 
-    disasm_sp->ParseInstructions(target, range, nullptr, prefer_file_cache);
+    disasm_sp->ParseInstructions(target, addr,
+                                 {Disassembler::Limit::Bytes, i * 2}, nullptr,
+                                 prefer_file_cache);
 
     uint32_t num_insns = disasm_sp->GetInstructionList().GetSize();
     if (num_insns) {
       prev_insn = disasm_sp->GetInstructionList().GetInstructionAtIndex(0);
       insn_size = prev_insn->GetOpcode().GetByteSize();
       if (i == 1 && insn_size == 2) {
         // This looks like a valid 2-byte instruction (but it could be a part
         // of upper 4 byte instruction).

lldb/source/Target/StackFrame.cpp

if (target) {
           break;
         LLVM_FALLTHROUGH;
 
       case Debugger::eStopDisassemblyTypeAlways:
         if (target) {
           const uint32_t disasm_lines = debugger.GetDisassemblyLineCount();
           if (disasm_lines > 0) {
             const ArchSpec &target_arch = target->GetArchitecture();
-            AddressRange pc_range;
-            pc_range.GetBaseAddress() = GetFrameCodeAddress();
-            pc_range.SetByteSize(disasm_lines *
-                                 target_arch.GetMaximumOpcodeByteSize());
             const char *plugin_name = nullptr;
             const char *flavor = nullptr;
             const bool mixed_source_and_assembly = false;
             Disassembler::Disassemble(
                 target->GetDebugger(), target_arch, plugin_name, flavor,
-                exe_ctx, pc_range, disasm_lines, mixed_source_and_assembly, 0,
+                exe_ctx, GetFrameCodeAddress(),
+                {Disassembler::Limit::Instructions, disasm_lines},
+                mixed_source_and_assembly, 0,
                 Disassembler::eOptionMarkPCAddress, strm);
           }
         }
         break;
       }
     }
   }
   return true;