[pushed] Fix PR tui/21216: TUI line breaks regression

  On 03/10/2017 12:59 PM, Jan Kratochvil wrote:
> On Fri, 10 Mar 2017 00:03:59 +0100, Jan Kratochvil wrote:
>> On Wed, 08 Mar 2017 01:19:12 +0100, Pedro Alves wrote:
>>> +# Make sure filtering/pagination is enabled, but make the window high
>>> +# enough that we don't paginate in practice.
>>> +gdb_test_no_output "set pagination on"
>>> +gdb_test_no_output "set height 2000"
>>
>> I get:
>> 	FAIL: gdb.tui/tui-nl-filtered-output.exp: set pagination on (timeout)
>> 	FAIL: gdb.tui/tui-nl-filtered-output.exp: set height 2000 (timeout)
>> But I have no idea why.
> 
> FAIL reproducibility requires: --with-system-readline
> F-25: not reproducible
> 	readline-6.3-8.fc24.x86_64
> 	ncurses-libs-6.0-6.20160709.fc25.x86_64
> F-26: not tested
> Rawhide: reproducible
> 	readline-7.0-5.fc26.x86_64
> 	ncurses-libs-6.0-8.20170212.fc26.x86_64

Curious, I've been poking at this, and had tested with both
F23 and F25, with and without --with-system-readline without
success.  Hadn't tested F24.

From your log, up until the test does "set height 2000", looks like
curses is issuing "cursor backward" (the 17D and 15D) commands:

 (gdb) set pagination onFAIL: gdb.tui/tui-nl-filtered-output.exp: set pagination on (timeout)
 (gdb) set height 2000FAIL: gdb.tui/tui-nl-filtered-output.exp: set height 2000 (timeout)

I don't get those, and I guess you don't either when testing outside
hammoc.

Looks like the terminal environment under hammock is different
somehow.  Could it be the TERM env variable that influences this? 

By adding 

 gdb_test "show environment" ".*"

just before tui enable, I see that for me, TERM is always
set to vt100, no matter what TERM is set to in the shell
outside dejagnu.

/me pokes some more.

Hmm, the screen height makes a difference.  That could be it.  If I
add a few more commands, then the TUI starts issuing escape sequences
once the command line reaches the bottom.  And if I run make check
in a smaller terminal window, the pristine test starts failing
for me too, due to unexpected escape sequences.

See test patch below, and attached resulting gdb.log.  Note that viewing
the log with "less" on the terminal directly "hides" some of the escape
sequences (they get interpreted directly).

From 8973caf1689f87632e479fa3d8101a1eab827d24 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 10 Mar 2017 12:37:27 +0000
Subject: [PATCH] env

---
 gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp | 7 +++++++
 1 file changed, 7 insertions(+)
  

On Fri, 10 Mar 2017 14:33:39 +0100, Pedro Alves wrote:
> See test patch below, and attached resulting gdb.log.  Note that viewing
> the log with "less" on the terminal directly "hides" some of the escape
> sequences (they get interpreted directly).

 Running gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp ...
+PASS: gdb.tui/tui-nl-filtered-output.exp: show environment
 PASS: gdb.tui/tui-nl-filtered-output.exp: tui enable
+PASS: gdb.tui/tui-nl-filtered-output.exp: show height
 PASS: gdb.tui/tui-nl-filtered-output.exp: set pagination on
 PASS: gdb.tui/tui-nl-filtered-output.exp: set height 2000
+PASS: gdb.tui/tui-nl-filtered-output.exp: set height 2000
+PASS: gdb.tui/tui-nl-filtered-output.exp: set height 2000
+PASS: gdb.tui/tui-nl-filtered-output.exp: set height 2000
+FAIL: gdb.tui/tui-nl-filtered-output.exp: set height 2000 (timeout)
 PASS: gdb.tui/tui-nl-filtered-output.exp: correct line breaks


Thanks,
Jan
  

Fixing this properly is turning out to be much trickier than
I expected.

The main issue is that the TUI/ncurses updates/refreshes to
the command window are done by drawing only what changed,
using cursor movement.

For example, with a small enough screen, we see:

 (gdb) PASS: gdb.tui/tui-nl-filtered-output.exp: set pagination on
 set height 2000

 (gdb) PASS: gdb.tui/tui-nl-filtered-output.exp: set height 2000
 printf "hello\nworld\n"printf "hello\nworld\n"

 hello                

 world

 (gdb) FAIL: gdb.tui/tui-nl-filtered-output.exp: correct line breaks

"2A" is move up two lines, "23D" is move left 23 lines, then
print "printf "hello\nworld\n"", then "hello\nworld"
then "K" to clear the line, and finally print the prompt.

I tried to work around it by forcing empty commands in between
commands, to force the screen to clear, but the "smart" refresh makes
the output of that even harder to match.

What you were seeing:

 (gdb) set pagination on


 (gdb) set height 2000


