Re: [RFA] Problem after hitting breakpoint on Windows (with GDBserver)

[Pedro Alves <palves@redhat.com>]

On 03/13/2012 09:59 PM, Joel Brobecker wrote:

>>> +  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.


If that's a concern, we can still keep it, like e.g.:

  /* Fetch the memory contents "under" the breakpoint, and save it in
     the shadow_contents buffer.  */
  readbuf = alloca (bp_tgt->placed_size);
  val = target_read_memory (bp_tgt->placed_address, readbuf,
			    bp_tgt->placed_size);
  if (val == 0)
    {
      /* Success, save it.  */
      bp_tgt->shadow_len = bp_tgt->placed_size;
      memcpy (bp_tgt->shadow_contents, readbuf, bp_tgt->placed_size);

      /* Now write the breakpoint instruction.  */
      val = target_write_raw_memory (bp_tgt->placed_address, bp,
				     bp_tgt->placed_size);
    }

> 
>>> +   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!).


Eh, it's meant to insure correctness.  :-)  Certainly a heap allocation on every
read is more expensive than a simple range check, and more so one that only
triggers when we have breakpoints in the range we're reading.

>

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


Oh, I was only thinking of something along the lines of what Jan did on
gdbserver.  That is, something like:

--- c/gdb/breakpoint.c
+++ w/gdb/breakpoint.c
@@ -1446,6 +1446,10 @@ breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,

     if (readbuf != NULL)
       {
+	gdb_assert (bl->target_info.shadow_contents >= readbuf + len
+		    || readbuf >= (bl->target_info.shadow_contents
+				   + bl->target_info.shadow_len));
+
 	/* Update the read buffer with this inserted breakpoint's
 	   shadow.  */
 	memcpy (readbuf + bp_addr - memaddr,

-- 
Pedro Alves