[PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Tom de Vries]

Hi,

There are two problems with the current catch-follow-exec.exp:
- the datadir indicated by INTERNAL_GDBFLAGS is not used
- remote host testing doesn't work correctly

Rewrite catch-follow-exec.c to use gdb_spawn_with_cmdline_opts, fixing both
problems.

Build on x86_64-linux with and without ubsan, tested with and without
--target_board=native-gdbserver.

OK for trunk?

Thanks,
- Tom

[gdb/testsuite] Rewrite catch-follow-exec.exp

2018-10-05  Tom de Vries  <tdevries@suse.de>

	PR gdb/23730
	* gdb.base/catch-follow-exec.c: Add copyright notice.
	* gdb.base/catch-follow-exec.exp: Rewrite to use
	gdb_spawn_with_cmdline_opts.

---
 gdb/testsuite/gdb.base/catch-follow-exec.c   | 17 +++++++++++
 gdb/testsuite/gdb.base/catch-follow-exec.exp | 44 ++++++++++++++++++++--------
 2 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.c b/gdb/testsuite/gdb.base/catch-follow-exec.c
index fa68a2a34e..1a59f58aa5 100644
--- a/gdb/testsuite/gdb.base/catch-follow-exec.c
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.c
@@ -1,3 +1,20 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.exp b/gdb/testsuite/gdb.base/catch-follow-exec.exp
index 0e32ed4a6f..6f977fef93 100644
--- a/gdb/testsuite/gdb.base/catch-follow-exec.exp
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.exp
@@ -22,11 +22,6 @@ if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
     return -1
 }
 
-if { ![remote_file target exists /bin/bash] } {
-    unsupported "no bash"
-    return
-}
-
 if { ![remote_file target exists /bin/ls] } {
     unsupported "no ls"
     return
@@ -34,24 +29,49 @@ if { ![remote_file target exists /bin/ls] } {
 
 proc catch_follow_exec { } {
     global binfile
-    global GDB
+    global gdb_spawn_id
 
     set test "catch-follow-exec"
 
     append FLAGS " \"$binfile\""
     append FLAGS " -batch"
+    append FLAGS " -ex \"target native\""
     append FLAGS " -ex \"catch exec\""
     append FLAGS " -ex \"set follow-exec-mode new\""
     append FLAGS " -ex \"run\""
     append FLAGS " -ex \"info prog\""
 
-    catch {exec /bin/bash -c "$GDB $FLAGS"} catchlog
-    send_log "$catchlog\n"
+    gdb_exit
+    if {[gdb_spawn_with_cmdline_opts "$FLAGS"] != 0} {
+	fail "spawn"
+	return
+    }
+
+    gdb_test_multiple "" "run til exit" {
+	"runtime error:" {
+	    # Error in case of --enable-ubsan
+	    fail "No runtime error"
+	}
+	eof {
+	    set result [wait -i $gdb_spawn_id]
+	    verbose $result
+
+	    gdb_assert { [lindex $result 2] == 0 }
+
+	    # We're not testing the "status returned by the spawned process",
+	    # because it's currently one, and we suspect it will be zero after
+	    # fixing PR23368 - "gdb goes to into background when hitting exec
+	    # catchpoint with follow-exec-mode new"
+            #gdb_assert { [lindex $result 3] == 0 }
+
+	    # Error in case of --disable-ubsan, we get
+	    # "CHILDKILLED SIGSEGV {segmentation violation}" as extra
+	    # argument(s).
+	    gdb_assert { [llength $result] == 4 }
+	}
 
-    if { [regexp {No selected thread} $catchlog] } {
-	pass $test
-    } else {
-	fail $test
+	remote_close host
+	clear_gdb_spawn_id
     }
 }
 

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Gary Benson]

Tom de Vries wrote:
>      append FLAGS " \"$binfile\""
>      append FLAGS " -batch"
> +    append FLAGS " -ex \"target native\""
>      append FLAGS " -ex \"catch exec\""
>      append FLAGS " -ex \"set follow-exec-mode new\""

I'm a little confused with this part, doesn't this force the test to
run on the host?

> +	    # We're not testing the "status returned by the spawned process",
> +	    # because it's currently one, and we suspect it will be zero after
> +	    # fixing PR23368 - "gdb goes to into background when hitting exec
> +	    # catchpoint with follow-exec-mode new"
> +            #gdb_assert { [lindex $result 3] == 0 }

I'm not sure we should commit commented-out code.  Why not have the
test assert { [lindex $result 3] == 1 } if that's what's happening
now, with the comment reworded to indicate that it might need changing
to zero when PR23368 is fixed.  That way, when PR23368 *is* fixed,
whoever's fixing it gets a failing test, they investigate, find the
comment, and update it as part of their series.

Everything else looks good.

Cheers,
Gary

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Tom de Vries]

On 10/9/18 3:51 PM, Gary Benson wrote:
> Tom de Vries wrote:
>>      append FLAGS " \"$binfile\""
>>      append FLAGS " -batch"
>> +    append FLAGS " -ex \"target native\""
>>      append FLAGS " -ex \"catch exec\""
>>      append FLAGS " -ex \"set follow-exec-mode new\""
> 
> I'm a little confused with this part, doesn't this force the test to
> run on the host?
> 

Hi,

thanks for the review.

The "target native" was an attempt to fix problems when running with
--target_board=native-gdbserver. Perhaps it's better to bail out in that
case, but I haven't yet figured out how to. Any advice here?

>> +	    # We're not testing the "status returned by the spawned process",
>> +	    # because it's currently one, and we suspect it will be zero after
>> +	    # fixing PR23368 - "gdb goes to into background when hitting exec
>> +	    # catchpoint with follow-exec-mode new"
>> +            #gdb_assert { [lindex $result 3] == 0 }
> 
> I'm not sure we should commit commented-out code.  Why not have the
> test assert { [lindex $result 3] == 1 } if that's what's happening
> now, with the comment reworded to indicate that it might need changing
> to zero when PR23368 is fixed.  That way, when PR23368 *is* fixed,
> whoever's fixing it gets a failing test, they investigate, find the
> comment, and update it as part of their series.
> 

Makes sense, will do.

Thanks,
- Tom

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Gary Benson]

Tom de Vries wrote:
> On 10/9/18 3:51 PM, Gary Benson wrote:
> > Tom de Vries wrote:
> > >      append FLAGS " \"$binfile\""
> > >      append FLAGS " -batch"
> > > +    append FLAGS " -ex \"target native\""
> > >      append FLAGS " -ex \"catch exec\""
> > >      append FLAGS " -ex \"set follow-exec-mode new\""
> > 
> > I'm a little confused with this part, doesn't this force the test to
> > run on the host?
> 
> The "target native" was an attempt to fix problems when running with
> --target_board=native-gdbserver. Perhaps it's better to bail out in
> that case, but I haven't yet figured out how to. Any advice here?

Tests that can't run remote usually bail with something like this at
the start:

  if ![isnative] then {
      return
  }

There should probably also be an 'append FLAGS " -nx"' too.

> > > +	    # We're not testing the "status returned by the spawned process",
> > > +	    # because it's currently one, and we suspect it will be zero after
> > > +	    # fixing PR23368 - "gdb goes to into background when hitting exec
> > > +	    # catchpoint with follow-exec-mode new"
> > > +            #gdb_assert { [lindex $result 3] == 0 }
> > 
> > I'm not sure we should commit commented-out code.  Why not have the
> > test assert { [lindex $result 3] == 1 } if that's what's happening
> > now, with the comment reworded to indicate that it might need changing
> > to zero when PR23368 is fixed.  That way, when PR23368 *is* fixed,
> > whoever's fixing it gets a failing test, they investigate, find the
> > comment, and update it as part of their series.
> > 
> 
> Makes sense, will do.

I'm guessing this whole function could be replaced with something more
regular (which would work remote) once PR23368 is fixed?  Something
like this:

  clean_restart ${binfile}
  gdb_test "catch exec" "Catchpoint \[0-9\]+ \\\(exec\\\).*"
  gdb_test "set follow-exec-mode new"
  gdb_run_cmd
   ...

If that is the case, could you note that in that comment?  Or just
paste the URL of this thread in the archive.

I'm happy with this patch with these changes, but I'm not a maintainer
so one of those guys will have to give the final approval.

Thanks,
Gary

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Simon Marchi]

