[v2,gdb/cli] Fix wrapping for TERM=ansi

Message ID 20230502133258.8380-1-tdevries@suse.de
State Superseded
Headers
Series [v2,gdb/cli] Fix wrapping for TERM=ansi |

Commit Message

Tom de Vries May 2, 2023, 1:32 p.m. UTC
  I. Auto-detected width (xterm vs. ansi)

Say we have a terminal with a width of 40 chars:
...
$ echo $COLUMNS
40
...

With TERM=xterm, we report a width of 40 chars:
...
$ TERM=xterm gdb
(gdb) show width
Number of characters gdb thinks are in a line is 40.
...

And with TERM=ansi, a width of 39 chars:
...
$ TERM=ansi gdb
(gdb) show width
Number of characters gdb thinks are in a line is 39.
...

Gdb uses readline to auto-detect screen size, and readline decides in the
TERM=ansi case that the terminal does not have reliable auto-wrap, and
consequently decides to hide the last terminal column from the readline user
(in other words GDB), hence we get 39 instead of 40.

II. Documented semantics of "set width"

Reading the documentation, the width setting is described in different ways.

There's the role of determining screen width:
...
Normally GDB knows the size of the screen from the terminal driver software.
For example, on Unix GDB uses the termcap data base together with the value of
the TERM environment variable and the stty rows and stty cols settings.  If
this is not correct, you can override it with the set height and set width
commands.
...

Also, there's something related to wrapping (using "when", AFAIU it could mean
either "whether" or "where" in this context):
...
the screen width setting determines when to wrap lines of output
...

The "set width" help text is more specific (using "where"):
...
$ gdb -ex "help set width"
Set number of characters where GDB should wrap lines of its output.
This affects where GDB wraps its output to fit the screen width.
Setting this to "unlimited" or zero prevents GDB from wrapping its output.
(gdb)
...

FWIW, in older gdb (7.5 and before) it used to be just:
...
Set number of characters gdb thinks are in a line.
...

Anyway, I'm confused.  Does set width:
- override auto-detected screen width, or does it
- determine where GDB should wrap lines of its output?

III. Types of wrapping

Looking a bit more in detail inside gdb, it seems there are two types of
wrapping:
- readline wrapping (in other words, prompt edit wrapping), and
- gdb output wrapping (can be observed by issuing "info sources").
  This type of wrapping attempts to wrap some of the gdb output earlier
  than the indicated width, to not break lines in inconvenient places.

IV. Readline wrapping, setting width to less than screen size

So, how does readline wrapping behave when setting width to less than screen
size, say 20?  Let's try that out.

First, let's note that the prompt prefix "(gdb) " is 6 chars long:
...
123456
(gdb)
...
so we'll tag on starting with 7.

With TERM=xterm, after typing the last 0, the cursor goes to the '(' at the
start of the line:
...
(gdb) set width 20
(gdb) 78901234567890
...
and after typing 1, we have:
...
(gdb) set width 20
1gdb) 78901234567890
...
Things go even worse after backspacing, which clears the entire line and then
goes into the "set width 20" line.

Alright, now with TERM=ansi:
...
(gdb) set width 20
(gdb) 7890123456789
0
...
Things do wrap, just after 19 instead of 20 chars.

So what happened here?

With TERM=xterm, readline decides that the terminal has reliable auto-wrap,
and does insert an '\r' at the point of wrapping but leaves it to the terminal
to insert the '\n'.  But because the terminal is actually 40 wide, that
doesn't happen, and so the '\r' brings the cursor to the start of the same
line.  In other words, the terminal is 40 wide, but we're telling the gdb that
it's 20 wide, which is incorrect, and we get into trouble.

With TERM=ansi, readline decides that the terminal does not have reliable
auto-wrap, and consequently decides to hide the last terminal column from the
user (hence 19 instead of 20), and inserts the '\n' itself.  Consequently, the
wrapping does work.  But, there's no need to reduce the number of columns.
The reason this is done is to avoid may-or-may-not auto-wrap in the last
column, but there's reliably no auto-wrap in column 20, because the the actual
screen width is 40.  In other words, the terminal is 40 wide, but we're telling
gdb that it's 20 wide, which is incorrect, and we get less usable width
than possible.

