* [PATCH] [gdb/tui] Don't show incorrect source file in source window
@ 2025-01-29 10:21 Tom de Vries
2025-01-29 11:25 ` Andrew Burgess
0 siblings, 1 reply; 4+ messages in thread
From: Tom de Vries @ 2025-01-29 10:21 UTC (permalink / raw)
To: gdb-patches
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;
+ }
+
if (this != tui_src_win ())
find_line_pc (s, cursal.line, &cursal.pc);
base-commit: d1b55b91df48f26a8541646bb88231ad5b9a0832
--
2.43.0
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [gdb/tui] Don't show incorrect source file in source window
2025-01-29 10:21 [PATCH] [gdb/tui] Don't show incorrect source file in source window Tom de Vries
@ 2025-01-29 11:25 ` Andrew Burgess
2025-01-29 12:27 ` Tom de Vries
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Burgess @ 2025-01-29 11:25 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
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.
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".
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.
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.
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
{
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [gdb/tui] Don't show incorrect source file in source window
2025-01-29 11:25 ` Andrew Burgess
@ 2025-01-29 12:27 ` Tom de Vries
2025-01-29 12:44 ` Tom de Vries
0 siblings, 1 reply; 4+ messages in thread
From: Tom de Vries @ 2025-01-29 12:27 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
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
> {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [gdb/tui] Don't show incorrect source file in source window
2025-01-29 12:27 ` Tom de Vries
@ 2025-01-29 12:44 ` Tom de Vries
0 siblings, 0 replies; 4+ messages in thread
From: Tom de Vries @ 2025-01-29 12:44 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
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
>> {
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-01-29 12:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-01-29 10:21 [PATCH] [gdb/tui] Don't show incorrect source file in source window Tom de Vries
2025-01-29 11:25 ` Andrew Burgess
2025-01-29 12:27 ` Tom de Vries
2025-01-29 12:44 ` Tom de Vries
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox