Message ID | 20220922160215.742183-1-simon.marchi@polymtl.ca |
---|---|
State | Committed |
Commit | 618ba27878a2c6f155eb5e1235c0484a55786a15 |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 554263857362 for <patchwork@sourceware.org>; Thu, 22 Sep 2022 16:02:55 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 554263857362 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1663862575; bh=S+Ox+k9NOPaHGFOcTPhL7h9b8rmrDjwXUYYRPUevhCU=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=AP0A5OQV8OHKyainGMcH2UbluBNRD8NxMd42Fxduko1U2WrgRwTsR+Ue8cVOC9k/i LpAOAbYOE53Xoy/YiC4aQX/zZgBaPIZG8lzDBwakXp/eRxgmLuwT3Pa0VlDiJ8ub6g AH0CtRc8OqS6GdSrbfbII9Onat/HJc0pKXcmR5IY= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 68C7B3857BB2 for <gdb-patches@sourceware.org>; Thu, 22 Sep 2022 16:02:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 68C7B3857BB2 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 28MG2G6M003882 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 22 Sep 2022 12:02:21 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 28MG2G6M003882 Received: from simark.localdomain (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 8FB9B1E0D3; Thu, 22 Sep 2022 12:02:16 -0400 (EDT) To: gdb-patches@sourceware.org Subject: [PATCH] gdb/testsuite/tui: start GDB with "set filename-display basename" Date: Thu, 22 Sep 2022 12:02:15 -0400 Message-Id: <20220922160215.742183-1-simon.marchi@polymtl.ca> X-Mailer: git-send-email 2.37.3 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 22 Sep 2022 16:02:16 +0000 X-Spam-Status: No, score=-3189.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_ASCII_DIVIDERS, KAM_SHORT, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Simon Marchi <simon.marchi@polymtl.ca> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
gdb/testsuite/tui: start GDB with "set filename-display basename"
|
|
Commit Message
Simon Marchi
Sept. 22, 2022, 4:02 p.m. UTC
The test gdb.tui/tui-missing-src.exp fails on my CI machine, and I concluded that it is caused by the long source directory name: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb The long name causes some particular redrawing that doesn't happen for shorter directories, and causes a Term::command call to return too early. This can be reproduced by cloning the binutils-gdb repo in a directory with a name similar to the one shown above. $ pwd /home/simark/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/build/gdb $ make check-read1 TESTS="gdb.tui/tui-missing-src.exp" FAIL: gdb.tui/tui-missing-src.exp: checking if inside f2 () FAIL: gdb.tui/tui-missing-src.exp: f2.c must be displayed in source window FAIL: gdb.tui/tui-missing-src.exp: check source box is empty after return FAIL: gdb.tui/tui-missing-src.exp: Back in main Note that using "make check" instead of "make check-read1" only shows the last 2 failures for me. When running gdb.tui/tui-missing-src.exp in a directory with a shorter name, the terminal looks like this by the time the "checking if inside f2" test runs: Screen Dump (size 80 columns x 24 rows, cursor at column 6, row 23): 0 +-...ld/binutils-gdb-noasan/gdb/testsuite/outputs/gdb.tui/tui-missing-src/f2.c-+ 1 | 1 | 2 | 2 int | 3 | 3 f2 (int x) | 4 | 4 { | 5 | > 5 x <<= 1; | 6 | 6 return x+5; | 7 | 7 } | 8 | 8 | 9 | 9 | 10 | 10 | 11 | 11 | 12 | 12 | 13 | 13 | 14 +------------------------------------------------------------------------------+ 15 multi-thre Thread 0x7ffff7cc07 In: f2 L5 PC: 0x555555555143 16 at /home/simark/build/binutils-gdb-noasan/gdb/testsuite/outputs/gdb.tui/tui- 17 missing-src/main.c:6 18 (gdb) next 19 (gdb) step 20 f2 (x=4) 21 at /home/simark/build/binutils-gdb-noasan/gdb/testsuite/outputs/gdb.tui/tui- 22 missing-src/f2.c:5 23 (gdb) PASS: gdb.tui/tui-missing-src.exp: checking if inside f2 () When running the `Term::command "step"` just before, GDB writes the "step", which makes the `wait_for` proc go in the "looking for the prompt" mode, to know when the command's execution is complete. As some new output appears, lines that must disappear are deleted using the "Delete Line" operation [1] and some new ones are drawn. The source window gets redrawn with the contents of the f2.c file. Then, GDB writes the prompt (at line 23 above), which satisfies `wait_for`, which then returns. The state of the terminal is therefore correct for the "check if inside f2" and "f2.c must be displayed in the source window" tests. In the non-working case, the terminal looks like this by the time the "check if inside f2" test runs: Screen Dump (size 80 columns x 24 rows, cursor at column 6, row 17): 0 +------------------------------------------------------------------------------+ 1 | | 2 | | 3 | | 4 | | 5 | | 6 | | 7 | [ No Source Available ] | 8 | | 9 | | 10 | | 11 | | 12 | | 13 | | 14 +------------------------------------------------------------------------------+ 15 multi-thre Thread 0x7ffff7cc1b In: main L7 PC: 0x555555555128 16 sing-src/main.c:6 17 (gdb) ary breakpoint 1, main () 18 at /home/simark/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd6 19 4/target_board/unix/src/binutils-gdb/build/gdb/testsuite/outputs/gdb.tui/tui-mis 20 sing-src/main.c:6 21 (gdb) next 22 (gdb) step 23 FAIL: gdb.tui/tui-missing-src.exp: checking if inside f2 () What happened is: GDB wrote the "step" command, which make the `wait_for` proc go in its "looking for the prompt" mode. However, curses decided to redraw whatever scrolled up to line 17 using some standard character insertion operations: +++ Cursor Down (1), cursor: (16, 0) -> (17, 0) +++ Inserting string '(' +++ Inserted char '(', cursor: (17, 0) -> (17, 1) +++ Inserted string '(', cursor: (17, 0) -> (17, 1) +++ Inserting string 'g' +++ Inserted char 'g', cursor: (17, 1) -> (17, 2) +++ Inserted string 'g', cursor: (17, 1) -> (17, 2) +++ Inserting string 'd' +++ Inserted char 'd', cursor: (17, 2) -> (17, 3) +++ Inserted string 'd', cursor: (17, 2) -> (17, 3) +++ Inserting string 'b' +++ Inserted char 'b', cursor: (17, 3) -> (17, 4) +++ Inserted string 'b', cursor: (17, 3) -> (17, 4) +++ Inserting string ')' +++ Inserted char ')', cursor: (17, 4) -> (17, 5) +++ Inserted string ')', cursor: (17, 4) -> (17, 5) +++ Inserting string ' ' +++ Inserted char ' ', cursor: (17, 5) -> (17, 6) +++ Inserted string ' ', cursor: (17, 5) -> (17, 6) And that causes `wait_for` to think the "step" command is complete. This is wrong, as the prompt at line 17 isn't the prompt drawn after the completion of the "step" command. The subsequent tests now run with a partially updated screen (what is shown above) and obviously fail. The ideal way to fix this would be for `wait_for` to be smarter, to avoid it confusing the different prompts drawn. However, I would also like to reduce the variations in TUI test results due to the directories (source and build) in which tests are ran. TUI tests are more prone to differences in test results due to variations in directory names than other tests, as it makes curses take different redrawing decisions. So in this patch, I propose to make TUI tests use "set filename-display basename", which makes GDB omit directory names when it prints file names. This way, regardless of where you run the tests, you should get the same results (all other things being equal). Doing this happens to fix my failures and makes my CI happy (which in turns makes me happy). To be clear, I understand that this does not fix the root issue of `proc wait_for` being confused. However, it makes TUI test runs be more similar for everyone, such that there's less chance of TUI tests randomly failing for somebody. If some other change triggers the `wait_for` problem again in the future, hopefully everybody will see the problem and we can work on getting it fixed more easily than if just one unlucky person sees the problem. Note that there are other reasons why TUI tests could vary, like different curses library versions taking different re-drawing decisions. However, I think my change is a good step towards more stable test results. [1] https://vt100.net/docs/vt510-rm/DL.html Change-Id: Ib18da83317e7b78a46f77892af0d2e39bd261bf5 --- gdb/testsuite/lib/tuiterm.exp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) base-commit: aaf3f3f3bb38a59125ea34afa0ef7e0e14c2e916
Comments
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
Simon> And that causes `wait_for` to think the "step" command is complete.
Simon> This is wrong, as the prompt at line 17 isn't the prompt drawn after the
Simon> completion of the "step" command. The subsequent tests now run with a
Simon> partially updated screen (what is shown above) and obviously fail.
Simon> The ideal way to fix this would be for `wait_for` to be smarter, to
Simon> avoid it confusing the different prompts drawn.
This is actually ridiculously hard. Not being able to solve this is why
we added the refresh-counting hack for TUI testing.
Simon> Doing this happens to fix my failures and makes my CI happy (which in
Simon> turns makes me happy). To be clear, I understand that this does not fix
Simon> the root issue of `proc wait_for` being confused. However, it makes TUI
Simon> test runs be more similar for everyone, such that there's less chance of
Simon> TUI tests randomly failing for somebody.
It seems like an improvement to me.
Simon> Note that there are other reasons why TUI tests could vary, like
Simon> different curses library versions taking different re-drawing decisions.
Simon> However, I think my change is a good step towards more stable test
Simon> results.
Indeed, I think this has happened already.
Anyway the patch looks good to me.
Tom
On 2022-09-23 09:50, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > > Simon> And that causes `wait_for` to think the "step" command is complete. > Simon> This is wrong, as the prompt at line 17 isn't the prompt drawn after the > Simon> completion of the "step" command. The subsequent tests now run with a > Simon> partially updated screen (what is shown above) and obviously fail. > > Simon> The ideal way to fix this would be for `wait_for` to be smarter, to > Simon> avoid it confusing the different prompts drawn. > > This is actually ridiculously hard. Not being able to solve this is why > we added the refresh-counting hack for TUI testing. Ok, I'm not aware of that. But yeah I thought about this problem for a day and didn't find any magic solution. The only semi-relevant idea I had was to track at which line the last prompt was printed. And when matching a prompt, it must be at a line >= where the last prompt was printed. If it's above, it means it's a redraw of an old prompt. > Simon> Doing this happens to fix my failures and makes my CI happy (which in > Simon> turns makes me happy). To be clear, I understand that this does not fix > Simon> the root issue of `proc wait_for` being confused. However, it makes TUI > Simon> test runs be more similar for everyone, such that there's less chance of > Simon> TUI tests randomly failing for somebody. > > It seems like an improvement to me. > > Simon> Note that there are other reasons why TUI tests could vary, like > Simon> different curses library versions taking different re-drawing decisions. > Simon> However, I think my change is a good step towards more stable test > Simon> results. > > Indeed, I think this has happened already. > > Anyway the patch looks good to me. Thanks, I'll push it. Simon
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp index bf094131eac0..6ca113fdd8ef 100644 --- a/gdb/testsuite/lib/tuiterm.exp +++ b/gdb/testsuite/lib/tuiterm.exp @@ -757,9 +757,15 @@ namespace eval Term { # clean_restart. proc clean_restart {rows cols {executable {}}} { global env stty_init - save_vars {env(TERM) stty_init} { + save_vars {env(TERM) stty_init ::GDBFLAGS} { setenv TERM ansi _setup $rows $cols + + # Make GDB not print the directory names. Use this setting to + # remove the differences in test runs due to varying directory + # names. + append ::GDBFLAGS " -ex \"set filename-display basename\"" + if {$executable == ""} { ::clean_restart } else {