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

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

On 1/29/25 13:27, Tom de Vries wrote:
> 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.
> 

FWIW, I investigated this as well, and tried to make a similar minimal 
fix, and ended up with:
...
diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
index a5d0c594545..07aca28eb53 100644
--- a/gdb/tui/tui-winsource.c
+++ b/gdb/tui/tui-winsource.c
@@ -459,9 +459,11 @@ tui_source_window_base::rerender ()
        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);
+	{
+	  update_source_window_with_addr (gdbarch, get_frame_pc (frame));
+	  return;
+	}

        /* This centering code is copied from 
tui_source_window::maybe_update.
...

Thanks,
- Tom

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