So, this doesn't work as expected in both cases.

The ansi case could be fixed by working around the off-by-one error.

The xterm case, not really, unless we trick readline into thinking that
auto-wrap is not supported.  We could do that by overriding _rl_term_autowrap
to 0, but as per PR build/10723 we try to get rid of dependencies on readlines
internal variables.  And even if we would use that, we'd have to avoid
overriding when setting width to the actual screen width, otherwise you'd get
'\n' both from readline and the terminal.  So, you'd need the actual screen
width, which "set width" also determines.  This just goes to show that the
idea of administering two separate concepts of "screen width" and "wrapping
location" in a single variable is not a good idea.

V. gdb output wrapping, setting width to less than screen size

So, how does the gdb output wrapping fare if we set width to less than screen
size?

Let's try in an 80 COLUMNS terminal with TERM=xterm:
...
(gdb) info sources
...
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/../sysdeps/x86_64/crtn.S,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/elf-init.c, /data/vries/hello.c,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/../sysdeps/x86_64/crti.S,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/init.c,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/../sysdeps/x86_64/start.S
...

Ok, now let's set width to 70, and try again:
...
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/../sysdeps/x86_64/crtn.S,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/elf-init.c,
/data/vries/hello.c,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/../sysdeps/x86_64/crti.S,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/init.c,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/../sysdeps/x86_64/start.S
...
We can see the effect in the placement of /data/vries/hello.c, which now is on
its own line.

We have the same at 60, but at 50, we're back at:
...
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/../sysdeps/x86_64/crtn.S,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/elf-init.c, /data/vries/hello.c,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/../sysdeps/x86_64/crti.S,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/init.c,
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/../sysdeps/x86_64/start.S
...

Apparantly GDB assumes that the screen size is 50, and that the terminal
breaks the line at 50.  We can see this by resizing the terminal to a width of
50, which gives us:
...
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/elf-ini
t.c, /data/vries/hello.c,
...
And that explains why gdb doesn't put /data/vries/hello.c on its own line.

So it seems that also for this type of wrapping, the width setting is assumed
to be set according to the screen size.

VI. Conclusion

Based on the above, I think that the notion that width can be set to determine
where GDB wraps is incorrect.

For both types of wrapping, it's essential that the width setting is set to the
width of the screen.

FWIW, we could introduce some "set wrap-width" setting, and then:
- for "readline wrapping", do the _rl_term_autowrap overriding as described
  above (and ask readline maintainer to change status to public), and
- for "gdb output wrapping" correctly detect (by comparing wrap-width with
  width) whether gdb can rely on the terminal to insert a linebreak, or
  whether gdb needs to do that itself.
But I consider this outside the scope of this patch.

VII. Readline wrapping, auto-detected screen size

Alright, so then how do we fare with the auto-detected screen widths?

[ COLUMNS == 40, TERM=xterm reporting width 40 and TERM=ansi reporting width
39. ]

Let's try with xterm:
...
$ TERM=xterm gdb
(gdb) 7890123456789012345678901234567890
123
...
That looks as expected, wrapping occurs after 40 chars.

Now, let's try with ansi:
...
$ TERM=ansi gdb
(gdb) 78901234567890123456789012345678
90123
...
It looks like wrapping occurred after 38, while readline should be capable of
wrapping after 39 chars.

This is caused by readline hiding the last column, twice.

In more detail:
- readline detects the screen width: 40,
- readline hides the last column, setting the readline screen width to 39,
- readline reports 39 to gdb as screen width,
- gdb sets its width setting to 39,
- gdb sets readline screen width to 39,
- readline hides the last column, again, setting the readline screen width to
  38.

This is reported as PR cli/30346.

VIII. gdb output wrapping, auto-detected screen size

Say we set the terminal width to 56. With TERM=xterm, we have:
...
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/elf-init.c,
/data/vries/hello.c,
...
but with TERM=ansi:
...
/home/abuild/rpmbuild/BUILD/glibc-2.31/csu/elf-init.c, /
data/vries/hello.c,
...

So what happened here?  With TERM=ansi, the width setting is auto-detected to
55, and gdb assumes the terminal inserts a line break there, which it doesn't
because the terminal width is 56.

