[1/2] Fix lwp_suspend/unsuspend imbalance in linux_wait_1
Commit Message
This patch fixes imbalanced lwp_suspend/unsuspend calls caused by the
premature choosing of another event for fairness.
select_event_lwp would switch the event before a call to
unsuspend_all_lwps, thus it would be called with the wrong event.
This caused an assertion failure: unsuspend LWP xx, suspended=-1 when
testing gdb.threads/non-stop-fair-events.exp with ARM range stepping in
GDBServer.
This patch moves the switch of event after the unsuspend/unstop calls.
No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-native.
gdb/gdbserver/ChangeLog:
* linux-low.c (linux_wait_1): Move event switch after unsuspend_lwps.
---
gdb/gdbserver/linux-low.c | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
Comments
On 08/31/2016 06:14 PM, Antoine Tremblay wrote:
> This patch fixes imbalanced lwp_suspend/unsuspend calls caused by the
> premature choosing of another event for fairness.
>
> select_event_lwp would switch the event before a call to
> unsuspend_all_lwps, thus it would be called with the wrong event.
Hmm, that does sound wrong.
Patch LGTM.
Thanks,
Pedro Alves
Pedro Alves writes:
> On 08/31/2016 06:14 PM, Antoine Tremblay wrote:
>> This patch fixes imbalanced lwp_suspend/unsuspend calls caused by the
>> premature choosing of another event for fairness.
>>
>> select_event_lwp would switch the event before a call to
>> unsuspend_all_lwps, thus it would be called with the wrong event.
>
> Hmm, that does sound wrong.
>
> Patch LGTM.
>
Thanks, pushed.
Antoine
On 08/31/2016 06:50 PM, Antoine Tremblay wrote:
>
> Pedro Alves writes:
>
>> On 08/31/2016 06:14 PM, Antoine Tremblay wrote:
>>> This patch fixes imbalanced lwp_suspend/unsuspend calls caused by the
>>> premature choosing of another event for fairness.
>>>
>>> select_event_lwp would switch the event before a call to
>>> unsuspend_all_lwps, thus it would be called with the wrong event.
>>
>> Hmm, that does sound wrong.
>>
>> Patch LGTM.
>>
>
> Thanks, pushed.
Could you push it to 7.12 as well?
I wonder whether this might fix:
https://sourceware.org/bugzilla/show_bug.cgi?id=20176
Thanks,
Pedro Alves
Pedro Alves writes:
> On 08/31/2016 06:50 PM, Antoine Tremblay wrote:
>>
>> Pedro Alves writes:
>>
>>> On 08/31/2016 06:14 PM, Antoine Tremblay wrote:
>>>> This patch fixes imbalanced lwp_suspend/unsuspend calls caused by the
>>>> premature choosing of another event for fairness.
>>>>
>>>> select_event_lwp would switch the event before a call to
>>>> unsuspend_all_lwps, thus it would be called with the wrong event.
>>>
>>> Hmm, that does sound wrong.
>>>
>>> Patch LGTM.
>>>
>>
>> Thanks, pushed.
>
> Could you push it to 7.12 as well?
>
OK, pushed to 7.12
> I wonder whether this might fix:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=20176
>
I hope they will retest.
Antoine Tremblay writes:
> Pedro Alves writes:
>
>> On 08/31/2016 06:50 PM, Antoine Tremblay wrote:
>>>
>>> Pedro Alves writes:
>>>
>>>> On 08/31/2016 06:14 PM, Antoine Tremblay wrote:
>>>>> This patch fixes imbalanced lwp_suspend/unsuspend calls caused by the
>>>>> premature choosing of another event for fairness.
>>>>>
>>>>> select_event_lwp would switch the event before a call to
>>>>> unsuspend_all_lwps, thus it would be called with the wrong event.
>>>>
>>>> Hmm, that does sound wrong.
>>>>
>>>> Patch LGTM.
>>>>
>>>
>>> Thanks, pushed.
>>
>> Could you push it to 7.12 as well?
>>
>
> OK, pushed to 7.12
>
>> I wonder whether this might fix:
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=20176
>>
>
> I hope they will retest.
BTW I always wanted to do this but never get to it, but it seems to me
that it would be nice to have a --fatal-asserts flags in GDB that would
create a core on assert.
That way we could get a backtrace of the assert and know if we fixed a
particular issue like this case.
Regards,
Antoine
On 08/31/2016 08:16 PM, Antoine Tremblay wrote:
> BTW I always wanted to do this but never get to it, but it seems to me
> that it would be nice to have a --fatal-asserts flags in GDB that would
> create a core on assert.
>
> That way we could get a backtrace of the assert and know if we fixed a
> particular issue like this case.
I agree.
Calling exit() as done today is fatal too, so that's a bit ambiguous.
Maybe follow along gdb's "maintenance set internal-error {corefile,quit}",
and call it "--internal-error={corefile,quit}".
Thanks,
Pedro Alves
Pedro Alves writes:
> On 08/31/2016 08:16 PM, Antoine Tremblay wrote:
>
>> BTW I always wanted to do this but never get to it, but it seems to me
>> that it would be nice to have a --fatal-asserts flags in GDB that would
>> create a core on assert.
>>
>> That way we could get a backtrace of the assert and know if we fixed a
>> particular issue like this case.
>
> I agree.
>
> Calling exit() as done today is fatal too, so that's a bit ambiguous.
> Maybe follow along gdb's "maintenance set internal-error {corefile,quit}",
> and call it "--internal-error={corefile,quit}".
>
Good idea and the same with --internal-warning
I'll add it as such to my todo list.
Thanks,
Antoine
@@ -3771,24 +3771,6 @@ linux_wait_1 (ptid_t ptid,
if (!non_stop)
stop_all_lwps (0, NULL);
- /* If we're not waiting for a specific LWP, choose an event LWP
- from among those that have had events. Giving equal priority
- to all LWPs that have had events helps prevent
- starvation. */
- if (ptid_equal (ptid, minus_one_ptid))
- {
- event_child->status_pending_p = 1;
- event_child->status_pending = w;
-
- select_event_lwp (&event_child);
-
- /* current_thread and event_child must stay in sync. */
- current_thread = get_lwp_thread (event_child);
-
- event_child->status_pending_p = 0;
- w = event_child->status_pending;
- }
-
if (step_over_finished)
{
if (!non_stop)
@@ -3813,6 +3795,25 @@ linux_wait_1 (ptid_t ptid,
}
}
+ /* If we're not waiting for a specific LWP, choose an event LWP
+ from among those that have had events. Giving equal priority
+ to all LWPs that have had events helps prevent
+ starvation. */
+ if (ptid_equal (ptid, minus_one_ptid))
+ {
+ event_child->status_pending_p = 1;
+ event_child->status_pending = w;
+
+ select_event_lwp (&event_child);
+
+ /* current_thread and event_child must stay in sync. */
+ current_thread = get_lwp_thread (event_child);
+
+ event_child->status_pending_p = 0;
+ w = event_child->status_pending;
+ }
+
+
/* Stabilize threads (move out of jump pads). */
if (!non_stop)
stabilize_threads ();