diff mbox

[1/2] Fix lwp_suspend/unsuspend imbalance in linux_wait_1

Message ID 20160831171406.24057-1-antoine.tremblay@ericsson.com
State New, archived
Headers

Commit Message

Antoine Tremblay Aug. 31, 2016, 5:14 p.m. UTC 
  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

Pedro Alves Aug. 31, 2016, 5:40 p.m. UTC | #1 
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
Antoine Tremblay Aug. 31, 2016, 5:50 p.m. UTC | #2 
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
Pedro Alves Aug. 31, 2016, 5:52 p.m. UTC | #3 
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
Antoine Tremblay Aug. 31, 2016, 6:24 p.m. UTC | #4 
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 Aug. 31, 2016, 7:16 p.m. UTC | #5 
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
Pedro Alves Sept. 1, 2016, 1:09 p.m. UTC | #6 
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
Antoine Tremblay Sept. 1, 2016, 3:12 p.m. UTC | #7 
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
diff mbox

Patch

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 45061ac..cdff436 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -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 ();