diff mbox

Fix batch exit status test failure on Fedora 28

Message ID 1535991124-28668-1-git-send-email-gbenson@redhat.com
State New, archived
Headers

Commit Message

Gary Benson Sept. 3, 2018, 4:12 p.m. UTC 
  Pedro Alves <palves@redhat.com> wrote:
> Yeah, if we compare with gdb.base/quit.exp, that is missing
> as well as the "remote_close host" call.  For the latter, when I last
> touched quit.exp, I remember fretting a bit about whether the remote_close
> was really necessary, but in the end decided to keep it, I don't recall
> exactly if I kept it because I found it's necessary (probably for
> remote hosts), or because it doesn't hurt.
>
> Now that I look at gdb.base/quit.exp again, I see that
> gdb.base/batch-exit-status.exp can hang forever in the same way that
> 15763a09d4ae fixed, isn't it?  Gary, any reason you didn't do the
> "eof" thing here too?

I didn't realize what it did; I thought it was something specific to
that test.  And I thought the gdb_exit at the start of the test would
clear $gdb_spawn_id but apparently not! :)  So I made a fix, basically
copied and pasted from the commit you referenced:

This commit changes the batch exit status test to detect GDB's exit
using gdb_test_multiple expecting 'eof', instead of using 'wait'.
This fixes failures reported by buildbot on Fedora 28, and also means
the testcase won't hang forever on failure as fixed in
gdb.base/quit.exp by commit 15763a09d4aea85fc3153d6746c040dd48565637.

Is this ok to commit?

Thanks,
Gary

--
gdb/testsuite/ChangeLog:

	* gdb.base/batch-exit-status.exp: Use gdb_test_multiple and expect
	'eof' before 'wait -i'.  Use remote_close and clear_gdb_spawn_id.
---
 gdb/testsuite/ChangeLog                      |  5 +++++
 gdb/testsuite/gdb.base/batch-exit-status.exp | 17 ++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

Comments

Pedro Alves Sept. 4, 2018, 11:39 a.m. UTC | #1 
On 09/03/2018 05:12 PM, Gary Benson wrote:
> Pedro Alves <palves@redhat.com> wrote:
>> Yeah, if we compare with gdb.base/quit.exp, that is missing
>> as well as the "remote_close host" call.  For the latter, when I last
>> touched quit.exp, I remember fretting a bit about whether the remote_close
>> was really necessary, but in the end decided to keep it, I don't recall
>> exactly if I kept it because I found it's necessary (probably for
>> remote hosts), or because it doesn't hurt.
>>
>> Now that I look at gdb.base/quit.exp again, I see that
>> gdb.base/batch-exit-status.exp can hang forever in the same way that
>> 15763a09d4ae fixed, isn't it?  Gary, any reason you didn't do the
>> "eof" thing here too?
> 
> I didn't realize what it did; I thought it was something specific to
> that test.  And I thought the gdb_exit at the start of the test would
> clear $gdb_spawn_id but apparently not! :)  So I made a fix, basically
> copied and pasted from the commit you referenced:
> 
> This commit changes the batch exit status test to detect GDB's exit
> using gdb_test_multiple expecting 'eof', instead of using 'wait'.

> This fixes failures reported by buildbot on Fedora 28, and also means

Please expand here what the failure was, for the logs.  Something concise
mentioning gdb_spawn_id, doesn't have to be long.  It just should be more
self contained so that someone reading back the logs later on understands
what it was.

> the testcase won't hang forever on failure as fixed in
> gdb.base/quit.exp by commit 15763a09d4aea85fc3153d6746c040dd48565637.

Please write the commit in Linux-commit-style:

