Sync thread state after infcalls with "set unwind-on-* on" (PR gdb/34148)
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Test passed
|
Commit Message
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(-)
base-commit: f6c1ca239d932db39a6f19d9bd343f4f4fddba76
Comments
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
@@ -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));
@@ -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" } {
@@ -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"
@@ -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.*" \