Message ID | f263c39c-813f-35bb-824f-69a9de62068b@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 85852 invoked by alias); 10 Mar 2017 13:33:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 84036 invoked by uid 89); 10 Mar 2017 13:33:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.5 required=5.0 tests=BAYES_05, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=influences, cursor, UD:20170212.fc26.x86_64, sk:ncurses X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 10 Mar 2017 13:33:41 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AF274C0528CD for <gdb-patches@sourceware.org>; Fri, 10 Mar 2017 13:33:41 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v2ADXerb005560; Fri, 10 Mar 2017 08:33:40 -0500 Subject: Re: [pushed] Fix PR tui/21216: TUI line breaks regression To: Jan Kratochvil <jkratoch@redhat.com> References: <1488932352-10885-3-git-send-email-palves@redhat.com> <20170309230359.GA503@host1.jankratochvil.net> <20170310125946.GA508@host1.jankratochvil.net> Cc: gdb-patches@sourceware.org From: Pedro Alves <palves@redhat.com> Message-ID: <f263c39c-813f-35bb-824f-69a9de62068b@redhat.com> Date: Fri, 10 Mar 2017 13:33:39 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170310125946.GA508@host1.jankratochvil.net> Content-Type: multipart/mixed; boundary="------------218D37B818F6EB628434423C" |
Commit Message
Pedro Alves
March 10, 2017, 1:33 p.m. UTC
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 on[17D[KFAIL: gdb.tui/tui-nl-filtered-output.exp: set pagination on (timeout) (gdb) set height 2000[15D[KFAIL: 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(+)
Comments
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"[2A[23Dprintf "hello\nworld\n" hello world[K (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[17D[K (gdb) set height 2000[15D[K ... 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
diff --git a/gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp b/gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp index d1f56f2..fd33e70 100644 --- a/gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp +++ b/gdb/testsuite/gdb.tui/tui-nl-filtered-output.exp @@ -37,6 +37,8 @@ if {[skip_tui_tests]} { return } +gdb_test "show environment" ".*" + # Enable the TUI. set test "tui enable" @@ -48,8 +50,13 @@ gdb_test_multiple "tui enable" $test { # Make sure filtering/pagination is enabled, but make the window high # enough that we don't paginate in practice. +gdb_test "show height" ".*" gdb_test_no_output "set pagination on" gdb_test_no_output "set height 2000" +gdb_test_no_output "set height 2000" +gdb_test_no_output "set height 2000" +gdb_test_no_output "set height 2000" +gdb_test_no_output "set height 2000" gdb_test \ {printf "hello\nworld\n"} \