Re: [PATCH] Prevent turning record on while threads are running (PR 20869)

[Luis Machado <lgustavo@codesourcery.com>]

On 11/29/2016 09:07 AM, Simon Marchi wrote:
> As stated in the bug description, trying to do "record" while threads
> are running leads to a broken state.  I tried to see in the code if
> there was anything to try to support the use case of enabling record
> while the program is running, but didn't find anything.
>
> When full/software recording is active, we are single stepping each
> instruction.  Transitioning from the state where threads are running
> freely (no recording) to recording enabled should therefore involve a
> step where we stop them to initiate the single stepping.  I can't find
> anything that looks like this, so my conclusion is that this was never a
> supported use case (please correct me if I'm wrong).
>
> The easy solution is to prevent the user for enabling record if a thread
> is running.
>
> I also wanted to know whether it was supported to start btrace bts/pt
> recording with threads running.  When I try it with btrace bts, it works
> halfway.  "record btrace bts" gives me the error:
>
>   Couldn't get registers: No such process.
>
> If I do the command again, gdb doesn't complain and then I'm able to
> interrupt the target and use reverse-next.  However, since the
> initialization function was interrupted halfway, I am not sure that
> everything is setup correctly.  If we want to allow it, we would first
> need to look into this issue.
>
> I have therefore put a check for running threads in record_preopen,
> which affects all recording methods.  It can always be moved to
> record_full_open if we only want to affect record full.
>
> No regression on the buildbot.
>
> gdb/ChangeLog:
>
> 	* record.c: Include gdbthread.h.
> 	(record_preopen): Check if any thread is running.
>
> gdb/testsuite:
>
> 	* gdb.reverse/record-while-running.c: New file.
> 	* gdb.reverse/record-while-running.exp: New file.
> ---
>  gdb/record.c                                       | 11 ++++++
>  gdb/testsuite/gdb.reverse/record-while-running.c   | 29 ++++++++++++++++
>  gdb/testsuite/gdb.reverse/record-while-running.exp | 40 ++++++++++++++++++++++
>  3 files changed, 80 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.reverse/record-while-running.c
>  create mode 100644 gdb/testsuite/gdb.reverse/record-while-running.exp
>
> diff --git a/gdb/record.c b/gdb/record.c
> index 34ebd1b..e0bc133 100644
> --- a/gdb/record.c
> +++ b/gdb/record.c
> @@ -26,6 +26,7 @@
>  #include "common/common-utils.h"
>  #include "cli/cli-utils.h"
>  #include "disasm.h"
> +#include "gdbthread.h"
>
>  #include <ctype.h>
>
> @@ -89,6 +90,16 @@ record_preopen (void)
>    if (find_record_target () != NULL)
>      error (_("The process is already being recorded.  Use \"record stop\" to "
>  	     "stop recording first."));
> +
> +  iterate_over_threads([] (struct thread_info *tp, void *) -> int {
> +    if (tp->state == thread_state::THREAD_RUNNING)
> +      error (_ ("Can't enable record while the program is running.  Use "
> +		"\"interrupt\" to stop it first."));
> +
> +    return 0;
> +  }, NULL);
> +
> +
>  }
>
>  /* See record.h.  */
> diff --git a/gdb/testsuite/gdb.reverse/record-while-running.c b/gdb/testsuite/gdb.reverse/record-while-running.c
> new file mode 100644
> index 0000000..f00ceb6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/record-while-running.c
> @@ -0,0 +1,29 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2016 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 <unistd.h>
> +
> +int
> +main ()
> +{
> +   int i;
> +
> +   for (i = 0; i < 30; i++)
> +      sleep (1);
> +
> +   return 0;
> +}
> diff --git a/gdb/testsuite/gdb.reverse/record-while-running.exp b/gdb/testsuite/gdb.reverse/record-while-running.exp
> new file mode 100644
> index 0000000..57e1df3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/record-while-running.exp
> @@ -0,0 +1,40 @@
> +# Copyright 2016 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/>.  */
> +
> +# Test that trying to turn on recording while the target is running is correctly
> +# handled.
> +
> +if ![supports_reverse] {

Add an explicit untested call here?

> +    return
> +}
> +
> +standard_testfile
> +
> +if { [prepare_for_testing $testfile.exp $testfile $srcfile] } {
> +    fail "failed to compile"
> +    return
> +}
> +
> +if { ![runto_main] } {
> +    fail "couldn't run to main"
> +    return
> +}
> +
> +proc test_record_while_running { } {
> +    gdb_test "continue &" "Continuing."
> +    gdb_test "record" "Can't enable record while the program is running.  Use \"interrupt\" to stop it first."

I have mixed feelings with the above test names. I'd know what to look 
for in case of failure, but more explicit test names wouldn't hurt for a 
quick inspection of the logs.

"move thread"
"switch record on when thread is moving"

Feel free to pick it up though. Not a hard requirement.