Re: [PATCH] Sync thread state after infcalls with "set unwind-on-* on" (PR gdb/34148)

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> Commit 519774805a1 ("Don't pretend infcalls don't set the inferior
> running (PR gdb/34082)") removed the special case in proceed that
> skipped set_state(THREAD_RUNNING) for infcalls.  That fixed
> gdb.threads/hand-call-new-thread.exp, but introduced a regression in
> gdb.compile/compile.exp:
>
>  ...
>  set unwind-on-signal on
>  (gdb) PASS: gdb.compile/compile.exp: set unwind-on-signal on
>  compile code *(volatile int *) 0 = 0;
>  The program being debugged received signal SIGSEGV, Segmentation fault
>  while in a function called from GDB.  GDB has restored the context
>  to what it was before the call.  To change this behavior use
>  "set unwind-on-signal off".  Evaluation of the expression containing
>  the function (_gdb_expr) will be abandoned.
>  (gdb) PASS: gdb.compile/compile.exp: compile code segfault second
>  break 132
>  Breakpoint 2 at 0x555555555262: file .../compile.c, line 132.
>  (gdb) continue
>  Cannot execute this command while the selected thread is running.
>  (gdb) FAIL: gdb.compile/compile.exp: continue to breakpoint: break-here
>
> The "compile code" command before the FAIL is an infcall under the
> hood.  That hits SIGSEGV with "set unwind-on-signal on" in effect, so
> GDB unwinds and abandons the call.  After that, "continue" is rejected
> because the thread is still marked THREAD_RUNNING from the proceed
> that started the infcall.
>
> When an infcall is unwound due to a signal, timeout, or terminating
> exception, call_thread_fsm::should_notify_stop returns false, and so
> normal_stop is not called from fetch_inferior_event.  normal_stop is
> what would normally call finish_thread_state to sync the public thread
> state back to THREAD_STOPPED.  run_inferior_call has a fallback
> finish_thread_state call for that purpose, but it is gated on
> stop_stack_dummy == STOP_STACK_DUMMY, which is only true for
> successful calls.
>
> Before the commit mentioned above, proceed never marked an infcall's
> thread as THREAD_RUNNING, so the missing RUNNING => STOPPED transition
> was harmless.  The old comment in infcall.c about the
> finish_thread_state call claimed "If the infcall does NOT succeed,
> normal_stop will have already finished the thread states", but that
> was already incorrect for the unwind paths.  It just happened to not
> matter.
>
> Fix this by dropping the STOP_STACK_DUMMY guard and updating the
> comment to describe the actual rule: sync regardless of how the call
> ended.  The !was_running check is kept since it is there to exclude
> the in-cond-eval case, where the thread is meant to stay marked
> running.  finish_thread_state is idempotent, so the call is harmless
> on paths where normal_stop also ran.
>
> Extend gdb.base/unwindonsignal.exp to exercise the "set
> unwind-on-signal on" path without having to rely on the "compile code"
> feature.  Without the fix, the test fails like so:
>
>  info threads
>    Id   Target Id                                           Frame
>  * 1    Thread 0x7ffff7f8f740 (LWP 239019) "unwindonsignal" (running)
>  (gdb) FAIL: gdb.base/unwindonsignal.exp: thread is stopped
>  continue
>  Cannot execute this command while the selected thread is running.
>  (gdb) FAIL: gdb.base/unwindonsignal.exp: continue until exit at after unwound infcall
>
> Similarly, extend gdb.cp/gdb2495.exp for "set
> unwind-on-terminating-exception on", and gdb.base/infcall-timeout.exp
> for "set unwind-on-timeout on".  Both would fail without the code fix,
> too.
>
> With the fix, gdb.compile/compile.exp now passes cleanly.
>
> Tested on x86_64-unknown-linux-gnu.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34148

Thanks for fixing this, and for the great explanation.  This LGTM.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew



> Change-Id: Idef0dcd4dd751b501869c58b752f77d4dadb6c72
> commit-id: 1a8b36d4
> ---
>  gdb/infcall.c                              | 23 ++++++++++++++--------
>  gdb/testsuite/gdb.base/infcall-timeout.exp | 13 ++++++++++++
>  gdb/testsuite/gdb.base/unwindonsignal.exp  | 10 ++++++++++
>  gdb/testsuite/gdb.cp/gdb2495.exp           |  6 ++++++
>  4 files changed, 44 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index 8b26f541de6..e6b24ff5310 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -918,12 +918,20 @@ run_inferior_call (std::unique_ptr<call_thread_fsm> sm,
>        current_ui->register_file_handler ();
>      }
>  
> -  /* If the infcall does NOT succeed, normal_stop will have already
> -     finished the thread states.  However, on success, normal_stop
> -     defers here, so that we can set back the thread states to what
> -     they were before the call.  Note that we must also finish the
> -     state of new threads that might have spawned while the call was
> -     running.  The main cases to handle are:
> +  /* Sync the user/frontend thread states from the internal thread
> +     states.  proceed marked threads in resume_ptid as THREAD_RUNNING
> +     for this infcall; we must now sync them back, regardless of how
> +     the call ended.  For the success path and for the unwind paths
> +     (unwind-on-{signal,timeout,terminating-exception}),
> +     call_thread_fsm::should_notify_stop returns false and normal_stop
> +     is skipped -- this call is the canonical place to do the sync.
> +     For other failure paths normal_stop does run and has already
> +     finished the thread state; finish_thread_state is idempotent, so
> +     calling it again here is harmless.  Note that we must also finish
> +     the state of new threads that might have spawned while the call
> +     was running.
> +
> +     The main cases to handle are:
>  
>       - "(gdb) print foo ()", or any other command that evaluates an
>       expression at the prompt.  (The thread was marked stopped before.)
> @@ -934,8 +942,7 @@ run_inferior_call (std::unique_ptr<call_thread_fsm> sm,
>       evaluates true and thus we'll present a user-visible stop is
>       decided elsewhere.  */
>    if (!was_running
> -      && call_thread_ptid == inferior_ptid
> -      && stop_stack_dummy == STOP_STACK_DUMMY)
> +      && call_thread_ptid == inferior_ptid)
>      finish_thread_state (call_thread->inf->process_target (),
>  			 user_visible_resume_ptid (0));
>  
> diff --git a/gdb/testsuite/gdb.base/infcall-timeout.exp b/gdb/testsuite/gdb.base/infcall-timeout.exp
> index 37aa6c0ef54..99d29624337 100644
> --- a/gdb/testsuite/gdb.base/infcall-timeout.exp
> +++ b/gdb/testsuite/gdb.base/infcall-timeout.exp
> @@ -86,6 +86,19 @@ proc run_test { target_async target_non_stop non_stop unwind } {
>  	gdb_test "bt" \
>  	    ".* function_that_never_returns .*<function called from gdb>.*"
>      }
> +
> +    # After the infcall, the thread should be stopped.  Regression
> +    # test for PR gdb/34148.
> +    if {$non_stop} {
> +	# Check the main thread only, in case we have system-spawned
> +	# threads.
> +	set thread_filter "1"
> +    } else {
> +	set thread_filter ""
> +    }
> +    gdb_test "info threads -running $thread_filter" \
> +	"No threads matched\\." \
> +	"thread is stopped"
>  }
>  
>  foreach_with_prefix target_async { "on" "off" } {
> diff --git a/gdb/testsuite/gdb.base/unwindonsignal.exp b/gdb/testsuite/gdb.base/unwindonsignal.exp
> index aed8ef6f4c7..2ae8cc4a6dd 100644
> --- a/gdb/testsuite/gdb.base/unwindonsignal.exp
> +++ b/gdb/testsuite/gdb.base/unwindonsignal.exp
> @@ -86,3 +86,13 @@ gdb_test_multiple "maint print dummy-frames" \
>  	pass $gdb_test_name
>      }
>  }
> +
> +# After the unwound infcall, the thread should be stopped, and a
> +# subsequent resumption command should be accepted.  Regression test
> +# for PR gdb/34148.
> +
> +gdb_test "info threads -running" \
> +    "No threads matched\\." \
> +    "thread is stopped"
> +
> +gdb_continue_to_end "after unwound infcall"
> diff --git a/gdb/testsuite/gdb.cp/gdb2495.exp b/gdb/testsuite/gdb.cp/gdb2495.exp
> index aa5a2a16e43..60396494161 100644
> --- a/gdb/testsuite/gdb.cp/gdb2495.exp
> +++ b/gdb/testsuite/gdb.cp/gdb2495.exp
> @@ -64,6 +64,12 @@ gdb_test "p exceptions.throw_function()" \
>      "The program being debugged entered a std::terminate call, .*" \
>      "call a function that raises an exception without a handler."
>  
> +# Make sure that the thread is stopped.  Regression test for PR
> +# gdb/34148.
> +gdb_test "info threads -running" \
> +    "No threads matched\\." \
> +    "thread is stopped"
> +
>  # Make sure that after rewinding we are back at the call parent.
>  gdb_test "bt" \
>      "#0  main.*" \
>
> base-commit: f6c1ca239d932db39a6f19d9bd343f4f4fddba76
> -- 
> 2.53.0