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

[Pedro Alves <pedro@palves.net>]

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