[lldb] Fix a regression introduced by D75730

[JDevlieghere]

Summary

In a new Range class was introduced to simplify and the Disassembler API and reduce duplication. It unintentionally broke the SBFrame::Disassemble functionality because it unconditionally converts the number of instructions to a Range{Limit::Instructions, num_instructions}. This is subtly different from the previous behavior, where now we're passing a Range and assume it's valid in the callee, the original code would propagate num_instructions and the callee would compare the value and decided between disassembling instructions or bytes.

Unfortunately the existing tests was not particularly strict:

disassembly = frame.Disassemble()
self.assertNotEqual(len(disassembly), 0, "Disassembly was empty.")

This would pass because without this patch we'd disassemble zero instructions, resulting in an error:

./bin/lldb /tmp/a.out -o 'b main' -o r -o 'script print(lldb.frame.Disassemble())'
(lldb) target create "/tmp/a.out"
Current executable set to '/tmp/a.out' (x86_64).
(lldb) b main
Breakpoint 1: where = a.out`main + 11 at foo.c:2:3, address = 0x0000000100003fab
(lldb) r
Process 22141 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x0000000100003fab a.out`main at foo.c:2:3
   1    int main() {
-> 2      return 10;
   3    }

Process 22141 launched: '/tmp/a.out' (x86_64)
(lldb) script print(lldb.frame.Disassemble())
error: error reading data from section __text

[jasonmolenda]

So in this patch you're setting the limit to be the size of the range (for someone who did 'disass -s 0x100 -e 0x200' they would be requesting 100 instructions) but the disassembly returned would be limited by the byte range requested, so this works OK. The asm(nop) is to guarantee that sum() has at least one instruction? There's no documentation for the Disassembler ctors where we might mention that the use case is either (1) an AddressRange, or (2) a start address and instruction count, but maybe there should be?

[jasonmolenda]

(*cough* "requesting 256 instructions", wrote too quickly, or rather I am speaking in gdb RSP and don't always prefix my base16 numbers).

[JDevlieghere]

In D89925#2346290, @jasonmolenda wrote:

So in this patch you're setting the limit to be the size of the range (for someone who did 'disass -s 0x100 -e 0x200' they would be requesting 100 instructions) but the disassembly returned would be limited by the byte range requested, so this works OK.

Not exactly. The patch uses the range of the current symbol when the number of lines to disassemble == 0. You can think of it as just disass (even though that goes through a different code path and continued to work).

The asm(nop) is to guarantee that sum() has at least one instruction?

The nop is just there so we can check that the output actually contains the instructions from the current frame. I think a nop should work everywhere our test suite runs.

There's no documentation for the Disassembler ctors where we might mention that the use case is either (1) an AddressRange, or (2) a start address and instruction count, but maybe there should be?

Once converted to a Range object it has to be either so I think it's implicit in the API. This API doesn't take a range but a uint32_t num_instructions which is expected to give you all the instructions for the current frame/symbol when 0.

[labath]

If needed, we can add some #ifdefs to produce suitable instructions on other architectures too.

lldb/source/Core/Disassembler.cpp

542–543

Maybe change this to take a StackFrame& argument? The only caller StackFrame::Disassemble, so the frame argument is always valid (and a lot of this defensive code is not needed).

lldb/include/lldb/Core/Disassembler.h

 }
 
 namespace lldb_private {
 class AddressRange;
 class DataExtractor;
 class Debugger;
 class Disassembler;
 class Module;
+class StackFrame;
 class Stream;
 class SymbolContext;
 class SymbolContextList;
 class Target;
 struct RegisterInfo;
 
 class Instruction {
 public:
public:
 
   static bool Disassemble(Debugger &debugger, const ArchSpec &arch,
                           const char *plugin_name, const char *flavor,
                           const ExecutionContext &exe_ctx, const Address &start,
                           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);
+  static bool Disassemble(Debugger &debugger, const ArchSpec &arch,
+                          StackFrame &frame, 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,
                          bool mixed_source_and_assembly,

lldb/source/Core/Disassembler.cpp

if (inst) {
                  &prev_sc, nullptr, address_text_size);
       strm.EOL();
     } else {
       break;
     }
   }
 }
 
 bool Disassembler::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) {
+                               StackFrame &frame, Stream &strm) {
   AddressRange range;
-  StackFrame *frame = exe_ctx.GetFramePtr();
-  if (frame) {
-    SymbolContext sc(
-        frame->GetSymbolContext(eSymbolContextFunction | eSymbolContextSymbol));
-    if (sc.function) {
-      range = sc.function->GetAddressRange();
-    } else if (sc.symbol && sc.symbol->ValueIsAddress()) {
-      range.GetBaseAddress() = sc.symbol->GetAddressRef();
-      range.SetByteSize(sc.symbol->GetByteSize());
-    } else {
-      range.GetBaseAddress() = frame->GetFrameCodeAddress();
-    }
+      frame.GetSymbolContext(eSymbolContextFunction | eSymbolContextSymbol));
+    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.GetBaseAddress(),
-      {Limit::Instructions, num_instructions}, mixed_source_and_assembly,
-      num_mixed_context_lines, options, strm);
+    Disassembler::Limit limit = {Disassembler::Limit::Bytes,
+                                 range.GetByteSize()};
+    if (limit.value == 0)
+      limit.value = DEFAULT_DISASM_BYTE_SIZE;
+
+    return Disassemble(debugger, arch, nullptr, nullptr, frame,
+                       range.GetBaseAddress(), limit, false, 0, 0, strm);
 }
 
 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;
 

