From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Snyder To: Mark Kettenis Cc: gdb-patches@sources.redhat.com Subject: Re: [PATCH RFA] lin-lwp.c pending events. Date: Wed, 06 Jun 2001 15:05:00 -0000 Message-id: <3B1EA910.C5415573@cygnus.com> References: <3B1C51FD.95EED165@cygnus.com> <200106061952.f56JqcL03531@delius.kettenis.local> X-SW-Source: 2001-06/msg00100.html Mark Kettenis wrote: > > 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. I'm fine with this -- only I'm curious why you didn't simply let resume_callback set the resumed flag. Please feel free to check it in. Michael > > 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; > }