From patchwork Mon Sep 29 18:28:51 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 3024 Received: (qmail 20344 invoked by alias); 29 Sep 2014 18:29:02 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 20335 invoked by uid 89); 29 Sep 2014 18:29:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.4 required=5.0 tests=AWL, BAYES_00, LOTS_OF_MONEY, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS, UNSUBSCRIBE_BODY autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 29 Sep 2014 18:29:00 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s8TISsNM010447 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 29 Sep 2014 14:28:55 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s8TISqcI029667; Mon, 29 Sep 2014 14:28:53 -0400 Message-ID: <5429A4E3.40108@redhat.com> Date: Mon, 29 Sep 2014 19:28:51 +0100 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: "Maciej W. Rozycki" CC: gdb-patches@sourceware.org, Luis Machado Subject: Re: [PATCH] Avoid software breakpoint's instruction shadow inconsistency References: <5421B1B3.7010106@redhat.com> In-Reply-To: On 09/23/2014 07:11 PM, Maciej W. Rozycki wrote: > On Tue, 23 Sep 2014, Pedro Alves wrote: >> It's totally fine to call gdbarch_breakpoint_from_pc on an already >> adjusted address. That method naturally has to be idempotent. It'll >> just return without adjusting anything, as the input address must already >> be a fine address to place a breakpoint, otherwise the first call that >> actually adjusted the address when the breakpoint was first >> inserted wouldn't have returned it in the first place. > > The thing is it can't, because on MIPS targets the ISA bit can be the > only place where the breakpoint type requirement is encoded -- if you set > a breakpoint by address in code that has no symbol information, e.g.: > > (gdb) break *0x87654321 > > is not the same as: > > (gdb) break *0x87654320 > > and consequently if there's no symbol information available for either of > 0x87654321 or 0x87654320, then `mips_breakpoint_from_pc' will return a > microMIPS (or MIPS16, as determined elsewhere) breakpoint for the value of > 0x87654321 in `placed_address' and a standard MIPS breakpoint for the > value of 0x87654320 there. So the value of 0x87654321 has to be stored > somewhere and subsequent calls to `mips_breakpoint_from_pc' have to see it > again (and convert to 0x87654320 in `placed_address'). > > Please note that this is not a theoretical or corner case, because we can > easily use breakpoints in code with no symbol information (e.g. > system-installed shared libraries; dynamic symbols associated with > exported entry points will likely not cover all the code) when > single-stepping with a software watchpoint enabled. OK, missed that. I was assuming things like mapping symbols sorting things out. But MIPS doesn't have those, and they can be stripped on ARM too, in any case. >>> This is also important for places >>> like `find_single_step_breakpoint' where a breakpoint's address is >>> compared to the raw value of $pc. >>> >> >> AFAICS, insert_single_step_breakpoint also doesn't do >> gdbarch_breakpoint_from_pc, so there doesn't seem to be a mismatch. > > The problem is `find_single_step_breakpoint' is called in the ordinary > breakpoint removal path too, so that if a single-step breakpoint is placed > at the same address, it is retained. I saw this place do bad things in > testing my change before I adjusted it to use `reqstd_address'. > >> In any case, find_single_step_breakpoint and insert_single_step_breakpoint >> and related deprecated bits are scheduled for deletion >> very soon: https://sourceware.org/ml/gdb-patches/2014-09/msg00242.html > > The relevant parts of my change can easily be removed if they do not make > it beforehand. The thing is that all that code was only added to get 7.8 out of the door and then do things better post 7.8. See discussion around: https://sourceware.org/ml/gdb-patches/2014-06/msg00052.html So not just the single-step bits; ISTM most of the patch ends up unnecessary. As soon as breakpoint_xfer_memory doesn't have to care about single-step breakpoints, one_breakpoint_xfer_memory can work with the location's bl->address instead of bl->target_info.placed_address. bl->address is always the same as your bl->target_info.reqstd_address, afaics. The reason one_breakpoint_xfer_memory can't work with bl->address instead today is that software single-step breakpoints aren't a location, only a target_info, so the original address is lost... Piling on workarounds due to these single-step breakpoint issues is exactly the sort of thing I'd like to avoid further. It's of course also completely unnecessary/bogus for GDB to be reinserting breakpoints when the target doesn't handle breakpoints itself, but that's another story. I'm confused on this part of your original description: > The issue is `insert_bp_location' overwrites the previously adjusted > value in `placed_address' with the original address, that is only replaced > back with the correct adjusted address later on when > `gdbarch_breakpoint_from_pc' is called. Meanwhile there's a window where > the value in `placed_address' does not correspond to data stored in > `shadow_contents', leading to incorrect instruction bytes being supplied > when `one_breakpoint_xfer_memory' is called to supply the instruction > overlaid by the breakpoint. It doesn't look like to me that this is really the problem, since default_memory_insert_breakpoint adjusts bp_tgt->placed_address before reading memory. Instead, the issue is that because the breakpoint is supposed to be inserted (we're re-inserting it), one_breakpoint_xfer_memory needs to store the breakpoint instruction on top of the memory we're about to write. And then one_breakpoint_xfer_memory gets the breakpoint instruction wrong exactly because it lost the ISA bit. Although we can now see this more easily since we now reinsert already inserted breakpoints, we should be able to trigger the issue even with that, if the user writes to memory at an address where a breakpoint with the ISA bit was inserted. Like: (gdb) set breakpoint always-inserted on (gdb) p /x *(char*) 0x87654321 $4 = 0x55 (gdb) break *0x87654321 (gdb) p /x *(char*) 0x87654321 = 0x55 $5 = 0x55 At this point, 0x87654321 should still contain the correct breakpoint insn (on the target), but due to the one_breakpoint_xfer_memory issue, it won't. BTW, if instead of: @@ -2543,7 +2543,7 @@ insert_bp_location (struct bp_location * 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.reqstd_address = bl->address; you did: bl->target_info.placed_address = bl->address; + bl->target_info.reqstd_address = bl->address; then it seems to me that most of the hunks that do something like this: @@ -975,7 +975,7 @@ arm_linux_hw_breakpoint_initialize (stru struct arm_linux_hw_breakpoint *p) { unsigned mask; - CORE_ADDR address = bp_tgt->placed_address; + CORE_ADDR address = bp_tgt->placed_address = bp_tgt->reqstd_address; including the default_memory_insert_breakpoint changes, would be unnecessary. I could be missing something else, of course. The patch below is what I'd like to push on top of the software single-step rework (which I've meanwhile slit and posted here https://sourceware.org/ml/gdb-patches/2014-09/msg00755.html) I've pushed that series with this patch on top here, for convenience: git@github.com:palves/gdb.git palves/mips_instruction_shadow_inconsistency Obviously, the mips-linux-gnu testing mentioned in the log is tentative. :-) -------- 8< -------- From e6e451ac1c15d69966233d07165c917d83e51d29 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Mon, 29 Sep 2014 17:24:47 +0100 Subject: [PATCH] [PATCH] Fix memory writes to regions with inserted breakpoints This patch fixes an issue with writing memory to addresses where we have breakpoints inserted, on architectures for which a breakpoint's `placed_address' may not be the same as the original requested address. This was exposed by this change: commit b775012e845380ed4c7421a1b87caf7bfae39f5f Author: Luis Machado Date: Fri Feb 24 15:10:59 2012 +0000 2012-02-24 Luis Machado * remote.c (remote_supports_cond_breakpoints): New forward declaration. [...] Although the issue triggers just as well if the user manually writes to memory at an address where such a breakpoint is inserted. When the target reports support for target-side conditional breakpoint evaluation, breakpoints are inserted and removed such that `insert_bp_location' can now be called with the breakpoint being handled already in place, while with such support the call is only ever made for breakpoints that have not been put in place. When writing a block of memory to an address where we have a breakpoint inserted, we need to make sure that: #1 - we update the inserted breakpoint's shadow buffers with the new memory contents, so that the breakpoint may later be correctly uninserted. #2 - so that the breakpoint remains inserted in target memory, the breakpoint instruction is overlaid on top of the memory buffer the caller wanted to have writen to the target, and the result is what is actually committed to target's memory. The problem is with #2: one_breakpoint_xfer_memory is passing the target_info's `placed_address' address to `gdbarch_breakpoint_from_pc' to determine which breakpoint instruction to overlay, but this address is not the one that mem-break.c originally used to decide which breakpoint instruction to use. See the comment added by the patch for further details. Regression tested on the mips-linux-gnu target and the following multilibs: -EB -EB -msoft-float -EB -mips16 -EB -mips16 -msoft-float -EB -mmicromips -EB -mmicromips -msoft-float -EB -mabi=n32 -EB -mabi=n32 -msoft-float -EB -mabi=64 -EB -mabi=64 -msoft-float and the -EL variants of same with no regressions and removing ~900 seemingly random failures for each of the little-endian microMIPS multilibs. Obviously in the big-endian case the bug did not cause the wrong breakpoint instruction to have been chosen. gdb/ 2014-09-29 Pedro Alves Maciej W. Rozycki * breakpoint.c (one_breakpoint_xfer_memory): Take a bl_location as parameter instead of a bp_target_info and a gdbarch. Pass the location's address to gdbarch_breakpoint_from_pc instead of the target_info's placed address. Add comment. (breakpoint_xfer_memory): Adjust. --- gdb/breakpoint.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index dacb867..de7320e 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1471,13 +1471,13 @@ static void one_breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf, const gdb_byte *writebuf_org, ULONGEST memaddr, LONGEST len, - struct bp_target_info *target_info, - struct gdbarch *gdbarch) + struct bp_location *bl) { /* Now do full processing of the found relevant range of elements. */ CORE_ADDR bp_addr = 0; int bp_size = 0; int bptoffset = 0; + struct bp_target_info *target_info = &bl->target_info; if (!breakpoint_address_match (target_info->placed_address_space, 0, current_program_space->aspace, 0)) @@ -1536,16 +1536,35 @@ one_breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf, else { const unsigned char *bp; - CORE_ADDR placed_address = target_info->placed_address; - int placed_size = target_info->placed_size; + CORE_ADDR address; + int placed_size; /* Update the shadow with what we want to write to memory. */ memcpy (target_info->shadow_contents + bptoffset, writebuf_org + bp_addr - memaddr, bp_size); /* Determine appropriate breakpoint contents and size for this - address. */ - bp = gdbarch_breakpoint_from_pc (gdbarch, &placed_address, &placed_size); + address. Note this must be the same address + default_memory_insert_breakpoint worked with. Calling + gdbarch_breakpoint_from_pc on an already adjusted address is + not idempotent. E.g., on MIPS targets the ISA bit can be the + only place where the breakpoint type requirement is encoded, + when one sets a breakpoint by address in code that has no + symbol information. For example: + + (gdb) break *0x87654321 + + is not the same as: + + (gdb) break *0x87654320 + + and consequently if there's no symbol information available + for either of 0x87654321 or 0x87654320, then + `mips_breakpoint_from_pc' will return a microMIPS (or MIPS16, + as determined elsewhere) breakpoint for 0x87654321 and a + standard MIPS breakpoint 0x87654320. */ + address = bl->address; + bp = gdbarch_breakpoint_from_pc (bl->gdbarch, &address, &placed_size); /* Update the final write buffer with this inserted breakpoint's INSN. */ @@ -1655,7 +1674,7 @@ breakpoint_xfer_memory (gdb_byte *readbuf, gdb_byte *writebuf, continue; one_breakpoint_xfer_memory (readbuf, writebuf, writebuf_org, - memaddr, len, &bl->target_info, bl->gdbarch); + memaddr, len, bl); } }