Re: [PATCH RFA] lin-lwp.c pending events.

Re: [PATCH RFA] lin-lwp.c pending events.

[Mark Kettenis]

   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.

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;
 	}

Re: [PATCH RFA] lin-lwp.c pending events.

[Michael Snyder]

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;
>         }

Re: [PATCH RFA] lin-lwp.c pending events.

[Mark Kettenis]

   Date: Wed, 06 Jun 2001 15:05:04 -0700
   From: Michael Snyder <msnyder@cygnus.com>

   I'm fine with this -- only I'm curious why you didn't simply let
   resume_callback set the resumed flag.

I went through that stage, but resume_callback isn't called when
lp->status is nonzero.

   Please feel free to check it in.

Done.

[PATCH RFA] lin-lwp.c pending events.

[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.

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.
2001-06-04  Michael Snyder  <msnyder@redhat.com>

	* lin-lwp.c (lin_lwp_resume): Set global 'solo_resume_pid' if
	resuming only one LWP.
	(lin_lwp_wait): Use global 'solo_resume_pid' to reject pending
	events if they belong to the wrong LWP.

Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.20
diff -c -3 -p -r1.20 lin-lwp.c
*** lin-lwp.c	2001/05/15 00:13:47	1.20
--- lin-lwp.c	2001/06/05 02:58:29
*************** resume_callback (struct lwp_info *lp, vo
*** 468,473 ****
--- 468,475 ----
    return 0;
  }
  
+ static pid_t solo_resume_pid = -1;
+ 
  static void
  lin_lwp_resume (ptid_t ptid, int step, enum target_signal signo)
  {
*************** lin_lwp_resume (ptid_t ptid, int step, e
*** 479,484 ****
--- 481,490 ----
       id'.  But if STEP is zero, then PID means `continue *all*
       processes, but give the signal only to this one'.  */
    resume_all = (PIDGET (ptid) == -1) || !step;
+   if (resume_all)
+     solo_resume_pid = -1;
+   else
+     solo_resume_pid = GET_LWP (ptid);
  
    /* If PID is -1, it's the current inferior that should be
       handled special.  */
*************** lin_lwp_wait (ptid_t ptid, struct target
*** 670,678 ****
    /* First check if there is a LWP with a wait status pending.  */
    if (pid == -1)
      {
!       /* Any LWP will do.  */
        lp = iterate_over_lwps (status_callback, NULL);
!       if (lp)
  	{
  	  if (debug_lin_lwp)
  	    fprintf_unfiltered (gdb_stdlog, 
--- 676,688 ----
    /* First check if there is a LWP with a wait status pending.  */
    if (pid == -1)
      {
!       /* We are asked to wait for any LWP.  */
        lp = iterate_over_lwps (status_callback, NULL);
!       /* However, if we resumed only one LWP, then we should accept 
! 	 a pending wait status only for the same LWP.  Otherwise we may 
!          really confuse GDB.  */
!       if (lp &&
! 	  (solo_resume_pid == -1 || solo_resume_pid == GET_LWP (lp->ptid)))
  	{
  	  if (debug_lin_lwp)
  	    fprintf_unfiltered (gdb_stdlog, 
From jimb@zwingli.cygnus.com Mon Jun 04 22:05:00 2001
From: Jim Blandy <jimb@zwingli.cygnus.com>
To: Fernando Nasser <fnasser@cygnus.com>
Cc: gdb-patches@sources.redhat.com
Subject: Re: RFA: tighten apropos pattern
Date: Mon, 04 Jun 2001 22:05:00 -0000
Message-id: <npelsz8qie.fsf@zwingli.cygnus.com>
References: <20010604220622.BC1095E9CB@zwingli.cygnus.com> <3B1C271A.EEADF8EC@cygnus.com>
X-SW-Source: 2001-06/msg00037.html
Content-length: 383

Fernando Nasser <fnasser@cygnus.com> writes:
> Jim Blandy wrote:
> > 
> > 2001-06-04  Jim Blandy  <jimb@redhat.com>
> > 
> >         * gdb.base/help.exp: Update pattern to excluse `print-load-map'
>                                                  ^^^^^^^
> 
> I guess you've meant exclude (s and d are just too close).
> 
> Yes, it is approved.

Committed, with your fix.  Thanks.