[PATCH RFA] lin-lwp.c pending events.
Michael Snyder
msnyder@cygnus.com
Wed Jun 6 15:05:00 GMT 2001
Mark Kettenis wrote:
>
> Date: Mon, 04 Jun 2001 20:29:01 -0700
> From: Michael Snyder <msnyder@cygnus.com>
>
> 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 <kettenis@gnu.org>
>
> * 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;
> }
More information about the Gdb-patches
mailing list