Re: [SCM] master: Switch to frysk.sys.ptrace.

Re: [SCM] master: Switch to frysk.sys.ptrace.

[Mark Wielaard]

Hi Andrew,

On Tue, 2008-02-12 at 18:50 +0000, cagney@sourceware.org wrote:
> -        private int peek(long index, byte[] bytes, int offset, int length)
> -	{
> -	  if (mem != null)
> -	    {
> -	      try
> -		{
> -		  return mem.pread(index, bytes, offset, length);
> -		}
> -	      catch (Errno e)
> -		{
> -		  // try again through address space.
> +        private void transfer(long index, byte[] bytes, int offset, int length,
> +			      boolean write) {
> +	    if (mem != null && !write) {
> +		try {
> +		    mem.pread(index, bytes, offset, length);
> +		    return;
> +		} catch (Errno e) {
> +		    // Give up on mem; and fall back to ptrace.  This
> +		    // can happen when /proc isn't mounted, or when a
> +		    // process is terminating and the kernel scrubs
> +		    // the /proc entry before its time.
> +		    mem = null;
>  		}
>  	    }

Are you sure this change is right? The comment is right about those 2
reasons for pread() possibly failing. But I don't believe that is the
only case it can happen. Previously when a read occurred that failed,
possibly because following a pointer in the inferior that was NULL or
not yet initialized that would just cause a error reading from that
particular (uninitialized) memory location. But now whenever that occurs
once for an inferior you blow away mem so pread() can never be used
anymore and we always fall back to having to do single peeks and pokes
through the ptrace address space.

Cheers,

Mark

Re: [SCM] master: Switch to frysk.sys.ptrace.

[Mark Wielaard]

Hi Andrew,

On Wed, 2008-02-13 at 09:47 +0100, Mark Wielaard wrote:
> On Tue, 2008-02-12 at 18:50 +0000, cagney@sourceware.org wrote:
> > -        private int peek(long index, byte[] bytes, int offset, int length)
> > -	{
> > -	  if (mem != null)
> > -	    {
> > -	      try
> > -		{
> > -		  return mem.pread(index, bytes, offset, length);
> > -		}
> > -	      catch (Errno e)
> > -		{
> > -		  // try again through address space.
> > +        private void transfer(long index, byte[] bytes, int offset, int length,
> > +			      boolean write) {
> > +	    if (mem != null && !write) {
> > +		try {
> > +		    mem.pread(index, bytes, offset, length);
> > +		    return;
> > +		} catch (Errno e) {
> > +		    // Give up on mem; and fall back to ptrace.  This
> > +		    // can happen when /proc isn't mounted, or when a
> > +		    // process is terminating and the kernel scrubs
> > +		    // the /proc entry before its time.
> > +		    mem = null;
> >  		}
> >  	    }
> 
> Are you sure this change is right? The comment is right about those 2
> reasons for pread() possibly failing. But I don't believe that is the
> only case it can happen. Previously when a read occurred that failed,
> possibly because following a pointer in the inferior that was NULL or
> not yet initialized that would just cause a error reading from that
> particular (uninitialized) memory location. But now whenever that occurs
> once for an inferior you blow away mem so pread() can never be used
> anymore and we always fall back to having to do single peeks and pokes
> through the ptrace address space.

Did you have time to investigate this? If not, shall we revert it to the
original implementation till you do have time?

Thanks,

Mark

Re: [SCM] master: Switch to frysk.sys.ptrace.

[Mark Wielaard]

Hi Andrew,

On Wed, 2008-02-27 at 12:59 +0100, Mark Wielaard wrote:
> On Wed, 2008-02-13 at 09:47 +0100, Mark Wielaard wrote:
> > On Tue, 2008-02-12 at 18:50 +0000, cagney@sourceware.org wrote:
> > > -        private int peek(long index, byte[] bytes, int offset, int length)
> > > -	{
> > > -	  if (mem != null)
> > > -	    {
> > > -	      try
> > > -		{
> > > -		  return mem.pread(index, bytes, offset, length);
> > > -		}
> > > -	      catch (Errno e)
> > > -		{
> > > -		  // try again through address space.
> > > +        private void transfer(long index, byte[] bytes, int offset, int length,
> > > +			      boolean write) {
> > > +	    if (mem != null && !write) {
> > > +		try {
> > > +		    mem.pread(index, bytes, offset, length);
> > > +		    return;
> > > +		} catch (Errno e) {
> > > +		    // Give up on mem; and fall back to ptrace.  This
> > > +		    // can happen when /proc isn't mounted, or when a
> > > +		    // process is terminating and the kernel scrubs
> > > +		    // the /proc entry before its time.
> > > +		    mem = null;
> > >  		}
> > >  	    }
> > 
> > Are you sure this change is right? The comment is right about those 2
> > reasons for pread() possibly failing. But I don't believe that is the
> > only case it can happen. Previously when a read occurred that failed,
> > possibly because following a pointer in the inferior that was NULL or
> > not yet initialized that would just cause a error reading from that
> > particular (uninitialized) memory location. But now whenever that occurs
> > once for an inferior you blow away mem so pread() can never be used
> > anymore and we always fall back to having to do single peeks and pokes
> > through the ptrace address space.
> 
> Did you have time to investigate this? If not, shall we revert it to
> the
> original implementation till you do have time?

Ping. Please do take a look. I really would like to see this resolved.

I appreciate that it is hard to just revert this hunk if you don't have
time right now to finalize it, because it was embedded into another
rewrite commit of other stuff. So it would really help if you made
separate commits for separate issues in the future. That makes these
kind of reviews easier.

Thanks,

Mark