... that's moving the cursor to the start of the line, and then
clearing it, making room for the next command.  I.e., both commands
appeared on the same vertical coordinate on the screen, presumably
because the command window only fitted one line or two.

So in general, we can't just strip escape sequences.  

Maybe we could pull it off by implementing a virtual terminal
that is aware of the escape sequences, understands the cursor
movement, line clearing, etc., something like the
un_ansi_vt procedure here:

 [handling of ANSI terminals using Expect]
 http://wiki.tcl.tk/9673

We'd send the test command to gdb, and feed gdb's output to that
procedure in a loop, which builds an array of lines, and then check
if the rendered command "screen" (an array of lines) had the
command result we wanted.

We'd need to match the virtual terminal's "bounding" with
the command window's size.  Ideally, we'd be able to force
the exact size of command window we want, instead of inheriting
that from the terminal dejagnu is running on.

Or maybe do something different.  Maybe dump the screen using scr_dump
and compare to a dump previously saved.  I'd need to investigate
further.

So while I think it'd be neat to do any of this, it's a lot more work
and investigation than this new test is worth it.  So for now, I'm
going to simply remove it.

Thanks,
Pedro Alves
  

On Fri, 10 Mar 2017 18:20:18 +0100, Pedro Alves wrote:
> Or maybe do something different.

Yes, I do know all these problems.  This is why I was going to separate TUI
output to a different project (with its own non-TCL testsuite) along with
separating MI, and replacing it all by some sane RPC variant later.
	https://git.jankratochvil.net/?p=gdbmicli.git;a=summary

But you ditched that and you were right, there are other debuggers which
already do it the right way.


Jan
  

On 03/10/2017 05:27 PM, Jan Kratochvil wrote:
> On Fri, 10 Mar 2017 18:20:18 +0100, Pedro Alves wrote:
>> Or maybe do something different.
> 
> Yes, I do know all these problems.  This is why I was going to separate TUI
> output to a different project (with its own non-TCL testsuite) along with
> separating MI, and replacing it all by some sane RPC variant later.
> 	https://git.jankratochvil.net/?p=gdbmicli.git;a=summary
> 

How you love pulling things out of context.  

Bringing up the separation-into-processes issues again suggests you're
thinking of testing at the core <-> TUI interface level.
We could have that too, by writing a unit test.  I thought of writing
one that exercised the tui_file class.  I may still write one.
Thing is, this case calls for a black-box test, to make sure that
final terminal output is how we intended.  That's why I spent the effort
to write the test, and more effort to try to preserve it.  How TUI
communicates with the core of the debugger is completely irrelevant
here.  Whatever testing you think would be appropriate for testing
TUI/ncurses _output_ in your project, we can consider for GDB too.  If
you have ideas for that, please share them.

> But you ditched that 

I didn't "ditch it".  I explained to you, with detail, why in my
opinion that wouldn't be a design we'd want.

> and you were right, there are other debuggers which
> already do it the right way.

I don't think I ever said such a thing.

Thanks,
Pedro Alves
  

On Fri, 10 Mar 2017 19:17:20 +0100, Pedro Alves wrote:
> How you love pulling things out of context.  

Because everything is interconnected.


> Whatever testing you think would be appropriate for testing TUI/ncurses
> _output_ in your project, we can consider for GDB too.  If you have ideas
> for that, please share them.

I do not think a front end UI really needs any testsuite as it is a trivial
code (in a proper language and with proper API - not MI; MI is not API and not
proper).  That is not the case of a front end intertwingled into GDB core.

Besides that maybe one could fix a TUI frontend for the testsuite purposes.
I have no idea how much usable ncurses is with "dumb":
$ TERM=dumb gdb -tui
Cannot enable the TUI: terminal doesn't support cursor addressing [TERM=dumb]


> > and you were right, there are other debuggers which
> > already do it the right way.
> 
> I don't think I ever said such a thing.

You said something different although IMO with the same practical result.
This text from an internal list but I do not find that too secret:

On Fri, 26 Jun 2015 18:44:11 +0200, Pedro Alves wrote:
# Basically you're saying we should rewrite the whole CLI from scratch.
# And it basically seems like writing a _different_ debugger from
# scratch to me.


Jan
  

On 03/10/2017 06:46 PM, Jan Kratochvil wrote:

>>> and you were right, there are other debuggers which
>>> already do it the right way.
>>
>> I don't think I ever said such a thing.
> 
> You said something different although IMO with the same practical result.

That's an incorrect interpretation.

> This text from an internal list but I do not find that too secret:
> 
> On Fri, 26 Jun 2015 18:44:11 +0200, Pedro Alves wrote:
> # Basically you're saying we should rewrite the whole CLI from scratch.
> # And it basically seems like writing a _different_ debugger from
> # scratch to me.

That I did say, but that "different" meant that it'd be a different 
debugger in the user's perspective and in the use cases that it
could support.  E.g., how with separate components in separate processes
you can't have an single integrated Python API.

Thanks,
Pedro Alves