Discussion:
[PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp
Tom de Vries
2018-10-05 10:11:24 UTC
Permalink
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 <***@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
}
}
Gary Benson
2018-10-09 13:51:56 UTC
Permalink
Post by Tom de Vries
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?
Post by Tom de Vries
+ # 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
Tom de Vries
2018-10-09 16:40:21 UTC
Permalink
Post by Gary Benson
Post by Tom de Vries
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?
Post by Gary Benson
Post by Tom de Vries
+ # 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
Gary Benson
2018-10-10 09:27:36 UTC
Permalink
Post by Tom de Vries
Post by Gary Benson
Post by Tom de Vries
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.
Post by Tom de Vries
Post by Gary Benson
Post by Tom de Vries
+ # 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
Simon Marchi
2018-10-10 13:28:55 UTC
Permalink
Post by Gary Benson
Post by Tom de Vries
Post by Gary Benson
Post by Tom de Vries
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
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
Gary Benson
2018-10-10 13:44:24 UTC
Permalink
Post by Simon Marchi
Post by Gary Benson
Post by Tom de Vries
Post by Gary Benson
Post by Tom de Vries
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
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
Tom de Vries
2018-10-11 07:47:45 UTC
Permalink
Post by Gary Benson
Post by Simon Marchi
Post by Gary Benson
Post by Tom de Vries
Post by Gary Benson
Post by Tom de Vries
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
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 <***@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
}
}
Gary Benson
2018-10-11 08:33:20 UTC
Permalink
Post by Tom de Vries
Post by Gary Benson
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
Post by Tom de Vries
[gdb/testsuite] Rewrite 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.
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
}
}
Simon Marchi
2018-10-13 22:18:07 UTC
Permalink
Post by Gary Benson
Post by Tom de Vries
Post by Gary Benson
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
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
Tom de Vries
2018-10-15 19:54:18 UTC
Permalink
Post by Gary Benson
Post by Tom de Vries
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
  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
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
Simon Marchi
2018-10-15 22:12:34 UTC
Permalink
Post by Tom de Vries
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
Simon Marchi
2018-10-23 21:04:30 UTC
Permalink
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
Tom de Vries
2018-10-23 21:05:59 UTC
Permalink
Post by Simon Marchi
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
Tom de Vries
2018-10-23 22:38:58 UTC
Permalink
Post by Tom de Vries
Post by Simon Marchi
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
Simon Marchi
2018-10-23 23:37:34 UTC
Permalink
Post by Tom de Vries
Post by Tom de Vries
Post by Simon Marchi
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
Tom de Vries
2018-10-24 11:47:11 UTC
Permalink
Post by Simon Marchi
Post by Tom de Vries
Post by Tom de Vries
Post by Simon Marchi
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.
Post by Simon Marchi
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.
Post by Simon Marchi
Instead of [0-9][0-9]*, I am
pretty sure you can use [0-9]+,
Done.
Post by Simon Marchi
or $decimal, which is provided by DejaGnu
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.
Post by Simon Marchi
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.
Post by Simon Marchi
(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.
Post by Simon Marchi
While at it, could you update the comment at the top of the file, which currently
# 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
Tom de Vries
2018-10-24 12:09:19 UTC
Permalink
[ was: Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp ]
Post by Simon Marchi
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
Simon Marchi
2018-10-24 14:04:48 UTC
Permalink
Post by Tom de Vries
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
Simon Marchi
2018-10-15 22:11:55 UTC
Permalink
Post by Tom de Vries
[gdb/testsuite] Rewrite 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...
Post by Tom de Vries
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.
Post by Tom de Vries
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.
Post by Tom de Vries
+ 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
Tom de Vries
2018-10-16 16:11:44 UTC
Permalink
+    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
Tom de Vries
2018-10-16 17:07:31 UTC
Permalink
Post by Simon Marchi
Post by Tom de Vries
[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
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.
Post by Simon Marchi
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.
Post by Simon Marchi
Post by Tom de Vries
+if { [target_info gdb_protocol] != "" } {
+    unsupported "not native"
# 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.
Post by Simon Marchi
Post by Tom de Vries
+    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
Simon Marchi
2018-10-16 20:12:01 UTC
Permalink
Post by Tom de Vries
Post by Simon Marchi
Post by Tom de Vries
[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
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.
Post by Simon Marchi
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.
Post by Tom de Vries
Post by Simon Marchi
Post by Tom de Vries
+if { [target_info gdb_protocol] != "" } {
+    unsupported "not native"
# 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.
Post by Simon Marchi
Post by Tom de Vries
+    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
Tom de Vries
2018-10-17 07:30:07 UTC
Permalink
[ was: Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp ]
Post by Simon Marchi
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
Simon Marchi
2018-10-17 12:07:24 UTC
Permalink
Post by Tom de Vries
[ was: Re: [PATCH][gdb/testsuite] Rewrite catch-follow-exec.exp ]
Post by Simon Marchi
Please use a lower case letter for the test name.
...
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
Tom de Vries
2018-10-18 12:56:22 UTC
Permalink
Post by Tom de Vries
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
Simon Marchi
2018-10-18 13:05:25 UTC
Permalink
Post by Tom de Vries
Post by Tom de Vries
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
Pedro Franco de Carvalho
2018-12-05 19:34:52 UTC
Permalink
Post by Tom de Vries
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
Post by Tom de Vries
[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
...
ERROR: Process no longer exists
...
...
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.
* 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 {
Loading...