public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] gdb/testsuite/tui: start GDB with "set filename-display basename"
@ 2022-09-22 16:02 Simon Marchi
  2022-09-23 13:50 ` Tom Tromey
  0 siblings, 1 reply; 3+ messages in thread
From: Simon Marchi @ 2022-09-22 16:02 UTC (permalink / raw)
  To: gdb-patches

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(-)

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 {

base-commit: aaf3f3f3bb38a59125ea34afa0ef7e0e14c2e916
-- 
2.37.3


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] gdb/testsuite/tui: start GDB with "set filename-display basename"
  2022-09-22 16:02 [PATCH] gdb/testsuite/tui: start GDB with "set filename-display basename" Simon Marchi
@ 2022-09-23 13:50 ` Tom Tromey
  2022-09-23 14:22   ` Simon Marchi
  0 siblings, 1 reply; 3+ messages in thread
From: Tom Tromey @ 2022-09-23 13:50 UTC (permalink / raw)
  To: Simon Marchi via Gdb-patches

>>>>> "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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] gdb/testsuite/tui: start GDB with "set filename-display basename"
  2022-09-23 13:50 ` Tom Tromey
@ 2022-09-23 14:22   ` Simon Marchi
  0 siblings, 0 replies; 3+ messages in thread
From: Simon Marchi @ 2022-09-23 14:22 UTC (permalink / raw)
  To: Tom Tromey, Simon Marchi via Gdb-patches



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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-09-23 14:22 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-22 16:02 [PATCH] gdb/testsuite/tui: start GDB with "set filename-display basename" Simon Marchi
2022-09-23 13:50 ` Tom Tromey
2022-09-23 14:22   ` Simon Marchi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).