lldb/source/Target/StackFrame.cpp

bool StackFrame::ChangePC(addr_t pc) {
   ThreadSP thread_sp(GetThread());
   if (thread_sp)
     thread_sp->ClearStackFrames();
   return true;
 }
 
 const char *StackFrame::Disassemble() {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
-  if (m_disassembly.Empty()) {
-    ExecutionContext exe_ctx(shared_from_this());
-    Target *target = exe_ctx.GetTargetPtr();
-    if (target) {
-      const char *plugin_name = nullptr;
-      const char *flavor = nullptr;
-      Disassembler::Disassemble(target->GetDebugger(),
-                                target->GetArchitecture(), plugin_name, flavor,
-                                exe_ctx, 0, false, 0, 0, m_disassembly);
-    }
-    if (m_disassembly.Empty())
-      return nullptr;
+  if (!m_disassembly.Empty())
+    return m_disassembly.GetData();
+
+  if (Target *target = exe_ctx.GetTargetPtr()) {
+    Disassembler::Disassemble(target->GetDebugger(), target->GetArchitecture(),
+                              *this, m_disassembly);
   }
 
-  return m_disassembly.GetData();
+  return m_disassembly.Empty() ? nullptr : m_disassembly.GetData();
 }
 
 Block *StackFrame::GetFrameBlock() {
   if (m_sc.block == nullptr && m_flags.IsClear(eSymbolContextBlock))
     GetSymbolContext(eSymbolContextBlock);
 
   if (m_sc.block) {
     Block *inline_block = m_sc.block->GetContainingInlinedBlock();

lldb/test/API/commands/disassemble/basic/TestFrameDisassemble.py

def frame_disassemble_test(self):
             len(threads) == 1,
             "There should be a thread stopped at our breakpoint")
 
         # The hit count for the breakpoint should be 1.
         self.assertEquals(breakpoint.GetHitCount(), 1)
 
         frame = threads[0].GetFrameAtIndex(0)
         disassembly = frame.Disassemble()
-        self.assertNotEqual(len(disassembly), 0, "Disassembly was empty.")
+        self.assertNotEqual(disassembly, "")
+        self.assertNotIn("error", disassembly)
+        self.assertIn(": nop", disassembly)

lldb/test/API/commands/disassemble/basic/main.cpp

 int
 sum (int a, int b)
 {
     int result = a + b; // Set a breakpoint here
+    asm("nop");
     return result;
 }
 
 int
 main(int argc, char const *argv[])
 {
 
     int array[3];
 
     array[0] = sum (1238, 78392);
     array[1] = sum (379265, 23674);
     array[2] = sum (872934, 234);
 
     return 0;
 }