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

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

[Joel Brobecker]

When debugging on Windows with GDBserver, the debugger starts
failing after hitting a breakpoint.  For instance:

    (gdb) b foo
    Breakpoint 1 at 0x40177e: file foo.adb, line 5.
    (gdb) cont
    Continuing.

    Breakpoint 1, foo () at foo.adb:5
    5          Put_Line ("Hello World.");  -- STOP
    (gdb) n

    Program received signal SIGSEGV, Segmentation fault.
    0x00401782 in foo () at foo.adb:5
    5          Put_Line ("Hello World.");  -- STOP

There are two issues:

  1. While trying to re-insert a breakpoint that is still inserted
     in memory, insert_bp_location wipes out the breakpoint location's
     shadow_contents.  As a consequence, we cannot restore the proper
     instruction when removing the breakpoint anymore.  That's why
     the inferior's behavior changes when trying to resume after
     the breakpoint was hit.

  2. mem-break.c:default_memory_insert_breakpoint passes a breakpoint
     location's shadow_contents as the buffer for a memory read.
     This reveals a limitation of the various memory-read target
     functions.  This patch documents this limitation and adjust
     the two calls that seem to hit that limitation.

gdb/ChangeLog:

2012-03-12  Joel Brobecker  <brobecker@adacore.com>
            Pedro Alves <palves@redhat.com>

        * breakpoint.c (insert_bp_location): Do not wipe bl->target_info out.

        * target.h (target_read): Document limitation.
        * target.c (memory_xfer_partial, target_xfer_partial)
        (target_read_memory): Document limitation.
        * mem-break.c: #include "gdb_string.h".
        (default_memory_insert_breakpoint): Do not call target_read_memory
        with a pointer to the breakpoint's shadow_contents buffer.  Use
        a local buffer instead.
        * m32r-tdep.c (m32r_memory_insert_breakpoint): Ditto.

Tested on x86-windows with AdaCore's testsuite (standalone and with
GDBserver). And tested on x86_64-linux, with the official testsuite,
in both standalone and native-gdbserver modes.  No regression detected.

The change in m32r-tdep.c could not be tested beyond simply compiling
the file. A careful code inspection would be appreciated.

OK to commit?

Thanks,
-- 
Joel

