Fix testsuite hangs when gdb_test_multiple body errors out (Re: GDB 8.2.90 available for testing)

Message ID 4d855905-32ce-ba4b-72f5-037f1796b37e@redhat.com
State New
Headers show
Series
  • Fix testsuite hangs when gdb_test_multiple body errors out (Re: GDB 8.2.90 available for testing)
Related show

Commit Message

Pedro Alves March 22, 2019, 12:39 p.m.
Hi Pedro,

Thanks for the detailed report.

On 03/07/2019 05:42 PM, Pedro Franco de Carvalho wrote:

> I'm not sure if there is a proper way to work around this in GDB, but it

> would be useful so that the testsuite doesn't hang in these cases.


I spent a while trying to fix this, and I came up with the patch below.

WDYT?

From 17e8f28072cce040524eab7b85b8767764a54cd2 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Fri, 22 Mar 2019 11:33:19 +0000
Subject: [PATCH] Fix testsuite hangs when gdb_test_multiple body errors out

This commit fixes a regression in the testsuite itself, triggered by
errors being raised from within gdb_test_multiple, originally reported
by Pedro Franco de Carvalho's at
<https://sourceware.org/ml/gdb-patches/2019-03/msg00160.html>.  Parts
of the commit message are based on his report.

This started happening due to a commit that was introduced recently,
and it can cause the testsuite to hang.

The commit that triggers this is:

 fe1a5cad302b5535030cdf62895e79512713d738
 [gdb/testsuite] Log wait status on process no longer exists error

