diff mbox

[1/6] gdb/testsuite: Better detection of auto-response at y/n prompts

Message ID 20190107090021.GR3456@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Jan. 7, 2019, 9 a.m. UTC
* Simon Marchi <simon.marchi@ericsson.com> [2019-01-02 21:48:06 +0000]:

> On 2019-01-01 5:45 p.m., Andrew Burgess wrote:
> > I noticed that when running this test:
> > 
> >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"
> > 
> > I would occasionally see some UNRESOLVED test results like this:
> > 
> >   (gdb)
> >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
> >   Expecting: ^(kill[
> >   ]+)?(.*[
> >   ]+[(]gdb[)]
> >   [ ]*)
> >   kill
> >   &"kill\n"
> >   ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
> >   =thread-group-exited,id="i1"
> >   ERROR: Got interactive prompt.
> >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
> > 
> > The problem appears to be that the expect buffer fills up to include
> > the '(y or n)' prompt without including the following lines.
> > 
> > The pattern supplied by the outer test script is looking for the
> > following lines.  As the following lines are not present then expect
> > matches on the interactive prompt case rather than the case for the
> > user supplied pattern.
> > 
> > The problem with this is that we are not really at an interactive
> > prompt, GDB is providing an answer for us and then moving on.  When I
> > examine a successful run of the test the output from GDB is identical,
> > the only difference is where expect happens to buffer the output from
> > GDB.
> > 
> > This patch introduces a second check inside the 'y or n' prompt case,
> > which gives expect a chance to refill its buffers and catches the
> > 'answered Y; input ...' text.
> > 
> > This second check is on a short 1 second timeout, I'm currently
> > assuming that the auto-answer text will either already be in expect,
> > or is waiting to be read in.  If after 1 second the auto-answer text
> > is not seen then we assume that GDB really is waiting at an
> > interactive prompt.
> > 
> > With this patch in place I can now leave the following loop running
> > indefinitely, where before it would fail usually after ~10
> > iterations.
> 
> For me it fails consistently the way you describe.
> 
> > diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
> > index d193592a843..48ea45d62c7 100644
> > --- a/gdb/testsuite/lib/mi-support.exp
> > +++ b/gdb/testsuite/lib/mi-support.exp
> > @@ -834,9 +834,24 @@ proc mi_gdb_test { args } {
> >  	     fail "$message"
> >  	}
> >  	 -re "\\(y or n\\) " {
> > -	    send_gdb "n\n"
> > -	    perror "Got interactive prompt."
> > -	     fail "$message"
> > +	    # If the expect buffer just happens to fill up to the 'y
> > +	    # or n' prompt then we can end up in this case, even
> > +	    # though GDB will automatically provide a response for us.
> > +	    # We give expect another chance here to look for the auto
> > +	    # answer text before declaring a fail.
> > +	    set auto_response_seen 0
> > +	    gdb_expect 1 {
> > +		-re ".answered Y; input not from terminal." {
> > +		    set auto_response_seen 1
> > +		}
> > +	    }
> > +	    if { ! $auto_response_seen } {
> > +		send_gdb "n\n"
> > +		perror "Got interactive prompt."
> > +		fail "$message"
> > +	    } else {
> > +		exp_continue
> > +	    }
> >  	}
> >  	 eof {
> >  	     perror "Process no longer exists"
> 
> Do we need this stanza at all?  I understand that it can save some time (avoid having
> to wait for the timeout) if the MI interpreter suddenly starts asking interactive
> "y or n" questions (which it should not do), but since it's causing this kind of trouble,
> maybe we can just get rid of it?

I don't see any real reason to keep it.  I tried deleting it and no
tests seem to regress, and the previously unstable test is now stable.

Is this OK to apply?

Thanks,
Andrew

--

gdb/testsuite: Remove interactive prompt case from mi_gdb_test

I noticed that when running this test:

  make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"

I would occasionally see some UNRESOLVED test results like this:

  (gdb)
  PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
  Expecting: ^(kill[
  ]+)?(.*[
  ]+[(]gdb[)]
  [ ]*)
  kill
  &"kill\n"
  ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
  =thread-group-exited,id="i1"
  ERROR: Got interactive prompt.
  UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:

The problem appears to be that the expect buffer fills up to include
the '(y or n)' prompt without including the following lines.

The pattern supplied by the outer test script is looking for the
following lines.  As the following lines are not present then expect
matches on the interactive prompt case rather than the case for the
user supplied pattern.

The problem with this is that we are not really at an interactive
prompt, GDB is providing an answer for us and then moving on.  When I
examine a successful run of the test the output from GDB is identical,
the only difference is where expect happens to buffer the output from
GDB.

This patch remove all special handling of the interactive prompt
case.  This means that if we ever break GDB and start seeing an
unexpected interactive prompt then tests will rely on a timeout to
fail, instead of having dedicated interactive prompt detection, but
this solves the problem that an auto-answered prompt looks very
similar to an interactive prompt.

With this patch in place I can now leave the following loop running
indefinitely, where before it would fail usually after ~10
iterations.

  while make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"; \
  do /bin/true; \
  done

gdb/testsuite/ChangeLog:

	* lib/mi-support.exp (mi_gdb_test): Remove interactive prompt
	case.
---
 gdb/testsuite/ChangeLog          | 5 +++++
 gdb/testsuite/lib/mi-support.exp | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Simon Marchi Jan. 9, 2019, 5:03 a.m. UTC | #1
On 2019-01-07 04:00, Andrew Burgess wrote:
> * Simon Marchi <simon.marchi@ericsson.com> [2019-01-02 21:48:06 +0000]:
> 
>> On 2019-01-01 5:45 p.m., Andrew Burgess wrote:
>> > I noticed that when running this test:
>> >
>> >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"
>> >
>> > I would occasionally see some UNRESOLVED test results like this:
>> >
>> >   (gdb)
>> >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
>> >   Expecting: ^(kill[
>> >   ]+)?(.*[
>> >   ]+[(]gdb[)]
>> >   [ ]*)
>> >   kill
>> >   &"kill\n"
>> >   ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
>> >   =thread-group-exited,id="i1"
>> >   ERROR: Got interactive prompt.
>> >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
>> >
>> > The problem appears to be that the expect buffer fills up to include
>> > the '(y or n)' prompt without including the following lines.
>> >
>> > The pattern supplied by the outer test script is looking for the
>> > following lines.  As the following lines are not present then expect
>> > matches on the interactive prompt case rather than the case for the
>> > user supplied pattern.
>> >
>> > The problem with this is that we are not really at an interactive
>> > prompt, GDB is providing an answer for us and then moving on.  When I
>> > examine a successful run of the test the output from GDB is identical,
>> > the only difference is where expect happens to buffer the output from
>> > GDB.
>> >
>> > This patch introduces a second check inside the 'y or n' prompt case,
>> > which gives expect a chance to refill its buffers and catches the
>> > 'answered Y; input ...' text.
>> >
>> > This second check is on a short 1 second timeout, I'm currently
>> > assuming that the auto-answer text will either already be in expect,
>> > or is waiting to be read in.  If after 1 second the auto-answer text
>> > is not seen then we assume that GDB really is waiting at an
>> > interactive prompt.
>> >
>> > With this patch in place I can now leave the following loop running
>> > indefinitely, where before it would fail usually after ~10
>> > iterations.
>> 
>> For me it fails consistently the way you describe.
>> 
>> > diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
>> > index d193592a843..48ea45d62c7 100644
>> > --- a/gdb/testsuite/lib/mi-support.exp
>> > +++ b/gdb/testsuite/lib/mi-support.exp
>> > @@ -834,9 +834,24 @@ proc mi_gdb_test { args } {
>> >  	     fail "$message"
>> >  	}
>> >  	 -re "\\(y or n\\) " {
>> > -	    send_gdb "n\n"
>> > -	    perror "Got interactive prompt."
>> > -	     fail "$message"
>> > +	    # If the expect buffer just happens to fill up to the 'y
>> > +	    # or n' prompt then we can end up in this case, even
>> > +	    # though GDB will automatically provide a response for us.
>> > +	    # We give expect another chance here to look for the auto
>> > +	    # answer text before declaring a fail.
>> > +	    set auto_response_seen 0
>> > +	    gdb_expect 1 {
>> > +		-re ".answered Y; input not from terminal." {
>> > +		    set auto_response_seen 1
>> > +		}
>> > +	    }
>> > +	    if { ! $auto_response_seen } {
>> > +		send_gdb "n\n"
>> > +		perror "Got interactive prompt."
>> > +		fail "$message"
>> > +	    } else {
>> > +		exp_continue
>> > +	    }
>> >  	}
>> >  	 eof {
>> >  	     perror "Process no longer exists"
>> 
>> Do we need this stanza at all?  I understand that it can save some 
>> time (avoid having
>> to wait for the timeout) if the MI interpreter suddenly starts asking 
>> interactive
>> "y or n" questions (which it should not do), but since it's causing 
>> this kind of trouble,
>> maybe we can just get rid of it?
> 
> I don't see any real reason to keep it.  I tried deleting it and no
> tests seem to regress, and the previously unstable test is now stable.
> 
> Is this OK to apply?
> 
> Thanks,
> Andrew
> 
> --
> 
> gdb/testsuite: Remove interactive prompt case from mi_gdb_test
> 
> I noticed that when running this test:
> 
>   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
> gdb.mi/mi-break.exp"
> 
> I would occasionally see some UNRESOLVED test results like this:
> 
>   (gdb)
>   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
>   Expecting: ^(kill[
>   ]+)?(.*[
>   ]+[(]gdb[)]
>   [ ]*)
>   kill
>   &"kill\n"
>   ~"Kill the program being debugged? (y or n) [answered Y; input not
> from terminal]\n"
>   =thread-group-exited,id="i1"
>   ERROR: Got interactive prompt.
>   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
> 
> The problem appears to be that the expect buffer fills up to include
> the '(y or n)' prompt without including the following lines.
> 
> The pattern supplied by the outer test script is looking for the
> following lines.  As the following lines are not present then expect
> matches on the interactive prompt case rather than the case for the
> user supplied pattern.
> 
> The problem with this is that we are not really at an interactive
> prompt, GDB is providing an answer for us and then moving on.  When I
> examine a successful run of the test the output from GDB is identical,
> the only difference is where expect happens to buffer the output from
> GDB.
> 
> This patch remove all special handling of the interactive prompt
> case.  This means that if we ever break GDB and start seeing an
> unexpected interactive prompt then tests will rely on a timeout to
> fail, instead of having dedicated interactive prompt detection, but
> this solves the problem that an auto-answered prompt looks very
> similar to an interactive prompt.
> 
> With this patch in place I can now leave the following loop running
> indefinitely, where before it would fail usually after ~10
> iterations.
> 
>   while make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
> gdb.mi/mi-break.exp"; \
>   do /bin/true; \
>   done
> 
> gdb/testsuite/ChangeLog:
> 
> 	* lib/mi-support.exp (mi_gdb_test): Remove interactive prompt
> 	case.
> ---
>  gdb/testsuite/ChangeLog          | 5 +++++
>  gdb/testsuite/lib/mi-support.exp | 5 -----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/mi-support.exp 
> b/gdb/testsuite/lib/mi-support.exp
> index d193592a843..a58c4f6e119 100644
> --- a/gdb/testsuite/lib/mi-support.exp
> +++ b/gdb/testsuite/lib/mi-support.exp
> @@ -832,11 +832,6 @@ proc mi_gdb_test { args } {
>  	    send_gdb "\n"
>  	    perror "Window too small."
>  	     fail "$message"
> -	}
> -	 -re "\\(y or n\\) " {
> -	    send_gdb "n\n"
> -	    perror "Got interactive prompt."
> -	     fail "$message"
>  	}
>  	 eof {
>  	     perror "Process no longer exists"


Thanks, this LGTM.

Btw, what you think about those "fail"s following "perror"s?  It's my 
understanding that perror throws an error, interrupting the execution 
flow, so the fail is not recorded, right?

And a note, maybe you are already aware of its existence, but I think 
that "make check-read1" could have been useful to reproduce the issue 
reliably here.  It forces the read syscall to read 1 byte at the time, 
so you are certain to reach a point where -re "\\(y or n\\) " matches.

Simon
Andrew Burgess Jan. 9, 2019, 11:24 a.m. UTC | #2
* Simon Marchi <simon.marchi@polymtl.ca> [2019-01-09 00:03:49 -0500]:

> On 2019-01-07 04:00, Andrew Burgess wrote:
> > * Simon Marchi <simon.marchi@ericsson.com> [2019-01-02 21:48:06 +0000]:
> > 
> > > On 2019-01-01 5:45 p.m., Andrew Burgess wrote:
> > > > I noticed that when running this test:
> > > >
> > > >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"
> > > >
> > > > I would occasionally see some UNRESOLVED test results like this:
> > > >
> > > >   (gdb)
> > > >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
> > > >   Expecting: ^(kill[
> > > >   ]+)?(.*[
> > > >   ]+[(]gdb[)]
> > > >   [ ]*)
> > > >   kill
> > > >   &"kill\n"
> > > >   ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
> > > >   =thread-group-exited,id="i1"
> > > >   ERROR: Got interactive prompt.
> > > >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
> > > >
> > > > The problem appears to be that the expect buffer fills up to include
> > > > the '(y or n)' prompt without including the following lines.
> > > >
> > > > The pattern supplied by the outer test script is looking for the
> > > > following lines.  As the following lines are not present then expect
> > > > matches on the interactive prompt case rather than the case for the
> > > > user supplied pattern.
> > > >
> > > > The problem with this is that we are not really at an interactive
> > > > prompt, GDB is providing an answer for us and then moving on.  When I
> > > > examine a successful run of the test the output from GDB is identical,
> > > > the only difference is where expect happens to buffer the output from
> > > > GDB.
> > > >
> > > > This patch introduces a second check inside the 'y or n' prompt case,
> > > > which gives expect a chance to refill its buffers and catches the
> > > > 'answered Y; input ...' text.
> > > >
> > > > This second check is on a short 1 second timeout, I'm currently
> > > > assuming that the auto-answer text will either already be in expect,
> > > > or is waiting to be read in.  If after 1 second the auto-answer text
> > > > is not seen then we assume that GDB really is waiting at an
> > > > interactive prompt.
> > > >
> > > > With this patch in place I can now leave the following loop running
> > > > indefinitely, where before it would fail usually after ~10
> > > > iterations.
> > > 
> > > For me it fails consistently the way you describe.
> > > 
> > > > diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
> > > > index d193592a843..48ea45d62c7 100644
> > > > --- a/gdb/testsuite/lib/mi-support.exp
> > > > +++ b/gdb/testsuite/lib/mi-support.exp
> > > > @@ -834,9 +834,24 @@ proc mi_gdb_test { args } {
> > > >  	     fail "$message"
> > > >  	}
> > > >  	 -re "\\(y or n\\) " {
> > > > -	    send_gdb "n\n"
> > > > -	    perror "Got interactive prompt."
> > > > -	     fail "$message"
> > > > +	    # If the expect buffer just happens to fill up to the 'y
> > > > +	    # or n' prompt then we can end up in this case, even
> > > > +	    # though GDB will automatically provide a response for us.
> > > > +	    # We give expect another chance here to look for the auto
> > > > +	    # answer text before declaring a fail.
> > > > +	    set auto_response_seen 0
> > > > +	    gdb_expect 1 {
> > > > +		-re ".answered Y; input not from terminal." {
> > > > +		    set auto_response_seen 1
> > > > +		}
> > > > +	    }
> > > > +	    if { ! $auto_response_seen } {
> > > > +		send_gdb "n\n"
> > > > +		perror "Got interactive prompt."
> > > > +		fail "$message"
> > > > +	    } else {
> > > > +		exp_continue
> > > > +	    }
> > > >  	}
> > > >  	 eof {
> > > >  	     perror "Process no longer exists"
> > > 
> > > Do we need this stanza at all?  I understand that it can save some
> > > time (avoid having
> > > to wait for the timeout) if the MI interpreter suddenly starts
> > > asking interactive
> > > "y or n" questions (which it should not do), but since it's causing
> > > this kind of trouble,
> > > maybe we can just get rid of it?
> > 
> > I don't see any real reason to keep it.  I tried deleting it and no
> > tests seem to regress, and the previously unstable test is now stable.
> > 
> > Is this OK to apply?
> > 
> > Thanks,
> > Andrew
> > 
> > --
> > 
> > gdb/testsuite: Remove interactive prompt case from mi_gdb_test
> > 
> > I noticed that when running this test:
> > 
> >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
> > gdb.mi/mi-break.exp"
> > 
> > I would occasionally see some UNRESOLVED test results like this:
> > 
> >   (gdb)
> >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
> >   Expecting: ^(kill[
> >   ]+)?(.*[
> >   ]+[(]gdb[)]
> >   [ ]*)
> >   kill
> >   &"kill\n"
> >   ~"Kill the program being debugged? (y or n) [answered Y; input not
> > from terminal]\n"
> >   =thread-group-exited,id="i1"
> >   ERROR: Got interactive prompt.
> >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
> > 
> > The problem appears to be that the expect buffer fills up to include
> > the '(y or n)' prompt without including the following lines.
> > 
> > The pattern supplied by the outer test script is looking for the
> > following lines.  As the following lines are not present then expect
> > matches on the interactive prompt case rather than the case for the
> > user supplied pattern.
> > 
> > The problem with this is that we are not really at an interactive
> > prompt, GDB is providing an answer for us and then moving on.  When I
> > examine a successful run of the test the output from GDB is identical,
> > the only difference is where expect happens to buffer the output from
> > GDB.
> > 
> > This patch remove all special handling of the interactive prompt
> > case.  This means that if we ever break GDB and start seeing an
> > unexpected interactive prompt then tests will rely on a timeout to
> > fail, instead of having dedicated interactive prompt detection, but
> > this solves the problem that an auto-answered prompt looks very
> > similar to an interactive prompt.
> > 
> > With this patch in place I can now leave the following loop running
> > indefinitely, where before it would fail usually after ~10
> > iterations.
> > 
> >   while make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
> > gdb.mi/mi-break.exp"; \
> >   do /bin/true; \
> >   done
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* lib/mi-support.exp (mi_gdb_test): Remove interactive prompt
> > 	case.
> > ---
> >  gdb/testsuite/ChangeLog          | 5 +++++
> >  gdb/testsuite/lib/mi-support.exp | 5 -----
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/gdb/testsuite/lib/mi-support.exp
> > b/gdb/testsuite/lib/mi-support.exp
> > index d193592a843..a58c4f6e119 100644
> > --- a/gdb/testsuite/lib/mi-support.exp
> > +++ b/gdb/testsuite/lib/mi-support.exp
> > @@ -832,11 +832,6 @@ proc mi_gdb_test { args } {
> >  	    send_gdb "\n"
> >  	    perror "Window too small."
> >  	     fail "$message"
> > -	}
> > -	 -re "\\(y or n\\) " {
> > -	    send_gdb "n\n"
> > -	    perror "Got interactive prompt."
> > -	     fail "$message"
> >  	}
> >  	 eof {
> >  	     perror "Process no longer exists"
> 
> 
> Thanks, this LGTM.

Thanks, I pushed this patch.
> 
> Btw, what you think about those "fail"s following "perror"s?  It's my
> understanding that perror throws an error, interrupting the execution flow,
> so the fail is not recorded, right?

That's not what I see.  As an experiment I created the file
gdb.base/perror-test.exp like this:

  perror "this is an error"
  pass "this is a pass"

With this I see:

  ERROR: this is an error
  
  		=== gdb Summary ===
  
  # of unresolved testcases	1

With the unresolved being instead of the 'PASS'.  Also the return
value from runtest is non-zero.

If I change perror-test.exp to this:

  perror "this is an error"
  fail "this is a fail"

Then the results are similar:

  ERROR: this is an error
  
  		=== gdb Summary ===
  
  # of unresolved testcases	1

Again, the unresolved replaces the 'FAIL', and the exit value is
non-zero.

Where it gets interesting is if I change perror-test.exp to this:

  perror "this is an error"

Then I get this result:

  ERROR: this is an error
  
  		=== gdb Summary ===

And alarmingly (maybe?) the exit value from runtest is 0, so it looks
like the test has passed.

My conclusions then:

  - perror doesn't interrupt control flow,
  - perror doesn't itself cause the test script to appear to fail,
  - perror causes the next pass/fail (at least) to become unresolved,

Given that the perror calls we're looking at here are in mi_gdb_test,
the entire pass/fail printing is wrapped up within this method, I
don't think the unresolved-ness should leak out into the next test,
so, in this case I think perror/fail is correct.

> 
> And a note, maybe you are already aware of its existence, but I think that
> "make check-read1" could have been useful to reproduce the issue reliably
> here.  It forces the read syscall to read 1 byte at the time, so you are
> certain to reach a point where -re "\\(y or n\\) " matches.

I did not know this.  Might be worth testing the whole testsuite
against this and look for regressions..... I'll add it to my (long)
list.

Thanks,
Andrew
Simon Marchi Jan. 9, 2019, 2:45 p.m. UTC | #3
On 2019-01-09 06:24, Andrew Burgess wrote:
> * Simon Marchi <simon.marchi@polymtl.ca> [2019-01-09 00:03:49 -0500]:
> 
>> On 2019-01-07 04:00, Andrew Burgess wrote:
>> > * Simon Marchi <simon.marchi@ericsson.com> [2019-01-02 21:48:06 +0000]:
>> >
>> > > On 2019-01-01 5:45 p.m., Andrew Burgess wrote:
>> > > > I noticed that when running this test:
>> > > >
>> > > >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver gdb.mi/mi-break.exp"
>> > > >
>> > > > I would occasionally see some UNRESOLVED test results like this:
>> > > >
>> > > >   (gdb)
>> > > >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
>> > > >   Expecting: ^(kill[
>> > > >   ]+)?(.*[
>> > > >   ]+[(]gdb[)]
>> > > >   [ ]*)
>> > > >   kill
>> > > >   &"kill\n"
>> > > >   ~"Kill the program being debugged? (y or n) [answered Y; input not from terminal]\n"
>> > > >   =thread-group-exited,id="i1"
>> > > >   ERROR: Got interactive prompt.
>> > > >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
>> > > >
>> > > > The problem appears to be that the expect buffer fills up to include
>> > > > the '(y or n)' prompt without including the following lines.
>> > > >
>> > > > The pattern supplied by the outer test script is looking for the
>> > > > following lines.  As the following lines are not present then expect
>> > > > matches on the interactive prompt case rather than the case for the
>> > > > user supplied pattern.
>> > > >
>> > > > The problem with this is that we are not really at an interactive
>> > > > prompt, GDB is providing an answer for us and then moving on.  When I
>> > > > examine a successful run of the test the output from GDB is identical,
>> > > > the only difference is where expect happens to buffer the output from
>> > > > GDB.
>> > > >
>> > > > This patch introduces a second check inside the 'y or n' prompt case,
>> > > > which gives expect a chance to refill its buffers and catches the
>> > > > 'answered Y; input ...' text.
>> > > >
>> > > > This second check is on a short 1 second timeout, I'm currently
>> > > > assuming that the auto-answer text will either already be in expect,
>> > > > or is waiting to be read in.  If after 1 second the auto-answer text
>> > > > is not seen then we assume that GDB really is waiting at an
>> > > > interactive prompt.
>> > > >
>> > > > With this patch in place I can now leave the following loop running
>> > > > indefinitely, where before it would fail usually after ~10
>> > > > iterations.
>> > >
>> > > For me it fails consistently the way you describe.
>> > >
>> > > > diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
>> > > > index d193592a843..48ea45d62c7 100644
>> > > > --- a/gdb/testsuite/lib/mi-support.exp
>> > > > +++ b/gdb/testsuite/lib/mi-support.exp
>> > > > @@ -834,9 +834,24 @@ proc mi_gdb_test { args } {
>> > > >  	     fail "$message"
>> > > >  	}
>> > > >  	 -re "\\(y or n\\) " {
>> > > > -	    send_gdb "n\n"
>> > > > -	    perror "Got interactive prompt."
>> > > > -	     fail "$message"
>> > > > +	    # If the expect buffer just happens to fill up to the 'y
>> > > > +	    # or n' prompt then we can end up in this case, even
>> > > > +	    # though GDB will automatically provide a response for us.
>> > > > +	    # We give expect another chance here to look for the auto
>> > > > +	    # answer text before declaring a fail.
>> > > > +	    set auto_response_seen 0
>> > > > +	    gdb_expect 1 {
>> > > > +		-re ".answered Y; input not from terminal." {
>> > > > +		    set auto_response_seen 1
>> > > > +		}
>> > > > +	    }
>> > > > +	    if { ! $auto_response_seen } {
>> > > > +		send_gdb "n\n"
>> > > > +		perror "Got interactive prompt."
>> > > > +		fail "$message"
>> > > > +	    } else {
>> > > > +		exp_continue
>> > > > +	    }
>> > > >  	}
>> > > >  	 eof {
>> > > >  	     perror "Process no longer exists"
>> > >
>> > > Do we need this stanza at all?  I understand that it can save some
>> > > time (avoid having
>> > > to wait for the timeout) if the MI interpreter suddenly starts
>> > > asking interactive
>> > > "y or n" questions (which it should not do), but since it's causing
>> > > this kind of trouble,
>> > > maybe we can just get rid of it?
>> >
>> > I don't see any real reason to keep it.  I tried deleting it and no
>> > tests seem to regress, and the previously unstable test is now stable.
>> >
>> > Is this OK to apply?
>> >
>> > Thanks,
>> > Andrew
>> >
>> > --
>> >
>> > gdb/testsuite: Remove interactive prompt case from mi_gdb_test
>> >
>> > I noticed that when running this test:
>> >
>> >   make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
>> > gdb.mi/mi-break.exp"
>> >
>> > I would occasionally see some UNRESOLVED test results like this:
>> >
>> >   (gdb)
>> >   PASS: gdb.mi/mi-break.exp: mi-mode=separate: breakpoint at main
>> >   Expecting: ^(kill[
>> >   ]+)?(.*[
>> >   ]+[(]gdb[)]
>> >   [ ]*)
>> >   kill
>> >   &"kill\n"
>> >   ~"Kill the program being debugged? (y or n) [answered Y; input not
>> > from terminal]\n"
>> >   =thread-group-exited,id="i1"
>> >   ERROR: Got interactive prompt.
>> >   UNRESOLVED: gdb.mi/mi-break.exp: mi-mode=separate:
>> >
>> > The problem appears to be that the expect buffer fills up to include
>> > the '(y or n)' prompt without including the following lines.
>> >
>> > The pattern supplied by the outer test script is looking for the
>> > following lines.  As the following lines are not present then expect
>> > matches on the interactive prompt case rather than the case for the
>> > user supplied pattern.
>> >
>> > The problem with this is that we are not really at an interactive
>> > prompt, GDB is providing an answer for us and then moving on.  When I
>> > examine a successful run of the test the output from GDB is identical,
>> > the only difference is where expect happens to buffer the output from
>> > GDB.
>> >
>> > This patch remove all special handling of the interactive prompt
>> > case.  This means that if we ever break GDB and start seeing an
>> > unexpected interactive prompt then tests will rely on a timeout to
>> > fail, instead of having dedicated interactive prompt detection, but
>> > this solves the problem that an auto-answered prompt looks very
>> > similar to an interactive prompt.
>> >
>> > With this patch in place I can now leave the following loop running
>> > indefinitely, where before it would fail usually after ~10
>> > iterations.
>> >
>> >   while make check-gdb RUNTESTFLAGS="--target_board=native-gdbserver
>> > gdb.mi/mi-break.exp"; \
>> >   do /bin/true; \
>> >   done
>> >
>> > gdb/testsuite/ChangeLog:
>> >
>> > 	* lib/mi-support.exp (mi_gdb_test): Remove interactive prompt
>> > 	case.
>> > ---
>> >  gdb/testsuite/ChangeLog          | 5 +++++
>> >  gdb/testsuite/lib/mi-support.exp | 5 -----
>> >  2 files changed, 5 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/gdb/testsuite/lib/mi-support.exp
>> > b/gdb/testsuite/lib/mi-support.exp
>> > index d193592a843..a58c4f6e119 100644
>> > --- a/gdb/testsuite/lib/mi-support.exp
>> > +++ b/gdb/testsuite/lib/mi-support.exp
>> > @@ -832,11 +832,6 @@ proc mi_gdb_test { args } {
>> >  	    send_gdb "\n"
>> >  	    perror "Window too small."
>> >  	     fail "$message"
>> > -	}
>> > -	 -re "\\(y or n\\) " {
>> > -	    send_gdb "n\n"
>> > -	    perror "Got interactive prompt."
>> > -	     fail "$message"
>> >  	}
>> >  	 eof {
>> >  	     perror "Process no longer exists"
>> 
>> 
>> Thanks, this LGTM.
> 
> Thanks, I pushed this patch.
>> 
>> Btw, what you think about those "fail"s following "perror"s?  It's my
>> understanding that perror throws an error, interrupting the execution 
>> flow,
>> so the fail is not recorded, right?
> 
> That's not what I see.  As an experiment I created the file
> gdb.base/perror-test.exp like this:
> 
>   perror "this is an error"
>   pass "this is a pass"
> 
> With this I see:
> 
>   ERROR: this is an error
> 
>   		=== gdb Summary ===
> 
>   # of unresolved testcases	1
> 
> With the unresolved being instead of the 'PASS'.  Also the return
> value from runtest is non-zero.
> 
> If I change perror-test.exp to this:
> 
>   perror "this is an error"
>   fail "this is a fail"
> 
> Then the results are similar:
> 
>   ERROR: this is an error
> 
>   		=== gdb Summary ===
> 
>   # of unresolved testcases	1
> 
> Again, the unresolved replaces the 'FAIL', and the exit value is
> non-zero.
> 
> Where it gets interesting is if I change perror-test.exp to this:
> 
>   perror "this is an error"
> 
> Then I get this result:
> 
>   ERROR: this is an error
> 
>   		=== gdb Summary ===
> 
> And alarmingly (maybe?) the exit value from runtest is 0, so it looks
> like the test has passed.
> 
> My conclusions then:
> 
>   - perror doesn't interrupt control flow,
>   - perror doesn't itself cause the test script to appear to fail,
>   - perror causes the next pass/fail (at least) to become unresolved,
> 
> Given that the perror calls we're looking at here are in mi_gdb_test,
> the entire pass/fail printing is wrapped up within this method, I
> don't think the unresolved-ness should leak out into the next test,
> so, in this case I think perror/fail is correct.

Ah, this is clearer when reading the documentation :)

https://www.gnu.org/software/dejagnu/manual/perror-procedure.html

It's a bit confusing, since it overrides the following pass/fail 
message.  If you don't know what perror does, it looks like the 
pass/fail is not reached...  Thanks for looking into it.

>> And a note, maybe you are already aware of its existence, but I think 
>> that
>> "make check-read1" could have been useful to reproduce the issue 
>> reliably
>> here.  It forces the read syscall to read 1 byte at the time, so you 
>> are
>> certain to reach a point where -re "\\(y or n\\) " matches.
> 
> I did not know this.  Might be worth testing the whole testsuite
> against this and look for regressions..... I'll add it to my (long)
> list.

At least, it can be a good practice to check the tests we are working 
with it.

Simon
diff mbox

Patch

diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index d193592a843..a58c4f6e119 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -832,11 +832,6 @@  proc mi_gdb_test { args } {
 	    send_gdb "\n"
 	    perror "Window too small."
 	     fail "$message"
-	}
-	 -re "\\(y or n\\) " {
-	    send_gdb "n\n"
-	    perror "Got interactive prompt."
-	     fail "$message"
 	}
 	 eof {
 	     perror "Process no longer exists"