[PATCH] fix record "run" regression

[PATCH] fix record "run" regression

[Tom Tromey]

This fixes the record "run" regression pointed out by Marc Khouzam:

    https://sourceware.org/ml/gdb/2014-06/msg00096.html

The bug is that target_require_runnable must agree with the handling
of the "run" target, but currently it is out of sync.  This patch
fixes the problem by changing target_require_runnable to also ignore
the record_stratum.

Built and regtested on x86-64 Fedora 20.
New test case included.

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

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

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

	* gdb.reverse/run-reverse.c: New file.
	* gdb.reverse/run-reverse.exp: New file.
---
 gdb/ChangeLog                             |  5 ++++
 gdb/target.c                              |  3 +-
 gdb/testsuite/ChangeLog                   |  5 ++++
 gdb/testsuite/gdb.reverse/run-reverse.c   | 21 +++++++++++++
 gdb/testsuite/gdb.reverse/run-reverse.exp | 49 +++++++++++++++++++++++++++++++
 5 files changed, 82 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.reverse/run-reverse.c
 create mode 100644 gdb/testsuite/gdb.reverse/run-reverse.exp

diff --git a/gdb/target.c b/gdb/target.c
index ece59e6..180ec26 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2423,10 +2423,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/run-reverse.c b/gdb/testsuite/gdb.reverse/run-reverse.c
new file mode 100644
index 0000000..fd2c0d7
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/run-reverse.c
@@ -0,0 +1,21 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008-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/run-reverse.exp b/gdb/testsuite/gdb.reverse/run-reverse.exp
new file mode 100644
index 0000000..1412dbb
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/run-reverse.exp
@@ -0,0 +1,49 @@
+# 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 .c
+
+if {[prepare_for_testing $testfile.exp $testfile [list $srcfile]]} {
+    return -1
+}
+
+# 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.
+set ok 1
+send_gdb "start\n"
+gdb_expect 60 {
+    -re "The program .* has been started already.*y or n. $" {
+	send_gdb "y\n"
+	exp_continue
+    }
+    -re "does not support \"run\"" {
+	set ok 0
+    }
+    -notransfer -re "Starting program: \[^\r\n\]*" {
+	set ok 1
+    }
+}
+if {$ok} {
+    pass "restarting inferior"
+} else {
+    fail "restarting inferior"
+}
-- 
1.9.3

Re: [PATCH] fix record "run" regression

[Pedro Alves]

On 07/01/2014 10:45 PM, Tom Tromey wrote:
> This fixes the record "run" regression pointed out by Marc Khouzam:
> 
>     https://sourceware.org/ml/gdb/2014-06/msg00096.html
> 
> The bug is that target_require_runnable must agree with the handling
> of the "run" target, but currently it is out of sync.  This patch
> fixes the problem by changing target_require_runnable to also ignore
> the record_stratum.

Thanks Tom.

> 
> Built and regtested on x86-64 Fedora 20.
> New test case included.
> 
> 2014-07-01  Tom Tromey  <tromey@redhat.com>
> 
> 	* target.c (target_require_runnable): Also check record_stratum.
> 	Update comment.
> 
> 2014-07-01  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.reverse/run-reverse.c: New file.
> 	* gdb.reverse/run-reverse.exp: New file.

This is precord specific, as opposed to (remote) targets
that can reverse themselves, and about re-running -- can I
suggest "rerun-prec.c|exp" ?  (following the existing "precsave"
practice).


> ---
>  gdb/ChangeLog                             |  5 ++++
>  gdb/target.c                              |  3 +-
>  gdb/testsuite/ChangeLog                   |  5 ++++
>  gdb/testsuite/gdb.reverse/run-reverse.c   | 21 +++++++++++++
>  gdb/testsuite/gdb.reverse/run-reverse.exp | 49 +++++++++++++++++++++++++++++++
>  5 files changed, 82 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.reverse/run-reverse.c
>  create mode 100644 gdb/testsuite/gdb.reverse/run-reverse.exp
> 
> diff --git a/gdb/target.c b/gdb/target.c
> index ece59e6..180ec26 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2423,10 +2423,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/run-reverse.c b/gdb/testsuite/gdb.reverse/run-reverse.c
> new file mode 100644
> index 0000000..fd2c0d7
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/run-reverse.c
> @@ -0,0 +1,21 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2008-2014 Free Software Foundation, Inc.

I think you can just use "2014" here.

> +
> +   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/run-reverse.exp b/gdb/testsuite/gdb.reverse/run-reverse.exp
> new file mode 100644
> index 0000000..1412dbb
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/run-reverse.exp
> @@ -0,0 +1,49 @@
> +# 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 .c

I believe ".c" is not necessary.

> +
> +# 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.
> +set ok 1
> +send_gdb "start\n"
> +gdb_expect 60 {

Did you try gdb_test_multiple ?

Then I'd suggest initializing ok to -1, and skipping the last
pass/fail if it is still -1, as gdb_test_multiple will have
already issued a fail in that case.

> +    -re "The program .* has been started already.*y or n. $" {
> +	send_gdb "y\n"
> +	exp_continue
> +    }
> +    -re "does not support \"run\"" {
> +	set ok 0

The test should first probe whether "start" with without
record actually worked.  I think you'll get an error
with --target_board=native-gdbserver as is, because
the first runto main will use something like
"target remote ...; tb main; c" instead of "run/start", while the target
really does not support "run".  I'd replace the first runto main
with explicit "start", and issue unsupported if "does not support"
comes out then.

> +    }
> +    -notransfer -re "Starting program: \[^\r\n\]*" {

I think this should not use -notransfer, and should consume output
until the prompt, otherwise the next test will get confused by getting
the stale prompt.   Might as well make the "does not support" case consume
the prompt too.

> +	set ok 1
> +    }
> +}
> +if {$ok} {
> +    pass "restarting inferior"
> +} else {
> +    fail "restarting inferior"
> +}

