[gdb/testsuite] Rewrite catch-follow-exec.exp
Commit Message
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(-)
Comments
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
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
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
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
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
@@ -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>
@@ -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
}
}