RFA: clean up logic in linux_child_wait

RFA: clean up logic in linux_child_wait

[Jim Blandy]

GDB still uses linux-thread.c on the S/390.

I don't think this function's callers actually noticed that it was
returning incorrect values, so this patch can't be tested, but it
wasn't analyzing things correctly.

2001-12-07  Jim Blandy  <jimb@redhat.com>

	* linux-thread.c (linux_child_wait): Rework logic to return proper
 	errno values, make it clearer that we've handled all cases, and
 	not depend on errno being preserved across successful system
 	calls.

Index: gdb/linux-thread.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/linux-thread.c,v
retrieving revision 1.17
diff -c -r1.17 linux-thread.c
*** gdb/linux-thread.c	2001/06/14 21:03:15	1.17
--- gdb/linux-thread.c	2001/12/08 04:01:45
***************
*** 1256,1306 ****
  int
  linux_child_wait (int pid, int *rpid, int *status)
  {
!   int save_errno;
  
    /* Note: inftarg has these inside the loop. */
    set_sigint_trap ();	/* Causes SIGINT to be passed on to the
  			   attached process. */
    set_sigio_trap  ();
  
!   errno = save_errno = 0;
    for (;;)
      {
!       errno = 0;
!       *rpid = waitpid (pid, status, __WCLONE | WNOHANG);
!       save_errno = errno;
  
        if (*rpid > 0)
  	{
  	  /* Got an event -- break out */
  	  break;
- 	}
-       if (errno == EINTR)	/* interrupted by signal, try again */
- 	{
- 	  continue;
  	}
  
!       errno = 0;
        *rpid = waitpid (pid, status, WNOHANG);
        if (*rpid > 0)
  	{
  	  /* Got an event -- break out */
  	  break;
- 	}
-       if (errno == EINTR)
- 	{
- 	  continue;
  	}
!       if (errno != 0 && save_errno != 0)
! 	{
! 	  break;
! 	}
        sigsuspend(&linuxthreads_block_mask);
      }
    clear_sigio_trap  ();
    clear_sigint_trap ();
  
!   return errno ? errno : save_errno;
  }
  
  
--- 1256,1364 ----
  int
  linux_child_wait (int pid, int *rpid, int *status)
  {
!   int return_errno = 0;
  
    /* Note: inftarg has these inside the loop. */
    set_sigint_trap ();	/* Causes SIGINT to be passed on to the
  			   attached process. */
    set_sigio_trap  ();
  
!   /* On older Linux kernels, there's no way to say "please wait for
!      both clones and normal processes."  If you specify __WCLONE, you
!      get clones; if you don't, you don't.  Since you might miss one
!      while waiting for the other, the only way to wait for both is to
!      wait for SIGCHLD, and then use WNOHANG to see what happened.  */
    for (;;)
      {
!       int clone_children_exist;
  
+       /* Check for clones.  */ 
+       *rpid = waitpid (pid, status, __WCLONE | WNOHANG);
+      
        if (*rpid > 0)
  	{
  	  /* Got an event -- break out */
  	  break;
  	}
+       else if (*rpid < 0)
+         {
+           /* Only when the system call fails can we make any
+              assumptions about errno's value.  A successful system
+              call may still set errno to something.  */
  
!           if (errno == EINTR)
!             /* interrupted by signal, try again */
!             continue;
!           else if (errno != ECHILD)
!             {
!               /* Some other error, which we should report to our
!                  caller.  */
!               return_errno = errno;
!               break;
!             }
! 
!           /* There are no clone children.  It's not just that they
!              exist but they don't have any status to report yet, since
!              we used WNOHANG --- it's that there aren't any.  */
!           clone_children_exist = 0;
!         }
!       else
!         /* Here *rpid is zero.  Some clone children exist, but we used
!            WNOHANG and they don't have any interesting status to
!            report to us yet.  Fall through and check for non-clone
!            children.  */
!         clone_children_exist = 1;
! 
!       /* We didn't find any clones with interesting status; check for
!          non-clones.  */
        *rpid = waitpid (pid, status, WNOHANG);
+       
        if (*rpid > 0)
  	{
  	  /* Got an event -- break out */
  	  break;
  	}
!       else if (*rpid < 0)
!         {
!           /* Only when the system call fails can we make any
!              assumptions about errno's value.  A successful system
!              call may still set errno to something.  */
! 
!           if (errno == EINTR)
!             /* interrupted by signal, try again */
!             continue;
!           else if (errno != ECHILD)
!             {
!               /* Some unexpected error, which we should report to our
!                  caller.  */
!               return_errno = errno;
!               break;
!             }
! 
!           /* There are no non-clone children.  If there weren't any
!              clone children either, then that's an error condition we
!              should report to our caller.  */
!           if (! clone_children_exist)
!             {
!               return_errno = ECHILD;
!               break;
!             }
! 
!           /* Otherwise, we should block, since there are clone
!              children.  */
!         }
!       else
!         /* Some non-clone children exist, but we used WNOHANG and they
!            don't have any interesting status to report to us yet.
!            Fall through and block waiting for SIGCHLD.  */
!         ;
! 
        sigsuspend(&linuxthreads_block_mask);
      }
    clear_sigio_trap  ();
    clear_sigint_trap ();
  
!   return return_errno;
  }
  
  

