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

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

* Simon Marchi <simon.marchi@polymtl.ca> [2022-01-29 21:17:43 -0500]:

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

Thanks for looking into this.  I hadn't considered the page up/down
case when I was looking for issues.

I guess you'll post another version without the for_ui flag?

Vasili, if you can find reproducers for the scrolling problems you are
seeing then we should probably look into those separately.

Thanks,
Andrew