On 2018-10-10 05:27, Gary Benson wrote:
> Tom de Vries wrote:
>> On 10/9/18 3:51 PM, Gary Benson wrote:
>> > Tom de Vries wrote:
>> > >      append FLAGS " \"$binfile\""
>> > >      append FLAGS " -batch"
>> > > +    append FLAGS " -ex \"target native\""
>> > >      append FLAGS " -ex \"catch exec\""
>> > >      append FLAGS " -ex \"set follow-exec-mode new\""
>> >
>> > I'm a little confused with this part, doesn't this force the test to
>> > run on the host?
>> 
>> The "target native" was an attempt to fix problems when running with
>> --target_board=native-gdbserver. Perhaps it's better to bail out in
>> that case, but I haven't yet figured out how to. Any advice here?
> 
> Tests that can't run remote usually bail with something like this at
> the start:
> 
>   if ![isnative] then {
>       return
>   }

I have not looked at the test (I can do it latter today if necessary), 
but this comment caught my attention.  isnative is likely not what you 
want to use, make sure to read the "Local vs Remote vs Native" section 
of the gdb/testsuite/README file.

Simon

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Gary Benson]

Simon Marchi wrote:
> On 2018-10-10 05:27, Gary Benson wrote:
> > Tom de Vries wrote:
> > > On 10/9/18 3:51 PM, Gary Benson wrote:
> > > > Tom de Vries wrote:
> > > > >      append FLAGS " \"$binfile\""
> > > > >      append FLAGS " -batch"
> > > > > +    append FLAGS " -ex \"target native\""
> > > > >      append FLAGS " -ex \"catch exec\""
> > > > >      append FLAGS " -ex \"set follow-exec-mode new\""
> > > >
> > > > I'm a little confused with this part, doesn't this force the
> > > > test to run on the host?
> > >
> > > The "target native" was an attempt to fix problems when running
> > > with --target_board=native-gdbserver. Perhaps it's better to
> > > bail out in that case, but I haven't yet figured out how to. Any
> > > advice here?
> >
> > Tests that can't run remote usually bail with something like this
> > at the start:
> >
> >   if ![isnative] then {
> >       return
> >   }
> 
> I have not looked at the test (I can do it latter today if
> necessary), but this comment caught my attention.  isnative is
> likely not what you want to use, make sure to read the "Local vs
> Remote vs Native" section of the gdb/testsuite/README file.

Oh!  Ok, so [target_info gdb_protocol] != ""] maybe?
Thanks Simon!

Gary

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Tom de Vries]

On Wed, Oct 10, 2018 at 02:44:24PM +0100, Gary Benson wrote:
> Simon Marchi wrote:
> > On 2018-10-10 05:27, Gary Benson wrote:
> > > Tom de Vries wrote:
> > > > On 10/9/18 3:51 PM, Gary Benson wrote:
> > > > > Tom de Vries wrote:
> > > > > >      append FLAGS " \"$binfile\""
> > > > > >      append FLAGS " -batch"
> > > > > > +    append FLAGS " -ex \"target native\""
> > > > > >      append FLAGS " -ex \"catch exec\""
> > > > > >      append FLAGS " -ex \"set follow-exec-mode new\""
> > > > >
> > > > > I'm a little confused with this part, doesn't this force the
> > > > > test to run on the host?
> > > >
> > > > The "target native" was an attempt to fix problems when running
> > > > with --target_board=native-gdbserver. Perhaps it's better to
> > > > bail out in that case, but I haven't yet figured out how to. Any
> > > > advice here?
> > >
> > > Tests that can't run remote usually bail with something like this
> > > at the start:
> > >
> > >   if ![isnative] then {
> > >       return
> > >   }
> > 
> > I have not looked at the test (I can do it latter today if
> > necessary), but this comment caught my attention.  isnative is
> > likely not what you want to use, make sure to read the "Local vs
> > Remote vs Native" section of the gdb/testsuite/README file.
> 
> Oh!  Ok, so [target_info gdb_protocol] != ""] maybe?

Attached patch uses this this.

OK for trunk?

Thanks,
- Tom

[gdb/testsuite] Rewrite catch-follow-exec.exp

There are two problems with the current catch-follow-exec.exp:
- INTERNAL_GDBFLAGS (containing the datadir setting) is not used
- remote host testing doesn't work

Fix the former by using gdb_spawn_with_cmdline_opts.  Fix the latter by
requiring gdb-native.

Build on x86_64-linux with and without ubsan, tested with and without
--target_board=native-gdbserver.

2018-10-05  Tom de Vries  <tdevries@suse.de>

	PR gdb/23730
	* gdb.base/catch-follow-exec.c: Add copyright notice.
	* gdb.base/catch-follow-exec.exp: Rewrite to use
	gdb_spawn_with_cmdline_opts.  Require gdb-native.

---
 gdb/testsuite/gdb.base/catch-follow-exec.c   | 17 ++++++++++++
 gdb/testsuite/gdb.base/catch-follow-exec.exp | 41 ++++++++++++++++++++++------
 2 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.c b/gdb/testsuite/gdb.base/catch-follow-exec.c
index fa68a2a34e..1a59f58aa5 100644
--- a/gdb/testsuite/gdb.base/catch-follow-exec.c
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.c
@@ -1,3 +1,20 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.exp b/gdb/testsuite/gdb.base/catch-follow-exec.exp
index 0e32ed4a6f..2ccd0115e7 100644
--- a/gdb/testsuite/gdb.base/catch-follow-exec.exp
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.exp
@@ -22,8 +22,8 @@ if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
     return -1
 }
 