Re: RFA: clean up logic in linux_child_wait

[Daniel Jacobowitz]

On Fri, Dec 07, 2001 at 11:06:32PM -0500, Jim Blandy wrote:
> 
> GDB still uses linux-thread.c on the S/390.

It shouldn't.

From the current sources I see:
NATDEPFILES= infptrace.o solib.o inftarg.o fork-child.o corelow.o s390-nat.o linux-thread.o core-aout.o  core-regset.o
# post 5.0 natdepfiles.
NATDEPFILES+= thread-db.o lin-lwp.o proc-service.o 

i.e. it's actually using _both_.  What dies horribly if you kill
linux-thread.o?  That module is pretty severely deprecated.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: RFA: clean up logic in linux_child_wait

[Jim Blandy]

Daniel Jacobowitz <drow@mvista.com> writes:
> > GDB still uses linux-thread.c on the S/390.
> 
> It shouldn't.
> 
> >From the current sources I see:
> NATDEPFILES= infptrace.o solib.o inftarg.o fork-child.o corelow.o s390-nat.o linux-thread.o core-aout.o  core-regset.o
> # post 5.0 natdepfiles.
> NATDEPFILES+= thread-db.o lin-lwp.o proc-service.o 
> 
> i.e. it's actually using _both_.  What dies horribly if you kill
> linux-thread.o?  That module is pretty severely deprecated.


Well, the only thing that dies horribly is the three testsuite
failures I was trying to resolve.

Do you want my job?

Re: RFA: clean up logic in linux_child_wait

[Daniel Jacobowitz]

On Sat, Dec 08, 2001 at 01:15:24PM -0500, Jim Blandy wrote:
> 
> Daniel Jacobowitz <drow@mvista.com> writes:
> > > GDB still uses linux-thread.c on the S/390.
> > 
> > It shouldn't.
> > 
> > >From the current sources I see:
> > NATDEPFILES= infptrace.o solib.o inftarg.o fork-child.o corelow.o s390-nat.o linux-thread.o core-aout.o  core-regset.o
> > # post 5.0 natdepfiles.
> > NATDEPFILES+= thread-db.o lin-lwp.o proc-service.o 
> > 
> > i.e. it's actually using _both_.  What dies horribly if you kill
> > linux-thread.o?  That module is pretty severely deprecated.
> 
> 
> Well, the only thing that dies horribly is the three testsuite
> failures I was trying to resolve.
> 
> Do you want my job?

Sorry for my tone there.

On Oct. 14, Mark Kettenis checked in a patch that was intended to make
all Linux targets use lin-lwp instead of linux-thread.  The fact that
S/390 still was afterwards was probably just an oversight; S/390 now
contains the only reference to that file out of any port.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: RFA: clean up logic in linux_child_wait

[Jim Blandy]

Daniel Jacobowitz <drow@mvista.com> writes:
> > Well, the only thing that dies horribly is the three testsuite
> > failures I was trying to resolve.
> > 
> > Do you want my job?
> 
> Sorry for my tone there.

No, no, there was nothing wrong with your tone.  Your suggestion was
quick, simplified the code, and relieved me of the responsibility to
track down some bugs that, at the time I posted, I had no real idea
where I was going to go with.  So I'm quite grateful.  The joke was
that my work might get done better if you were doing it.  :)

> On Oct. 14, Mark Kettenis checked in a patch that was intended to make
> all Linux targets use lin-lwp instead of linux-thread.  The fact that
> S/390 still was afterwards was probably just an oversight; S/390 now
> contains the only reference to that file out of any port.

Yeah, I figured there was some kind of history behind it.

Re: RFA: clean up logic in linux_child_wait

[Andrew Cagney]

> On Oct. 14, Mark Kettenis checked in a patch that was intended to make
> all Linux targets use lin-lwp instead of linux-thread.  The fact that
> S/390 still was afterwards was probably just an oversight; S/390 now
> contains the only reference to that file out of any port.


Probably me committing DJ Barrow's s390 changes.  If someone is trying 
to eliminate something, let me know and I'll add it to the ARI list.

	Andrew