Re: [PATCH 2/4] gdb, infrun, record: fix hang when step-over fails with no-history

[Pedro Alves <pedro@palves.net>]

On 2021-03-16 09:34, Markus Metzger via Gdb-patches wrote:
> When trying to step over a breakpoint at the end of the trace while
> another thread is replaying, the step-over will fail with no-history.

Some information seems missing here.  The no-history event is for the other thread that
is replaying, right?  We don't use displaced stepping when using a record target, so why did that
other thread report the no-history event?  Without displaced stepping, GDB will only resume the
thread stepping over the breakpoint, so I'm confused on how the other thread's position
in the replay log affects the stepping thread.

> This does not clear step_over_info so a subsequent resume will cause GDB
> to not resume the thread and expect a SIGTRAP to complete the step-over.
> This will never come causing GDB to hang in the wait-for-event poll.
> 
> This is a variant of the issue fixed in the parent commit.  That commit
> addressed the issue for a single-threaded process and fixed an issue with
> reverse/replay stepping in general.
> 
> This commit addresses the issue for a multi-threaded process.  In this
> case, the single-step does not complete.
> 
> Finish an in-flight step-over when a thread stopped with NO_HISTORY.
> Since we did not move, we will simply start the step-over again.
> 
> gdb/ChangeLog:
> 2021-03-02  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* infrun.c (finish_step_over): New declaration.
> 	(handle_inferior_event): Call finish_step_over.
> 
> gdb/testsuite/ChangeLog:
> 2021-03-02  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* gdb.btrace/multi-thread-break-hang.exp: New file.
> ---
>  gdb/infrun.c                                  |  6 ++
>  .../gdb.btrace/multi-thread-break-hang.exp    | 92 +++++++++++++++++++
>  2 files changed, 98 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 591fda93d21..e79094ad2b2 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -95,6 +95,8 @@ static void resume (gdb_signal sig);
>  
>  static void wait_for_inferior (inferior *inf);
>  
> +static int finish_step_over (struct execution_control_state *ecs);
> +
>  /* Asynchronous signal handler registered as event loop source for
>     when we have pending events ready to be passed to the core.  */
>  static struct async_event_handler *infrun_async_inferior_event_token;
> @@ -5543,6 +5545,10 @@ handle_inferior_event (struct execution_control_state *ecs)
>  	return;
>  
>        gdb::observers::no_history.notify ();
> +
> +      /* Cancel an in-flight step-over.  It will not succeed since we
> +	 won't be able to step at the end of the execution history.  */
> +      finish_step_over (ecs);
>        stop_waiting (ecs);
>        return;
>      }
> diff --git a/gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp b/gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
> new file mode 100644
> index 00000000000..8496de5b36c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/multi-thread-break-hang.exp
> @@ -0,0 +1,92 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2021 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 we cancel an in-flight step-over at the end of the execution
> +# history as long as some other thread is still replaying.
> +#
> +# This used to cause GDB to hang in poll ().
> +
> +if { [skip_btrace_tests] } {
> +    unsupported "target does not support record-btrace"
> +    return -1
> +}
> +
> +standard_testfile multi-thread-step.c
> +if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug libs=-lpthread}] {

Should use the "pthreads" option as in "{debug pthreads}" instead of linking
with -lpthread directly.

> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "failed to run to main"
> +    return -1
> +}
> +
> +# Set up breakpoints.
> +set bp_1 [gdb_get_line_number "bp.1" $srcfile]
> +set bp_2 [gdb_get_line_number "bp.2" $srcfile]
> +
> +# Trace the code between the two breakpoints.
> +gdb_breakpoint $srcfile:$bp_1
> +gdb_continue_to_breakpoint "continue to bp.1" ".*$srcfile:$bp_1\r\n.*"
> +
> +# Make sure GDB knows about the new thread.

This is not necessary nowadays, as normal_stop always calls update_thread_list.

> +gdb_test "info threads" ".*"
> +gdb_test_no_output "record btrace"
> +
> +# We have two threads at or close to bp.1 but handled only one stop event.
> +# Remove the breakpoint so we do not need to deal with the 2nd event.
> +delete_breakpoints
> +gdb_breakpoint $srcfile:$bp_2
> +gdb_continue_to_breakpoint "continue to bp.2" ".*$srcfile:$bp_2\r\n.*"
> +
> +# Determine the thread that reported the breakpoint.
> +set thread "bad"
> +gdb_test_multiple "thread" "thread" {
> +    -re "Current thread is \($decimal\).*\r\n$gdb_prompt $" {
> +	set thread $expect_out(1,string)
> +    }
> +}

This is fine, but FYI, it could be done with a single liner:

  set thread [get_integer_valueof "\$_thread" bad]

> +
> +# Determine the other thread.
> +set other "bad"
> +if { $thread == 1 } {
> +    set other 2
> +} elseif { $thread == 2 } {
> +    set other 1
> +}
> +
> +# This test works for scheduler-locking 'on' or 'step'; 'replay' would
> +# implicitly stop replaying, avoiding the problem; 'off' would step one
> +# and resume the other.  The test would work for the current lock-step
> +# implementation.

What does the last sentence about "lock-step" mean?  Also, I find "work" ambiguous.
I take it here is means "exercises the original bug".  How about saying that?

> +gdb_test_no_output "set scheduler-locking step"
> +
> +# Start replaying the other thread.  This will prevent stepping the thread
> +# that reported the event.

What is the "this" in "This will prevent"?  Is it the use of "record goto" you're
referring to, or something else?

> +gdb_test "thread apply $other record goto begin" ".*"
> +gdb_test "thread apply $other info record" "Replay in progress.*"
> +
> +# We're at a breakpoint so this triggers step-over.  Since we're at the
> +# end of the trace, the step will fail.

What is "we" here?  It's the "other" thread, not the stepping thread, right?
Please clarify the comments.

> +gdb_test "stepi" "No more reverse-execution history.*" "stepi.1"
> +
> +# We used to hang at the second step since step-over insisted on polling
> +# the next event.
> +gdb_test "stepi" "No more reverse-execution history.*" "stepi.2"
> +
> +# Do one more just in case.
> +gdb_test "stepi" "No more reverse-execution history.*" "stepi.3"
>