-if { ![remote_file target exists /bin/bash] } {
-    unsupported "no bash"
+if { [target_info gdb_protocol] != "" } {
+    unsupported "not native"
     return
 }
 
@@ -34,7 +34,7 @@ if { ![remote_file target exists /bin/ls] } {
 
 proc catch_follow_exec { } {
     global binfile
-    global GDB
+    global gdb_spawn_id
 
     set test "catch-follow-exec"
 
@@ -45,13 +45,36 @@ proc catch_follow_exec { } {
     append FLAGS " -ex \"run\""
     append FLAGS " -ex \"info prog\""
 
-    catch {exec /bin/bash -c "$GDB $FLAGS"} catchlog
-    send_log "$catchlog\n"
+    gdb_exit
+    if {[gdb_spawn_with_cmdline_opts "$FLAGS"] != 0} {
+	fail "spawn"
+	return
+    }
+
+    gdb_test_multiple "" "run til exit" {
+	"runtime error:" {
+	    # Error in case of --enable-ubsan
+	    fail "No runtime error"
+	}
+	eof {
+	    set result [wait -i $gdb_spawn_id]
+	    verbose $result
+
+	    gdb_assert { [lindex $result 2] == 0 }
+
+	    # We suspect this will be zero instead of one after fixing PR23368
+	    # - "gdb goes to into background when hitting exec catchpoint with
+	    # follow-exec-mode new"
+            gdb_assert { [lindex $result 3] == 1 }
+
+	    # Error in case of --disable-ubsan, we get
+	    # "CHILDKILLED SIGSEGV {segmentation violation}" as extra
+	    # argument(s).
+	    gdb_assert { [llength $result] == 4 }
+	}
 
-    if { [regexp {No selected thread} $catchlog] } {
-	pass $test
-    } else {
-	fail $test
+	remote_close host
+	clear_gdb_spawn_id
     }
 }
 

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Gary Benson]

Tom de Vries wrote:
> On Wed, Oct 10, 2018 at 02:44:24PM +0100, Gary Benson wrote:
> > Oh!  Ok, so [target_info gdb_protocol] != ""] maybe?
> 
> Attached patch uses this this.
> 
> OK for trunk?

Please reorder the checks at the start like this, to minimize the
work done before bailing:

  1. check gdb_protocol native
  2. check /bin/ls exists on target
  3. build_executable

The patch is ok by me with these changes, but please wait for a
maintainer to give the final approval, I don't have those powers :)
And thanks for doing the work Tom!

Cheers,
Gary

> [gdb/testsuite] Rewrite catch-follow-exec.exp
> 
> There are two problems with the current catch-follow-exec.exp:
> - INTERNAL_GDBFLAGS (containing the datadir setting) is not used
> - remote host testing doesn't work
> 
> Fix the former by using gdb_spawn_with_cmdline_opts.  Fix the latter by
> requiring gdb-native.
> 
> Build on x86_64-linux with and without ubsan, tested with and without
> --target_board=native-gdbserver.
> 
> 2018-10-05  Tom de Vries  <tdevries@suse.de>
> 
> 	PR gdb/23730
> 	* gdb.base/catch-follow-exec.c: Add copyright notice.
> 	* gdb.base/catch-follow-exec.exp: Rewrite to use
> 	gdb_spawn_with_cmdline_opts.  Require gdb-native.
> 
> ---
>  gdb/testsuite/gdb.base/catch-follow-exec.c   | 17 ++++++++++++
>  gdb/testsuite/gdb.base/catch-follow-exec.exp | 41 ++++++++++++++++++++++------
>  2 files changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.c b/gdb/testsuite/gdb.base/catch-follow-exec.c
> index fa68a2a34e..1a59f58aa5 100644
> --- a/gdb/testsuite/gdb.base/catch-follow-exec.c
> +++ b/gdb/testsuite/gdb.base/catch-follow-exec.c
> @@ -1,3 +1,20 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2018 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
>  #include <stdio.h>
>  #include <string.h>
>  #include <unistd.h>
> diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.exp b/gdb/testsuite/gdb.base/catch-follow-exec.exp
> index 0e32ed4a6f..2ccd0115e7 100644
> --- a/gdb/testsuite/gdb.base/catch-follow-exec.exp
> +++ b/gdb/testsuite/gdb.base/catch-follow-exec.exp
> @@ -22,8 +22,8 @@ if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
>      return -1
>  }
>  
> -if { ![remote_file target exists /bin/bash] } {
> -    unsupported "no bash"
> +if { [target_info gdb_protocol] != "" } {
> +    unsupported "not native"
>      return
>  }
>  
> @@ -34,7 +34,7 @@ if { ![remote_file target exists /bin/ls] } {
>  
>  proc catch_follow_exec { } {
>      global binfile
> -    global GDB
> +    global gdb_spawn_id
>  
>      set test "catch-follow-exec"
>  
> @@ -45,13 +45,36 @@ proc catch_follow_exec { } {
>      append FLAGS " -ex \"run\""
>      append FLAGS " -ex \"info prog\""
>  
> -    catch {exec /bin/bash -c "$GDB $FLAGS"} catchlog
> -    send_log "$catchlog\n"
> +    gdb_exit
> +    if {[gdb_spawn_with_cmdline_opts "$FLAGS"] != 0} {
> +	fail "spawn"
> +	return
> +    }
> +
> +    gdb_test_multiple "" "run til exit" {
> +	"runtime error:" {
> +	    # Error in case of --enable-ubsan
> +	    fail "No runtime error"
> +	}
> +	eof {
> +	    set result [wait -i $gdb_spawn_id]
> +	    verbose $result
> +
> +	    gdb_assert { [lindex $result 2] == 0 }
> +
> +	    # We suspect this will be zero instead of one after fixing PR23368
> +	    # - "gdb goes to into background when hitting exec catchpoint with
> +	    # follow-exec-mode new"
> +            gdb_assert { [lindex $result 3] == 1 }
> +
> +	    # Error in case of --disable-ubsan, we get
> +	    # "CHILDKILLED SIGSEGV {segmentation violation}" as extra
> +	    # argument(s).
> +	    gdb_assert { [llength $result] == 4 }
> +	}
>  
> -    if { [regexp {No selected thread} $catchlog] } {
> -	pass $test
> -    } else {
> -	fail $test
> +	remote_close host
> +	clear_gdb_spawn_id
>      }
>  }
>  
> 

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Simon Marchi]

On 2018-10-11 04:33, Gary Benson wrote:
> Tom de Vries wrote:
>> On Wed, Oct 10, 2018 at 02:44:24PM +0100, Gary Benson wrote:
>> > Oh!  Ok, so [target_info gdb_protocol] != ""] maybe?
>> 
>> Attached patch uses this this.
>> 
>> OK for trunk?
> 
> Please reorder the checks at the start like this, to minimize the
> work done before bailing:
> 
>   1. check gdb_protocol native
>   2. check /bin/ls exists on target
>   3. build_executable
> 
> The patch is ok by me with these changes, but please wait for a
> maintainer to give the final approval, I don't have those powers :)
> And thanks for doing the work Tom!
> 
> Cheers,
> Gary

Just wondering.  Would it make life easier if we fixed PR 23368, which 
is the reason we have to do the test in an unnatural way?  I don't know 
if the fix I proposed here is totally correct:

https://sourceware.org/bugzilla/show_bug.cgi?id=23368#c4

but I think it would be an improvement compared to what we have now.  I 
ran it through the buildbot and there are no regressions.  If you want I 
can submit a proper patch for that, and then we can rewrite this test in 
a more normal way.

Simon

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Tom de Vries]

On 10/14/18 12:18 AM, Simon Marchi wrote:
> On 2018-10-11 04:33, Gary Benson wrote:
>> Tom de Vries wrote:
>>> On Wed, Oct 10, 2018 at 02:44:24PM +0100, Gary Benson wrote:
>>> > Oh!  Ok, so [target_info gdb_protocol] != ""] maybe?
>>>
>>> Attached patch uses this this.
>>>
>>> OK for trunk?
>>
>> Please reorder the checks at the start like this, to minimize the
>> work done before bailing:
>>
>>   1. check gdb_protocol native
>>   2. check /bin/ls exists on target
>>   3. build_executable
>>
>> The patch is ok by me with these changes, but please wait for a
>> maintainer to give the final approval, I don't have those powers :)
>> And thanks for doing the work Tom!
>>
>> Cheers,
>> Gary
> 
> Just wondering.  Would it make life easier if we fixed PR 23368, which
> is the reason we have to do the test in an unnatural way?

Yes.

> I don't know
> if the fix I proposed here is totally correct:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=23368#c4
> 
> but I think it would be an improvement compared to what we have now.  I
> ran it through the buildbot and there are no regressions.  If you want I
> can submit a proper patch for that, and then we can rewrite this test in
> a more normal way.
> 

That makes sense, but also I propose to commit the patch I've submitted
(with the early-out checks reorder as Gary suggested above), since
that's also an improvement on the current situation.

OK?

Thanks,
- Tom

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Simon Marchi]

On 2018-10-15 15:54, Tom de Vries wrote:
> That makes sense, but also I propose to commit the patch I've submitted
> (with the early-out checks reorder as Gary suggested above), since
> that's also an improvement on the current situation.
> 
> OK?
> 
> Thanks,
> - Tom

Sure, I am fine with making the situation better in the mean time.  I 
have replied to your patch directly.

Simon

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Simon Marchi]

On 2018-10-11 03:47, Tom de Vries wrote:
> [gdb/testsuite] Rewrite catch-follow-exec.exp
> 
> There are two problems with the current catch-follow-exec.exp:
> - INTERNAL_GDBFLAGS (containing the datadir setting) is not used
> - remote host testing doesn't work
> 
> Fix the former by using gdb_spawn_with_cmdline_opts.  Fix the latter by
> requiring gdb-native.
> 
> Build on x86_64-linux with and without ubsan, tested with and without
> --target_board=native-gdbserver.

My build of GDB (and probably some on the buildbot too?) uses 
-fsanitize=address, and on it the test does not pass.  On a build 
without -fsanitize=address, it does pass.  The failing test is:

FAIL: gdb.base/catch-follow-exec.exp: [lindex $result 3] == 1

and the value of $result is "17872 exp10 0 23".  This is because ASan 
exits with 23 if it detects leaks. If there's a way to set 
LSAN_OPTIONS='exitcode=1' in the environment GDB runs in, it would 
probably make it work...

