RFA: EINTR in procfs_wait

RFA: EINTR in procfs_wait

[Fernando Nasser]

Folks,

I got this patch and it seems that we did forget to test for EINTR in
procfs_wait(). It looks like an "obvious fix" but I would like someone
else to double check it.

Thanks in advance.

Fernando



2001-04-05  Adam Mirowski  <Adam.Mirowski@Sun.COM>

	Fixed Insight on Solaris. It was not possible to debug a process 
	because of EINTR "errors".
	* procfs.c: (procfs_wait): if proc_wait_for_stop() fails
	with EINTR, retry the call.


Index: gdb/procfs.c
------- procfs.c -------
*** /tmp/dMKayx_	Tue Apr 10 16:20:54 2001
--- procfs.c	Thu Apr  5 22:21:40 2001
***************
*** 3518,3531 ****
--- 3518,3533 ----
  
  	      if (retval != PIDGET (inferior_pid))	/* wrong child? */
  		error ("procfs: couldn't stop process %d: wait returned %d\n",
  		       inferior_pid, retval);
  	      /* FIXME: might I not just use waitpid?
  		 Or try find_procinfo to see if I know about this child? */
  	    }
+ 	  else if (errno == EINTR)
+ 	    goto wait_again;
  	  else
  	    {
  	      /* Unknown error from wait_for_stop. */
  	      proc_error (pi, "target_wait (wait_for_stop)", __LINE__);
  	    }
  	}
        else

Re: RFA: EINTR in procfs_wait

[Kevin Buettner]

Fernando,

I think something like this is needed.  I have two concerns though...

  1) The patch below is causing the retry counter to be incremented.
     It's not clear to me if this is the right thing to do or not.
     (I honestly don't know what it should be.  I'd have the same
     concern if I saw that it wasn't being incremented.)

  2) I think EAGAIN should be tested for in addition to EINTR.  See
     http://sources.redhat.com/ml/gdb/2001-04/msg00078.html .  Hmm...
     now that I look at this some more, it seems that we might
     need another retry test somewhere else as well.

But, regardless, I think this fix ought to go in.  We can add any
necessary adjustments later on.

Kevin

On Apr 10, 11:04am, Fernando Nasser wrote:
> Subject: RFA: EINTR in procfs_wait
> Folks,
> 
> I got this patch and it seems that we did forget to test for EINTR in
> procfs_wait(). It looks like an "obvious fix" but I would like someone
> else to double check it.
> 
> Thanks in advance.
> 
> Fernando
> 
> 
> 
> 2001-04-05  Adam Mirowski  <Adam.Mirowski@Sun.COM>
> 
> 	Fixed Insight on Solaris. It was not possible to debug a process 
> 	because of EINTR "errors".
> 	* procfs.c: (procfs_wait): if proc_wait_for_stop() fails
> 	with EINTR, retry the call.
> 
> 
> Index: gdb/procfs.c
> ------- procfs.c -------
> *** /tmp/dMKayx_	Tue Apr 10 16:20:54 2001
> --- procfs.c	Thu Apr  5 22:21:40 2001
> ***************
> *** 3518,3531 ****
> --- 3518,3533 ----
>   
>   	      if (retval != PIDGET (inferior_pid))	/* wrong child? */
>   		error ("procfs: couldn't stop process %d: wait returned %d\n",
>   		       inferior_pid, retval);
>   	      /* FIXME: might I not just use waitpid?
>   		 Or try find_procinfo to see if I know about this child? */
>   	    }
> + 	  else if (errno == EINTR)
> + 	    goto wait_again;
>   	  else
>   	    {
>   	      /* Unknown error from wait_for_stop. */
>   	      proc_error (pi, "target_wait (wait_for_stop)", __LINE__);
>   	    }
>   	}
>         else
>-- End of excerpt from Fernando Nasser

Re: RFA: EINTR in procfs_wait

[Fernando Nasser]

Kevin Buettner wrote:
> 
> Fernando,
> 
> I think something like this is needed.  I have two concerns though...
> 
>   1) The patch below is causing the retry counter to be incremented.
>      It's not clear to me if this is the right thing to do or not.
>      (I honestly don't know what it should be.  I'd have the same
>      concern if I saw that it wasn't being incremented.)
> 
>   2) I think EAGAIN should be tested for in addition to EINTR.  See
>      http://sources.redhat.com/ml/gdb/2001-04/msg00078.html .  Hmm...
>      now that I look at this some more, it seems that we might
>      need another retry test somewhere else as well.
> 
> But, regardless, I think this fix ought to go in.  We can add any
> necessary adjustments later on.
> 
> Kevin
> 

Thank you Kevin.

Michael Snyder is going to take a look at some old procfs.c code to see how it was done back them.  I will wait until Thursday before checking it in so he has a chance to do this.

Best regards,
Fernando


