From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Kettenis To: msnyder@cygnus.com Cc: gdb-patches@sources.redhat.com Subject: Re: [PATCH RFA] lin-lwp.c pending events. Date: Wed, 06 Jun 2001 12:52:00 -0000 Message-id: <200106061952.f56JqcL03531@delius.kettenis.local> References: <3B1C51FD.95EED165@cygnus.com> X-SW-Source: 2001-06/msg00091.html Date: Mon, 04 Jun 2001 20:29:01 -0700 From: Michael Snyder Mark, even though we are 'co-maintainers' of this code, I do like to get your approval and feedback. Linus threads are so horrendously complicated, it's good to have two pairs of eyes. Defenitely! This change catches the somewhat rare circumstance where GDB calls target_resume with a specific PTID, but then calls target_wait with a wild-card. You could argue that GDB shouldn't do that, but it turns out that it would be very difficult to prevent it. When it happens, we want to make sure that we don't try to handle a 'pending' event from a different LWP, because breakpoints may not be inserted and that different LWP will run away. Sounds reasonable. I'm not quite happy with your solution though: I'm a bit allergic to global variables like solo_resume_pid. I'd prefer adding a flag to `struct lwp_info', say `resumed', that would record whether the LWP has been resumed or not. How about the attached patch? In light of your remark above I think we should revert part of your 2001-05-25 patch. The code dealing with newly attached threads in lin_lwp_wait isn't used when we use the thread_db layer so I don't think your patch had any effect anyway. I left a comment in place. Mark Index: ChangeLog from Mark Kettenis * lin-lwp.c (struct lwp_info): Add member `resumed'. (iterate_over_lwps): Make sure we can handle CALLBACK deleting the LWP it's called for. (lin_lwp_attach): Mark LWP as resumed to make sure the fake SIGSTOP is reported. (resume_clear_callback): New function. (resume_set_callback): New function. (lin_lwp_resume): Mark all LWP's that we're going to resume as resumed, and unmark all others. (status_callback): Only report a pending wait status if we pretend that LP has been resumed. (resumed_callback): New function. (lin_lwp_wait): Add assertions to check that LWP's are properly marked as resumed. Partially revert 2001-05-25 patch by Michael Snyder: do not resume all threads. Add comment explaining the problems associated with this bit of code. Index: lin-lwp.c =================================================================== RCS file: /cvs/src/src/gdb/lin-lwp.c,v retrieving revision 1.23 diff -u -p -r1.23 lin-lwp.c --- lin-lwp.c 2001/06/06 16:31:32 1.23 +++ lin-lwp.c 2001/06/06 19:45:09 @@ -82,6 +82,14 @@ struct lwp_info /* Non-zero if this LWP is stopped. */ int stopped; + /* Non-zero if this LWP will be/has been resumed. Note that an LWP + can be marked both as stopped and resumed at the same time. This + happens if we try to resume an LWP that has a wait status + pending. We shouldn't let the LWP run until that wait status has + been processed, but we should not report that wait status if GDB + didn't try to let the LWP run. */ + int resumed; + /* If non-zero, a pending wait status. */ int status; @@ -249,11 +257,14 @@ find_lwp_pid (ptid_t ptid) struct lwp_info * iterate_over_lwps (int (*callback) (struct lwp_info *, void *), void *data) { - struct lwp_info *lp; + struct lwp_info *lp, *lpnext; - for (lp = lwp_list; lp; lp = lp->next) - if ((*callback) (lp, data)) - return lp; + for (lp = lwp_list; lp; lp = lpnext) + { + lpnext = lp->next; + if ((*callback) (lp, data)) + return lp; + } return NULL; } @@ -357,6 +368,7 @@ lin_lwp_attach (char *args, int from_tty /* Fake the SIGSTOP that core GDB expects. */ lp->status = W_STOPCODE (SIGSTOP); + lp->resumed = 1; } static int @@ -475,6 +487,20 @@ resume_callback (struct lwp_info *lp, vo return 0; } +static int +resume_clear_callback (struct lwp_info *lp, void *data) +{ + lp->resumed = 0; + return 0; +} + +static int +resume_set_callback (struct lwp_info *lp, void *data) +{ + lp->resumed = 1; + return 0; +} + static void lin_lwp_resume (ptid_t ptid, int step, enum target_signal signo) { @@ -487,6 +513,11 @@ lin_lwp_resume (ptid_t ptid, int step, e processes, but give the signal only to this one'. */ resume_all = (PIDGET (ptid) == -1) || !step; + if (resume_all) + iterate_over_lwps (resume_set_callback, NULL); + else + iterate_over_lwps (resume_clear_callback, NULL); + /* If PID is -1, it's the current inferior that should be handled specially. */ if (PIDGET (ptid) == -1) @@ -500,6 +531,9 @@ lin_lwp_resume (ptid_t ptid, int step, e /* Remember if we're stepping. */ lp->step = step; + /* Mark this LWP as resumed. */ + lp->resumed = 1; + /* If we have a pending wait status for this thread, there is no point in resuming the process. */ if (lp->status) @@ -663,7 +697,9 @@ stop_wait_callback (struct lwp_info *lp, static int status_callback (struct lwp_info *lp, void *data) { - return (lp->status != 0); + /* Only report a pending wait status if we pretend that this has + indeed been resumed. */ + return (lp->status != 0 && lp->resumed); } /* Return non-zero if LP isn't stopped. */ @@ -674,6 +710,14 @@ running_callback (struct lwp_info *lp, v return (lp->stopped == 0); } +/* Return non-zero if LP has been resumed. */ + +static int +resumed_callback (struct lwp_info *lp, void *data) +{ + return lp->resumed; +} + static ptid_t lin_lwp_wait (ptid_t ptid, struct target_waitstatus *ourstatus) { @@ -691,6 +735,9 @@ lin_lwp_wait (ptid_t ptid, struct target retry: + /* Make sure there is at least one thread that has been resumed. */ + gdb_assert (iterate_over_lwps (resumed_callback, NULL)); + /* First check if there is a LWP with a wait status pending. */ if (pid == -1) { @@ -754,6 +801,7 @@ lin_lwp_wait (ptid_t ptid, struct target child_resume (pid_to_ptid (GET_LWP (lp->ptid)), lp->step, TARGET_SIGNAL_0); lp->stopped = 0; + gdb_assert (lp->resumed); /* This should catch the pending SIGSTOP. */ stop_wait_callback (lp, NULL); @@ -840,6 +888,7 @@ lin_lwp_wait (ptid_t ptid, struct target child_resume (pid_to_ptid (GET_LWP (lp->ptid)), lp->step, TARGET_SIGNAL_0); lp->stopped = 0; + gdb_assert (lp->resumed); /* Discard the event. */ status = 0; @@ -883,14 +932,13 @@ lin_lwp_wait (ptid_t ptid, struct target && signal_print_state (signo) == 0 && signal_pass_state (signo) == 1) { - /* First mark this LWP as "not stopped", so that - resume_callback will not resume it. */ - lp->stopped = 0; - /* Resume all threads except this one - (mainly to get the newly attached ones). */ - iterate_over_lwps (resume_callback, NULL); - /* Now resume this thread, forwarding the signal to it. */ + /* FIMXE: kettenis/2001-06-06: Should we resume all threads + here? It is not clear we should. GDB may not expect + other threads to run. On the other hand, not resuming + newly attached threads may cause an unwanted delay in + getting them running. */ child_resume (pid_to_ptid (GET_LWP (lp->ptid)), lp->step, signo); + lp->stopped = 0; status = 0; goto retry; }