Re: [PATCH] [gdb/tui] Don't disassemble non-code sections and section holes

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

> I was investigating TUI behaviour on ppc64-linux using the executable from
> test-case gdb.tui/basic.exp, and after scrolling in the asm window ended up
> with:
> ...
> +----------------------------------------------------------------------------+
> |   0x920 <__libc_start_main@plt+4> b       0x8f0 <__glink_PLTresolve>       |
> |   0x924 <__gmon_start__@plt>      li      r0,1                             |
> |   0x928 <__gmon_start__@plt+4>    b       0x8f0 <__glink_PLTresolve>       |
> |   0x92c <__cxa_finalize@plt>      li      r0,2                             |
> |   0x930 <__cxa_finalize@plt+4>    b       0x8f0 <__glink_PLTresolve>       |
> |   0x934 <._fini>                  mflr    r0                               |
> |   0x938 <._fini+4>                std     r0,16(r1)                        |
> |   0x93c <._fini+8>                stdu    r1,-112(r1)                      |
> |   0x940 <._fini+12>               addi    r1,r1,112                        |
> |   0x944 <._fini+16>               ld      r0,16(r1)                        |
> |   0x948 <._fini+20>               mtlr    r0                               |
> |   0x94c <._fini+24>               blr                                      |
> |   0x950 <_IO_stdin_used>          .long 0x20001                            |
> |   0x954                           .long 0x11b033b                          |
> |   0x958                           .long 0x18                               |
> |   0x95c                           .long 0x2                                |
> |   0x960                           .long 0xffffff60                         |
> |   0x964                           .long 0x48                               |
> |   0x968                           .long 0xffffff9c                         |
> |   0x96c                           .long 0x30                               |
> +----------------------------------------------------------------------------+
> exec No process (asm) In:                                        L??   PC: ??
> (gdb)
> ...
>
> The view corresponds to this objdump -D output:
> ...
> Disassembly of section .text:
>
>   ...
>
> 000000000000091c <__libc_start_main@plt>:
>  91c:   38 00 00 00     li      r0,0
>  920:   4b ff ff d0     b       8f0 <__glink_PLTresolve>
>
> 0000000000000924 <__gmon_start__@plt>:
>  924:   38 00 00 01     li      r0,1
>  928:   4b ff ff c8     b       8f0 <__glink_PLTresolve>
>
> 000000000000092c <__cxa_finalize@plt>:
>  92c:   38 00 00 02     li      r0,2
>  930:   4b ff ff c0     b       8f0 <__glink_PLTresolve>
>
> Disassembly of section .fini:
>
> 0000000000000934 <._fini>:
>  934:   7c 08 02 a6     mflr    r0
>  938:   f8 01 00 10     std     r0,16(r1)
>  93c:   f8 21 ff 91     stdu    r1,-112(r1)
>  940:   38 21 00 70     addi    r1,r1,112
>  944:   e8 01 00 10     ld      r0,16(r1)
>  948:   7c 08 03 a6     mtlr    r0
>  94c:   4e 80 00 20     blr
>
> Disassembly of section .rodata:
>
> 0000000000000950 <_IO_stdin_used>:
>  950:   00 02 00 01     .long 0x20001
>
> Disassembly of section .eh_frame_hdr:
>
> 0000000000000954 <__GNU_EH_FRAME_HDR>:
>  954:   01 1b 03 3b     .long 0x11b033b
>  958:   00 00 00 18     .long 0x18
>  95c:   00 00 00 02     .long 0x2
>  960:   ff ff ff 60     .long 0xffffff60
>  964:   00 00 00 48     .long 0x48
>  968:   ff ff ff 9c     .long 0xffffff9c
>  96c:   00 00 00 30     .long 0x30
> ...
>
> ISTM that we should not be disassembling the .rodata section.
>
> My first thought was to require tui_disassemble to stay in the same section,
> but after thinking about it a bit more decided that that was too restrictive,
> and instead went for requiring that we stay in code sections.
>
> So, I came up with this fix:
> ...
>       struct obj_section *section = find_pc_section (pc);
>       if (section != nullptr
> 	  && (bfd_section_flags (section->the_bfd_section) & SEC_CODE) == 0)
> 	return pc;
> ...
>
> Then I stumbled on PR tui/34399, which reports not being able to scroll back
> up after scrolling down.  The problem there is that we disassemble code from a
> section hole (0x401034-0x402000):
> ...
> Section Headers:
>   [Nr] Name              Type             Address           Offset
>        Size              EntSize          Flags  Link  Info  Align
>   [ 0]                   NULL             0000000000000000  00000000
>        0000000000000000  0000000000000000           0     0     0
>   [ 1] .note.gnu.pr[...] NOTE             0000000000400190  00000190
>        0000000000000030  0000000000000000   A       0     0     8
>   [ 2] .note.gnu.bu[...] NOTE             00000000004001c0  000001c0
>        0000000000000024  0000000000000000   A       0     0     4
>   [ 3] .text             PROGBITS         0000000000401000  00001000
>        0000000000000034  0000000000000000  AX       0     0     1
>   [ 4] .rodata           PROGBITS         0000000000402000  00002000
>        0000000000000006  0000000000000000   A       0     0     1
>   [ 5] .symtab           SYMTAB           0000000000000000  00002008
>        0000000000000078  0000000000000018           6     1     8
>   [ 6] .strtab           STRTAB           0000000000000000  00002080
>        0000000000000019  0000000000000000           0     0     1
>   [ 7] .shstrtab         STRTAB           0000000000000000  00002099
>        000000000000004f  0000000000000000           0     0     1
> Key to Flags:
>   W (write), A (alloc), X (execute), M (merge), S (strings), I (info),
>   L (link order), O (extra OS processing required), G (group), T (TLS),
>   C (compressed), x (unknown), o (OS specific), E (exclude),
>   D (mbind), l (large), p (processor specific)
> ...
> and once we call tui_find_backward_disassembly_start_address with an address
> ADDR for which find_pc_section (ADDR - 1) == nullptr, we are stuck in the
> section hole.
>
> Fix this by not disassembling section holes.
>
> This is achieved by slightly modifying the earlier fix:
> ...
>       if (section == nullptr
> 	  || (bfd_section_flags (section->the_bfd_section) & SEC_CODE) == 0)
> 	return pc;
> ...

TUI disassembly has been an ongoing issue for as long as I've been
contributing to GDB :-/ Getting the logic right for scrolling
forward/backward has always been an issue.

I'm not sure this change is the right fix though.  I have two concerns.

In CLI mode, if I stop the inferior and then use `disassemble START,END`
syntax, then I can disassemble any range of memory, not just executable
sections.

If I switch to TUI mode, without this patch and try the same disassemble
command then I'll get a screen full of instructions starting at START.
Is that useful given these aren't code sections?  I'm not really sure.

But if I apply your patch and try the same disassemble command then I
get a blank ASM window.

There are for sure ways that a user can work around this, `x/i` will
dump the instructions to the CMD window, even in TUI mode.  But the
blank window still looks wrong to me, and I think it is not what users
will expect.

The other problem I see is that this will, if I understand correctly,
prevent disassembly of memory that is not backed by an ELF section.  For
example, a JIT compiling interpreter.  Given such a tool I'd expect to
be able to disassemble the generated code regions, but these will likely
not be backed by an ELF section.

Thanks,
Andrew