That commit introduces a new "eof" block in gdb_test_multiple.  That
is not incorrect itself, but dejagnu's remote_expect is picking that
block as the "default" action when an error is raised from within the
commands inside a call to gdb_test_multiple:

  # remote_expect works basically the same as standard expect, but it
  # also takes care of getting the file descriptor from the specified
  # host and also calling the timeout/eof/default section if there is an
  # error on the expect call.
  #
  proc remote_expect { board timeout args } {

I find that "feature" surprising, and I don't really know why it
exists, but this means that the eof section that remote_expect picks
as the error block can be executed even when there was no actual eof
and the GDB process is still running, so the wait introduced in the
commit that tries to get the exit status of GDB hangs forever, while
GDB itself waits for input.

This only happens when there are internal testsuite errors (not
testcase failures).  This can be reproduced easily with a testcase
such as:

  gdb_start
  gdb_test_multiple "show version" "show version" {
    -re ".*" {
       error "forced error"
    }
  }

I think that working around this in GDB is useful so that the
testsuite doesn't hang in these cases.

Adding an empty "default" block at the end of the expect body in
gdb_test_multiple doesn't work, because dejagnu gives preference to
"eof" blocks:

	    if { $x eq "eof" } {
		set save_next 1
	    } elseif { $x eq "default" || $x eq "timeout" } {
		if { $error_sect eq "" } {
		    set save_next 1
		}
	    }

And we do have "eof" blocks.  So we need to make sure that the last
"eof" block is safe to use as the default error block.  It's also
pedantically incorrect to print

 "ERROR: Process no longer exists"

which is what we'd get if the last eof block we have was selected
(more below on this).

So this commit solves this by appending an "eof" with an empty
spawn_id list, so that it won't ever match.

Now, why is the first "eof" block selected today as the error block,
instead of the last one?

The reason is that remote_expect, while parsing the body to select the
default block to execute after an error, is affected by the comments
in the body (since they are also parsed).

If this comment in gdb_test_multiple

 # patterns below apply to any spawn id specified.

is changed to

 # The patterns below apply to any spawn id specified.

then the second eof block is selected and there is no hang.

Any comment at that same place with an even-numbered of tokens also
works.

This is IMO a coincidence caused by how comments work in TCL.
Comments should only appear in places where a command can appear.  And
here, remote_expect is parsing a list of options, not commands, so
it's not unreasonable to not parse comments, similarly to how this:

  set a_list {
     an_element
     # another_element
  }

results in a list with three elements, not one element.

The fact that comments with an even number of tokens work is just a
coincidence of how remote_expect's little state machine is
implemented.

I thought we could solve this by stripping out comment lines in
gdb_expect, but I didn't find an easy way to do that.  Particularly, a
couple naive approaches I tried run into complications.  For example,
we have gdb_test calls with regular expressions that include sequences
like "\r\n#", and by the time we get to gdb_expect, the \r\n have
already been expanded to a real newline, so just splitting the whole
body at newline boundaries, looking for lines that start with #
results in incorrectly stripping out half of the gdb_text regexp.  I
think it's better (at least in this commit), to move the comments out
of the list, because it's much simpler and risk free.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* lib/gdb.exp (gdb_test_multiple): Split appends to $code and
          move comments outside list.  Append '-i "" eof' section.
---
 gdb/testsuite/lib/gdb.exp | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

-- 
2.14.4

Comments

Simon Marchi March 22, 2019, 2:42 p.m. | #1
On 2019-03-22 08:39, Pedro Alves wrote:
> Hi Pedro,

> 

> Thanks for the detailed report.

> 

> On 03/07/2019 05:42 PM, Pedro Franco de Carvalho wrote:

> 

>> I'm not sure if there is a proper way to work around this in GDB, but 

>> it

>> would be useful so that the testsuite doesn't hang in these cases.

> 

> I spent a while trying to fix this, and I came up with the patch below.

> 

> WDYT?

> 

> From 17e8f28072cce040524eab7b85b8767764a54cd2 Mon Sep 17 00:00:00 2001

> From: Pedro Alves <palves@redhat.com>

> Date: Fri, 22 Mar 2019 11:33:19 +0000

> Subject: [PATCH] Fix testsuite hangs when gdb_test_multiple body errors 

> out

> 

> This commit fixes a regression in the testsuite itself, triggered by

> errors being raised from within gdb_test_multiple, originally reported

> by Pedro Franco de Carvalho's at

> <https://sourceware.org/ml/gdb-patches/2019-03/msg00160.html>.  Parts

> of the commit message are based on his report.

> 

> This started happening due to a commit that was introduced recently,

> and it can cause the testsuite to hang.

> 

> The commit that triggers this is:

> 

>  fe1a5cad302b5535030cdf62895e79512713d738

>  [gdb/testsuite] Log wait status on process no longer exists error

> 

> That commit introduces a new "eof" block in gdb_test_multiple.  That

> is not incorrect itself, but dejagnu's remote_expect is picking that

> block as the "default" action when an error is raised from within the

> commands inside a call to gdb_test_multiple:

> 

>   # remote_expect works basically the same as standard expect, but it

>   # also takes care of getting the file descriptor from the specified

>   # host and also calling the timeout/eof/default section if there is 

> an

>   # error on the expect call.

>   #

>   proc remote_expect { board timeout args } {

> 

> I find that "feature" surprising, and I don't really know why it

> exists, but this means that the eof section that remote_expect picks

> as the error block can be executed even when there was no actual eof

> and the GDB process is still running, so the wait introduced in the

> commit that tries to get the exit status of GDB hangs forever, while

> GDB itself waits for input.

> 

> This only happens when there are internal testsuite errors (not

> testcase failures).  This can be reproduced easily with a testcase

> such as:

> 

>   gdb_start

>   gdb_test_multiple "show version" "show version" {

>     -re ".*" {

>        error "forced error"

>     }

>   }

> 

> I think that working around this in GDB is useful so that the

> testsuite doesn't hang in these cases.

> 

> Adding an empty "default" block at the end of the expect body in

> gdb_test_multiple doesn't work, because dejagnu gives preference to

> "eof" blocks:

> 

> 	    if { $x eq "eof" } {

> 		set save_next 1

> 	    } elseif { $x eq "default" || $x eq "timeout" } {

> 		if { $error_sect eq "" } {

> 		    set save_next 1

> 		}

> 	    }

> 

> And we do have "eof" blocks.  So we need to make sure that the last

> "eof" block is safe to use as the default error block.  It's also

> pedantically incorrect to print

> 

>  "ERROR: Process no longer exists"

> 

> which is what we'd get if the last eof block we have was selected

> (more below on this).

> 

> So this commit solves this by appending an "eof" with an empty

> spawn_id list, so that it won't ever match.

> 

> Now, why is the first "eof" block selected today as the error block,

> instead of the last one?

> 

> The reason is that remote_expect, while parsing the body to select the

> default block to execute after an error, is affected by the comments

> in the body (since they are also parsed).

> 

> If this comment in gdb_test_multiple

> 

>  # patterns below apply to any spawn id specified.

> 

> is changed to

> 

>  # The patterns below apply to any spawn id specified.

> 

> then the second eof block is selected and there is no hang.

> 

> Any comment at that same place with an even-numbered of tokens also

> works.

> 

> This is IMO a coincidence caused by how comments work in TCL.

> Comments should only appear in places where a command can appear.  And

> here, remote_expect is parsing a list of options, not commands, so

> it's not unreasonable to not parse comments, similarly to how this:

> 

>   set a_list {

>      an_element

>      # another_element

>   }

> 

> results in a list with three elements, not one element.

> 

> The fact that comments with an even number of tokens work is just a

> coincidence of how remote_expect's little state machine is

> implemented.

> 

> I thought we could solve this by stripping out comment lines in

> gdb_expect, but I didn't find an easy way to do that.  Particularly, a

> couple naive approaches I tried run into complications.  For example,

> we have gdb_test calls with regular expressions that include sequences

> like "\r\n#", and by the time we get to gdb_expect, the \r\n have

> already been expanded to a real newline, so just splitting the whole

> body at newline boundaries, looking for lines that start with #

> results in incorrectly stripping out half of the gdb_text regexp.  I

> think it's better (at least in this commit), to move the comments out

> of the list, because it's much simpler and risk free.


Wow, this is top-notch TCL/expect/DejaGnu investigation skills.

I agree with just moving the comments out, since that's the correct 
thing to do.  Trying to strip them programmatically just adds some dark 
magic that can potentially introduce more weird behaviors.

But we need to remember this when writing tests, should we have an entry 
in the testcase wiki page?

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook

Simon
Pedro Franco de Carvalho March 22, 2019, 8:44 p.m. | #2
Pedro Alves <palves@redhat.com> writes:

Hello,

> I spent a while trying to fix this, and I came up with the patch below.

>

> WDYT?


Makes sense to me.

> So this commit solves this by appending an "eof" with an empty

> spawn_id list, so that it won't ever match.


This is a clever solution, but just to be sure, can we actually rely on
this behavior when the list is empty?  This did work when I tested it,
but could some Expect version conceivably do something else like
returning an error when parsing a body with a "-i" that uses an empty
list?

> gdb/ChangeLog:

> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>


I think you meant gdb/testsuite/ChangeLog

Thanks for the patch!

--
Pedro Franco de Carvalho
Pedro Alves March 25, 2019, 1:21 p.m. | #3
On 03/22/2019 08:44 PM, Pedro Franco de Carvalho wrote:
> Pedro Alves <palves@redhat.com> writes:

> 

> Hello,

> 

>> I spent a while trying to fix this, and I came up with the patch below.

>>

>> WDYT?

> 

> Makes sense to me.

> 

>> So this commit solves this by appending an "eof" with an empty

>> spawn_id list, so that it won't ever match.

> 

> This is a clever solution, but just to be sure, can we actually rely on

> this behavior when the list is empty?  This did work when I tested it,

> but could some Expect version conceivably do something else like

> returning an error when parsing a body with a "-i" that uses an empty

> list?


I'd think not, but I can't predict the future so I can't be sure,
of course.

I looked at expect's source code a little and (in my untrained eyes)
it didn't seem like there would be a reason for it not to work,
since -i's argument is just read as a list.  I think a reasonable use
case would be to allow an empty variable as spawn id, so that
some patterns would be disabled depending on the variable being
empty or not (e.g., "-i $my_spawn_id_if_any").

When I thought of this trick, I was going to use an indirect
spawn id:

 The -i flag may also name a global variable in which case the variable
 is read for a list of spawn ids. The variable is reread whenever it changes.
 This provides a way of changing the I/O source while the command is in
 execution. Spawn ids provided this way are called "indirect" spawn ids. 

and having that global variable be an empty list.  That came to mind
since I had experience with using indirect spawn ids in another case
when I needed an empty spawn id list:

    # Use an indirect spawn id list, and remove the inferior spawn id
    # from the expected output as soon as it matches, in case
    # $inferior_pattern happens to be a prefix of the resulting full
    # gdb pattern below (e.g., "\r\n").
    global gdb_test_stdio_spawn_id_list
    set gdb_test_stdio_spawn_id_list "$inferior_spawn_id"

And then I realized the to-the-point '-i ""' would work just as well.

I think that if this stops working, likely either empty variable or
indirect spawn id pointing at an empty list would still keep working,
so we would likely be able to switch to one of those.

Maybe meanwhile, we could ask on the dejagnu list what's the purpose
of this picking one of eof/timeout/default as an error block, see if
that could be removed.  I'd guess that dejagnu would change faster
than expect here, or maybe I should say, less slowly.  :-)

Thanks,
Pedro Alves
Pedro Alves March 25, 2019, 1:23 p.m. | #4
On 03/22/2019 02:42 PM, Simon Marchi wrote:
> On 2019-03-22 08:39, Pedro Alves wrote:

>> Hi Pedro,

>>

>> Thanks for the detailed report.

>>

>> On 03/07/2019 05:42 PM, Pedro Franco de Carvalho wrote:

>>

>>> I'm not sure if there is a proper way to work around this in GDB, but it

>>> would be useful so that the testsuite doesn't hang in these cases.

>>

>> I spent a while trying to fix this, and I came up with the patch below.

>>

>> WDYT?

>>

>> From 17e8f28072cce040524eab7b85b8767764a54cd2 Mon Sep 17 00:00:00 2001

>> From: Pedro Alves <palves@redhat.com>

>> Date: Fri, 22 Mar 2019 11:33:19 +0000

>> Subject: [PATCH] Fix testsuite hangs when gdb_test_multiple body errors out

>>

>> This commit fixes a regression in the testsuite itself, triggered by

>> errors being raised from within gdb_test_multiple, originally reported

>> by Pedro Franco de Carvalho's at

>> <https://sourceware.org/ml/gdb-patches/2019-03/msg00160.html>.  Parts

>> of the commit message are based on his report.

>>

>> This started happening due to a commit that was introduced recently,

>> and it can cause the testsuite to hang.

>>

>> The commit that triggers this is:

>>

>>  fe1a5cad302b5535030cdf62895e79512713d738

>>  [gdb/testsuite] Log wait status on process no longer exists error

>>

>> That commit introduces a new "eof" block in gdb_test_multiple.  That

>> is not incorrect itself, but dejagnu's remote_expect is picking that

>> block as the "default" action when an error is raised from within the

>> commands inside a call to gdb_test_multiple:

>>

>>   # remote_expect works basically the same as standard expect, but it

>>   # also takes care of getting the file descriptor from the specified

>>   # host and also calling the timeout/eof/default section if there is an

>>   # error on the expect call.

>>   #

>>   proc remote_expect { board timeout args } {

>>

>> I find that "feature" surprising, and I don't really know why it

>> exists, but this means that the eof section that remote_expect picks

>> as the error block can be executed even when there was no actual eof

>> and the GDB process is still running, so the wait introduced in the

>> commit that tries to get the exit status of GDB hangs forever, while

>> GDB itself waits for input.

>>

>> This only happens when there are internal testsuite errors (not

>> testcase failures).  This can be reproduced easily with a testcase

>> such as:

>>

>>   gdb_start

>>   gdb_test_multiple "show version" "show version" {

>>     -re ".*" {

>>        error "forced error"

>>     }

>>   }

>>

>> I think that working around this in GDB is useful so that the

>> testsuite doesn't hang in these cases.

>>

>> Adding an empty "default" block at the end of the expect body in

>> gdb_test_multiple doesn't work, because dejagnu gives preference to

>> "eof" blocks:

>>

>>         if { $x eq "eof" } {

>>         set save_next 1

>>         } elseif { $x eq "default" || $x eq "timeout" } {

>>         if { $error_sect eq "" } {

>>             set save_next 1

>>         }

>>         }

>>

>> And we do have "eof" blocks.  So we need to make sure that the last

>> "eof" block is safe to use as the default error block.  It's also

>> pedantically incorrect to print

>>

>>  "ERROR: Process no longer exists"

>>

>> which is what we'd get if the last eof block we have was selected

>> (more below on this).

>>

>> So this commit solves this by appending an "eof" with an empty

>> spawn_id list, so that it won't ever match.

>>

>> Now, why is the first "eof" block selected today as the error block,

>> instead of the last one?

>>

>> The reason is that remote_expect, while parsing the body to select the

>> default block to execute after an error, is affected by the comments

>> in the body (since they are also parsed).

>>

>> If this comment in gdb_test_multiple

>>

>>  # patterns below apply to any spawn id specified.

>>

>> is changed to

>>

>>  # The patterns below apply to any spawn id specified.

>>

>> then the second eof block is selected and there is no hang.

>>

>> Any comment at that same place with an even-numbered of tokens also

>> works.

>>

>> This is IMO a coincidence caused by how comments work in TCL.

>> Comments should only appear in places where a command can appear.  And

>> here, remote_expect is parsing a list of options, not commands, so

>> it's not unreasonable to not parse comments, similarly to how this:

>>

>>   set a_list {

>>      an_element

>>      # another_element

>>   }

>>

>> results in a list with three elements, not one element.

>>

>> The fact that comments with an even number of tokens work is just a

>> coincidence of how remote_expect's little state machine is

>> implemented.

>>

>> I thought we could solve this by stripping out comment lines in

>> gdb_expect, but I didn't find an easy way to do that.  Particularly, a

>> couple naive approaches I tried run into complications.  For example,

>> we have gdb_test calls with regular expressions that include sequences

>> like "\r\n#", and by the time we get to gdb_expect, the \r\n have

>> already been expanded to a real newline, so just splitting the whole

>> body at newline boundaries, looking for lines that start with #

>> results in incorrectly stripping out half of the gdb_text regexp.  I

>> think it's better (at least in this commit), to move the comments out

>> of the list, because it's much simpler and risk free.

> 

> Wow, this is top-notch TCL/expect/DejaGnu investigation skills.


Thanks.  Team work here, the initial analysis came from
Pedro Franco de Carvalho.

> I agree with just moving the comments out, since that's the correct thing to do.  Trying to strip them programmatically just adds some dark magic that can potentially introduce more weird behaviors.

> 

> But we need to remember this when writing tests, should we have an entry in the testcase wiki page?

> 

> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook


That might be a good idea.

I'm merging the patch to master now.

Thanks,
Pedro Alves
Pedro Franco de Carvalho March 25, 2019, 7:43 p.m. | #5
Pedro Alves <palves@redhat.com> writes:

> On 03/22/2019 08:44 PM, Pedro Franco de Carvalho wrote:

>> Pedro Alves <palves@redhat.com> writes:

>> 

>> Hello,

>> 

>>> I spent a while trying to fix this, and I came up with the patch below.

>>>

>>> WDYT?

>> 

>> Makes sense to me.

>> 

>>> So this commit solves this by appending an "eof" with an empty

>>> spawn_id list, so that it won't ever match.

>> 

>> This is a clever solution, but just to be sure, can we actually rely on

>> this behavior when the list is empty?  This did work when I tested it,

>> but could some Expect version conceivably do something else like

>> returning an error when parsing a body with a "-i" that uses an empty

>> list?

>

> I'd think not, but I can't predict the future so I can't be sure,

> of course.

>

> I looked at expect's source code a little and (in my untrained eyes)

> it didn't seem like there would be a reason for it not to work,

> since -i's argument is just read as a list.  I think a reasonable use

> case would be to allow an empty variable as spawn id, so that

> some patterns would be disabled depending on the variable being

> empty or not (e.g., "-i $my_spawn_id_if_any").

>

> When I thought of this trick, I was going to use an indirect

> spawn id:

>

>  The -i flag may also name a global variable in which case the variable

>  is read for a list of spawn ids. The variable is reread whenever it changes.

>  This provides a way of changing the I/O source while the command is in

>  execution. Spawn ids provided this way are called "indirect" spawn ids. 

>

> and having that global variable be an empty list.  That came to mind

> since I had experience with using indirect spawn ids in another case

> when I needed an empty spawn id list:

>

>     # Use an indirect spawn id list, and remove the inferior spawn id

>     # from the expected output as soon as it matches, in case

>     # $inferior_pattern happens to be a prefix of the resulting full

>     # gdb pattern below (e.g., "\r\n").

>     global gdb_test_stdio_spawn_id_list

>     set gdb_test_stdio_spawn_id_list "$inferior_spawn_id"

>

> And then I realized the to-the-point '-i ""' would work just as well.

>

> I think that if this stops working, likely either empty variable or

> indirect spawn id pointing at an empty list would still keep working,

> so we would likely be able to switch to one of those.


Ok! Makes sense to me, thanks for the explanation.

> Maybe meanwhile, we could ask on the dejagnu list what's the purpose

> of this picking one of eof/timeout/default as an error block, see if

> that could be removed.  I'd guess that dejagnu would change faster

> than expect here, or maybe I should say, less slowly.  :-)


I can do that if you want, and let them know about the comment tokens
affecting the error block selection too.

>

> Thanks,

> Pedro Alves


Thanks for fixing this!

--
Pedro Franco de Carvalho
Pedro Alves March 26, 2019, 6:58 p.m. | #6
On 03/25/2019 07:43 PM, Pedro Franco de Carvalho wrote:
> Pedro Alves <palves@redhat.com> writes:


>> Maybe meanwhile, we could ask on the dejagnu list what's the purpose

>> of this picking one of eof/timeout/default as an error block, see if

>> that could be removed.  I'd guess that dejagnu would change faster

>> than expect here, or maybe I should say, less slowly.  :-)