This is reported as PR cli/30411.

IX. Fix documented semantics of "set width"

Fix the confusion about semantics of set width by updating doc and help text
to make it more clear that "set width" means "set screen width".

For the doc, we now have (using "whether" instead of "where"):
...
the screen width setting determines whether to wrap lines of output.
...

And for the help text:
...
(gdb) help set width
Set number of characters gdb thinks are in a line.
This indicates the screen width.  Auto-detected by default.
Setting this to "unlimited" or zero prevents GDB from wrapping its output.
(gdb) help show width
Show number of characters gdb thinks are in a line.
This indicates the screen width.  Auto-detected by default.
Setting this to "unlimited" or zero prevents GDB from wrapping its output.
...

X. Fix PRs

Fix both mentioned PRs by taking into account the hidden column when readline
reports the screen width in init_page_info, and updating chars_per_line
accordingly.

Note that now we report the same width for both TERM=xterm and TERM=ansi,
which is much clearer.

The point where readline respectively expects or ensures wrapping is still
indicated by "maint info screen", for xterm:
...
Number of characters readline reports are in a line is 40.
...
and ansi:
...
Number of characters readline reports are in a line is 39.
...

XI. Testing

The new test-case gdb/testsuite/gdb.base/wrap-line.exp functions as regression
test for PR cli/30346.

I didn't manage to come up with a regression test for PR cli/30411.  Perhaps
that would be easier if we had a maintenance command that echoes its arguments
while applying gdb output wrapping.

Tested on x86_64-linux.

PR cli/30346
PR cli/30411
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30346
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30411
---
 gdb/doc/gdb.texinfo                  |   2 +-
 gdb/testsuite/gdb.base/wrap-line.exp | 114 +++++++++++++++++++++++++++
 gdb/utils.c                          |   8 +-
 3 files changed, 119 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/wrap-line.exp


base-commit: 077a1f08485e88f3b234af1dbb8b907b16045e6a
  

Comments

Tom de Vries May 5, 2023, 1:27 p.m. UTC | #1
I've submitted a v3.

Changes since v2:
- testsuite part is now just removing KFAILs
- I've dropped the bit about "set width" semantics.  I still think it's
   worthwhile, but I think I should sent it as a separate patch
   (and I'm thinking about adding a warning when setting width to
   something different than 0 or the auto-detected width).

I'm fairly happy with the v3, and intend to commit end of next week, 
unless there are comments.

Thanks,
- Tom
  
Tom de Vries May 12, 2023, 9:45 a.m. UTC | #2
On 5/5/23 15:27, Tom de Vries wrote:
> I've submitted a v3.
> 
> Changes since v2:
> - testsuite part is now just removing KFAILs
> - I've dropped the bit about "set width" semantics.  I still think it's
>    worthwhile, but I think I should sent it as a separate patch
>    (and I'm thinking about adding a warning when setting width to
>    something different than 0 or the auto-detected width).
> 
> I'm fairly happy with the v3, and intend to commit end of next week, 
> unless there are comments.
> 

Committed, after cleaning up commit message a bit.

Thanks,
- Tom
  

Patch

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b21823d966d..8d89c8fb9bf 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27059,7 +27059,7 @@  information output to the screen.  To help you read all of it,
 output.  Type @key{RET} when you want to see one more page of output,
 @kbd{q} to discard the remaining output, or @kbd{c} to continue
 without paging for the rest of the current command.  Also, the screen
-width setting determines when to wrap lines of output.  Depending on
+width setting determines whether to wrap lines of output.  Depending on
 what is being printed, @value{GDBN} tries to break the line at a
 readable place, rather than simply letting it overflow onto the
 following line.
