If I run to main with "start", and then enable the TUI, the current line (stopped at main) isn't highlighted, nor is the current line centered on the source window. If I do a step, then the highlight appears. If I enable the TUI _before_ running to main, then the current line once GDB stops at main, is highlighted. This seems like a recent breakage. At least, I hadn't noticed it before. Observed on commit 9bec569fda7c, from Mar 21 2024.
Not sure what broke this, but changing tui_enable to pass the selected frame to tui_show_frame_info fixes it. This seems reasonable to me, but maybe it is a change if you exit the TUI and then re-enter it in some scenario. Personally I'm not too concerned about this.
I found out while testing that there is a similar bug if you "tui enable" and then open a core file. You can see it in the screen dump from gdb.tui/corefile-run.exp
> Not sure what broke this, but changing tui_enable to pass > the selected frame to tui_show_frame_info fixes it. Thanks, I gave that a try, but it still doesn't behave like before. It shows the current line at the top of the source window, instead of in the middle. That's a bit of an usability regression, as it doesn't even show the line with the function name. > This seems reasonable to me, but maybe it is a change if > you exit the TUI and then re-enter it in some scenario. > Personally I'm not too concerned about this. I do that frequently with c-x,a. I'll be debugging gdb, and stepping/nexting with the TUI enabled, then decide I'm at the right spot the program that I want to inspect variables or run a command that I know is going to output many lines, so I'll switch out of the TUI, issue whatever commands, and then re-enable the TUI to continue stepping, etc. I don't know if passing the frame causes the TUI to lose its selected context/line when it was active or not. Seems like not from a quick experimentation. BTW, this is what I tried following your instructions: --- c/gdb/tui/tui.c +++ w/gdb/tui/tui.c @@ -467,7 +467,7 @@ tui_enable (void) tui_set_term_width_to (COLS); def_prog_mode (); - tui_show_frame_info (0); + tui_show_frame_info (get_selected_frame (nullptr)); (Of course, this misbehaves if you enable the TUI before starting a process.)
I used diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c index dea6ffbf954..638f91f5612 100644 --- a/gdb/tui/tui.c +++ b/gdb/tui/tui.c @@ -467,7 +467,7 @@ tui_enable (void) tui_set_term_width_to (COLS); def_prog_mode (); - tui_show_frame_info (0); + tui_show_frame_info (deprecated_safe_get_selected_frame ()); tui_set_initial_layout (); tui_set_win_focus_to (TUI_SRC_WIN); keypad (TUI_CMD_WIN->handle.get (), TRUE); Maybe we need to bisect this one now.
(In reply to Tom Tromey from comment #4) > Maybe we need to bisect this one now. Bisects to: ... commit ee1e9bbb5139d766d70cfa036979cec73a1223b7 Author: Tom de Vries <tdevries@suse.de> Date: Fri Dec 8 17:36:35 2023 +0100 [gdb/tui] Fix displaying main after resizing ...
Thank you for bisecting that. I was hoping the commit would be a bit more enlightening than it turned out to be, like I don't immediately see the connection.
I investigated a bit, by setting a breakpoint on tui_source_window::set_contents, and observing what sal.line is used. With commit ee1e9bbb513^, we have (for the "tui enable" command): - sal.line == 6 (set by tui_source_window_base::rerender) - sal.line == 0 (set by tui_update_source_windows_with_addr) - sal.line == 2 (set by tui_source_window::maybe_update) With commit ee1e9bbb513 (as well as with the fix of comment 4), we have: - sal.line == 6 (set by tui_source_window_base::rerender) The desired behaviour of centering on a source line is only present in tui_source_window::maybe_update. Duplicating that functionality in tui_source_window_base::rerender: ... diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c index 52c0b5b69a4..c140e9053be 100644 --- a/gdb/tui/tui-winsource.c +++ b/gdb/tui/tui-winsource.c @@ -482,6 +482,10 @@ tui_source_window_base::rerender () struct symtab *s = find_pc_line_symtab (get_frame_pc (frame)); if (this != TUI_SRC_WIN) find_line_pc (s, cursal.line, &cursal.pc); + int start_line = (cursal.line - ((height - box_size ()) / 2)) + 1; + if (start_line <= 0) + start_line = 1; + cursal.line = start_line; update_source_window (gdbarch, cursal); } else ... gives us the desired centering. It passes the TUI tests.
Thanks! Indeed, I played a bit with that patch, and the centering seems to work just like it did before, in all the use cases I threw at it. Tromey's change is also needed for the highlight. The old code did: - if (deprecated_safe_get_selected_frame ()) - tui_show_frame_info (deprecated_safe_get_selected_frame ()); - else - tui_display_main (); and I wondered whether we need that tui_display_main call, but I couldn't spot a behavior difference without it. I tried enabling the TUI before starting the program, with and without using "list foo" to move the current source position elsewhere. IMO, it would be nice if we centered in more cases, like after "frame", for example, or before running the program and you enable the TUI (TUI shows main), but those are not regressions AFAICT.
Created attachment 15440 [details] tentative patch including test-case
https://sourceware.org/pipermail/gdb-patches/2024-March/207641.html
Oops, didn't mean to close it.
(In reply to Pedro Alves from comment #8) > Thanks! Indeed, I played a bit with that patch, and the centering seems to > work just like it did before, in all the use cases I threw at it. Tromey's > change is also needed for the highlight. > Thanks for trying it out (maybe consider adding a tested-by tag to the submitted patch?). > The old code did: > > - if (deprecated_safe_get_selected_frame ()) > - tui_show_frame_info (deprecated_safe_get_selected_frame ()); > - else > - tui_display_main (); > > and I wondered whether we need that tui_display_main call, but I couldn't > spot a behavior difference without it. I tried enabling the TUI before > starting the program, with and without using "list foo" to move the current > source position elsewhere. > Ok, understood. > IMO, it would be nice if we centered in more cases, like after "frame", for > example, or before running the program and you enable the TUI (TUI shows > main), but those are not regressions AFAICT. Agreed. I played a bit with 'gdb -q a.out -tui' vs 'gdb -q -a.out -ex "tui enable"', where there's also a difference in behaviour. Anyway, since this is a regression fix, I went with this minimal fix, and added a comment at the fix that we could try to improve the fix.
Filed PR tui/31570 for those.
(In reply to Pedro Alves from comment #3) > > This seems reasonable to me, but maybe it is a change if > > you exit the TUI and then re-enter it in some scenario. > > Personally I'm not too concerned about this. > > I do that frequently with c-x,a. I'll be debugging gdb, and > stepping/nexting with the TUI enabled, then decide I'm at the right spot the > program that I want to inspect variables or run a command [...] What I meant here is that I was wondering if there are cases where "tui disable" + "tui enable" would preserve the state of the source window, even if there were changes (like stepping, whatever) in between. I tend to think that it is fine for "tui enable" to just recenter the selected frame's line -- but is that ok with you?
The master branch has been updated by Tom de Vries <vries@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b35013e29f3bcf9028aa22291f378010420322fe commit b35013e29f3bcf9028aa22291f378010420322fe Author: Tom de Vries <tdevries@suse.de> Date: Tue Apr 2 16:09:10 2024 +0200 [gdb/tui] Fix centering and highlighting of current line After starting TUI like this with a hello world a.out: ... $ gdb -q a.out -ex start -ex "tui enable" ... we get: ... ââhello.câââââââââââââââââââââââââââââââ â 5 { â â 6 printf ("hello\n"); â â 7 â â 8 return 0; â â 9 } â â â ââââââââââââââââââââââââââââââââââââââââ ... This is a regression since commit ee1e9bbb513 ("[gdb/tui] Fix displaying main after resizing"), before which we had instead: ... ââhello.câââââââââââââââââââââââââââââââ â 4 main (void) â â 5 { â â > 6 [7m printf ("hello\n");[0m â â 7 â â 8 return 0; â â 9 } â ââââââââââââââââââââââââââââââââââââââââ ... In other words, the problems are: - the active line (source line 6) is no longer highlighted, and - the active line is not vertically centered (screen line 2 out 6 instead of screen line 3 out of 6). Fix these problems respectively by: - in tui_enable, instead of "tui_show_frame_info (0)" using 'tui_show_frame_info (deprecated_safe_get_selected_frame ())", and - in tui_source_window_base::rerender, adding centering functionality. Tested on aarch64-linux. Co-Authored-By: Tom Tromey <tom@tromey.com> Approved-By: Tom Tromey <tom@tromey.com> PR tui/31522 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31522
Fixed.