> 

> I can do that if you want, and let them know about the comment tokens

> affecting the error block selection too.


I've done that today, after digging into expect's sources, to see how
it manages to handle comments.

[Why does remote_expect call the timeout/eof/default section if there is an error on the expect call?]
http://lists.gnu.org/archive/html/dejagnu/2019-03/msg00010.html

[remote_expect gets confused by comments, unlike raw expect]
http://lists.gnu.org/archive/html/dejagnu/2019-03/msg00011.html

Thanks,
Pedro Alves
Pedro Franco de Carvalho March 26, 2019, 9:01 p.m. | #7
Pedro Alves <palves@redhat.com> writes:

> I've done that today, after digging into expect's sources, to see how

> it manages to handle comments.


Thanks!

Should this patch also be included in the 8.3 branch?  I haven't seen it
there, but the patch that triggered the issue is.

--
Pedro Franco de Carvalho
Joel Brobecker March 28, 2019, 5:36 p.m. | #8
> > I've done that today, after digging into expect's sources, to see how

> > it manages to handle comments.

> 

> Should this patch also be included in the 8.3 branch?  I haven't seen it

> there, but the patch that triggered the issue is.


Pedro and I agreed it was safe for 8.3 at:
    https://www.sourceware.org/ml/gdb-patches/2019-03/msg00619.html
    https://www.sourceware.org/ml/gdb-patches/2019-03/msg00644.html