---
 gdb/breakpoint.c |   11 +++++++++--
 gdb/m32r-tdep.c  |    3 ++-
 gdb/mem-break.c  |    7 ++++++-
 gdb/target.c     |   19 ++++++++++++++++---
 gdb/target.h     |    7 ++++++-
 5 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1158f0e..06d1c6f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2082,8 +2082,15 @@ insert_bp_location (struct bp_location *bl,
   if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
     return 0;
 
-  /* Initialize the target-specific information.  */
-  memset (&bl->target_info, 0, sizeof (bl->target_info));
+  /* Note we don't initialize bl->target_info, as that wipes out
+     the breakpoint location's shadow_contents if the breakpoint
+     is still inserted at that location.  This in turn breaks
+     target_read_memory which depends on these buffers when
+     a memory read is requested at the breakpoint location:
+     Once the target_info has been wiped, we fail to see that
+     we have a breakpoint inserted at that address and thus
+     read the breakpoint instead of returning the data saved in
+     the breakpoint location's shadow contents.  */
   bl->target_info.placed_address = bl->address;
   bl->target_info.placed_address_space = bl->pspace->aspace;
   bl->target_info.length = bl->length;
diff --git a/gdb/m32r-tdep.c b/gdb/m32r-tdep.c
index 72872bd..d504eb3 100644
--- a/gdb/m32r-tdep.c
+++ b/gdb/m32r-tdep.c
@@ -85,7 +85,7 @@ m32r_memory_insert_breakpoint (struct gdbarch *gdbarch,
   CORE_ADDR addr = bp_tgt->placed_address;
   int val;
   gdb_byte buf[4];
-  gdb_byte *contents_cache = bp_tgt->shadow_contents;
+  gdb_byte contents_cache[4];
   gdb_byte bp_entry[] = { 0x10, 0xf1 };	/* dpt */
 
   /* Save the memory contents.  */
@@ -93,6 +93,7 @@ m32r_memory_insert_breakpoint (struct gdbarch *gdbarch,
   if (val != 0)
     return val;			/* return error */
 
+  memcpy (bp_tgt->shadow_contents, contents_cache, 4);
   bp_tgt->placed_size = bp_tgt->shadow_len = 4;
 
   /* Determine appropriate breakpoint contents and size for this address.  */
diff --git a/gdb/mem-break.c b/gdb/mem-break.c
index 7d0e3f1..41c1cd9 100644
--- a/gdb/mem-break.c
+++ b/gdb/mem-break.c
@@ -29,6 +29,7 @@
 #include "breakpoint.h"
 #include "inferior.h"
 #include "target.h"
+#include "gdb_string.h"
 
 
 /* Insert a breakpoint on targets that don't have any better
@@ -46,6 +47,7 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch,
 {
   int val;
   const unsigned char *bp;
+  gdb_byte *readbuf;
 
   /* Determine appropriate breakpoint contents and size for this address.  */
   bp = gdbarch_breakpoint_from_pc
@@ -55,8 +57,11 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch,
 
   /* Save the memory contents.  */
   bp_tgt->shadow_len = bp_tgt->placed_size;
-  val = target_read_memory (bp_tgt->placed_address, bp_tgt->shadow_contents,
+  readbuf = alloca (bp_tgt->placed_size);
+  val = target_read_memory (bp_tgt->placed_address, readbuf,
 			    bp_tgt->placed_size);
+  if (val == 0)
+    memcpy (bp_tgt->shadow_contents, readbuf, bp_tgt->placed_size);
 
   /* Write the breakpoint.  */
   if (val == 0)
diff --git a/gdb/target.c b/gdb/target.c
index cffea2c..6e13ae9 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1608,7 +1608,11 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 }
 
 /* Perform a partial memory transfer.  For docs see target.h,
-   to_xfer_partial.  */
+   to_xfer_partial.
+
+   In addition, READBUF must not be the shadow_contents buffer of
+   one of the breakpoint locations.  Otherwise, this shadow_contents
+   buffer will become corrupted.  */
 
 static LONGEST
 memory_xfer_partial (struct target_ops *ops, enum target_object object,
@@ -1665,7 +1669,12 @@ make_show_memory_breakpoints_cleanup (int show)
 		       (void *) (uintptr_t) current);
 }
 
-/* For docs see target.h, to_xfer_partial.  */
+/* For docs see target.h, to_xfer_partial.
+
+   In addition, READBUF must not be the shadow_contents buffer of
+   one of the breakpoint locations when OBJECT is TARGET_OBJECT_MEMORY
+   or TARGET_OBJECT_STACK_MEMORY.  Otherwise, this shadow_contents
+   buffer will become corrupted.  */
 
 static LONGEST
 target_xfer_partial (struct target_ops *ops,
@@ -1754,7 +1763,11 @@ target_xfer_partial (struct target_ops *ops,
    filling the buffer with good data.  There is no way for the caller to know
    how much good data might have been transfered anyway.  Callers that can
    deal with partial reads should call target_read (which will retry until
-   it makes no progress, and then return how much was transferred).  */
+   it makes no progress, and then return how much was transferred).
+
+   As a limitation, MYADDR must not be the shadow_contents buffer of one
+   of the breakpoint locations.  Passing a breakpoint's shadow_contents
+   buffer will cause that buffer to become corrupted.  */
 
 int
 target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
diff --git a/gdb/target.h b/gdb/target.h
index 50a0ea6..1e7d01a 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -310,8 +310,13 @@ DEF_VEC_P(static_tracepoint_marker_p);
    transfer is not supported or otherwise fails.  Return of a positive
    value less than LEN indicates that no further transfer is possible.
    Unlike the raw to_xfer_partial interface, callers of these
-   functions do not need to retry partial transfers.  */
+   functions do not need to retry partial transfers.
 
+   As a limitation when OBJECT is TARGET_OBJECT_MEMORY or
+   TARGET_OBJECT_STACK_MEMORY,  MYADDR must not be the shadow_contents
+   buffer of one of the breakpoint locations.  Passing a breakpoint's
+   shadow_contents buffer in that situation will cause that buffer
+   to become corrupted.  */
 extern LONGEST target_read (struct target_ops *ops,
 			    enum target_object object,
 			    const char *annex, gdb_byte *buf,
-- 
1.7.1

Problem after hitting breakpoint on Windows (with GDBserver)

[Joel Brobecker]

Hello,

This is a patch that fixes a problem that started appearing on Feb 24th
after support for target-side condition evaluation got added. The problem
occurs on Windows when using GDB+GDBserver.

I decided to write a long-ish introductory message rather than putting
that information with the patch, because it's a lot of details which
I was afraid might dilute a bit the important piece for the patch email.
You may just look at the patch email itself, where the revision history
should provide the minimum amount of info to understand the problem.

In a nutshell, GDB uses memory read/writes to insert/remove breakpoints,
and breakpoint restoration fails. This eventually leads to this kind of
problem after hitting a breakpoint:

        (gdb) b foo
        Breakpoint 1 at 0x40177e: file foo.adb, line 5.
        (gdb) cont
        Continuing.

        Breakpoint 1, foo () at foo.adb:5
        5          Put_Line ("Hello World.");  -- STOP
        (gdb) n

        Program received signal SIGSEGV, Segmentation fault.
        0x00401782 in foo () at foo.adb:5
        5          Put_Line ("Hello World.");  -- STOP

Here is what's happening, in chonological order:

  . When the user types "cont", our breakpoint gets inserted.
    Contrary to GNU/Linux systems, the z0/Z0 packets are not
    supported, so we use memory read/writes instead.

    So, mem-break.c:default_memory_insert_breakpoint reads the
    (piece of) instruction that will be replaced by our breakpoint,
    saves that in the breakpoint location's shadow_contents buffer,
    and then writes the breakpoint instruction.

    So far, so good.

  . GDB receives a new-shared-library event from the kernel.
    GDB processes it, calling solib_add, which calls breakpoint_re_set.
    Basically, new shared library => we need to re-eval our breakpoints,
    to see if something changed. Unfortunately, part of the process
    involves update_breakpoint_locations replacing the breakpoint's
    bp_loc chain by an entirely new chain, as so:

     b->loc = NULL
     [...]
     for (all sals)
       [...]
       new_loc = add_location_to_breakpoint (b, &(sals.sals[i]));

    A side-effect of this is the fact that the location's condition_changed
    flag got reset to condition_modified (see init_bp_location's call to
    mark_breakpoint_location_modified).

  . As a result of this, update_global_location_list sets the new
    location's needs_update flag. (it also makes sure that the new
    location's "inserted" flag is set again).

  . This eventually leads to insert_bp_location trying to re-insert
    the breakpoint...

  . That's when things get interesting: The first thing that happens
    is that insert_bp_location actually wipes out the location's
    target_info data, which means that the breakpoint's shadow_contents
    info is lost. As a result, we are unable to restore the original
    instruction under our breakpoint after we hit it (before transfering
    control back to the user).  This explains the SIGTRAP, because we have
    modified the code executed by the inferior by restoring a corrupted
    shadow_contents.

    The first part of the fix avoids the re-initialization of the
    whole target_info structure, avoid the wipe.

  . But that's not enough. Still while trying to re-insert the breakpoint,
    we land inside mem-break.c:default_memory_insert_breakpoint.
    This function does not seem to have enough information to determine
    whether the breakpoint is already inserted or not. So it just
    assumes that it hasn't and performs two actions:
       - Save the original code in the shadow_contents buffer;
       - Write the breakpoint instruction in its place.

    That's where we hit an unexpected limitation of target_read_memory.
    This funtion knows about breakpoints and uses the shadow_contents
    buffer if the read touches one of the areas where a breakpoint is
    still inserted. Unfortunately, the way the code is written, here is
    what happens:
       - memory_xfer_partial fetches the raw memory, potentially polluted
         by inserted breakpoints, saves the result in the target buffer;
       - memory_xfer_partial then calls breakpoint_xfer_memory to update
         the target buffer with pieces of shadow_contents as appropriate
         to remove the inserted breakpoints from the result.
    This approach breaks down when the buffer passed to these read
    functions is actually a shadow_contents buffer, because the first
    read ends up overwriting the shadow_contents, thus rendering the
    call to breakpoint_xfer_memory useless.

    One way to fix that problem was to lift that limitation, by using
    a temporary buffer inside memory_xfer_partial, and then copy the
    contents of that buffer into the target buffer after the two steps
    are complete. But Pedro felt it was too heavy a hammer.

    So instead, we fixed the two call sites where a target-read
    operation is performed with a breakpoint shadow_contents buffer
    as the argument.

One side comment:

  * It looks like "b->loc = NULL" in update_breakpoint_locations
    is a memory leak, since the locations' ref counters are not
    decremented?

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

[Jan Kratochvil]

On Tue, 13 Mar 2012 02:39:15 +0100, Joel Brobecker wrote:
>     One way to fix that problem was to lift that limitation, by using
>     a temporary buffer inside memory_xfer_partial, and then copy the
>     contents of that buffer into the target buffer after the two steps
>     are complete. But Pedro felt it was too heavy a hammer.

This problem was affecting also gdbserver which I fixed in December:
	[patch] gdbserver: Fix overlapping memcpy (safe now)
	http://sourceware.org/ml/gdb-patches/2011-12/msg00057.html

I tried now shortly and I cannot reproduce the valgrind errors for GDB with
gdbserver with disabled z* packets but according to the code and according to
you description it looks as the same problem.

Isn't it safer to fix all the cases?


Thanks,
Jan

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

[Pedro Alves]

On 03/13/2012 02:24 PM, Jan Kratochvil wrote:

> On Tue, 13 Mar 2012 02:39:15 +0100, Joel Brobecker wrote:
>>     One way to fix that problem was to lift that limitation, by using
>>     a temporary buffer inside memory_xfer_partial, and then copy the
>>     contents of that buffer into the target buffer after the two steps
>>     are complete. But Pedro felt it was too heavy a hammer.
> 
> This problem was affecting also gdbserver which I fixed in December:
> 	[patch] gdbserver: Fix overlapping memcpy (safe now)
> 	http://sourceware.org/ml/gdb-patches/2011-12/msg00057.html
> 
> I tried now shortly and I cannot reproduce the valgrind errors for GDB with
> gdbserver with disabled z* packets but according to the code and according to
> you description it looks as the same problem.


Yeah, it's similar.

> Isn't it safer to fix all the cases?


Joel's patch is equivalent to yours to gdbserver.  It fixes the only code
that needs to bother with this, which is the code that writes to the
shadow buffers.  All other callers don't need to worry about this overlap,
and thus we can avoid heap allocating a temporary buffer as large as the
transfer len for all reads.

I think we could put asserts in breakpoint_xfer_memory like those of
check_mem_read/check_mem_write.

-- 
Pedro Alves

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

[Pedro Alves]

On 03/13/2012 01:39 AM, Joel Brobecker wrote:

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


Merge?

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

Otherwise this is fine with me.  An assertion in breakpoint_xfer_memory
to catch that READBUF or WRITEBUF doesn't overlap bp->target_info.shadow_contents
would be nice.  We could even avoid the new comments in target.h that way, as
there's be no way to write a new hook that doesn't trip on it instantly.

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.

-- 
Pedro Alves

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

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

[-- Attachment #2: 0001-Problem-after-hitting-breakpoint-on-Windows-with-GDB.patch --]
[-- Type: text/x-diff, Size: 7065 bytes --]

From 6f9ed095afee3065abf3dcda8e364153734938f4 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Mon, 12 Mar 2012 22:44:05 +0100
Subject: [PATCH] Problem after hitting breakpoint on Windows (with GDBserver)

gdb/ChangeLog:

        * breakpoint.c (insert_bp_location): Do not wipe bl->target_info out.

        * target.h (target_read): Document limitation.
        * target.c (memory_xfer_partial, target_xfer_partial)
        (target_read_memory): Document limitation.
        * mem-break.c: #include "gdb_string.h".
        (default_memory_insert_breakpoint): Do not call target_read_memory
        with a pointer to the breakpoint's shadow_contents buffer.  Use
        a local buffer instead.
        * m32r-tdep.c (m32r_memory_insert_breakpoint): Ditto.
---
 gdb/breakpoint.c |   11 +++++++++--
 gdb/m32r-tdep.c  |    3 ++-
 gdb/mem-break.c  |   17 +++++++++++------
 gdb/target.c     |   19 ++++++++++++++++---
 gdb/target.h     |    7 ++++++-
 5 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d35704d..95d8783 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2082,8 +2082,15 @@ insert_bp_location (struct bp_location *bl,
   if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
     return 0;
 
-  /* Initialize the target-specific information.  */
-  memset (&bl->target_info, 0, sizeof (bl->target_info));
+  /* Note we don't initialize bl->target_info, as that wipes out
+     the breakpoint location's shadow_contents if the breakpoint
+     is still inserted at that location.  This in turn breaks
+     target_read_memory which depends on these buffers when
+     a memory read is requested at the breakpoint location:
+     Once the target_info has been wiped, we fail to see that
+     we have a breakpoint inserted at that address and thus
+     read the breakpoint instead of returning the data saved in
+     the breakpoint location's shadow contents.  */
   bl->target_info.placed_address = bl->address;
   bl->target_info.placed_address_space = bl->pspace->aspace;
   bl->target_info.length = bl->length;
diff --git a/gdb/m32r-tdep.c b/gdb/m32r-tdep.c
index 72872bd..d504eb3 100644
--- a/gdb/m32r-tdep.c
+++ b/gdb/m32r-tdep.c
@@ -85,7 +85,7 @@ m32r_memory_insert_breakpoint (struct gdbarch *gdbarch,
   CORE_ADDR addr = bp_tgt->placed_address;
   int val;
   gdb_byte buf[4];
-  gdb_byte *contents_cache = bp_tgt->shadow_contents;
+  gdb_byte contents_cache[4];
   gdb_byte bp_entry[] = { 0x10, 0xf1 };	/* dpt */
 
   /* Save the memory contents.  */
@@ -93,6 +93,7 @@ m32r_memory_insert_breakpoint (struct gdbarch *gdbarch,
   if (val != 0)
     return val;			/* return error */
 
+  memcpy (bp_tgt->shadow_contents, contents_cache, 4);
   bp_tgt->placed_size = bp_tgt->shadow_len = 4;
 
   /* Determine appropriate breakpoint contents and size for this address.  */
diff --git a/gdb/mem-break.c b/gdb/mem-break.c
index 7d0e3f1..bd34fb2 100644
--- a/gdb/mem-break.c
+++ b/gdb/mem-break.c
@@ -29,6 +29,7 @@
 #include "breakpoint.h"
 #include "inferior.h"
 #include "target.h"
+#include "gdb_string.h"
 
 
 /* Insert a breakpoint on targets that don't have any better
@@ -46,6 +47,7 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch,
 {
   int val;
   const unsigned char *bp;
+  gdb_byte *readbuf;
 
   /* Determine appropriate breakpoint contents and size for this address.  */
   bp = gdbarch_breakpoint_from_pc
@@ -53,15 +55,18 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch,
   if (bp == NULL)
     error (_("Software breakpoints not implemented for this target."));
 
-  /* Save the memory contents.  */
+  /* Save the memory contents in the shadow_contents buffer and then
+     write the breakpoint instruction.  */
   bp_tgt->shadow_len = bp_tgt->placed_size;
-  val = target_read_memory (bp_tgt->placed_address, bp_tgt->shadow_contents,
+  readbuf = alloca (bp_tgt->placed_size);
+  val = target_read_memory (bp_tgt->placed_address, readbuf,
 			    bp_tgt->placed_size);
-
-  /* Write the breakpoint.  */
   if (val == 0)
-    val = target_write_raw_memory (bp_tgt->placed_address, bp,
-				   bp_tgt->placed_size);
+    {
+      memcpy (bp_tgt->shadow_contents, readbuf, bp_tgt->placed_size);
+      val = target_write_raw_memory (bp_tgt->placed_address, bp,
+				     bp_tgt->placed_size);
+    }
 
   return val;
 }
diff --git a/gdb/target.c b/gdb/target.c
index cffea2c..2afae74 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1608,7 +1608,11 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 }
 
 /* Perform a partial memory transfer.  For docs see target.h,
-   to_xfer_partial.  */
+   to_xfer_partial.
+
+   In addition, READBUF must not be the shadow_contents buffer of
+   one of the breakpoint locations.  Otherwise, this shadow_contents
+   buffer will become corrupted.  */
 
 static LONGEST
 memory_xfer_partial (struct target_ops *ops, enum target_object object,
@@ -1665,7 +1669,12 @@ make_show_memory_breakpoints_cleanup (int show)
 		       (void *) (uintptr_t) current);
 }
 
-/* For docs see target.h, to_xfer_partial.  */
+/* For docs see target.h, to_xfer_partial.
+
+   In addition, READBUF must not be the shadow_contents buffer of
+   one of the breakpoint locations when OBJECT is TARGET_OBJECT_MEMORY
+   or TARGET_OBJECT_STACK_MEMORY.  Otherwise, this shadow_contents
+   buffer will become corrupted.  */
 
 static LONGEST
 target_xfer_partial (struct target_ops *ops,
@@ -1754,7 +1763,11 @@ target_xfer_partial (struct target_ops *ops,
    filling the buffer with good data.  There is no way for the caller to know
    how much good data might have been transfered anyway.  Callers that can
    deal with partial reads should call target_read (which will retry until
-   it makes no progress, and then return how much was transferred).  */
+   it makes no progress, and then return how much was transferred).
+
+   MYADDR must not be the shadow_contents buffer of one of the breakpoint
+   locations.  Passing a breakpoint's shadow_contents buffer will cause
+   that buffer to become corrupted.  */
 
 int
 target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
diff --git a/gdb/target.h b/gdb/target.h
index 50a0ea6..69d7a5d 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -310,7 +310,12 @@ DEF_VEC_P(static_tracepoint_marker_p);
    transfer is not supported or otherwise fails.  Return of a positive
    value less than LEN indicates that no further transfer is possible.
    Unlike the raw to_xfer_partial interface, callers of these
-   functions do not need to retry partial transfers.  */
+   functions do not need to retry partial transfers.
+
+   When OBJECT is TARGET_OBJECT_MEMORY or TARGET_OBJECT_STACK_MEMORY,
+   MYADDR must not be the shadow_contents buffer of one of the breakpoint
+   locations.  Passing a breakpoint's shadow_contents buffer in that
+   situation will cause that buffer to become corrupted.  */
 
 extern LONGEST target_read (struct target_ops *ops,
 			    enum target_object object,
-- 
1.7.1

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

[Pedro Alves]

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

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

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

Nah, I don't think it's all that important. The code is fairly
compact and self-evident.

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

Ah, of course! It's a little simpler than what I had in mind.

Attached is a new patch which adds the assertion, as well as updates
breakpoint_xfer_memory's description to document the assertion.
You'll notice that the description says that any overlap should
trigger an exception whereas this is not quite accurate. The assertion
will only trigger if the (memaddr,len) overlaps with an inserted
breakpoint location and the readbuf buffer overlaps with that
breakpoint location's shadow_contents buffer. So, technically,
we're going to miss situations where we pass the shadow_contents
of another breakpoint location.  But I didn't feel that it was
worth the complication in the explanation.  What do you think?

I think that you also might prefer if I remove all the changes that
document the fact that the read buffer should not be a shadow_contents,
and just document this in one place.  I will do that next. I am just
sending this patch now, to have a trace of the patch before I undo
some of my changes.

Thanks,
-- 
Joel

[-- Attachment #2: 0001-Problem-after-hitting-breakpoint-on-Windows-with-GDB.patch --]
[-- Type: text/x-diff, Size: 8265 bytes --]

From a2e5d0069a33d21722710ce13cdd0ce178f50c84 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Mon, 12 Mar 2012 22:44:05 +0100
Subject: [PATCH] Problem after hitting breakpoint on Windows (with GDBserver)

gdb/ChangeLog:

        * breakpoint.c (insert_bp_location): Do not wipe bl->target_info out.

        * target.h (target_read): Document limitation.
        * target.c (memory_xfer_partial, target_xfer_partial)
        (target_read_memory): Document limitation.
        * breakpoint.c (breakpoint_xfer_memory): Add assertion.
        Update function description.
        * mem-break.c: #include "gdb_string.h".
        (default_memory_insert_breakpoint): Do not call target_read_memory
        with a pointer to the breakpoint's shadow_contents buffer.  Use
        a local buffer instead.
        * m32r-tdep.c (m32r_memory_insert_breakpoint): Ditto.
---
 gdb/breakpoint.c |   21 +++++++++++++++++++--
 gdb/m32r-tdep.c  |    3 ++-
 gdb/mem-break.c  |   17 +++++++++++------
 gdb/target.c     |   19 ++++++++++++++++---
 gdb/target.h     |    7 ++++++-
 5 files changed, 54 insertions(+), 13 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d35704d..debf2b2 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1318,6 +1318,10 @@ bp_location_has_shadow (struct bp_location *bl)
 /* Update BUF, which is LEN bytes read from the target address MEMADDR,
    by replacing any memory breakpoints with their shadowed contents.
 
+   If READBUF is not NULL, this buffer must not overlap with any of
+   the breakpoint location's shadow_contents buffers.  Otherwise,
+   a failed assertion internal error will be raised.
+
    The range of shadowed area by each bp_location is:
      bl->address - bp_location_placed_address_before_address_max
      up to bl->address + bp_location_shadow_len_after_address_max
@@ -1446,6 +1450,12 @@ breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
 
     if (readbuf != NULL)
       {
+	/* Verify that the readbuf buffer does not overlap with
+	   the shadow_contents buffer.  */
+	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,
@@ -2082,8 +2092,15 @@ insert_bp_location (struct bp_location *bl,
   if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
     return 0;
 
-  /* Initialize the target-specific information.  */
-  memset (&bl->target_info, 0, sizeof (bl->target_info));
+  /* Note we don't initialize bl->target_info, as that wipes out
+     the breakpoint location's shadow_contents if the breakpoint
+     is still inserted at that location.  This in turn breaks
+     target_read_memory which depends on these buffers when
+     a memory read is requested at the breakpoint location:
+     Once the target_info has been wiped, we fail to see that
+     we have a breakpoint inserted at that address and thus
+     read the breakpoint instead of returning the data saved in
+     the breakpoint location's shadow contents.  */
   bl->target_info.placed_address = bl->address;
   bl->target_info.placed_address_space = bl->pspace->aspace;
   bl->target_info.length = bl->length;
diff --git a/gdb/m32r-tdep.c b/gdb/m32r-tdep.c
index 72872bd..d504eb3 100644
--- a/gdb/m32r-tdep.c
+++ b/gdb/m32r-tdep.c
@@ -85,7 +85,7 @@ m32r_memory_insert_breakpoint (struct gdbarch *gdbarch,
   CORE_ADDR addr = bp_tgt->placed_address;
   int val;
   gdb_byte buf[4];
-  gdb_byte *contents_cache = bp_tgt->shadow_contents;
+  gdb_byte contents_cache[4];
   gdb_byte bp_entry[] = { 0x10, 0xf1 };	/* dpt */
 
   /* Save the memory contents.  */
@@ -93,6 +93,7 @@ m32r_memory_insert_breakpoint (struct gdbarch *gdbarch,
   if (val != 0)
     return val;			/* return error */
 
+  memcpy (bp_tgt->shadow_contents, contents_cache, 4);
   bp_tgt->placed_size = bp_tgt->shadow_len = 4;
 
   /* Determine appropriate breakpoint contents and size for this address.  */
diff --git a/gdb/mem-break.c b/gdb/mem-break.c
index 7d0e3f1..bd34fb2 100644
--- a/gdb/mem-break.c
+++ b/gdb/mem-break.c
@@ -29,6 +29,7 @@
 #include "breakpoint.h"
 #include "inferior.h"
 #include "target.h"
+#include "gdb_string.h"
 
 
 /* Insert a breakpoint on targets that don't have any better
@@ -46,6 +47,7 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch,
 {
   int val;
   const unsigned char *bp;
+  gdb_byte *readbuf;
 
   /* Determine appropriate breakpoint contents and size for this address.  */
   bp = gdbarch_breakpoint_from_pc
@@ -53,15 +55,18 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch,
   if (bp == NULL)
     error (_("Software breakpoints not implemented for this target."));
 
-  /* Save the memory contents.  */
+  /* Save the memory contents in the shadow_contents buffer and then
+     write the breakpoint instruction.  */
   bp_tgt->shadow_len = bp_tgt->placed_size;
-  val = target_read_memory (bp_tgt->placed_address, bp_tgt->shadow_contents,
+  readbuf = alloca (bp_tgt->placed_size);
+  val = target_read_memory (bp_tgt->placed_address, readbuf,
 			    bp_tgt->placed_size);
-
-  /* Write the breakpoint.  */
   if (val == 0)
-    val = target_write_raw_memory (bp_tgt->placed_address, bp,
-				   bp_tgt->placed_size);
+    {
+      memcpy (bp_tgt->shadow_contents, readbuf, bp_tgt->placed_size);
+      val = target_write_raw_memory (bp_tgt->placed_address, bp,
+				     bp_tgt->placed_size);
+    }
 
   return val;
 }
diff --git a/gdb/target.c b/gdb/target.c
index cffea2c..2afae74 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1608,7 +1608,11 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 }
 
 /* Perform a partial memory transfer.  For docs see target.h,
-   to_xfer_partial.  */
+   to_xfer_partial.
+
+   In addition, READBUF must not be the shadow_contents buffer of
+   one of the breakpoint locations.  Otherwise, this shadow_contents
+   buffer will become corrupted.  */
 
 static LONGEST
 memory_xfer_partial (struct target_ops *ops, enum target_object object,
@@ -1665,7 +1669,12 @@ make_show_memory_breakpoints_cleanup (int show)
 		       (void *) (uintptr_t) current);
 }
 
-/* For docs see target.h, to_xfer_partial.  */
+/* For docs see target.h, to_xfer_partial.
+
+   In addition, READBUF must not be the shadow_contents buffer of
+   one of the breakpoint locations when OBJECT is TARGET_OBJECT_MEMORY
+   or TARGET_OBJECT_STACK_MEMORY.  Otherwise, this shadow_contents
+   buffer will become corrupted.  */
 
 static LONGEST
 target_xfer_partial (struct target_ops *ops,
@@ -1754,7 +1763,11 @@ target_xfer_partial (struct target_ops *ops,
    filling the buffer with good data.  There is no way for the caller to know
    how much good data might have been transfered anyway.  Callers that can
    deal with partial reads should call target_read (which will retry until
-   it makes no progress, and then return how much was transferred).  */
+   it makes no progress, and then return how much was transferred).
+
+   MYADDR must not be the shadow_contents buffer of one of the breakpoint
+   locations.  Passing a breakpoint's shadow_contents buffer will cause
+   that buffer to become corrupted.  */
 
 int
 target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
diff --git a/gdb/target.h b/gdb/target.h
index 50a0ea6..69d7a5d 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -310,7 +310,12 @@ DEF_VEC_P(static_tracepoint_marker_p);
    transfer is not supported or otherwise fails.  Return of a positive
    value less than LEN indicates that no further transfer is possible.
    Unlike the raw to_xfer_partial interface, callers of these
-   functions do not need to retry partial transfers.  */
+   functions do not need to retry partial transfers.
+
+   When OBJECT is TARGET_OBJECT_MEMORY or TARGET_OBJECT_STACK_MEMORY,
+   MYADDR must not be the shadow_contents buffer of one of the breakpoint
+   locations.  Passing a breakpoint's shadow_contents buffer in that
+   situation will cause that buffer to become corrupted.  */
 
 extern LONGEST target_read (struct target_ops *ops,
 			    enum target_object object,
-- 
1.7.1

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 446 bytes --]

> I think that you also might prefer if I remove all the changes that
> document the fact that the read buffer should not be a shadow_contents,
> and just document this in one place.  I will do that next. I am just
> sending this patch now, to have a trace of the patch before I undo
> some of my changes.

And attached is the simplified version without the extra comments.
Tested on x86_64-linux, with and without GDBserver...

Thanks,
-- 
Joel

[-- Attachment #2: 0001-Problem-after-hitting-breakpoint-on-Windows-with-GDB.patch --]
[-- Type: text/x-diff, Size: 5330 bytes --]

From 8a1c2bb5f60332adcbb00a3d9c20b9e924e3e90b Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Mon, 12 Mar 2012 22:44:05 +0100
Subject: [PATCH] Problem after hitting breakpoint on Windows (with GDBserver)

gdb/ChangeLog:

        * breakpoint.c (breakpoint_xfer_memory): Add assertion.
        Update function description.
        (insert_bp_location): Do not wipe bl->target_info out.
        * mem-break.c: #include "gdb_string.h".
        (default_memory_insert_breakpoint): Do not call target_read_memory
        with a pointer to the breakpoint's shadow_contents buffer.  Use
        a local buffer instead.
        * m32r-tdep.c (m32r_memory_insert_breakpoint): Ditto.
---
 gdb/breakpoint.c |   21 +++++++++++++++++++--
 gdb/m32r-tdep.c  |    3 ++-
 gdb/mem-break.c  |   17 +++++++++++------
 3 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d35704d..debf2b2 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1318,6 +1318,10 @@ bp_location_has_shadow (struct bp_location *bl)
 /* Update BUF, which is LEN bytes read from the target address MEMADDR,
    by replacing any memory breakpoints with their shadowed contents.
 
+   If READBUF is not NULL, this buffer must not overlap with any of
+   the breakpoint location's shadow_contents buffers.  Otherwise,
+   a failed assertion internal error will be raised.
+
    The range of shadowed area by each bp_location is:
      bl->address - bp_location_placed_address_before_address_max
      up to bl->address + bp_location_shadow_len_after_address_max
@@ -1446,6 +1450,12 @@ breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf,
 
     if (readbuf != NULL)
       {
+	/* Verify that the readbuf buffer does not overlap with
+	   the shadow_contents buffer.  */
+	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,
@@ -2082,8 +2092,15 @@ insert_bp_location (struct bp_location *bl,
   if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
     return 0;
 
-  /* Initialize the target-specific information.  */
-  memset (&bl->target_info, 0, sizeof (bl->target_info));
+  /* Note we don't initialize bl->target_info, as that wipes out
+     the breakpoint location's shadow_contents if the breakpoint
+     is still inserted at that location.  This in turn breaks
+     target_read_memory which depends on these buffers when
+     a memory read is requested at the breakpoint location:
+     Once the target_info has been wiped, we fail to see that
+     we have a breakpoint inserted at that address and thus
+     read the breakpoint instead of returning the data saved in
+     the breakpoint location's shadow contents.  */
   bl->target_info.placed_address = bl->address;
   bl->target_info.placed_address_space = bl->pspace->aspace;
   bl->target_info.length = bl->length;
diff --git a/gdb/m32r-tdep.c b/gdb/m32r-tdep.c
index 72872bd..d504eb3 100644
--- a/gdb/m32r-tdep.c
+++ b/gdb/m32r-tdep.c
@@ -85,7 +85,7 @@ m32r_memory_insert_breakpoint (struct gdbarch *gdbarch,
   CORE_ADDR addr = bp_tgt->placed_address;
   int val;
   gdb_byte buf[4];
-  gdb_byte *contents_cache = bp_tgt->shadow_contents;
+  gdb_byte contents_cache[4];
   gdb_byte bp_entry[] = { 0x10, 0xf1 };	/* dpt */
 
   /* Save the memory contents.  */
@@ -93,6 +93,7 @@ m32r_memory_insert_breakpoint (struct gdbarch *gdbarch,
   if (val != 0)
     return val;			/* return error */
 
+  memcpy (bp_tgt->shadow_contents, contents_cache, 4);
   bp_tgt->placed_size = bp_tgt->shadow_len = 4;
 
   /* Determine appropriate breakpoint contents and size for this address.  */
diff --git a/gdb/mem-break.c b/gdb/mem-break.c
index 7d0e3f1..bd34fb2 100644
--- a/gdb/mem-break.c
+++ b/gdb/mem-break.c
@@ -29,6 +29,7 @@
 #include "breakpoint.h"
 #include "inferior.h"
 #include "target.h"
+#include "gdb_string.h"
 
 
 /* Insert a breakpoint on targets that don't have any better
@@ -46,6 +47,7 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch,
 {
   int val;
   const unsigned char *bp;
+  gdb_byte *readbuf;
 
   /* Determine appropriate breakpoint contents and size for this address.  */
   bp = gdbarch_breakpoint_from_pc
@@ -53,15 +55,18 @@ default_memory_insert_breakpoint (struct gdbarch *gdbarch,
   if (bp == NULL)
     error (_("Software breakpoints not implemented for this target."));
 
-  /* Save the memory contents.  */
+  /* Save the memory contents in the shadow_contents buffer and then
+     write the breakpoint instruction.  */
   bp_tgt->shadow_len = bp_tgt->placed_size;
-  val = target_read_memory (bp_tgt->placed_address, bp_tgt->shadow_contents,
+  readbuf = alloca (bp_tgt->placed_size);
+  val = target_read_memory (bp_tgt->placed_address, readbuf,
 			    bp_tgt->placed_size);
-
-  /* Write the breakpoint.  */
   if (val == 0)
-    val = target_write_raw_memory (bp_tgt->placed_address, bp,
-				   bp_tgt->placed_size);
+    {
+      memcpy (bp_tgt->shadow_contents, readbuf, bp_tgt->placed_size);
+      val = target_write_raw_memory (bp_tgt->placed_address, bp,
+				     bp_tgt->placed_size);
+    }
 
   return val;
 }
-- 
1.7.1

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

[Pedro Alves]

On 03/15/2012 01:03 AM, Joel Brobecker wrote:

> And attached is the simplified version without the extra comments.
> Tested on x86_64-linux, with and without GDBserver...


I have no further comments.  Looks good to me.  Thanks.

-- 
Pedro Alves

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

[Joel Brobecker]

> I have no further comments.  Looks good to me.  Thanks.

Thanks, Pedro.  Now checked in.

-- 
Joel