From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17744 invoked by alias); 14 Mar 2012 21:50:50 -0000 Received: (qmail 17735 invoked by uid 22791); 14 Mar 2012 21:50:48 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,TW_CP X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 14 Mar 2012 21:50:34 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 9C7CE1C6B74; Wed, 14 Mar 2012 17:50:33 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id j82eeiHFN9f4; Wed, 14 Mar 2012 17:50:33 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 44D731C6AB0; Wed, 14 Mar 2012 17:50:33 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 6FB96145615; Wed, 14 Mar 2012 14:50:24 -0700 (PDT) Date: Wed, 14 Mar 2012 21:50:00 -0000 From: Joel Brobecker To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [RFA] Problem after hitting breakpoint on Windows (with GDBserver) Message-ID: <20120314215024.GL2853@adacore.com> References: <1331602756-23567-1-git-send-email-brobecker@adacore.com> <1331602756-23567-2-git-send-email-brobecker@adacore.com> <4F5F6187.50209@redhat.com> <20120313215956.GT2853@adacore.com> <4F5FD262.9030708@redhat.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="o0ZfoUVt4BxPQnbU" Content-Disposition: inline In-Reply-To: <4F5FD262.9030708@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-03/txt/msg00518.txt.bz2 --o0ZfoUVt4BxPQnbU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 1288 > 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 --o0ZfoUVt4BxPQnbU Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-Problem-after-hitting-breakpoint-on-Windows-with-GDB.patch" Content-length: 8266 >From a2e5d0069a33d21722710ce13cdd0ce178f50c84 Mon Sep 17 00:00:00 2001 From: Joel Brobecker 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 --o0ZfoUVt4BxPQnbU--