diff mbox series

[1/2] gdb/testsuite: make gdb_test_multiple return immediately if send_gdb fails

Message ID 20221122155546.599061-1-simon.marchi@efficios.com
State Committed
Commit d56614a9925791b15d7052eb4c798ab24a724611
Headers
Series [1/2] gdb/testsuite: make gdb_test_multiple return immediately if send_gdb fails |

Commit Message

Simon Marchi Nov. 22, 2022, 3:55 p.m. UTC 
  From: Simon Marchi <simon.marchi@polymtl.ca>

In the failure seen by Philippe here:

  https://inbox.sourceware.org/gdb-patches/20221120173024.3647464-1-philippe.waroquiers@skynet.be/

... the testsuite only outputs PASSes, and an ERROR, resulting from an
uncaught exception.  This is a bit sneaky, because ERRORs are not
reported in the test summary.  In certain circumstances, it can be easy
to miss.

Normally, gdb_test_multiple outputs an UNRESOLVED when GDB crashes.  But
this is only if it manages to send the command, and it's that command
that crashes GDB.  Here, the ERROR is due to the fact that GDB had
already crashed by the time we entered gdb_test_multiple and tried to
send a command.  GDB was crashed by the previous "file" command, sent by
gdb_unload.  Because gdb_unload uses bare expect, it didn't record a
test failure when crashing GDB (this will be addressed separately).

In this patch, I propose to make gdb_test_multiple call unresolved
directly and return -1 send_gdb fails.  This way, if GDB is already
crashed by the time we enter gdb_test_multiple, it will leave a trace in
the test results in the form of an UNRESOLVED.  It will also spare us of
the not-so-useful-in-my-opinion TCL backtrace.

