On Saturday 29 October 2011 13:15:13, Yao Qi wrote:
> On 10/27/2011 11:59 PM, Pedro Alves wrote:
> >> > Note that bp->old_data saves the first byte of jmp insn
> >> > rather than original insn.
> > _This_ is the bug.  I must have introduced it as a last minute change
> > before pushing fast tracepoints upstream, because I'm sure this used
> > to work, proof being all the comments in the code about it.  :-/
> > 
> > The idea is that the shadow of all breakpoints and fast tracepoint
> > jumps always has the contents of the memory as if no breakpoint
> > of fast tracepoint was inserted.
> > 
> 
> Usually, shadow of both breakpoints and fast tracepoint jumps is a copy
> of *original* insn.  That is absolutely correct.
> 
> > The problem is that check_mem_write does:
> > 
> >  - loop over fast tracepoint jumps, updating their shadow buffers,
> >    _and_ clobbers the input buffer.
> >  - loop over raw breakpoints, updating their shadow buffers, but
> >    at this point, the input buffer is already overwritten with
> >    fast tracepoint jumps...
> > 
> > The fix is to split updating the shadow buffers from stapling fast
> > tracepoint jumps and breakpoints on top of the input buffer.
> > 
> 
> Your fix looks right to me, except one concern on performance.  Your
> patch doubles the time on iterating over fast tracepoint jump list and
> raw breakpoint list.  It is not a big deal when the number of fast
> tracepoint and breakpoint is small.  I did an experiment on showing how
> much time step-over breakpoint may cost as the number of tracepoint goes
> up.  In my experiment, I set thousands (from 1*1296 to 6*1296) of
> tracepoints at some different unreachable places to make sure the
> breakpoint list in gdbserver is long enough, and set four breakpoints in
> program.  GDB will step-over these breakpoint until the end of program,
> and I'll calculate the time spent.  The result shows there is no notable
> slowdown with your patch applied, it is good, and I am over-worried :)

Yeah, I wasn't worried about it, but looking again, 
it's actually easy to avoid.  We're already working with an xmalloc'ed copy
of the data to write to the inferior, so we can just pass both buffers
to check_mem_write.

> > While at it, I did several changes to the new tests.  Thanks a lot
> > for writing them BTW.  They're awesome.  I wish I had done so before...
> > I'll point out the main issues below.
> > 
> 
> The tests after your modifications looks better.  Please check them in
> with your patch.  Thanks for reviewing them.

Alright, I've checked this in.  Thanks!

-- 
Pedro Alves