> 2018-10-05  Tom de Vries  <tdevries@suse.de>
> 
> 	PR gdb/23730
> 	* gdb.base/catch-follow-exec.c: Add copyright notice.
> 	* gdb.base/catch-follow-exec.exp: Rewrite to use
> 	gdb_spawn_with_cmdline_opts.  Require gdb-native.
> 
> ---
>  gdb/testsuite/gdb.base/catch-follow-exec.c   | 17 ++++++++++++
>  gdb/testsuite/gdb.base/catch-follow-exec.exp | 41 
> ++++++++++++++++++++++------
>  2 files changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.c
> b/gdb/testsuite/gdb.base/catch-follow-exec.c
> index fa68a2a34e..1a59f58aa5 100644
> --- a/gdb/testsuite/gdb.base/catch-follow-exec.c
> +++ b/gdb/testsuite/gdb.base/catch-follow-exec.c
> @@ -1,3 +1,20 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2018 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or 
> modify
> +   it under the terms of the GNU General Public License as published 
> by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.  */
> +
>  #include <stdio.h>
>  #include <string.h>
>  #include <unistd.h>
> diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.exp
> b/gdb/testsuite/gdb.base/catch-follow-exec.exp
> index 0e32ed4a6f..2ccd0115e7 100644
> --- a/gdb/testsuite/gdb.base/catch-follow-exec.exp
> +++ b/gdb/testsuite/gdb.base/catch-follow-exec.exp
> @@ -22,8 +22,8 @@ if {[build_executable "failed to prepare" $testfile
> $srcfile debug] == -1} {
>      return -1
>  }
> 
> -if { ![remote_file target exists /bin/bash] } {
> -    unsupported "no bash"
> +if { [target_info gdb_protocol] != "" } {
> +    unsupported "not native"

Please add a comment here, something like:

# Even though the feature under features being tested are supported by 
gdbserver,
# the way this test is written doesn't make it easy with a remote 
target.

>      return
>  }
> 
> @@ -34,7 +34,7 @@ if { ![remote_file target exists /bin/ls] } {
> 
>  proc catch_follow_exec { } {
>      global binfile
> -    global GDB
> +    global gdb_spawn_id
> 
>      set test "catch-follow-exec"
> 
> @@ -45,13 +45,36 @@ proc catch_follow_exec { } {
>      append FLAGS " -ex \"run\""
>      append FLAGS " -ex \"info prog\""
> 
> -    catch {exec /bin/bash -c "$GDB $FLAGS"} catchlog
> -    send_log "$catchlog\n"
> +    gdb_exit
> +    if {[gdb_spawn_with_cmdline_opts "$FLAGS"] != 0} {
> +	fail "spawn"
> +	return
> +    }
> +
> +    gdb_test_multiple "" "run til exit" {
> +	"runtime error:" {
> +	    # Error in case of --enable-ubsan
> +	    fail "No runtime error"
> +	}

Can you explain what this catches?  If this isn't there but UBSan is 
enabled, the crash isn't detected properly by the code below?

Please use a lower case letter for the test name.

> +	eof {
> +	    set result [wait -i $gdb_spawn_id]
> +	    verbose $result
> +
> +	    gdb_assert { [lindex $result 2] == 0 }
> +
> +	    # We suspect this will be zero instead of one after fixing 
> PR23368
> +	    # - "gdb goes to into background when hitting exec catchpoint 
> with
> +	    # follow-exec-mode new"
> +            gdb_assert { [lindex $result 3] == 1 }
> +
> +	    # Error in case of --disable-ubsan, we get
> +	    # "CHILDKILLED SIGSEGV {segmentation violation}" as extra
> +	    # argument(s).
> +	    gdb_assert { [llength $result] == 4 }
> +	}
> 
> -    if { [regexp {No selected thread} $catchlog] } {
> -	pass $test
> -    } else {
> -	fail $test
> +	remote_close host
> +	clear_gdb_spawn_id
>      }
>  }

Simon

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Tom de Vries]

On 10/16/18 12:11 AM, Simon Marchi wrote:
>> +    gdb_test_multiple "" "run til exit" {
>> +    "runtime error:" {
>> +        # Error in case of --enable-ubsan
>> +        fail "No runtime error"
>> +    }
> 
> Can you explain what this catches?  If this isn't there but UBSan is
> enabled, the crash isn't detected properly by the code below?

Indeed.

If I disable the fix this test-case is testing (so, I'm trying to make
the test-case fail), and I build with --disable-ubsan, I get:
...
27079 exp8 0 0 CHILDKILLED SIGSEGV {segmentation violation}
PASS: gdb.base/catch-follow-exec.exp: [lindex $result 2] == 0
FAIL: gdb.base/catch-follow-exec.exp: [lindex $result 3] == 1
FAIL: gdb.base/catch-follow-exec.exp: [llength $result] == 4
...
and with --enable-ubsan:
...
^[[1m/data/gdb_versions/devel/src/gdb/infcmd.c:2099:11:^[[1m^[[31m
runtime error: ^[[1m^[[0m^[[1mmember access wi\
thin null pointer of type 'struct thread_info'^[[1m^[[0m^M
FAIL: gdb.base/catch-follow-exec.exp: No runtime error
...

Without the extra "runtime error:" clause, I'm not detecting the fail:
...
^[[1m/data/gdb_versions/devel/src/gdb/infcmd.c:2099:11:^[[1m^[[31m
runtime error: ^[[1m^[[0m^[[1mmember access wi\
thin null pointer of type 'struct thread_info'^[[1m^[[0m^M
PASS: gdb.base/catch-follow-exec.exp: [lindex $result 2] == 0
PASS: gdb.base/catch-follow-exec.exp: [lindex $result 3] == 1
PASS: gdb.base/catch-follow-exec.exp: [llength $result] == 4
...

Thanks,
- Tom

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]

On 10/16/18 12:11 AM, Simon Marchi wrote:
> On 2018-10-11 03:47, Tom de Vries wrote:
>> [gdb/testsuite] Rewrite catch-follow-exec.exp
> My build of GDB (and probably some on the buildbot too?) uses
> -fsanitize=address, and on it the test does not pass.  On a build
> without -fsanitize=address, it does pass.  The failing test is:
> 
> FAIL: gdb.base/catch-follow-exec.exp: [lindex $result 3] == 1
> 
> and the value of $result is "17872 exp10 0 23".  This is because ASan
> exits with 23 if it detects leaks.

I had trouble reproducing this, until I tried -fsanitize=leak.

> If there's a way to set
> LSAN_OPTIONS='exitcode=1' in the environment GDB runs in, it would
> probably make it work...
> 

I've fixed this by changing the test from == 1 to != 0.

>> +if { [target_info gdb_protocol] != "" } {
>> +    unsupported "not native"
> 
> Please add a comment here, something like:
>> # Even though the feature under features being tested are supported by
> gdbserver,
> # the way this test is written doesn't make it easy with a remote target.
> 

Done.

>> +    gdb_test_multiple "" "run til exit" {
>> +    "runtime error:" {
>> +        # Error in case of --enable-ubsan
>> +        fail "No runtime error"
>> +    }
> 
> Please use a lower case letter for the test name.
> 

Done.

OK for trunk?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Rewrite-catch-follow-exec.exp.patch --]
[-- Type: text/x-patch, Size: 4014 bytes --]

[gdb/testsuite] Rewrite catch-follow-exec.exp

There are two problems with the current catch-follow-exec.exp:
- INTERNAL_GDBFLAGS (containing the datadir setting) is not used
- remote host testing doesn't work

Fix the former by using gdb_spawn_with_cmdline_opts.  Fix the latter by
requiring gdb-native.

Build on x86_64-linux with and without ubsan, and tested.

2018-10-05  Tom de Vries  <tdevries@suse.de>

	PR gdb/23730
	* gdb.base/catch-follow-exec.c: Add copyright notice.
	* gdb.base/catch-follow-exec.exp: Rewrite to use
	gdb_spawn_with_cmdline_opts.  Require gdb-native.

---
 gdb/testsuite/gdb.base/catch-follow-exec.c   | 17 +++++++++
 gdb/testsuite/gdb.base/catch-follow-exec.exp | 52 +++++++++++++++++++++-------
 2 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.c b/gdb/testsuite/gdb.base/catch-follow-exec.c
index fa68a2a34e..1a59f58aa5 100644
--- a/gdb/testsuite/gdb.base/catch-follow-exec.c
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.c
@@ -1,3 +1,20 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.exp b/gdb/testsuite/gdb.base/catch-follow-exec.exp
index 0e32ed4a6f..c3c7c7ecdd 100644
--- a/gdb/testsuite/gdb.base/catch-follow-exec.exp
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.exp
@@ -18,12 +18,11 @@
 
 standard_testfile
 
-if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
-    return -1
-}
-
-if { ![remote_file target exists /bin/bash] } {
-    unsupported "no bash"
+if { [target_info gdb_protocol] != "" } {
+    # Even though the feature under features being tested are supported by
+    # gdbserver, the way this test is written doesn't make it easy with a
+    # remote target.
+    unsupported "not native"
     return
 }
 
@@ -32,9 +31,13 @@ if { ![remote_file target exists /bin/ls] } {
     return
 }
 
+if { [build_executable "failed to prepare" $testfile $srcfile debug] == -1 } {
+    return -1
+}
+
 proc catch_follow_exec { } {
     global binfile
-    global GDB
+    global gdb_spawn_id
 
     set test "catch-follow-exec"
 
@@ -45,13 +48,36 @@ proc catch_follow_exec { } {
     append FLAGS " -ex \"run\""
     append FLAGS " -ex \"info prog\""
 
-    catch {exec /bin/bash -c "$GDB $FLAGS"} catchlog
-    send_log "$catchlog\n"
+    gdb_exit
+    if {[gdb_spawn_with_cmdline_opts "$FLAGS"] != 0} {
+	fail "spawn"
+	return
+    }
+
+    gdb_test_multiple "" "run til exit" {
+	"runtime error:" {
+	    # Error in case of --enable-ubsan
+	    fail "no runtime error"
+	}
+	eof {
+	    set result [wait -i $gdb_spawn_id]
+	    verbose $result
+
+	    gdb_assert { [lindex $result 2] == 0 }
+
+	    # We suspect this will be zero instead of one after fixing PR23368
+	    # - "gdb goes to into background when hitting exec catchpoint with
+	    # follow-exec-mode new"
+	    gdb_assert { [lindex $result 3] != 0 }
+
+	    # Error in case of --disable-ubsan, we get
+	    # "CHILDKILLED SIGSEGV {segmentation violation}" as extra
+	    # argument(s).
+	    gdb_assert { [llength $result] == 4 }
+	}
 
-    if { [regexp {No selected thread} $catchlog] } {
-	pass $test
-    } else {
-	fail $test
+	remote_close host
+	clear_gdb_spawn_id
     }
 }
 

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Simon Marchi]

On 2018-10-16 13:07, Tom de Vries wrote:
> On 10/16/18 12:11 AM, Simon Marchi wrote:
>> On 2018-10-11 03:47, Tom de Vries wrote:
>>> [gdb/testsuite] Rewrite catch-follow-exec.exp
>> My build of GDB (and probably some on the buildbot too?) uses
>> -fsanitize=address, and on it the test does not pass.  On a build
>> without -fsanitize=address, it does pass.  The failing test is:
>> 
>> FAIL: gdb.base/catch-follow-exec.exp: [lindex $result 3] == 1
>> 
>> and the value of $result is "17872 exp10 0 23".  This is because ASan
>> exits with 23 if it detects leaks.
> 
> I had trouble reproducing this, until I tried -fsanitize=leak.
> 
>> If there's a way to set
>> LSAN_OPTIONS='exitcode=1' in the environment GDB runs in, it would
>> probably make it work...
>> 
> 
> I've fixed this by changing the test from == 1 to != 0.

Ah, good.

>>> +if { [target_info gdb_protocol] != "" } {
>>> +    unsupported "not native"
>> 
>> Please add a comment here, something like:
>>> # Even though the feature under features being tested are supported 
>>> by
>> gdbserver,
>> # the way this test is written doesn't make it easy with a remote 
>> target.
>> 
> 
> Done.
> 
>>> +    gdb_test_multiple "" "run til exit" {
>>> +    "runtime error:" {
>>> +        # Error in case of --enable-ubsan
>>> +        fail "No runtime error"
>>> +    }
>> 
>> Please use a lower case letter for the test name.
>> 
> 
> Done.
> 
> OK for trunk?

LGTM, thanks!

Simon

Upper case test names

[Tom de Vries]

[ was: Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp ]

On 10/16/18 12:11 AM, Simon Marchi wrote:
> Please use a lower case letter for the test name.

Hmm, that happens more often, appearantly:
...
find gdb/testsuite/ -type f -name "*.exp" | grep -v /lib/ | xargs egrep
$'[\t ](fail|pass|unsupported|untested|xfail|kfail) "[A-Z][a-z]' | egrep
-v 'PowerPC|Rust'
gdb/testsuite/gdb.ada/bp_inlined_func.exp:   fail "Cannot run to main,
testcase aborted"
gdb/testsuite/gdb.ada/excep_handle.exp:   fail "Cannot run to main,
testcase aborted"
gdb/testsuite/gdb.ada/mi_string_access.exp:   fail "Cannot run to main,
testcase aborted"
gdb/testsuite/gdb.ada/mi_var_union.exp:   fail "Cannot run to main,
testcase aborted"
gdb/testsuite/gdb.arch/arc-analyze-prologue.exp:    fail "Can't run to main"
gdb/testsuite/gdb.arch/arc-decode-insn.exp:    fail "Can't run to main"
gdb/testsuite/gdb.base/readnever.exp:    untested "Couldn't compile
${srcfile}"
gdb/testsuite/gdb.fortran/printing-types.exp:    untested "Could not run
to breakpoint MAIN__"
gdb/testsuite/gdb.guile/scm-lazy-string.exp:    fail "Can't run to main"
...

Does this need fixing?

Thanks,
- Tom

Re: Upper case test names

[Simon Marchi]

On 2018-10-17 03:30, Tom de Vries wrote:
> [ was: Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp ]
> 
> On 10/16/18 12:11 AM, Simon Marchi wrote:
>> Please use a lower case letter for the test name.
> 
> Hmm, that happens more often, appearantly:
> ...
> find gdb/testsuite/ -type f -name "*.exp" | grep -v /lib/ | xargs egrep
> $'[\t ](fail|pass|unsupported|untested|xfail|kfail) "[A-Z][a-z]' | 
> egrep
> -v 'PowerPC|Rust'
> gdb/testsuite/gdb.ada/bp_inlined_func.exp:   fail "Cannot run to main,
> testcase aborted"
> gdb/testsuite/gdb.ada/excep_handle.exp:   fail "Cannot run to main,
> testcase aborted"
> gdb/testsuite/gdb.ada/mi_string_access.exp:   fail "Cannot run to main,
> testcase aborted"
> gdb/testsuite/gdb.ada/mi_var_union.exp:   fail "Cannot run to main,
> testcase aborted"
> gdb/testsuite/gdb.arch/arc-analyze-prologue.exp:    fail "Can't run to 
> main"
> gdb/testsuite/gdb.arch/arc-decode-insn.exp:    fail "Can't run to main"
> gdb/testsuite/gdb.base/readnever.exp:    untested "Couldn't compile
> ${srcfile}"
> gdb/testsuite/gdb.fortran/printing-types.exp:    untested "Could not 
> run
> to breakpoint MAIN__"
> gdb/testsuite/gdb.guile/scm-lazy-string.exp:    fail "Can't run to 
> main"
> ...
> 
> Does this need fixing?

Sure, it would need fixing, if you want to take the time to do it.  
Here's the reference in our wiki where that rule is stated, for 
reference.

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

Thanks,

Simon

Re: Upper case test names

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 367 bytes --]

On 10/17/18 2:07 PM, Simon Marchi wrote:
>> Does this need fixing?
> 
> Sure, it would need fixing, if you want to take the time to do it. 
> Here's the reference in our wiki where that rule is stated, for reference.
> 
> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Follow_the_test_name_convention
> 

Tested on x86_64-linux.

OK for trunk?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Fix-capitalized-test-names.patch --]
[-- Type: text/x-patch, Size: 5381 bytes --]

[gdb/testsuite] Fix capitalized test names

At https://sourceware.org/gdb/wiki/GDBTestcaseCookbook\
  #Follow_the_test_name_convention we find:
..
Test names should start with a lower case and don't need to end with a period
(they are not sentences).
...

Fix some capitalized test names.

2018-10-18  Tom de Vries  <tdevries@suse.de>

	* gdb.ada/bp_inlined_func.exp: Fix capitalized test name.
	* gdb.ada/excep_handle.exp: Same.
	* gdb.ada/mi_string_access.exp: Same.
	* gdb.ada/mi_var_union.exp: Same.
	* gdb.arch/arc-analyze-prologue.exp: Same.
	* gdb.arch/arc-decode-insn.exp: Same.
	* gdb.base/readnever.exp: Same.
	* gdb.fortran/printing-types.exp: Same.
	* gdb.guile/scm-lazy-string.exp: Same.

---
 gdb/testsuite/gdb.ada/bp_inlined_func.exp       | 2 +-
 gdb/testsuite/gdb.ada/excep_handle.exp          | 2 +-
 gdb/testsuite/gdb.ada/mi_string_access.exp      | 2 +-
 gdb/testsuite/gdb.ada/mi_var_union.exp          | 2 +-
 gdb/testsuite/gdb.arch/arc-analyze-prologue.exp | 2 +-
 gdb/testsuite/gdb.arch/arc-decode-insn.exp      | 2 +-
 gdb/testsuite/gdb.base/readnever.exp            | 2 +-
 gdb/testsuite/gdb.fortran/printing-types.exp    | 2 +-
 gdb/testsuite/gdb.guile/scm-lazy-string.exp     | 2 +-
 9 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/gdb/testsuite/gdb.ada/bp_inlined_func.exp b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
index 2013e6be82..f7aa1c38cc 100644
--- a/gdb/testsuite/gdb.ada/bp_inlined_func.exp
+++ b/gdb/testsuite/gdb.ada/bp_inlined_func.exp
@@ -24,7 +24,7 @@ if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" }
 clean_restart ${testfile}
 
 if ![runto_main] then {
-   fail "Cannot run to main, testcase aborted"
+   fail "cannot run to main, testcase aborted"
    return 0
 }
 
diff --git a/gdb/testsuite/gdb.ada/excep_handle.exp b/gdb/testsuite/gdb.ada/excep_handle.exp
index c693fd30ea..b7cd6dcd70 100644
--- a/gdb/testsuite/gdb.ada/excep_handle.exp
+++ b/gdb/testsuite/gdb.ada/excep_handle.exp
@@ -43,7 +43,7 @@ set catchpoint_storage_error_msg \
 ############################################
 
 if ![runto_main] then {
-   fail "Cannot run to main, testcase aborted"
+   fail "cannot run to main, testcase aborted"
    return 0
 }
 
diff --git a/gdb/testsuite/gdb.ada/mi_string_access.exp b/gdb/testsuite/gdb.ada/mi_string_access.exp
index 8ecf907b1e..adae4ab13f 100644
--- a/gdb/testsuite/gdb.ada/mi_string_access.exp
+++ b/gdb/testsuite/gdb.ada/mi_string_access.exp
@@ -34,7 +34,7 @@ mi_gdb_reinitialize_dir $srcdir/$subdir
 mi_gdb_load ${binfile}
 
 if ![mi_run_to_main] then {
-   fail "Cannot run to main, testcase aborted"
+   fail "cannot run to main, testcase aborted"
    return 0
 }
 
diff --git a/gdb/testsuite/gdb.ada/mi_var_union.exp b/gdb/testsuite/gdb.ada/mi_var_union.exp
index 26a7ed0dd4..51147a491e 100644
--- a/gdb/testsuite/gdb.ada/mi_var_union.exp
+++ b/gdb/testsuite/gdb.ada/mi_var_union.exp
@@ -36,7 +36,7 @@ mi_gdb_reinitialize_dir $srcdir/$subdir
 mi_gdb_load ${binfile}
 
 if ![mi_run_to_main] then {
-   fail "Cannot run to main, testcase aborted"
+   fail "cannot run to main, testcase aborted"
    return 0
 }
 
diff --git a/gdb/testsuite/gdb.arch/arc-analyze-prologue.exp b/gdb/testsuite/gdb.arch/arc-analyze-prologue.exp
index d874515d2b..6a4d60eebc 100644
--- a/gdb/testsuite/gdb.arch/arc-analyze-prologue.exp
+++ b/gdb/testsuite/gdb.arch/arc-analyze-prologue.exp
@@ -27,7 +27,7 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
 }
 
 if ![runto_main] {
-    fail "Can't run to main"
+    fail "can't run to main"
     return 0
 }
 
diff --git a/gdb/testsuite/gdb.arch/arc-decode-insn.exp b/gdb/testsuite/gdb.arch/arc-decode-insn.exp
index 7ee666b0d7..a4cd7c7948 100644
--- a/gdb/testsuite/gdb.arch/arc-decode-insn.exp
+++ b/gdb/testsuite/gdb.arch/arc-decode-insn.exp
@@ -43,7 +43,7 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
 }
 
 if ![runto_main] {
-    fail "Can't run to main"
+    fail "can't run to main"
     return 0
 }
 
diff --git a/gdb/testsuite/gdb.base/readnever.exp b/gdb/testsuite/gdb.base/readnever.exp
index 805ecd218a..c712dae9e5 100644
--- a/gdb/testsuite/gdb.base/readnever.exp
+++ b/gdb/testsuite/gdb.base/readnever.exp
@@ -16,7 +16,7 @@
 standard_testfile .c
 
 if { [build_executable "failed to build" $testfile $srcfile { debug }] == -1 } {
-    untested "Couldn't compile ${srcfile}"
+    untested "couldn't compile ${srcfile}"
     return -1
 }
 
diff --git a/gdb/testsuite/gdb.fortran/printing-types.exp b/gdb/testsuite/gdb.fortran/printing-types.exp
index 9237907ed6..885bb64f8a 100644
--- a/gdb/testsuite/gdb.fortran/printing-types.exp
+++ b/gdb/testsuite/gdb.fortran/printing-types.exp
@@ -22,7 +22,7 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug f90}]} {
 }
 
 if {![runto MAIN__]} then {
-    untested "Could not run to breakpoint MAIN__"
+    untested "could not run to breakpoint MAIN__"
     return -1
 }
 
diff --git a/gdb/testsuite/gdb.guile/scm-lazy-string.exp b/gdb/testsuite/gdb.guile/scm-lazy-string.exp
index 52211b30f7..13cc885315 100644
--- a/gdb/testsuite/gdb.guile/scm-lazy-string.exp
+++ b/gdb/testsuite/gdb.guile/scm-lazy-string.exp
@@ -33,7 +33,7 @@ if { [skip_guile_tests] } { continue }
 # The following tests require execution.
 
 if ![gdb_guile_runto_main] {
-    fail "Can't run to main"
+    fail "can't run to main"
     return
 }
 

Re: Upper case test names

[Simon Marchi]

On 2018-10-18 08:56, Tom de Vries wrote:
> On 10/17/18 2:07 PM, Simon Marchi wrote:
>>> Does this need fixing?
>> 
>> Sure, it would need fixing, if you want to take the time to do it. 
>> Here's the reference in our wiki where that rule is stated, for 
>> reference.
>> 
>> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Follow_the_test_name_convention
>> 
> 
> Tested on x86_64-linux.
> 
> OK for trunk?

This is ok, thanks!

Simon

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Simon Marchi]

On 2018-10-15 3:54 p.m., Tom de Vries wrote:
>> Just wondering.  Would it make life easier if we fixed PR 23368, which
>> is the reason we have to do the test in an unnatural way?
> 
> Yes.

Hi Tom,

PR 23368 should be fixed now.  Do you plan on updating catch-follow-exec.exp
to be written in a more standard way?

Simon

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Tom de Vries]

On 10/23/18 11:04 PM, Simon Marchi wrote:
> On 2018-10-15 3:54 p.m., Tom de Vries wrote:
>>> Just wondering.  Would it make life easier if we fixed PR 23368, which
>>> is the reason we have to do the test in an unnatural way?
>>
>> Yes.
> 
> Hi Tom,
> 
> PR 23368 should be fixed now.  Do you plan on updating catch-follow-exec.exp
> to be written in a more standard way?

Sure, will do.

Thanks,
- Tom

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

On 10/23/18 11:05 PM, Tom de Vries wrote:
> On 10/23/18 11:04 PM, Simon Marchi wrote:
>> On 2018-10-15 3:54 p.m., Tom de Vries wrote:
>>>> Just wondering.  Would it make life easier if we fixed PR 23368, which
>>>> is the reason we have to do the test in an unnatural way?
>>>
>>> Yes.
>>
>> Hi Tom,
>>
>> PR 23368 should be fixed now.  Do you plan on updating catch-follow-exec.exp
>> to be written in a more standard way?
> 
> Sure, will do.

How does this look?

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Rewrite-catch-follow-exec.exp-using-gdb_test.patch --]
[-- Type: text/x-patch, Size: 3071 bytes --]

[gdb/testsuite]	Rewrite catch-follow-exec.exp using gdb_test

The testcase catch-follow-exec.exp is written use gdb -batch in order to avoid
a GDB SIGTTOU.  After the commit of "Avoid GDB SIGTTOU on catch exec + set
follow-exec-mode new (PR 23368)", that no longer is necessary.

Rewrite the test using regular gdb_test commands.

Tested with x86_64-linux.

2018-10-24  Tom de Vries  <tdevries@suse.de>

	* gdb.base/catch-follow-exec.exp: Rewrite using gdb_test.

---
 gdb/testsuite/gdb.base/catch-follow-exec.exp | 64 +++++++++-------------------
 1 file changed, 19 insertions(+), 45 deletions(-)

diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.exp b/gdb/testsuite/gdb.base/catch-follow-exec.exp
index c3c7c7ecdd..25d14b8465 100644
--- a/gdb/testsuite/gdb.base/catch-follow-exec.exp
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.exp
@@ -18,66 +18,40 @@
 
 standard_testfile
 
-if { [target_info gdb_protocol] != "" } {
-    # Even though the feature under features being tested are supported by
-    # gdbserver, the way this test is written doesn't make it easy with a
-    # remote target.
-    unsupported "not native"
-    return
-}
-
 if { ![remote_file target exists /bin/ls] } {
     unsupported "no ls"
     return
 }
 
-if { [build_executable "failed to prepare" $testfile $srcfile debug] == -1 } {
-    return -1
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+    return
 }
 
 proc catch_follow_exec { } {
-    global binfile
-    global gdb_spawn_id
+    global gdb_prompt gdb_spawn_id
 
-    set test "catch-follow-exec"
+    gdb_test "catch exec" \
+	{Catchpoint [0-9][0-9]* \(exec\)} \
+	"catch exec"
 
-    append FLAGS " \"$binfile\""
-    append FLAGS " -batch"
-    append FLAGS " -ex \"catch exec\""
-    append FLAGS " -ex \"set follow-exec-mode new\""
-    append FLAGS " -ex \"run\""
-    append FLAGS " -ex \"info prog\""
+    gdb_test "set follow-exec-mode new" \
+	"" \
+	"set follow-exec-mode new"
 
-    gdb_exit
-    if {[gdb_spawn_with_cmdline_opts "$FLAGS"] != 0} {
-	fail "spawn"
-	return
+    gdb_run_cmd
+    gdb_expect {
+        -re ".*hit Catchpoint.*${gdb_prompt} $" {
+	    pass "run"
+        }
     }
 
-    gdb_test_multiple "" "run til exit" {
-	"runtime error:" {
-	    # Error in case of --enable-ubsan
-	    fail "no runtime error"
+    gdb_test_multiple "info prog" "info prog" {
+	-i "$gdb_spawn_id" eof {
+	    fail "info prog"
 	}
-	eof {
-	    set result [wait -i $gdb_spawn_id]
-	    verbose $result
-
-	    gdb_assert { [lindex $result 2] == 0 }
-
-	    # We suspect this will be zero instead of one after fixing PR23368
-	    # - "gdb goes to into background when hitting exec catchpoint with
-	    # follow-exec-mode new"
-	    gdb_assert { [lindex $result 3] != 0 }
-
-	    # Error in case of --disable-ubsan, we get
-	    # "CHILDKILLED SIGSEGV {segmentation violation}" as extra
-	    # argument(s).
-	    gdb_assert { [llength $result] == 4 }
+	-i "$gdb_spawn_id" "No selected thread\."  {
+	    pass "info prog"
 	}
-
-	remote_close host
-	clear_gdb_spawn_id
     }
 }
 

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Simon Marchi]

On 2018-10-23 6:38 p.m., Tom de Vries wrote:
> On 10/23/18 11:05 PM, Tom de Vries wrote:
>> On 10/23/18 11:04 PM, Simon Marchi wrote:
>>> On 2018-10-15 3:54 p.m., Tom de Vries wrote:
>>>>> Just wondering.  Would it make life easier if we fixed PR 23368, which
>>>>> is the reason we have to do the test in an unnatural way?
>>>>
>>>> Yes.
>>>
>>> Hi Tom,
>>>
>>> PR 23368 should be fixed now.  Do you plan on updating catch-follow-exec.exp
>>> to be written in a more standard way?
>>
>> Sure, will do.
> 
> How does this look?

Hi Tom,

Thanks for looking into this so quickly.  I have some superficial suggestions that
can help shorten the test a bit and make it more readable (some of them can be personal
preference though...).

When the test name is omitted, it defaults to the command.  So instead of

    gdb_test "catch exec" \
	{Catchpoint [0-9][0-9]* \(exec\)} \
	"catch exec"

You can write

    gdb_test "catch exec" {Catchpoint [0-9][0-9]* \(exec\)}

and the test name will be "catch exec".  Instead of [0-9][0-9]*, I am
pretty sure you can use [0-9]+, or $decimal, which is provided by DejaGnu
(/usr/share/dejagnu/runtest.exp):

  101:    set decimal "\[0-9\]+"

Except in the {} string, $decimal won't work, because it won't get
substituted.

For this:

    gdb_test "set follow-exec-mode new" \
	"" \
	"set follow-exec-mode new"

You can use

    gdb_test_no_output "set follow-exec-mode new"

(again, omitting the test name makes it default to the command)

I'd suggest replacing

    gdb_test_multiple "info prog" "info prog" {
	-i "$gdb_spawn_id" eof {
	    fail "info prog"
	}
	-i "$gdb_spawn_id" "No selected thread\."  {
	    pass "info prog"
	}
    }

with the simpler

    gdb_test "info prog" "No selected thread."

If GDB crashes as it did before your fix, the test will be unresolved, which is
treated the same as a FAIL.  If you decide to keep the gdb_test_multiple, I
think you don't need to specify -i "$gdb_spawn_id", it's the default.  Also, it's
common practice to factor out the test name, to make sure it's constant.  And
because the test name is the same as the command, you could do

set test "info prog"
gdb_test_multiple $test $test {
  eof {
    fail $test
  }
  -re "No selected thread\." {
    pass $test
  }
}

While at it, could you update the comment at the top of the file, which currently
says:

# Check whether finish respects the print pretty user setting when printing the
# function result.

Thanks!

Simon

Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]

On 10/24/18 1:37 AM, Simon Marchi wrote:
> On 2018-10-23 6:38 p.m., Tom de Vries wrote:
>> On 10/23/18 11:05 PM, Tom de Vries wrote:
>>> On 10/23/18 11:04 PM, Simon Marchi wrote:
>>>> On 2018-10-15 3:54 p.m., Tom de Vries wrote:
>>>>>> Just wondering.  Would it make life easier if we fixed PR 23368, which
>>>>>> is the reason we have to do the test in an unnatural way?
>>>>>
>>>>> Yes.
>>>>
>>>> Hi Tom,
>>>>
>>>> PR 23368 should be fixed now.  Do you plan on updating catch-follow-exec.exp
>>>> to be written in a more standard way?
>>>
>>> Sure, will do.
>>
>> How does this look?
> 
> Hi Tom,
> 
> Thanks for looking into this so quickly.

And thanks for the quick review.

>  I have some superficial suggestions that
> can help shorten the test a bit and make it more readable (some of them can be personal
> preference though...).
> 
> When the test name is omitted, it defaults to the command.  So instead of
> 
>     gdb_test "catch exec" \
> 	{Catchpoint [0-9][0-9]* \(exec\)} \
> 	"catch exec"
> 
> You can write
> 
>     gdb_test "catch exec" {Catchpoint [0-9][0-9]* \(exec\)}
> 
> and the test name will be "catch exec".

Done.

>  Instead of [0-9][0-9]*, I am
> pretty sure you can use [0-9]+,

Done.

> or $decimal, which is provided by DejaGnu
> (/usr/share/dejagnu/runtest.exp):
> 
>   101:    set decimal "\[0-9\]+"
> 
> Except in the {} string, $decimal won't work, because it won't get
> substituted.

Indeed. I prefer the {} quoting over "" quoting if that means less
escaping, so I went with {} here.

> 
> For this:
> 
>     gdb_test "set follow-exec-mode new" \
> 	"" \
> 	"set follow-exec-mode new"
> 
> You can use
> 
>     gdb_test_no_output "set follow-exec-mode new"
> 

Done.

> (again, omitting the test name makes it default to the command)
> 
> I'd suggest replacing
> 
>     gdb_test_multiple "info prog" "info prog" {
> 	-i "$gdb_spawn_id" eof {
> 	    fail "info prog"
> 	}
> 	-i "$gdb_spawn_id" "No selected thread\."  {
> 	    pass "info prog"
> 	}
>     }
> 
> with the simpler
> 
>     gdb_test "info prog" "No selected thread."
> 
> If GDB crashes as it did before your fix, the test will be unresolved, which is
> treated the same as a FAIL.

Done.

> While at it, could you update the comment at the top of the file, which currently
> says:
> 
> # Check whether finish respects the print pretty user setting when printing the
> # function result.
> 

Done.

Also, I realized that by using runto_main at the start, I could replace
gdb_run_cmd/gdb_expect with a regular gdb_test continue.

Committed as attached.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Rewrite-catch-follow-exec.exp-using-gdb_test.patch --]
[-- Type: text/x-patch, Size: 3223 bytes --]

[gdb/testsuite]	Rewrite catch-follow-exec.exp using gdb_test

The testcase catch-follow-exec.exp is written use gdb -batch in order to avoid
a GDB SIGTTOU.  After the commit of "Avoid GDB SIGTTOU on catch exec + set
follow-exec-mode new (PR 23368)", that no longer is necessary.

Rewrite the test using regular gdb_test commands.

Tested with x86_64-linux.

2018-10-24  Tom de Vries  <tdevries@suse.de>

	* gdb.base/catch-follow-exec.exp: Rewrite using gdb_test.

---
 gdb/testsuite/gdb.base/catch-follow-exec.exp | 63 ++++++----------------------
 1 file changed, 13 insertions(+), 50 deletions(-)

diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.exp b/gdb/testsuite/gdb.base/catch-follow-exec.exp
index c3c7c7ecdd..5f7db25265 100644
--- a/gdb/testsuite/gdb.base/catch-follow-exec.exp
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.exp
@@ -13,72 +13,35 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-# Check whether finish respects the print pretty user setting when printing the
-# function result.
+# Test whether info prog crashes gdb at a catch point in follow-exec-mode new.
 
 standard_testfile
 
-if { [target_info gdb_protocol] != "" } {
-    # Even though the feature under features being tested are supported by
-    # gdbserver, the way this test is written doesn't make it easy with a
-    # remote target.
-    unsupported "not native"
-    return
-}
-
 if { ![remote_file target exists /bin/ls] } {
     unsupported "no ls"
     return
 }
 
-if { [build_executable "failed to prepare" $testfile $srcfile debug] == -1 } {
-    return -1
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+    return
 }
 
 proc catch_follow_exec { } {
-    global binfile
-    global gdb_spawn_id
-
-    set test "catch-follow-exec"
-
-    append FLAGS " \"$binfile\""
-    append FLAGS " -batch"
-    append FLAGS " -ex \"catch exec\""
-    append FLAGS " -ex \"set follow-exec-mode new\""
-    append FLAGS " -ex \"run\""
-    append FLAGS " -ex \"info prog\""
-
-    gdb_exit
-    if {[gdb_spawn_with_cmdline_opts "$FLAGS"] != 0} {
-	fail "spawn"
-	return
+    if { ![runto_main] } {
+	untested "could not run to main"
+	return -1
     }
 
-    gdb_test_multiple "" "run til exit" {
-	"runtime error:" {
-	    # Error in case of --enable-ubsan
-	    fail "no runtime error"
-	}
-	eof {
-	    set result [wait -i $gdb_spawn_id]
-	    verbose $result
+    gdb_test "catch exec" \
+	{Catchpoint [0-9]+ \(exec\)}
 
-	    gdb_assert { [lindex $result 2] == 0 }
+    gdb_test_no_output "set follow-exec-mode new"
 
-	    # We suspect this will be zero instead of one after fixing PR23368
-	    # - "gdb goes to into background when hitting exec catchpoint with
-	    # follow-exec-mode new"
-	    gdb_assert { [lindex $result 3] != 0 }
+    gdb_test "continue" \
+	".*hit Catchpoint.*"
 
-	    # Error in case of --disable-ubsan, we get
-	    # "CHILDKILLED SIGSEGV {segmentation violation}" as extra
-	    # argument(s).
-	    gdb_assert { [llength $result] == 4 }
-	}
-
-	remote_close host
-	clear_gdb_spawn_id
-    }
+    gdb_test "info prog" \
+	"No selected thread."
 }
 
 catch_follow_exec

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

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

[ was: Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp ]
On 10/24/18 1:47 PM, Tom de Vries wrote:
>> I'd suggest replacing
>>
>>     gdb_test_multiple "info prog" "info prog" {
>> 	-i "$gdb_spawn_id" eof {
>> 	    fail "info prog"
>> 	}
>> 	-i "$gdb_spawn_id" "No selected thread\."  {
>> 	    pass "info prog"
>> 	}
>>     }
>>
>> with the simpler
>>
>>     gdb_test "info prog" "No selected thread."
>>
>> If GDB crashes as it did before your fix, the test will be unresolved, which is
>> treated the same as a FAIL.
> Done.
> 

Btw, I noticed that gdb_test_multiple emits an "ERROR: Process no longer
exists" preceding the unresolved message, but does not give details
about the process exit status, which might be helpful.

How about this patch?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Log-wait-status-on-process-no-longer-exists-error.patch --]
[-- Type: text/x-patch, Size: 1471 bytes --]

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

Proc gdb_test_multiple can run into a process no longer exists error, but when
that happens it shows no details about the process:
...
ERROR: Process no longer exists
...

Fix this by showing the wait status of the process in the log:
...
ERROR: Gdb process no longer exists
Gdb process exited with wait status 8106 exp8 0 0 CHILDKILLED SIGSEGV \
  {segmentation violation}
...

In order to run the wait commmand we need an explicit pid, so we can't use
any_spawn_id, and duplicate the "-i any_spawn_id eof" patter for gdb_spawn_id,
and add the wait status logging there.

Build and tested on x86_64-linux.

2018-10-24  Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (gdb_test_multiple): Log wait status on process no
	longer exists error.

---
 gdb/testsuite/lib/gdb.exp | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 2d197d9b5c..f68664b0ef 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -980,6 +980,16 @@ proc gdb_test_multiple { command message user_code } {
 	    set result -1
 	}
 
+	-i $gdb_spawn_id
+	eof {
+	    perror "Gdb process no longer exists"
+	    verbose -log "Gdb process exited with wait status [wait -i $gdb_spawn_id]"
+	    if { $message != "" } {
+		fail "$message"
+	    }
+	    return -1
+	}
+
 	# Patterns below apply to any spawn id specified.
 	-i $any_spawn_id
 	eof {

Re: [PATCH][gdb/testsuite] Log wait status on process no longer exists error

[Simon Marchi]

On 2018-10-24 08:09, Tom de Vries wrote:
> Btw, I noticed that gdb_test_multiple emits an "ERROR: Process no 
> longer
> exists" preceding the unresolved message, but does not give details
> about the process exit status, which might be helpful.
> 
> How about this patch?

Thanks, this is ok.  I'd write GDB instead of Gdb though.

Simon

Re: [PATCH][gdb/testsuite] Log wait status on process no longer exists error

[Pedro Franco de Carvalho]

Tom de Vries <tdevries@suse.de> writes:

> Btw, I noticed that gdb_test_multiple emits an "ERROR: Process no longer
> exists" preceding the unresolved message, but does not give details
> about the process exit status, which might be helpful.
>
> How about this patch?
>
> Thanks,
> - Tom

Hello,

I noticed that in certain cases the wait call used to find out the exit
status can hang forever waiting for the GDB process, due to a dejagnu
oddity.

The gdb_test_multiple procedure uses gdb_expect, which itself uses
remote_expect (provided by dejagnu).  The remote_expect procedure parses
the expect body passed to it and uses the eof block as a "default" case
to be executed in case of an error, which means that it can be executed
even when there was no actual eof and the GDB process is still running.

You can see this happen with a testcase like:

gdb_start

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

I'm not sure how to solve this.  This was triggered by some testcases
when gdbserver failed to start, the testcases don't check this and use
the gdbserver spawn_id even though it no longer exists, which is what
causes the error to be raised.  This gdbserver issue has been fixed
since, so for now this will probably not happen.

Thanks!

--
Pedro Franco de Carvalho

> [gdb/testsuite] Log wait status on process no longer exists error
>
> Proc gdb_test_multiple can run into a process no longer exists error, but when
> that happens it shows no details about the process:
> ...
> ERROR: Process no longer exists
> ...
>
> Fix this by showing the wait status of the process in the log:
> ...
> ERROR: Gdb process no longer exists
> Gdb process exited with wait status 8106 exp8 0 0 CHILDKILLED SIGSEGV \
>   {segmentation violation}
> ...
>
> In order to run the wait commmand we need an explicit pid, so we can't use
> any_spawn_id, and duplicate the "-i any_spawn_id eof" patter for gdb_spawn_id,
> and add the wait status logging there.
>
> Build and tested on x86_64-linux.
>
> 2018-10-24  Tom de Vries  <tdevries@suse.de>
>
> 	* lib/gdb.exp (gdb_test_multiple): Log wait status on process no
> 	longer exists error.
>
> ---
>  gdb/testsuite/lib/gdb.exp | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 2d197d9b5c..f68664b0ef 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -980,6 +980,16 @@ proc gdb_test_multiple { command message user_code } {
>  	    set result -1
>  	}
>
> +	-i $gdb_spawn_id
> +	eof {
> +	    perror "Gdb process no longer exists"
> +	    verbose -log "Gdb process exited with wait status [wait -i $gdb_spawn_id]"
> +	    if { $message != "" } {
> +		fail "$message"
> +	    }
> +	    return -1
> +	}
> +
>  	# Patterns below apply to any spawn id specified.
>  	-i $any_spawn_id
>  	eof {