Before, it looks like:

    ERROR: Couldn't send python print(objfile.filename) to GDB.
    ERROR: : spawn id exp9 not open
        while executing
    "expect {
    -i exp9 -timeout 10
            -re ".*A problem internal to GDB has been detected" {
                fail "$message (GDB internal error)"
                gdb_internal_error..."
        ("uplevel" body line 1)
        invoked from within
    "uplevel $body" NONE : spawn id exp9 not open

And after:

    Couldn't send python print(objfile.filename) to GDB.
    UNRESOLVED: gdb.python/py-objfile.exp: objfile.filename after objfile is unloaded

Change-Id: I72af8dc0d687826fc3f76911c27a9e5f91b677ba
---
 gdb/testsuite/lib/gdb.exp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Philippe Waroquiers Nov. 23, 2022, 10:59 a.m. UTC | #1 
Thanks for this, it will for sure help me in the future to not miss anymore
some regressions I introduce.

One small question:
Is a GDB crash the only reason for which we could have a message such as:
   Couldn't send python print(objfile.filename) to GDB.
?
Possibly better then to indicate that GDB crashed (on an internal error) ?
Or if there are some other reasons, it might be good to give more details about the
reason ?
Note that if this is not straightforward to do, not a big deal
as just making the problem visible with UNRESOLVED should draw the attention
and the detailed log will show the crash I guess.

Thanks
Philippe


On Tue, 2022-11-22 at 10:55 -0500, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> In the failure seen by Philippe here:
> 
>   https://inbox.sourceware.org/gdb-patches/20221120173024.3647464-1-philippe.waroquiers@skynet.be/
> 
> ... the testsuite only outputs PASSes, and an ERROR, resulting from an
> uncaught exception.  This is a bit sneaky, because ERRORs are not
> reported in the test summary.  In certain circumstances, it can be easy
> to miss.
> 
> Normally, gdb_test_multiple outputs an UNRESOLVED when GDB crashes.  But
> this is only if it manages to send the command, and it's that command
> that crashes GDB.  Here, the ERROR is due to the fact that GDB had
> already crashed by the time we entered gdb_test_multiple and tried to
> send a command.  GDB was crashed by the previous "file" command, sent by
> gdb_unload.  Because gdb_unload uses bare expect, it didn't record a
> test failure when crashing GDB (this will be addressed separately).
> 
> In this patch, I propose to make gdb_test_multiple call unresolved
> directly and return -1 send_gdb fails.  This way, if GDB is already
> crashed by the time we enter gdb_test_multiple, it will leave a trace in
> the test results in the form of an UNRESOLVED.  It will also spare us of
> the not-so-useful-in-my-opinion TCL backtrace.
> 
> Before, it looks like:
> 
>     ERROR: Couldn't send python print(objfile.filename) to GDB.
>     ERROR: : spawn id exp9 not open
>         while executing
>     "expect {
>     -i exp9 -timeout 10
>             -re ".*A problem internal to GDB has been detected" {
>                 fail "$message (GDB internal error)"
>                 gdb_internal_error..."
>         ("uplevel" body line 1)
>         invoked from within
>     "uplevel $body" NONE : spawn id exp9 not open
> 
> And after:
> 
>     Couldn't send python print(objfile.filename) to GDB.
>     UNRESOLVED: gdb.python/py-objfile.exp: objfile.filename after objfile is unloaded
> 
> Change-Id: I72af8dc0d687826fc3f76911c27a9e5f91b677ba
> ---
>  gdb/testsuite/lib/gdb.exp | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 7d05fbe557bb..fcd54c88f251 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -1161,7 +1161,9 @@ proc gdb_test_multiple { command message args } {
>  	    if { $foo < [expr $len - 1] } {
>  		set str [string range "$string" 0 $foo]
>  		if { [send_gdb "$str"] != "" } {
> -		    perror "Couldn't send $command to GDB."
> +		    verbose -log "Couldn't send $command to GDB."
> +		    unresolved $message
> +		    return -1
>  		}
>  		# since we're checking if each line of the multi-line
>  		# command are 'accepted' by GDB here,
> @@ -1180,7 +1182,9 @@ proc gdb_test_multiple { command message args } {
>  	}
>  	if { "$string" != "" } {
>  	    if { [send_gdb "$string"] != "" } {
> -		perror "Couldn't send $command to GDB."
> +		verbose -log "Couldn't send $command to GDB."
> +		unresolved $message
> +		return -1
>  	    }
>  	}
>      }
Simon Marchi Nov. 23, 2022, 2:12 p.m. UTC | #2 
On 11/23/22 05:59, Philippe Waroquiers wrote:
> Thanks for this, it will for sure help me in the future to not miss anymore
> some regressions I introduce.
> 
> One small question:
> Is a GDB crash the only reason for which we could have a message such as:
>    Couldn't send python print(objfile.filename) to GDB.
> ?
> Possibly better then to indicate that GDB crashed (on an internal error) ?
> Or if there are some other reasons, it might be good to give more details about the
> reason ?

If send_gdb fails, it's generally because the underlying send (the
expect proc) has failed, because GDB's spawn id is no longer valid.  And
that is normally due to a crash.  But the reality is that we do not know
for sure, it could also be a testsuite problem, where we closed GDB and
failed to reinitialize $gdb_spawn_id.  So saying GDB crashed could also
give you a wrong lead.  I prefer to stick with what we know.

> Note that if this is not straightforward to do, not a big deal
> as just making the problem visible with UNRESOLVED should draw the attention
> and the detailed log will show the crash I guess.

I think that's the important part, it's the previous command that caused
the crash, that one should show up in the logs.  Looking at lib/gdb.exp
there are many more uses of gdb_expect that could be converted to
gdb_test_multiple.

Simon
Tom de Vries Nov. 29, 2022, 4:21 p.m. UTC | #3 
On 11/22/22 16:55, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> In the failure seen by Philippe here:
> 
>    https://inbox.sourceware.org/gdb-patches/20221120173024.3647464-1-philippe.waroquiers@skynet.be/
> 
> ... the testsuite only outputs PASSes, and an ERROR, resulting from an
> uncaught exception.  This is a bit sneaky, because ERRORs are not
> reported in the test summary.  In certain circumstances, it can be easy
> to miss.
> 
> Normally, gdb_test_multiple outputs an UNRESOLVED when GDB crashes.  But > this is only if it manages to send the command, and it's that command
> that crashes GDB.  Here, the ERROR is due to the fact that GDB had
> already crashed by the time we entered gdb_test_multiple and tried to
> send a command.  GDB was crashed by the previous "file" command, sent by
> gdb_unload.  Because gdb_unload uses bare expect, it didn't record a
> test failure when crashing GDB (this will be addressed separately).
> 
> In this patch, I propose to make gdb_test_multiple call unresolved
> directly and return -1 send_gdb fails.  This way, if GDB is already
> crashed by the time we enter gdb_test_multiple, it will leave a trace in
> the test results in the form of an UNRESOLVED.  It will also spare us of

spare us of the -> spare us the ?

> the not-so-useful-in-my-opinion TCL backtrace.
> 

Agreed.

> Before, it looks like:
> 
>      ERROR: Couldn't send python print(objfile.filename) to GDB.
>      ERROR: : spawn id exp9 not open
>          while executing
>      "expect {
>      -i exp9 -timeout 10
>              -re ".*A problem internal to GDB has been detected" {
>                  fail "$message (GDB internal error)"
>                  gdb_internal_error..."
>          ("uplevel" body line 1)
>          invoked from within
>      "uplevel $body" NONE : spawn id exp9 not open
> 
> And after:
> 
>      Couldn't send python print(objfile.filename) to GDB.
>      UNRESOLVED: gdb.python/py-objfile.exp: objfile.filename after objfile is unloaded
> 

LGTM, thanks for doing this.

Thanks,
- Tom

> Change-Id: I72af8dc0d687826fc3f76911c27a9e5f91b677ba
> ---
>   gdb/testsuite/lib/gdb.exp | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 7d05fbe557bb..fcd54c88f251 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -1161,7 +1161,9 @@ proc gdb_test_multiple { command message args } {
>   	    if { $foo < [expr $len - 1] } {
>   		set str [string range "$string" 0 $foo]
>   		if { [send_gdb "$str"] != "" } {
> -		    perror "Couldn't send $command to GDB."
> +		    verbose -log "Couldn't send $command to GDB."
> +		    unresolved $message
> +		    return -1
>   		}
>   		# since we're checking if each line of the multi-line
>   		# command are 'accepted' by GDB here,
> @@ -1180,7 +1182,9 @@ proc gdb_test_multiple { command message args } {
>   	}
>   	if { "$string" != "" } {
>   	    if { [send_gdb "$string"] != "" } {
> -		perror "Couldn't send $command to GDB."
> +		verbose -log "Couldn't send $command to GDB."
> +		unresolved $message
> +		return -1
>   	    }
>   	}
>       }
Simon Marchi Nov. 29, 2022, 4:41 p.m. UTC | #4 
On 11/29/22 11:21, Tom de Vries wrote:
> On 11/22/22 16:55, Simon Marchi via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> In the failure seen by Philippe here:
>>
>>    https://inbox.sourceware.org/gdb-patches/20221120173024.3647464-1-philippe.waroquiers@skynet.be/
>>
>> ... the testsuite only outputs PASSes, and an ERROR, resulting from an
>> uncaught exception.  This is a bit sneaky, because ERRORs are not
>> reported in the test summary.  In certain circumstances, it can be easy
>> to miss.
>>
>> Normally, gdb_test_multiple outputs an UNRESOLVED when GDB crashes.  But > this is only if it manages to send the command, and it's that command
>> that crashes GDB.  Here, the ERROR is due to the fact that GDB had
>> already crashed by the time we entered gdb_test_multiple and tried to
>> send a command.  GDB was crashed by the previous "file" command, sent by
>> gdb_unload.  Because gdb_unload uses bare expect, it didn't record a
>> test failure when crashing GDB (this will be addressed separately).
>>
>> In this patch, I propose to make gdb_test_multiple call unresolved
>> directly and return -1 send_gdb fails.  This way, if GDB is already
>> crashed by the time we enter gdb_test_multiple, it will leave a trace in
>> the test results in the form of an UNRESOLVED.  It will also spare us of
> 
> spare us of the -> spare us the ?

Fixed.

>> the not-so-useful-in-my-opinion TCL backtrace.
>>
> 
> Agreed.
> 
>> Before, it looks like:
>>
>>      ERROR: Couldn't send python print(objfile.filename) to GDB.
>>      ERROR: : spawn id exp9 not open
>>          while executing
>>      "expect {
>>      -i exp9 -timeout 10
>>              -re ".*A problem internal to GDB has been detected" {
>>                  fail "$message (GDB internal error)"
>>                  gdb_internal_error..."
>>          ("uplevel" body line 1)
>>          invoked from within
>>      "uplevel $body" NONE : spawn id exp9 not open
>>
>> And after:
>>
>>      Couldn't send python print(objfile.filename) to GDB.
>>      UNRESOLVED: gdb.python/py-objfile.exp: objfile.filename after objfile is unloaded
>>
> 
> LGTM, thanks for doing this.

Thanks, will push.

Simon
diff mbox series

Patch

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 7d05fbe557bb..fcd54c88f251 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1161,7 +1161,9 @@  proc gdb_test_multiple { command message args } {
 	    if { $foo < [expr $len - 1] } {
 		set str [string range "$string" 0 $foo]
 		if { [send_gdb "$str"] != "" } {
-		    perror "Couldn't send $command to GDB."
+		    verbose -log "Couldn't send $command to GDB."
+		    unresolved $message
+		    return -1
 		}
 		# since we're checking if each line of the multi-line
 		# command are 'accepted' by GDB here,
@@ -1180,7 +1182,9 @@  proc gdb_test_multiple { command message args } {
 	}
 	if { "$string" != "" } {
 	    if { [send_gdb "$string"] != "" } {
-		perror "Couldn't send $command to GDB."
+		verbose -log "Couldn't send $command to GDB."
+		unresolved $message
+		return -1
 	    }
 	}
     }