Re: [PATCH] fix record "run" regression

[Pedro Alves <palves@redhat.com>]

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