optimize for architectures with a fixed opcode size, add backwards disassembly test

Author: eronnen

lldb/include/lldb/API/SBTarget.h

@@ -349,6 +349,10 @@ class LLDB_API SBTarget {
 
   SBError SetLabel(const char *label);
 
+  uint32_t GetMinimumOpcodeByteSize() const;
+
+  uint32_t GetMaximumOpcodeByteSize() const;
+
   /// Architecture data byte width accessor
   ///
   /// \return

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py

@@ -136,7 +136,6 @@ def __init__(
         self.initialized = False
         self.frame_scopes = {}
         self.init_commands = init_commands
-        self.disassembled_instructions = {}
 
     @classmethod
     def encode_content(cls, s: str) -> bytes:
@@ -735,11 +734,15 @@ def request_disconnect(self, terminateDebuggee=None):
         return self.send_recv(command_dict)
 
     def request_disassemble(
-        self, memoryReference, offset=0, instructionCount=200, resolveSymbols=True
+        self,
+        memoryReference,
+        instructionOffset=-50,
+        instructionCount=200,
+        resolveSymbols=True,
     ):
         args_dict = {
             "memoryReference": memoryReference,
-            "offset": offset,
+            "instructionOffset": instructionOffset,
             "instructionCount": instructionCount,
             "resolveSymbols": resolveSymbols,
         }
@@ -748,9 +751,7 @@ def request_disassemble(
             "type": "request",
             "arguments": args_dict,
         }
-        instructions = self.send_recv(command_dict)["body"]["instructions"]
-        for inst in instructions:
-            self.disassembled_instructions[inst["address"]] = inst
+        return self.send_recv(command_dict)["body"]["instructions"]
 
     def request_readMemory(self, memoryReference, offset, count):
         args_dict = {

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py

@@ -346,10 +346,14 @@ def disassemble(self, threadId=None, frameIndex=None):
         memoryReference = stackFrames[0]["instructionPointerReference"]
         self.assertIsNotNone(memoryReference)
 
-        if memoryReference not in self.dap_server.disassembled_instructions:
-            self.dap_server.request_disassemble(memoryReference=memoryReference)
+        instructions = self.dap_server.request_disassemble(
+            memoryReference=memoryReference
+        )
+        disassembled_instructions = {}
+        for inst in instructions:
+            disassembled_instructions[inst["address"]] = inst
 
-        return self.dap_server.disassembled_instructions[memoryReference]
+        return disassembled_instructions, disassembled_instructions[memoryReference]
 
     def attach(
         self,

lldb/source/API/SBTarget.cpp

@@ -1668,6 +1668,26 @@ SBError SBTarget::SetLabel(const char *label) {
   return Status::FromError(target_sp->SetLabel(label));
 }
 
+uint32_t SBTarget::GetMinimumOpcodeByteSize() const {
+  LLDB_INSTRUMENT_VA(this);
+
+  TargetSP target_sp(GetSP());
+  if (target_sp) {
+    return target_sp->GetArchitecture().GetMinimumOpcodeByteSize();
+  }
+  return 0;
+}
+
+uint32_t SBTarget::GetMaximumOpcodeByteSize() const {
+  LLDB_INSTRUMENT_VA(this);
+
+  TargetSP target_sp(GetSP());
+  if (target_sp) {
+    return target_sp->GetArchitecture().GetMaximumOpcodeByteSize();
+  }
+  return 0;
+}
+
 uint32_t SBTarget::GetDataByteSize() {
   LLDB_INSTRUMENT_VA(this);
 

lldb/test/API/tools/lldb-dap/disassemble/TestDAP_disassemble.py

@@ -20,21 +20,48 @@ def test_disassemble(self):
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(program)
         source = "main.c"
-        self.source_path = os.path.join(os.getcwd(), source)
-        self.set_source_breakpoints(
-            source,
-            [
-                line_number(source, "// breakpoint 1"),
-            ],
-        )
+        self.set_source_breakpoints(source, [line_number(source, "// breakpoint 1")])
         self.continue_to_next_stop()
 
-        pc_assembly = self.disassemble(frameIndex=0)
+        _, pc_assembly = self.disassemble(frameIndex=0)
         self.assertIn("location", pc_assembly, "Source location missing.")
         self.assertIn("instruction", pc_assembly, "Assembly instruction missing.")
 
         # The calling frame (qsort) is coming from a system library, as a result
         # we should not have a source location.
-        qsort_assembly = self.disassemble(frameIndex=1)
+        _, qsort_assembly = self.disassemble(frameIndex=1)
         self.assertNotIn("location", qsort_assembly, "Source location not expected.")
         self.assertIn("instruction", pc_assembly, "Assembly instruction missing.")
+
+    def test_disassemble_backwards(self):
+        """
+        Tests the 'disassemble' request with a backwards disassembly range.
+        """
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        source = "main.c"
+        self.set_source_breakpoints(source, [line_number(source, "// breakpoint 1")])
+        self.continue_to_next_stop()
+
+        instruction_pointer_reference = self.get_stackFrames()[1][
+            "instructionPointerReference"
+        ]
+        backwards_instructions = 50
+        instructions = self.dap_server.request_disassemble(
+            memoryReference=instruction_pointer_reference,
+            instructionOffset=-backwards_instructions,
+        )
+
+        frame_instruction_index = next(
+            (
+                i
+                for i, instruction in enumerate(instructions)
+                if instruction["address"] == instruction_pointer_reference
+            ),
+            -1,
+        )
+        self.assertEqual(
+            frame_instruction_index,
+            backwards_instructions,
+            f"requested instruction should be preceeded by {backwards_instructions} instructions. Actual index: {frame_instruction_index}",
+        )

lldb/test/API/tools/lldb-dap/disassemble/main.c

@@ -27,4 +27,4 @@ int main(void) {
 
   printf("\n");
   return 0;
-}
+}

lldb/test/API/tools/lldb-dap/instruction-breakpoint/TestDAP_instruction_breakpoint.py

@@ -66,16 +66,15 @@ def instruction_breakpoint_test(self):
         )
 
         # Check disassembly view
-        instruction = self.disassemble(frameIndex=0)
+        disassembled_instructions, instruction = self.disassemble(frameIndex=0)
         self.assertEqual(
             instruction["address"],
             intstructionPointerReference[0],
             "current breakpoint reference is not in the disaasembly view",
         )
 
         # Get next instruction address to set instruction breakpoint
-        disassembled_instruction_list = self.dap_server.disassembled_instructions
-        instruction_addr_list = list(disassembled_instruction_list.keys())
+        instruction_addr_list = list(disassembled_instructions.keys())
         index = instruction_addr_list.index(intstructionPointerReference[0])
         if len(instruction_addr_list) >= (index + 1):
             next_inst_addr = instruction_addr_list[index + 1]

lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp

@@ -105,29 +105,48 @@ DisassembleRequestHandler::disassembleBackwards(
     const char *flavor_string, bool resolve_symbols) const {
   std::vector<DisassembledInstruction> instructions;
 
-  // TODO: Simply disassemble from `addr` - `instruction_count` *
-  // `instruction_size` in architectures with a fixed instruction size.
-
-  // need to disassemble backwards, let's try from the start of the symbol if
-  // available.
-  auto symbol = addr.GetSymbol();
-  if (symbol.IsValid()) {
-    // add valid instructions before the current instruction using the symbol.
-    lldb::SBInstructionList symbol_insts = dap.target.ReadInstructions(
-        symbol.GetStartAddress(), addr, flavor_string);
-    if (symbol_insts.IsValid()) {
-      size_t backwards_insts_start =
-          symbol_insts.GetSize() >= instruction_count
-              ? symbol_insts.GetSize() - instruction_count
-              : 0;
-      for (size_t i = backwards_insts_start;
-           i < symbol_insts.GetSize() &&
-           instructions.size() < instruction_count;
-           ++i) {
-        lldb::SBInstruction inst = symbol_insts.GetInstructionAtIndex(i);
+  if (dap.target.GetMinimumOpcodeByteSize() ==
+      dap.target.GetMaximumOpcodeByteSize()) {
+    // If the target has a fixed opcode size, we can disassemble backwards
+    // directly.
+    lldb::addr_t disassemble_start_load_addr =
+        addr.GetLoadAddress(dap.target) -
+        (instruction_count * dap.target.GetMinimumOpcodeByteSize());
+    lldb::SBAddress disassemble_start_addr(disassemble_start_load_addr,
+                                           dap.target);
+    lldb::SBInstructionList backwards_insts =
+        dap.target.ReadInstructions(addr, instruction_count, flavor_string);
+    if (backwards_insts.IsValid()) {
+      for (size_t i = 0; i < backwards_insts.GetSize(); ++i) {
+        lldb::SBInstruction inst = backwards_insts.GetInstructionAtIndex(i);
         instructions.push_back(
             SBInstructionToDisassembledInstruction(inst, resolve_symbols));
       }
+      return instructions;
+    }
+  } else {
+    // There is no opcode fixed size so we have no idea where are the valid
+    // instructions before the current address. let's try from the start of the
+    // symbol if available.
+    auto symbol = addr.GetSymbol();
+    if (symbol.IsValid()) {
+      // add valid instructions before the current instruction using the symbol.
+      lldb::SBInstructionList symbol_insts = dap.target.ReadInstructions(
+          symbol.GetStartAddress(), addr, flavor_string);
+      if (symbol_insts.IsValid()) {
+        size_t backwards_insts_start =
+            symbol_insts.GetSize() >= instruction_count
+                ? symbol_insts.GetSize() - instruction_count
+                : 0;
+        for (size_t i = backwards_insts_start;
+             i < symbol_insts.GetSize() &&
+             instructions.size() < instruction_count;
+             ++i) {
+          lldb::SBInstruction inst = symbol_insts.GetInstructionAtIndex(i);
+          instructions.push_back(
+              SBInstructionToDisassembledInstruction(inst, resolve_symbols));
+        }
+      }
     }
   }