diff --git a/gdb/testsuite/gdb.base/wrap-line.exp b/gdb/testsuite/gdb.base/wrap-line.exp
new file mode 100644
index 00000000000..735b589dc45
--- /dev/null
+++ b/gdb/testsuite/gdb.base/wrap-line.exp
@@ -0,0 +1,114 @@ 
+# Copyright 2023 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/>.
+
+# Check that wrapping occurs at the expected location.
+
+# We set TERM on build, but we need to set it on host.  That only works if
+# build == host.
+require {!is_remote host}
+
+# Test both ansi (no auto-wrap) and xterm (auto-wrap).
+set terms {ansi xterm}
+
+foreach_with_prefix term $terms  {
+    save_vars { env(TERM) INTERNAL_GDBFLAGS } {
+
+	setenv TERM $term
+
+	# Let's use the width as determined by readline to get correct
+	# wrapping in the auto-wrap case.  Avoid "set width" argument.
+	set INTERNAL_GDBFLAGS \
+	    [string map {{-iex "set width 0"} ""} $INTERNAL_GDBFLAGS]
+
+	# Avoid "set width" in default_gdb_start.
+	gdb_exit
+	gdb_spawn
+    }
+
+    set test "initial prompt"
+    gdb_test_multiple "" $test {
+	-re "^$gdb_prompt $" {
+	    pass "$test"
+	}
+    }
+
+    if { ! [readline_is_used] } {
+	continue
+    }
+
+    set gdb_width 0
+    set readline_width 0
+    set re1 "Number of characters gdb thinks are in a line is ($decimal)\\."
+    set re2 \
+	"Number of characters readline reports are in a line is ($decimal)\\."
+
+    gdb_test_multiple "maint info screen" "" {
+	-re -wrap "$re1\r\n$re2\r\n.*" {
+	    set gdb_width $expect_out(1,string)
+	    set readline_width $expect_out(2,string)
+	    pass $gdb_test_name
+	}
+    }
+
+    if { $gdb_width == 0 || $readline_width == 0 } {
+	continue
+    }
+
+    if { $term == "xterm" } {
+	gdb_assert { $gdb_width == $readline_width }
+    } else {
+	gdb_assert { $gdb_width == [expr $readline_width + 1] }
+    }
+
+    # New prompt, but avoid emitting a pass in order to avoid ending the line
+    # after the prompt in gdb.log.  This make it a bit easier in gdb.log to
+    # understand where wrapping occurred.
+    gdb_test_multiple "print 1" "" {
+	-re -wrap " = 1" {
+	}
+    }
+
+    # Take into account that the prompt also takes space.
+    set prefix [string length "(gdb) "]
+    set start [expr $prefix + 1]
+
+    # Print chars without wrapping.
+    set i $start
+    while { 1 } {
+	send_gdb [expr $i % 10]
+	if { $i == $readline_width } {
+	    break
+	}
+	incr i
+    }
+
+    # Now print the first char we expect to wrap.
+    send_gdb "W"
+
+    # Generate a prompt.
+    send_gdb "\003"
+
+    # Note the difference between autowrap and no autowrap.  In the autowrap
+    # case, readline doesn't emit a '\n', the terminal takes care of that.
+    if { $term == "xterm" } {
+	# xterm, autowrap.
+	set re "^$start\[^\r\n\]* \rWQuit"
+    } else {
+	# ansi, no autowrap.
+	set re "^$start\[^\r\n\]*\r\n\rWQuit"
+    }
+
+    gdb_test "" $re "wrap"
+}
diff --git a/gdb/utils.c b/gdb/utils.c
index e10198accd0..d16158eead8 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1166,7 +1166,7 @@  init_page_info (void)
       readline_hidden_cols = _rl_term_autowrap ? 0 : 1;
 
       lines_per_page = rows;
-      chars_per_line = cols;
+      chars_per_line = cols + readline_hidden_cols;
 
       /* Readline should have fetched the termcap entry for us.
 	 Only try to use tgetnum function if rl_get_screen_size
@@ -3686,9 +3686,9 @@  void
 _initialize_utils ()
 {
   add_setshow_uinteger_cmd ("width", class_support, &chars_per_line, _("\
-Set number of characters where GDB should wrap lines of its output."), _("\
-Show number of characters where GDB should wrap lines of its output."), _("\
-This affects where GDB wraps its output to fit the screen width.\n\
+Set number of characters gdb thinks are in a line."), _("\
+Show number of characters gdb thinks are in a line."),_("\
+This indicates the screen width.  Auto-detected by default.\n\
 Setting this to \"unlimited\" or zero prevents GDB from wrapping its output."),
 			    set_width_command,
 			    show_chars_per_line,