So I re-tested the patch in the gdb-8.3-branch, and then pushed it.
    https://www.sourceware.org/ml/gdb-cvs/2019-03/msg00192.html

-- 
Joel

Patch

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 6800c74187..4600d2c347 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -906,10 +906,13 @@  proc gdb_test_multiple { command message user_code } {
 	}
     }
     append code $processed_code
+
+    # Reset the spawn id, in case the processed code used -i.
     append code {
-	# Reset the spawn id, in case the processed code used -i.
 	-i "$gdb_spawn_id"
+    }
 
+    append code {
 	-re "Ending remote debugging.*$gdb_prompt $" {
 	    if ![isnative] then {
 		warning "Can`t communicate to remote target."
@@ -990,8 +993,10 @@  proc gdb_test_multiple { command message user_code } {
 	    }
 	    return -1
 	}
+    }
 
-	# Patterns below apply to any spawn id specified.
+    # Now patterns that apply to any spawn id specified.
+    append code {
 	-i $any_spawn_id
 	eof {
 	    perror "Process no longer exists"
@@ -1013,6 +1018,20 @@  proc gdb_test_multiple { command message user_code } {
 	}
     }
 
+    # remote_expect calls the eof section if there is an error on the
+    # expect call.  We already have "eof" sections above, and we don't
+    # want them to get called in that situation.  Since the last "eof"
+    # section becomes the error section, here we define another "eof"
+    # section, but with an empty spawn_id list, so that it won't ever
+    # match anything.
+    append code {
+	-i "" eof {
+	    # This comment is here because the eof section must not be
+	    # the empty string, otherwise remote_expect won't realize
+	    # it exists.
+	}
+    }
+
     set result 0
     set code [catch {gdb_expect $code} string]
     if {$code == 1} {