Re: [PATCH v4] gdb/tui/disassembly view: make symbol name appear on a line of its own

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2022-01-28 20:20, Simon Marchi via Gdb-patches wrote:
>> This default `false` concerned my initially.  The only times we call
>> tui_disassemble is either because we want to redraw the screen
>> contents, or we want to know what _would_ be drawn to the screen
>> if/when we do the redraw.
>>
>> Only, now, we draw the screen differently for these two cases, so, my
>> thinking goes, surely there's going to be some edge case where we ask,
>> what address would be on the screen if .... and we'll get the wrong
>> answer back.
>>
>> I played with this for a while, but couldn't get anything obvious to
>> break - I suspect that if there are bugs, they are going to be super
>> subtle, which addresses appear on the screen doesn't change much,
>> usually just one instruction different I think, so maybe it doesn't
>> matter.
>>
>> And given I couldn't spot anything, maybe I'm over thinking this, and
>> there is no problem...
>>
>> I guess my question is, did you already consider this already?  Is
>> there a reason why having two strategies is known to be OK?
> 
> No, I haven't considered this, it is a good question.  I really don't
> know the TUI code well (if at all), so my thinking was that if the TUI
> experts say it's ok, it's because it's ok :).  But indeed, it would be
> good to understand exactly what happens here.
> 
> I'll git a little bit.  Vasili, if you happen to know why we have these
> two behaviors (for_ui and !for_ui), feel free to answer.

Ok, so it in fact "breaks" page up / down, in the sense that the number
of lines scrolled up and down is not right.  This makes sense, as the
other uses of tui_disassemble are to help with scrolling.  It is used to
answer the question: assuming that we are currently showing disassembly
starting at this PC, what would be the starting PC if going up/down N
lines?

Without this patch, let's say that my asm windows shows:

│    0x110c <__do_global_dtors_aux+76>       nopl   0x0(%rax)                                   │
│    0x1110 <frame_dummy>                    endbr64                                            │
│    0x1114 <frame_dummy+4>                  jmp    0x1080 <register_tm_clones>                 │
│    0x1119 <main>                           push   %rbp                                        │
│    0x111a <main+1>                         mov    %rsp,%rbp                                   │
│    0x111d <main+4>                         movq   $0x0,-0x8(%rbp)                             │
│    0x1125 <main+12>                        mov    -0x8(%rbp),%rax                             │
│    0x1129 <main+16>                        mov    (%rax),%eax                                 │
│    0x112b <main+18>                        pop    %rbp                                        │
│    0x112c <main+19>                        ret                                                │

Doing page down, the goal (at least my understanding of it) is that the
last shown line becomes the first one.  The result is:

│    0x112c <main+19>                ret                                                        │
│    0x112d                          nopl   (%rax)                                              │
│    0x1130 <__libc_csu_init>        endbr64                                                    │
│    0x1134 <__libc_csu_init+4>      push   %r15                                                │
│    0x1136 <__libc_csu_init+6>      lea    0x2ceb(%rip),%r15        # 0x3e28                   │
│    0x113d <__libc_csu_init+13>     push   %r14                                                │
│    0x113f <__libc_csu_init+15>     mov    %rdx,%r14                                           │
│    0x1142 <__libc_csu_init+18>     push   %r13                                                │
│    0x1144 <__libc_csu_init+20>     mov    %rsi,%r13                                           │
│    0x1147 <__libc_csu_init+23>     push   %r12                                                │

With this patch applied, with the same starting position (0x110c being
the first instruction shown), I have:

│    __do_global_dtors_aux:                                                                     │
│    0x110c <+76>    nopl   0x0(%rax)                                                           │
│    frame_dummy:                                                                               │
│    0x1110 <+0>     endbr64                                                                    │
│    0x1114 <+4>     jmp    0x1080 <register_tm_clones>                                         │
│    main:                                                                                      │
│    0x1119 <+0>     push   %rbp                                                                │
│    0x111a <+1>     mov    %rsp,%rbp                                                           │
│    0x111d <+4>     movq   $0x0,-0x8(%rbp)                                                     │
│    0x1125 <+12>    mov    -0x8(%rbp),%rax                                                     │

After page down:

│    main:                                                                                      │
│    0x112c <+19>    ret                                                                        │
│                    nopl   (%rax)                                                              │
│    __libc_csu_init:                                                                           │
│    0x1130 <+0>     endbr64                                                                    │
│    0x1134 <+4>     push   %r15                                                                │
│    0x1136 <+6>     lea    0x2ceb(%rip),%r15        # 0x3e28                                   │
│    0x113d <+13>    push   %r14                                                                │
│    0x113f <+15>    mov    %rdx,%r14                                                           │
│    0x1142 <+18>    push   %r13                                                                │

Since 0x1125 is the last instruction shown before page down, it should
be the first one after page down.  But it is 0x11c2 - the same as we
would have gotten before the patch.  It's clear now: since the code to
compute where we land after a page down passes for_ui == false, that
computation is done using the lines of the old layout, so the new
starting address is the same, 0x112c.  But since the layout shown in the
UI is different, that means we completely miss instructions 0x1129 and
0x112b.

So it is now obvious that the for_ui flag is wrong, we need to do the
scrolling computation considering the new layout.

Simon