Hi Pedro,

Thanks for the review!

> > +  if (val == 0)
> > +    memcpy (bp_tgt->shadow_contents, readbuf, bp_tgt->placed_size);
> >  
> >    /* Write the breakpoint.  */
> >    if (val == 0)
> 
> Merge?

I actually started that way, with the two blocks merged. But I felt
that it was breaking the separation between the two steps. With the
comments clearly separating the two steps, I didn't want to break
that unless asked. So now I changed it.

> > +   As a limitation, MYADDR must not be the shadow_contents buffer of one
> 
> I wouldn't call it a limitation; it's more a design choice thing, like
> memcpy doesn't handle overlapping buffers.

OK - I just removed the "As a limitation" from the comments.

> Otherwise this is fine with me.

Thanks! Attached is a new version of the patch. The only changes
should be the changes you pointed out.

> An assertion in breakpoint_xfer_memory to catch that READBUF or
> WRITEBUF doesn't overlap bp->target_info.shadow_contents would be
> nice.

I thought about that, but decided to look at that separately, since
it doesn't help correctness, and can potentially be a little expensive
(at least compared to just allocating a buffer on the heap - I think!).

But I don't mind writing a patch - probably a function in breakpoint.c
and a gdb_assert calling that breakpoint?

> As we discussed yesterday on IRC, the current code always reinserts
> locations, which means that gdb > 7.4 now does an extra read off of
> inferior memory to fill the shadow on breakpoint re-sets.  That'd be
> possible to avoid (and avoiding this whole problem along the way),
> though I think Joel's patch is a good one even knowing that.

Agreed. We've seen a number of emails on the GDB lists showing that
every increase in traffic can have a significant impact in some
situations...

-- 
Joel