commit 15763a09d4ae ("Fix 'gdb.base/quit.exp hangs forever' if
the test fails").

to help the human reading the logs.

> Is this ok to commit?

OK.

Thanks,
Pedro Alves
Gary Benson Sept. 4, 2018, 2:37 p.m. UTC | #2 
Pedro Alves wrote:
> On 09/03/2018 05:12 PM, Gary Benson wrote:
> > Pedro Alves <palves@redhat.com> wrote:
> > > Yeah, if we compare with gdb.base/quit.exp, that is missing
> > > as well as the "remote_close host" call.  For the latter, when I last
> > > touched quit.exp, I remember fretting a bit about whether the remote_close
> > > was really necessary, but in the end decided to keep it, I don't recall
> > > exactly if I kept it because I found it's necessary (probably for
> > > remote hosts), or because it doesn't hurt.
> > >
> > > Now that I look at gdb.base/quit.exp again, I see that
> > > gdb.base/batch-exit-status.exp can hang forever in the same way that
> > > 15763a09d4ae fixed, isn't it?  Gary, any reason you didn't do the
> > > "eof" thing here too?
> > 
> > I didn't realize what it did; I thought it was something specific to
> > that test.  And I thought the gdb_exit at the start of the test would
> > clear $gdb_spawn_id but apparently not! :)  So I made a fix, basically
> > copied and pasted from the commit you referenced:
> > 
> > This commit changes the batch exit status test to detect GDB's exit
> > using gdb_test_multiple expecting 'eof', instead of using 'wait'.
> 
> > This fixes failures reported by buildbot on Fedora 28, and also means
> 
> Please expand here what the failure was, for the logs.  Something concise
> mentioning gdb_spawn_id, doesn't have to be long.  It just should be more
> self contained so that someone reading back the logs later on understands
> what it was.
> 
> > the testcase won't hang forever on failure as fixed in
> > gdb.base/quit.exp by commit 15763a09d4aea85fc3153d6746c040dd48565637.
> 
> Please write the commit in Linux-commit-style:
> 
> commit 15763a09d4ae ("Fix 'gdb.base/quit.exp hangs forever' if
> the test fails").
> 
> to help the human reading the logs.

I've pushed the patch with the following message:

  Fix batch exit status test failure on Fedora 28
  
  This commit adds calls to remote_close and clear_gdb_spawn_id to
  gdb.base/batch-exit-status.exp, fixing failures reported by buildbot
  on Fedora 28 where gdb_spawn_id not being reset by the previous test
  caused default_gdb_spawn to return without spawning.
  
  This commit also changes the test to use detect GDB's exit using
  gdb_test_multiple expecting 'eof', rather than using 'wait -i' alone.
  This means the testcase won't hang forever on failure as fixed in
  gdb.base/quit.exp by commit 15763a09d4ae ("Fix 'gdb.base/quit.exp
  hangs forever' if the test fails").
  
  gdb/testsuite/ChangeLog:

        * gdb.base/batch-exit-status.exp: Use gdb_test_multiple and expect
        'eof' before 'wait -i'.  Use remote_close and clear_gdb_spawn_id.

Thanks,
Gary
diff mbox

Patch

diff --git a/gdb/testsuite/gdb.base/batch-exit-status.exp b/gdb/testsuite/gdb.base/batch-exit-status.exp
index bee4d72..3b5de59 100644
--- a/gdb/testsuite/gdb.base/batch-exit-status.exp
+++ b/gdb/testsuite/gdb.base/batch-exit-status.exp
@@ -29,11 +29,18 @@  proc _test_exit_status {expect_status cmdline_opts} {
 	return
     }
 
-    set result [wait -i $gdb_spawn_id]
-    verbose $result
-    gdb_assert { [lindex $result 2] == 0 }
-    set actual_status [lindex $result 3]
-    gdb_assert { $actual_status == $expect_status }
+    gdb_test_multiple "" "run til exit" {
+	eof {
+	    set result [wait -i $gdb_spawn_id]
+	    verbose $result
+
+	    gdb_assert { [lindex $result 2] == 0 }
+	    gdb_assert { [lindex $result 3] == $expect_status }
+
+	    remote_close host
+	    clear_gdb_spawn_id
+	}
+    }
 }
 
 proc test_exit_status {expect_status cmdline_opts} {