fix record "run" regression

Message ID 87r41xi3v2.fsf@fleche.redhat.com
State Superseded
Headers

Commit Message

Tom Tromey July 7, 2014, 3:29 p.m. UTC
  Pedro> So we should be able to do:
Pedro>  gdb_run_cmd
Pedro>  gdb_test_multiple "" "restarting inferior" {
[...]
Pedro> You've removed the -notransfer, but any reason you didn't add
Pedro> the $gdb_prompt matching?

How's this?

Tom

2014-07-07  Tom Tromey  <tromey@redhat.com>

	* target.c (target_require_runnable): Also check record_stratum.
	Update comment.

2014-07-07  Tom Tromey  <tromey@redhat.com>

	* gdb.reverse/rerun-prec.c: New file.
	* gdb.reverse/rerun-prec.exp: New file.
  

Comments

Pedro Alves July 8, 2014, 9:26 a.m. UTC | #1
On 07/07/2014 04:29 PM, Tom Tromey wrote:
> Pedro> So we should be able to do:
> Pedro>  gdb_run_cmd
> Pedro>  gdb_test_multiple "" "restarting inferior" {
> [...]
> Pedro> You've removed the -notransfer, but any reason you didn't add
> Pedro> the $gdb_prompt matching?
> 
> How's this?
> 
> Tom
> 
> 2014-07-07  Tom Tromey  <tromey@redhat.com>
> 
> 	* target.c (target_require_runnable): Also check record_stratum.
> 	Update comment.
> 
> 2014-07-07  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.reverse/rerun-prec.c: New file.
> 	* gdb.reverse/rerun-prec.exp: New file.
> 
> diff --git a/gdb/target.c b/gdb/target.c
> index c9c5e4b..07d029a 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2425,10 +2425,11 @@ target_require_runnable (void)
>        if (t->to_create_inferior != NULL)
>  	return;
>  
> -      /* Do not worry about thread_stratum targets that can not
> +      /* Do not worry about targets at certain strata that can not
>  	 create inferiors.  Assume they will be pushed again if
>  	 necessary, and continue to the process_stratum.  */
>        if (t->to_stratum == thread_stratum
> +	  || t->to_stratum == record_stratum
>  	  || t->to_stratum == arch_stratum)
>  	continue;
>  
> diff --git a/gdb/testsuite/gdb.reverse/rerun-prec.c b/gdb/testsuite/gdb.reverse/rerun-prec.c
> new file mode 100644
> index 0000000..c0f90cb
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/rerun-prec.c
> @@ -0,0 +1,21 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2014 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/>.  */
> +
> +int main (int argc, char **argv)
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.reverse/rerun-prec.exp b/gdb/testsuite/gdb.reverse/rerun-prec.exp
> new file mode 100644
> index 0000000..c168ead
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/rerun-prec.exp
> @@ -0,0 +1,44 @@
> +# Copyright 2014 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/>.  */
> +
> +if {![supports_reverse] || ![supports_process_record]} {
> +    return
> +}
> +
> +standard_testfile
> +
> +if {[prepare_for_testing $testfile.exp $testfile [list $srcfile]]} {
> +    return -1
> +}
> +
> +if {$use_gdb_stub} {
> +    unsupported "re-running not supported on this board"
> +    return
> +}
> +
> +# The bug is a regression in the sequence "run; record; run".
> +runto main
> +gdb_test_no_output "record" "Turn on process record"
> +
> +# We can't use gdb_run_cmd or the like since we need to detect errors.

The comment is stale now.  If we can indeed use gdb_run_cmd,
then I don't think we need the $use_gdb_stub check?
That gets us coverage for "target rem; record; target rem;"
too.  If we remove the $use_gdb_stub restriction, then the
"Starting program" below won't match, but we can match the
hit at main instead.

> +gdb_run_cmd
> +gdb_test_multiple "" "restarting inferior" {
> +    -re "does not support \"run\"" {

This one also missed the prompt matching.  But, I think
you can just remove this, as gdb_test_multiple will
fail anyway without it.

> +	fail "restarting inferior"
> +    }
> +    -re "Starting program: .*${gdb_prompt} " {
> +	pass "restarting inferior"
> +    }
> +}

Thanks,
  
Tom Tromey July 11, 2014, 5:26 p.m. UTC | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> The comment is stale now.  If we can indeed use gdb_run_cmd,
Pedro> then I don't think we need the $use_gdb_stub check?

The use_gdb_stub check is there so that we skip the test on target
remote, which I think shouldn't be expected to work, as you can't re-run
there.  But I think I must be missing something.

Tom
  
Pedro Alves July 11, 2014, 6:05 p.m. UTC | #3
On 07/11/2014 06:26 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> The comment is stale now.  If we can indeed use gdb_run_cmd,
> Pedro> then I don't think we need the $use_gdb_stub check?
> 
> The use_gdb_stub check is there so that we skip the test on target
> remote, which I think shouldn't be expected to work, as you can't re-run
> there.  But I think I must be missing something.

It was explained in the sentence just below:

> That gets us coverage for "target rem; record; target rem;"
> too.

... because gdb_run_cmd does "target remote" instead of "run"
against remote targets.

In general, if we can write a test in a way that runs against
any target, it's best to not restrict it, even if what is
tested against some target board isn't exactly the bug the
test was written for, as it gives us wider coverage.

In this case, if we write:

 gdb_run_cmd
 gdb_test_multiple "" "restarting inferior" {
     -re "Breakpoint .* main .*${gdb_prompt} " {
 	pass "restarting inferior"
     }
 }

We'll still make sure that "run; record; run" works,
but in addition, we'll also make sure that
"target rem; record; target rem" (and likewise target sim
or whatever means gdb_run_cmd uses to spawn a particular
target) works too.
  

Patch

diff --git a/gdb/target.c b/gdb/target.c
index c9c5e4b..07d029a 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2425,10 +2425,11 @@  target_require_runnable (void)
       if (t->to_create_inferior != NULL)
 	return;
 
-      /* Do not worry about thread_stratum targets that can not
+      /* Do not worry about targets at certain strata that can not
 	 create inferiors.  Assume they will be pushed again if
 	 necessary, and continue to the process_stratum.  */
       if (t->to_stratum == thread_stratum
+	  || t->to_stratum == record_stratum
 	  || t->to_stratum == arch_stratum)
 	continue;
 
diff --git a/gdb/testsuite/gdb.reverse/rerun-prec.c b/gdb/testsuite/gdb.reverse/rerun-prec.c
new file mode 100644
index 0000000..c0f90cb
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/rerun-prec.c
@@ -0,0 +1,21 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+int main (int argc, char **argv)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.reverse/rerun-prec.exp b/gdb/testsuite/gdb.reverse/rerun-prec.exp
new file mode 100644
index 0000000..c168ead
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/rerun-prec.exp
@@ -0,0 +1,44 @@ 
+# Copyright 2014 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/>.  */
+
+if {![supports_reverse] || ![supports_process_record]} {
+    return
+}
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile [list $srcfile]]} {
+    return -1
+}
+
+if {$use_gdb_stub} {
+    unsupported "re-running not supported on this board"
+    return
+}
+
+# The bug is a regression in the sequence "run; record; run".
+runto main
+gdb_test_no_output "record" "Turn on process record"
+
+# We can't use gdb_run_cmd or the like since we need to detect errors.
+gdb_run_cmd
+gdb_test_multiple "" "restarting inferior" {
+    -re "does not support \"run\"" {
+	fail "restarting inferior"
+    }
+    -re "Starting program: .*${gdb_prompt} " {
+	pass "restarting inferior"
+    }
+}