> On Apr 10, 11:04am, Fernando Nasser wrote:
> > Subject: RFA: EINTR in procfs_wait
> > Folks,
> >
> > I got this patch and it seems that we did forget to test for EINTR in
> > procfs_wait(). It looks like an "obvious fix" but I would like someone
> > else to double check it.
> >
> > Thanks in advance.
> >
> > Fernando
> >
> >
> >
> > 2001-04-05  Adam Mirowski  <Adam.Mirowski@Sun.COM>
> >
> >       Fixed Insight on Solaris. It was not possible to debug a process
> >       because of EINTR "errors".
> >       * procfs.c: (procfs_wait): if proc_wait_for_stop() fails
> >       with EINTR, retry the call.
> >
> >
> > Index: gdb/procfs.c
> > ------- procfs.c -------
> > *** /tmp/dMKayx_      Tue Apr 10 16:20:54 2001
> > --- procfs.c  Thu Apr  5 22:21:40 2001
> > ***************
> > *** 3518,3531 ****
> > --- 3518,3533 ----
> >
> >             if (retval != PIDGET (inferior_pid))      /* wrong child? */
> >               error ("procfs: couldn't stop process %d: wait returned %d\n",
> >                      inferior_pid, retval);
> >             /* FIXME: might I not just use waitpid?
> >                Or try find_procinfo to see if I know about this child? */
> >           }
> > +       else if (errno == EINTR)
> > +         goto wait_again;
> >         else
> >           {
> >             /* Unknown error from wait_for_stop. */
> >             proc_error (pi, "target_wait (wait_for_stop)", __LINE__);
> >           }
> >       }
> >         else
> >-- End of excerpt from Fernando Nasser

-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: RFA: EINTR in procfs_wait

[Andrew Cagney]

Fernando Nasser wrote:
> 
> Folks,
> 
> I got this patch and it seems that we did forget to test for EINTR in
> procfs_wait(). It looks like an "obvious fix" but I would like someone
> else to double check it.

Anything involving a goto is not an obvious fix :-)

> +           goto wait_again;

In theory, procfs_wait() should be able to return (target.h):

    /* This is used for target async and extended-async
       only. Remote_async_wait() returns this when there is an event
       on the inferior, but the rest of the world is not interested in
       it. The inferior has not stopped, but has just sent some output
       to the console, for instance. In this case, we want to go back
       to the event loop and wait there for another event from the
       inferior, rather than being stuck in the remote_async_wait()
       function. This way the event loop is responsive to other events,
       like for instance the user typing.  */
    TARGET_WAITKIND_IGNORE

and let the event-loop force the reentry.  Looking at the code
(infrun.c:handle_inferior_event()):

        /* We had an event in the inferior, but we are not interested
           in handling it at this level. The lower layers have already
           done what needs to be done, if anything. This case can
           occur only when the target is async or extended-async. One
           of the circumstamces for this to happen is when the
           inferior produces output for the console. The inferior has
           not stopped, and we are ignoring the event. */
      case TARGET_WAITKIND_IGNORE:
        ecs->wait_some_more = 1;
        return;

I think the restriction that that KIND only apples to
async/extended-async is incorrect.

	Andrew

Re: RFA: EINTR in procfs_wait

[Michael Snyder]

Kevin Buettner wrote:
> 
> Fernando,
> 
> I think something like this is needed.  I have two concerns though...
> 
>   1) The patch below is causing the retry counter to be incremented.
>      It's not clear to me if this is the right thing to do or not.
>      (I honestly don't know what it should be.  I'd have the same
>      concern if I saw that it wasn't being incremented.)

I wouldn't be too concerned about it -- the retry counter is 
a heuristic (and that's putting it kindly).

>   2) I think EAGAIN should be tested for in addition to EINTR.  See
>      http://sources.redhat.com/ml/gdb/2001-04/msg00078.html .  Hmm...
>      now that I look at this some more, it seems that we might
>      need another retry test somewhere else as well.

That's not unreasonable, but unlike EINTR, EAGAIN was never in there before,
so that would be a new test.  You're welcome to try it out, and so long as
nothing immediately breaks, put it in there.  You might want to have it
print a message, so that in case it does break something, we'll know why.

> But, regardless, I think this fix ought to go in.  We can add any
> necessary adjustments later on.

I'm inclined to agree, now that I've verified that the old procfs module
used to test for EINTR in a similar-but-not-identical context.

Fernando, you want to do the honors?

> On Apr 10, 11:04am, Fernando Nasser wrote:
> > Subject: RFA: EINTR in procfs_wait
> > Folks,
> >
> > I got this patch and it seems that we did forget to test for EINTR in
> > procfs_wait(). It looks like an "obvious fix" but I would like someone
> > else to double check it.
> >
> > Thanks in advance.
> >
> > Fernando
> >
> >
> >
> > 2001-04-05  Adam Mirowski  <Adam.Mirowski@Sun.COM>
> >
> >       Fixed Insight on Solaris. It was not possible to debug a process
> >       because of EINTR "errors".
> >       * procfs.c: (procfs_wait): if proc_wait_for_stop() fails
> >       with EINTR, retry the call.
> >
> >
> > Index: gdb/procfs.c
> > ------- procfs.c -------
> > *** /tmp/dMKayx_      Tue Apr 10 16:20:54 2001
> > --- procfs.c  Thu Apr  5 22:21:40 2001
> > ***************
> > *** 3518,3531 ****
> > --- 3518,3533 ----
> >
> >             if (retval != PIDGET (inferior_pid))      /* wrong child? */
> >               error ("procfs: couldn't stop process %d: wait returned %d\n",
> >                      inferior_pid, retval);
> >             /* FIXME: might I not just use waitpid?
> >                Or try find_procinfo to see if I know about this child? */
> >           }
> > +       else if (errno == EINTR)
> > +         goto wait_again;
> >         else
> >           {
> >             /* Unknown error from wait_for_stop. */
> >             proc_error (pi, "target_wait (wait_for_stop)", __LINE__);
> >           }
> >       }
> >         else
> >-- End of excerpt from Fernando Nasser

