Re: [PATCH v3 10/17] Implement all-stop on top of a target running non-stop mode

Pedro Alves
2015-04-22T20:16:00Z

On 04/21/2015 12:08 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>> gdb/ChangeLog:
>> 2015-04-17 Pedro Alves <palves@redhat.com>
>>
>> * NEWS: Mention "maint set/show target-non-stop".
>> * breakpoint.c (update_global_location_list): Check
>> target_is_non_stop_p instead of non_stop.
>> * infcmd.c (attach_command_post_wait, attach_command): Likewise.
>> * infrun.c (show_can_use_displaced_stepping)
>> (can_use_displaced_stepping_p, start_step_over_inferior):
>> Likewise.
>> (resume): Always resume a single thread if the target is in
>> non-stop mode.
>> (proceed): Check target_is_non_stop_p instead of non_stop. If in
>> all-mode but the target is always in non-stop mode, start all the
> ^^^^^^^^
>
> all-stop mode.

Thanks, fixed.

>
>> (handle_signal_stop) <random signal>: If we get a signal while
>> stepping over a breakpoint, and the target is always in non-stop
>> mode, restart all threads.
>
>> @@ -5614,6 +5666,17 @@ handle_signal_stop (struct execution_control_state *ecs)
>> /* Reset trap_expected to ensure breakpoints are re-inserted. */
>> ecs->event_thread->control.trap_expected = 0;
>>
>> + if (!non_stop && target_is_non_stop_p ())
>> + {
>
> This path is about the case that a signal is got while in in-line
> stepping, isn't? If so, non_stop should be an invariant false. We
> don't need to check it.

Hmm, not sure what you mean:

- We need to do this with displaced stepping too, because we can't
deliver signals while doing a displaced step. See comments at the
top of displaced_step_prepare and in do_target_resume.

- We can certainly get a signal while doing an in-line step-over.
The simplest would be, trying to step-over a breakpoint here:

*(volatile int *)0 = 1;

which usually results SIGSEGV.

- We can step past breakpoints in-line in non-stop mode too.

However, I agree there's something wrong here. If we get here
with "set non-stop on", and we were doing an in-line step-over,
then we also need to restart threads, not just
in all-stop + "target always-non-stop". I've applied this change
on top now, and it tested OK on x86-64, native with
both displaced on/off, and gdbserver.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index e236510..3e60313 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5732,6 +5732,8 @@ handle_signal_stop (struct execution_control_state *ecs)
&& ecs->event_thread->control.trap_expected
&& ecs->event_thread->control.step_resume_breakpoint == NULL)
{
+ int was_in_line;
+
/* We were just starting a new sequence, attempting to
single-step off of a breakpoint and expecting a SIGTRAP.
Instead this signal arrives. This signal will take us out
@@ -5747,20 +5749,29 @@ handle_signal_stop (struct execution_control_state *ecs)
"infrun: signal arrived while stepping over "
"breakpoint\n");

+ was_in_line = step_over_info_valid_p ();
clear_step_over_info ();
insert_hp_step_resume_breakpoint_at_frame (frame);
ecs->event_thread->step_after_step_resume_breakpoint = 1;
/* Reset trap_expected to ensure breakpoints are re-inserted. */
ecs->event_thread->control.trap_expected = 0;

- if (!non_stop && target_is_non_stop_p ())
+ if (target_is_non_stop_p ())
{
keep_going (ecs);

- /* We've canceled the step-over temporarily while the
- signal handler executes. Let other threads run,
- according to schedlock. */
- restart_threads (ecs->event_thread);
+ /* The step-over has been canceled temporarily while the
+ signal handler executes. */
+ if (was_in_line)
+ {
+ /* We had paused all threads, restart them. */
+ restart_threads (ecs->event_thread);
+ }
+ else if (debug_infrun)
+ {
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: no need to restart threads\n");
+ }
return;
}

--
1.9.3

I've now folded that into the patch that teaches non-stop about
in-line step overs.

>> + keep_going (ecs);
>> +
>> + /* We've canceled the step-over temporarily while the
>> + signal handler executes. Let other threads run,
>> + according to schedlock. */
>
> IMO, we don't need to run other threads according to schedlock. GDB
> resumes some threads here and operations should be invisible to user,
> schedlock, as a user visible option, shouldn't affect what threads
> should be resumed. On the other hand, restart_threads is to undo the
> changes done by stop_all_threads, so no user preference (schedlock)
> should be considered here.

Yes, you're right, thanks for noticing this. The code in restart_threads does
not in fact look at schedlock (through e.g., user_visible_resume_ptid) at
all, it's the comment that is stale from a previous attempt, from
before https://sourceware.org/ml/gdb-patches/2015-03/msg00293.html.
It was issues like that led to that schedlock patch, even.