This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Fix a regression introduced by D75730
ClosedPublic

Authored by JDevlieghere on Oct 21 2020, 9:09 PM.

Details

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

Diff Detail

Event Timeline

JDevlieghere created this revision.Oct 21 2020, 9:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2020, 9:09 PM
Herald added a subscriber: pengfei. · View Herald Transcript
JDevlieghere requested review of this revision.Oct 21 2020, 9:09 PM

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?

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

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 accepted this revision.Oct 22 2020, 2:47 AM

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).

This revision is now accepted and ready to land.Oct 22 2020, 2:47 AM
This revision was landed with ongoing or failed builds.Oct 22 2020, 8:38 AM
This revision was automatically updated to reflect the committed changes.