Thanks,
Pedro Alves

Re: [PATCH] fix record "run" regression

[Tom Tromey]

Pedro> The test should first probe whether "start" with without
Pedro> record actually worked.  I think you'll get an error
Pedro> with --target_board=native-gdbserver as is, because
Pedro> the first runto main will use something like
Pedro> "target remote ...; tb main; c" instead of "run/start", while the target
Pedro> really does not support "run".  I'd replace the first runto main
Pedro> with explicit "start", and issue unsupported if "does not support"
Pedro> comes out then.

I ended up having it check $use_gdb_stub.

I think this version addresses all your comments.
Let me know what you think.

Tom

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

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

2014-07-03  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 ece59e6..180ec26 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2423,10 +2423,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..cd2305b
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/rerun-prec.exp
@@ -0,0 +1,47 @@
+# 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_test_multiple start "restarting inferior" {
+    -re "The program .* has been started already.*y or n. $" {
+	send_gdb "y\n"
+	exp_continue
+    }
+    -re "does not support \"run\"" {
+	fail "restarting inferior"
+    }
+    -re "Starting program: \[^\r\n\]*" {
+	pass "restarting inferior"
+    }
+}

Re: [PATCH] fix record "run" regression

[Pedro Alves]

On 07/03/2014 07:12 PM, Tom Tromey wrote:
> Pedro> The test should first probe whether "start" with without
> Pedro> record actually worked.  I think you'll get an error
> Pedro> with --target_board=native-gdbserver as is, because
> Pedro> the first runto main will use something like
> Pedro> "target remote ...; tb main; c" instead of "run/start", while the target
> Pedro> really does not support "run".  I'd replace the first runto main
> Pedro> with explicit "start", and issue unsupported if "does not support"
> Pedro> comes out then.
> 
> I ended up having it check $use_gdb_stub.

Thanks, that works too.

> 
> I think this version addresses all your comments.
> Let me know what you think.
> 
> Tom
> 
> 2014-07-03  Tom Tromey  <tromey@redhat.com>
> 
> 	* target.c (target_require_runnable): Also check record_stratum.
> 	Update comment.
> 
> 2014-07-03  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 ece59e6..180ec26 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2423,10 +2423,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..cd2305b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/rerun-prec.exp
> @@ -0,0 +1,47 @@
> +# 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.

Hmm, I think you should be able to.  gdb_run_cmd explicitly
does not consume the prompt (using -notransfer) exactly for these
cases.

 # N.B. This function does not wait for gdb to return to the prompt,
 # that is the caller's responsibility.

 proc gdb_run_cmd {args} {

So we should be able to do:

 gdb_run_cmd
 gdb_test_multiple "" "restarting inferior" {
 ...

> +gdb_test_multiple start "restarting inferior" {
> +    -re "The program .* has been started already.*y or n. $" {
> +	send_gdb "y\n"
> +	exp_continue
> +    }
> +    -re "does not support \"run\"" {
> +	fail "restarting inferior"
> +    }
> +    -re "Starting program: \[^\r\n\]*" {
> +	pass "restarting inferior"
> +    }

You've removed the -notransfer, but any reason you didn't add
the $gdb_prompt matching?

-- 
Pedro Alves

Re: [PATCH] fix record "run" regression

[Tom Tromey]

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.
+gdb_run_cmd
+gdb_test_multiple "" "restarting inferior" {
+    -re "does not support \"run\"" {
+	fail "restarting inferior"
+    }
+    -re "Starting program: .*${gdb_prompt} " {
+	pass "restarting inferior"
+    }
+}

Re: [PATCH] fix record "run" regression

[Pedro Alves]

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,
-- 
Pedro Alves

Re: [PATCH] fix record "run" regression

[Tom Tromey]

>>>>> "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

Re: [PATCH] fix record "run" regression

[Pedro Alves]

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.

-- 
Pedro Alves

Re: [PATCH] fix record "run" regression

[Tom Tromey]

>> 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.

Pedro> It was explained in the sentence just below:

Ok.  Somehow I didn't make this connection.

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

Fully agreed.

New patch appended.

Tom

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

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

2014-07-11  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..995577e
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/rerun-prec.exp
@@ -0,0 +1,35 @@
+# 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
+}
+
+# The bug is a regression in the sequence "run; record; run".
+runto main
+gdb_test_no_output "record" "Turn on process record"
+
+gdb_run_cmd
+gdb_test_multiple "" "restarting inferior" {
+    -re "Breakpoint .*${gdb_prompt} " {
+	pass "restarting inferior"
+    }
+}

Re: [PATCH] fix record "run" regression

[Pedro Alves]

On 07/11/2014 07:10 PM, Tom Tromey wrote:

> New patch appended.

Looks good, thanks!

-- 
Pedro Alves

Re: [PATCH] fix record "run" regression

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 07/11/2014 07:10 PM, Tom Tromey wrote:
>> New patch appended.

Pedro> Looks good, thanks!

Thanks.  I'll push this and the other regression fix to the trunk and
release branch next week.

Tom