Re: [PATCH] [gdb/tui] Don't show incorrect source file in source window

[Tom de Vries <tdevries@suse.de>]

On 1/29/25 12:25, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> Consider the test-case sources main.c and foo.c:
>> ...
>> $ cat main.c
>> extern int foo (void);
>>
>> int
>> main (void)
>> {
>>    return foo ();
>> }
>> $ cat foo.c
>> extern int foo (void);
>>
>> int
>> foo (void)
>> {
>>    return 0;
>> }
>> ...
>> and main.c compiled with debug info, and foo.c without:
>> ...
>> $ gcc -g main.c -c
>> $ gcc foo.c -c
>> $ gcc -g main.o foo.o
>> ...
>>
>> In TUI mode, if we run to foo:
>> ...
>> $ gdb -q a.out -tui -ex "b foo" -ex run
>> ...
>> it gets us "[ No Source Available ]":
>> ...
>> ┌─main.c─────────────────────────────────────────┐
>> │                                                │
>> │                                                │
>> │                                                │
>> │            [ No Source Available ]             │
>> │                                                │
>> │                                                │
>> └────────────────────────────────────────────────┘
>> (src) In: foo                  L??   PC: 0x400566
>> db.so.1".
>>
>> Breakpoint 1, 0x0000000000400566 in foo ()
>> (gdb)
>> ...
>>
>> But after resizing (pressing ctrl-<minus> in the gnome-terminal), we get
>> instead the source for main.c:
>> ...
>> ┌─main.c───────────────────────────────────────────────────┐
>> │        3 int                                             │
>> │        4 main (void)                                     │
>> │        5 {                                               │
>> │        6   return foo ();                                │
>> │        7 }                                               │
>> │                                                          │
>> │                                                          │
>> └──────────────────────────────────────────────────────────┘
>> (src) In: foo                            L??   PC: 0x400566
>> db.so.1".
>>
>> Breakpoint 1, 0x0000000000400566 in foo ()
>> (gdb)
>> ...
>> which is inappropriate because we're stopped in function foo, which is not in
>> main.c.
>>
>> Fix this in tui_source_window_base::rerender.
>>
>> Tested on x86_64-linux.
>>
>> Reported-By: Andrew Burgess <aburgess@redhat.com>
>>
>> PR tui/32614
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32614
>> ---
>>   gdb/testsuite/gdb.tui/resize-3-foo.c  | 24 ++++++++++
>>   gdb/testsuite/gdb.tui/resize-3-main.c | 24 ++++++++++
>>   gdb/testsuite/gdb.tui/resize-3.exp    | 63 +++++++++++++++++++++++++++
>>   gdb/tui/tui-winsource.c               |  6 +++
>>   4 files changed, 117 insertions(+)
>>   create mode 100644 gdb/testsuite/gdb.tui/resize-3-foo.c
>>   create mode 100644 gdb/testsuite/gdb.tui/resize-3-main.c
>>   create mode 100644 gdb/testsuite/gdb.tui/resize-3.exp
>>
>> diff --git a/gdb/testsuite/gdb.tui/resize-3-foo.c b/gdb/testsuite/gdb.tui/resize-3-foo.c
>> new file mode 100644
>> index 00000000000..653b24ba92b
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.tui/resize-3-foo.c
>> @@ -0,0 +1,24 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2025 Free Software Foundation, Inc.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +extern int foo (void);
>> +
>> +int
>> +foo (void)
>> +{
>> +  return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.tui/resize-3-main.c b/gdb/testsuite/gdb.tui/resize-3-main.c
>> new file mode 100644
>> index 00000000000..95fdd3b6ba0
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.tui/resize-3-main.c
>> @@ -0,0 +1,24 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2025 Free Software Foundation, Inc.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +extern int foo (void);
>> +
>> +int
>> +main (void)
>> +{
>> +  return foo ();
>> +}
>> diff --git a/gdb/testsuite/gdb.tui/resize-3.exp b/gdb/testsuite/gdb.tui/resize-3.exp
>> new file mode 100644
>> index 00000000000..ec022b4a177
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.tui/resize-3.exp
>> @@ -0,0 +1,63 @@
>> +# Copyright 2025 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# Test TUI resizing while showing "No Source Available".
>> +
>> +require allow_tui_tests
>> +
>> +standard_testfile -main.c -foo.c
>> +
>> +if { [build_executable_from_specs "failed to prepare" \
>> +	  $testfile {debug} \
>> +	  $srcfile {debug} \
>> +	  $srcfile2 {nodebug}] == -1 } {
>> +    return -1
>> +}
>> +
>> +tuiterm_env
>> +
>> +Term::clean_restart 24 80 $testfile
>> +
>> +# It would be simpler to run directly to foo and then enter TUI, but that
>> +# fails to trigger PR32614.  So instead, we first run to main, enter TUI and
>> +# then run to foo.
>> +if {![runto_main]} {
>> +    perror "test suppressed"
>> +    return
>> +}
>> +
>> +# Set a breakpoint on foo, easier to do before entering TUI.
>> +gdb_breakpoint foo
>> +
>> +if {![Term::enter_tui]} {
>> +    unsupported "TUI not supported"
>> +    return
>> +}
>> +
>> +# Continue to foo.
>> +Term::command continue
>> +
>> +with_test_prefix "before resize" {
>> +    Term::check_contents "Source window empty" \
>> +	"No Source Available"
>> +}
>> +
>> +Term::resize 40 90
>> +
>> +with_test_prefix "after resize" {
>> +    # Regression test for PR32614.
>> +    Term::check_contents "Source window empty" \
>> +	"No Source Available"
>> +}
>> diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
>> index a5d0c594545..bf4052755df 100644
>> --- a/gdb/tui/tui-winsource.c
>> +++ b/gdb/tui/tui-winsource.c
>> @@ -460,6 +460,12 @@ tui_source_window_base::rerender ()
>>         struct gdbarch *gdbarch = get_frame_arch (frame);
>>   
>>         struct symtab *s = find_pc_line_symtab (get_frame_pc (frame));
>> +      if (s == nullptr)
>> +	{
>> +	  erase_source_content ();
>> +	  return;
>> +	}
> 
> This didn't look quite right to me.  But while trying to find the set of
> steps to show why I don't think this change is exactly right ... I ran
> into another bug, which means that this isn't wrong right now ... but
> might be once the other bug is fixed ... maybe ....
> 
> So, what worried me, is that this ::rerender is called for both the SRC
> window and the ASM window.  My thinking was that if this change is
> fixing things so we call erase_source_content() when we cannot find the
> source file, then we're also going to call erase_source_content() for
> the ASM window when we cannot find the source file, and that cannot be
> right, we should always be able to update the ASM window with the
> correct disassembler output.
> 

Hi Andrew,

thanks for the review, and bringing up this point.

I've submitted a v2 that limits the impact of the fix to the source 
window ( 
https://sourceware.org/pipermail/gdb-patches/2025-January/215043.html ).

> So I tried the following with an UNPATCHED gdb:
> 
>    1. layout src
>    2. step into a region where "No Source Available" is shown.
>    3. layout asm
> 
> My expectation is that I should immediately see the disassembler output
> for the current $pc location.  What I actually see is "No Assembly
> Available".
> 

I can reproduce this.

> Turns out we hit the erase_source_content() call in
> tui_source_window_base::update_source_window_as_is because the sal.pc
> within that function is 0, and so the set_contents call returns false.
> 
> If I then `si` the ASM window immediately updates with the disassembler
> output.  This time the update originates from tui_show_frame_info, which
> gets the symtab_and_line via a different approach than the ::rerender
> function does.
> 
> So I wondered, would a better solution be to rewrite at least part of
> ::rerender based on tui_show_frame_info?
> 
> Below is a _rough_ patch which does just this.  There's no commit
> message or anything yet.  It passes all the gdb.tui/ tests, including
> your new test that you added with this patch.
> 
> This also seems to resolve the src -> asm switching issue I detail
> above.
> 
> I'd love to hear what you think about this approach.
> 

I like it because it make the resulting code much shorter, and 
straight-line code.

Superficially it doesn't look wrong to me, but unfortunately, my 
understanding of the TUI code is not sufficient to approve or even 
review it.

FWIW, it would be good to known whether all the paths in the old code 
are covered by the test-suite.

> If you think this is the way to go then I'd propose that I write this
> up, steal your new test, and add another test to expose the src to asm
> switching issue.
> 

We could do that, or go with the v2 I proposed (if that is indeed 
acceptable) and work from there.  I have a very slight preference for 
the latter.

Thanks,
- Tom

> But I'm worried that I might be missing something.
> 
> Thanks,
> Andrew
> 
> ---
> 
> diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
> index a5d0c594545..b9c26e88a3a 100644
> --- a/gdb/tui/tui-winsource.c
> +++ b/gdb/tui/tui-winsource.c
> @@ -454,25 +454,10 @@ tui_source_window_base::rerender ()
>       }
>     else if (deprecated_safe_get_selected_frame () != NULL)
>       {
> -      symtab_and_line cursal
> -	= get_current_source_symtab_and_line (current_program_space);
>         frame_info_ptr frame = deprecated_safe_get_selected_frame ();
> -      struct gdbarch *gdbarch = get_frame_arch (frame);
> -
> -      struct symtab *s = find_pc_line_symtab (get_frame_pc (frame));
> -      if (this != tui_src_win ())
> -	find_line_pc (s, cursal.line, &cursal.pc);
> -
> -      /* This centering code is copied from tui_source_window::maybe_update.
> -	 It would be nice to do centering more often, and do it in just one
> -	 location.  But since this is a regression fix, handle this
> -	 conservatively for now.  */
> -      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);
> +      symtab_and_line cursal = find_frame_sal (frame);
> +
> +      maybe_update (frame, cursal);
>       }
>     else
>       {