Re: RFA: EINTR in procfs_wait

[Michael Snyder]

Fernando Nasser wrote:
> 
> Folks,
> 
> I got this patch and it seems that we did forget to test for EINTR in
> procfs_wait(). It looks like an "obvious fix" but I would like someone
> else to double check it.

OK, I've looked at the old procfs module, and it did indeed have a similar
test in a similar (though not identical) context.  The change makes sense
to me.  Fernando, do you want to do the honors?


> 
> Thanks in advance.
> 
> Fernando
> 
> 2001-04-05  Adam Mirowski  <Adam.Mirowski@Sun.COM>
> 
>         Fixed Insight on Solaris. It was not possible to debug a process
>         because of EINTR "errors".
>         * procfs.c: (procfs_wait): if proc_wait_for_stop() fails
>         with EINTR, retry the call.
> 
> Index: gdb/procfs.c
> ------- procfs.c -------
> *** /tmp/dMKayx_        Tue Apr 10 16:20:54 2001
> --- procfs.c    Thu Apr  5 22:21:40 2001
> ***************
> *** 3518,3531 ****
> --- 3518,3533 ----
> 
>               if (retval != PIDGET (inferior_pid))      /* wrong child? */
>                 error ("procfs: couldn't stop process %d: wait returned %d\n",
>                        inferior_pid, retval);
>               /* FIXME: might I not just use waitpid?
>                  Or try find_procinfo to see if I know about this child? */
>             }
> +         else if (errno == EINTR)
> +           goto wait_again;
>           else
>             {
>               /* Unknown error from wait_for_stop. */
>               proc_error (pi, "target_wait (wait_for_stop)", __LINE__);
>             }
>         }
>         else

Re: RFA: EINTR in procfs_wait

[Fernando Nasser]

Michael Snyder wrote:
> 
> Fernando Nasser wrote:
> >
> > Folks,
> >
> > I got this patch and it seems that we did forget to test for EINTR in
> > procfs_wait(). It looks like an "obvious fix" but I would like someone
> > else to double check it.
> 
> OK, I've looked at the old procfs module, and it did indeed have a similar
> test in a similar (though not identical) context.  The change makes sense
> to me.  Fernando, do you want to do the honors?
> 

I checked Adam's patch in.

Fernando


> >
> > Thanks in advance.
> >
> > Fernando
> >
> > 2001-04-05  Adam Mirowski  <Adam.Mirowski@Sun.COM>
> >
> >         Fixed Insight on Solaris. It was not possible to debug a process
> >         because of EINTR "errors".
> >         * procfs.c: (procfs_wait): if proc_wait_for_stop() fails
> >         with EINTR, retry the call.
> >
> > Index: gdb/procfs.c
> > ------- procfs.c -------
> > *** /tmp/dMKayx_        Tue Apr 10 16:20:54 2001
> > --- procfs.c    Thu Apr  5 22:21:40 2001
> > ***************
> > *** 3518,3531 ****
> > --- 3518,3533 ----
> >
> >               if (retval != PIDGET (inferior_pid))      /* wrong child? */
> >                 error ("procfs: couldn't stop process %d: wait returned %d\n",
> >                        inferior_pid, retval);
> >               /* FIXME: might I not just use waitpid?
> >                  Or try find_procinfo to see if I know about this child? */
> >             }
> > +         else if (errno == EINTR)
> > +           goto wait_again;
> >           else
> >             {
> >               /* Unknown error from wait_for_stop. */
> >               proc_error (pi, "target_wait (wait_for_stop)", __LINE__);
> >             }
> >         }
> >         else

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9