Message ID | 5429A4E3.40108@redhat.com |
---|---|
State | New, archived |
Headers |
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: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> 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 <palves@redhat.com> 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" <macro@codesourcery.com> CC: gdb-patches@sourceware.org, Luis Machado <lgustavo@codesourcery.com> Subject: Re: [PATCH] Avoid software breakpoint's instruction shadow inconsistency References: <alpine.DEB.1.10.1409140447170.27075@tp.orcam.me.uk> <5421B1B3.7010106@redhat.com> <alpine.DEB.1.10.1409231850520.27075@tp.orcam.me.uk> In-Reply-To: <alpine.DEB.1.10.1409231850520.27075@tp.orcam.me.uk> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit |
Commit Message
Pedro Alves
Sept. 29, 2014, 6:28 p.m. UTC
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 <palves@redhat.com> 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 <luisgpm@br.ibm.com> Date: Fri Feb 24 15:10:59 2012 +0000 2012-02-24 Luis Machado <lgustavo@codesourcery.com> * 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 <palves@redhat.com> Maciej W. Rozycki <macro@codesourcery.com> * 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(-)
Comments
On Mon, 29 Sep 2014, Pedro Alves wrote: > >>> 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... Exactly, if originating `bl->address' was available, it could be used by `gdbarch_breakpoint_from_pc'. > 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. Agreed, for locally created memory breakpoints it looks horrible to me too. That's not how I'd write this code if designing the feature from the beginning rather than retrofitting it. > 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. Not true (from `mips_breakpoint_from_pc'): insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status); size = status ? 2 : mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4; *pcptr = unmake_compact_addr (pc); (hmm, weird indentation here, will have to fix) -- as you can see `mips_fetch_instruction' (that reads the instruction under `pc') is called before the ISA bit is stripped as `pc' is written back to `*pcptr', and `pc' has to have the ISA bit set for the reasons I stated in the last mail. Maybe I could work it around by writing `*pcptr' back first (and still calling `mips_fetch_instruction' with the original `pc'), but that looks hackish to me; first of all there is no contract in the API between the implementation of `gdbarch_breakpoint_from_pc' and its callers that memory behind `pcptr' is the address used for breakpoint shadowing. I think the data structures used for shadowing should simply be consistent all the time. > 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. But as I noted that breaks mips_breakpoint_from_pc, you must not overwrite `placed_address' once the instruction shadow has been made. > 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. :-) I'll push it through testing, although given what I wrote above I have little hope, so it'll be just a smoke test with a microMIPS multilib and the offending test case first. Maciej
On Mon, 29 Sep 2014, Maciej W. Rozycki wrote: > > 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. > > Not true (from `mips_breakpoint_from_pc'): > > insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status); > size = status ? 2 > : mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4; > *pcptr = unmake_compact_addr (pc); > > (hmm, weird indentation here, will have to fix) -- as you can see > `mips_fetch_instruction' (that reads the instruction under `pc') is called > before the ISA bit is stripped as `pc' is written back to `*pcptr', and > `pc' has to have the ISA bit set for the reasons I stated in the last > mail. > > Maybe I could work it around by writing `*pcptr' back first (and still > calling `mips_fetch_instruction' with the original `pc'), but that looks > hackish to me; first of all there is no contract in the API between the > implementation of `gdbarch_breakpoint_from_pc' and its callers that memory > behind `pcptr' is the address used for breakpoint shadowing. I think the > data structures used for shadowing should simply be consistent all the > time. > > > 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. As a further note -- as long as symbol information is available the instruction used for the breakpoint is always a valid encoding for the ISA being used at the breakpoint location even if the ISA bit has been lost, because the ISA bit is only used to determine the encoding as the last resort. The ELF `st_other' symbol flags are a preferred way to determine the ISA and are available for the majority of failures I see because of this defect. Only cases like software watchpoint tests that single-step through possibly stripped system library code will trip on the lost ISA bit. Of course getting the ISA right does not guarantee the right size of the breakpoint instruction, which is the matter of this bug. > > 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. :-) > > I'll push it through testing, although given what I wrote above I have > little hope, so it'll be just a smoke test with a microMIPS multilib and > the offending test case first. So I smoke-tested gdb.base/break.exp that fails horribly with our current trunk and the `-EL -mmicromips' multilib and it still fails the same way with your tree: (gdb) PASS: gdb.base/break.exp: run until file:function(1) breakpoint continue Continuing. 720 Breakpoint 2, marker2 (a=43) at /scratch/macro/mips-linux/src/gdb-trunk/gdb/testsuite/gdb.base/break1.c:63 63 int marker2 (a) int a; { return (1); } /* set breakpoint 9 here */ (gdb) PASS: gdb.base/break.exp: run until quoted breakpoint continue Continuing. Program received signal SIGILL, Illegal instruction. 0x0040098d in marker2 (a=43) at /scratch/macro/mips-linux/src/gdb-trunk/gdb/testsuite/gdb.base/break1.c:63 63 int marker2 (a) int a; { return (1); } /* set breakpoint 9 here */ (gdb) FAIL: gdb.base/break.exp: run until file:linenum breakpoint break +1 Breakpoint 10 at 0x4009a3: file /scratch/macro/mips-linux/src/gdb-trunk/gdb/testsuite/gdb.base/break1.c, line 64. (gdb) FAIL: gdb.base/break.exp: breakpoint offset +1 step Program terminated with signal SIGILL, Illegal instruction. The program no longer exists. (gdb) FAIL: gdb.base/break.exp: step onto breakpoint Child terminated with signal = 0x4 (SIGILL) GDBserver exiting [...] Sorry. This test script scores "all passed" with my change. Maciej
On 09/29/2014 08:11 PM, Maciej W. Rozycki wrote: > On Mon, 29 Sep 2014, Pedro Alves wrote: >> 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. > > Not true (from `mips_breakpoint_from_pc'): > > insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status); > size = status ? 2 > : mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4; > *pcptr = unmake_compact_addr (pc); > > (hmm, weird indentation here, will have to fix) -- as you can see > `mips_fetch_instruction' (that reads the instruction under `pc') is called > before the ISA bit is stripped as `pc' is written back to `*pcptr', and > `pc' has to have the ISA bit set for the reasons I stated in the last > mail. Ah! That's the part that I was missing. I see now. > > Maybe I could work it around by writing `*pcptr' back first (and still > calling `mips_fetch_instruction' with the original `pc'), but that looks > hackish to me; first of all there is no contract in the API between the > implementation of `gdbarch_breakpoint_from_pc' and its callers that memory > behind `pcptr' is the address used for breakpoint shadowing. I think the > data structures used for shadowing should simply be consistent all the > time. Agreed. So, we could fix this by not ever trying to re-insert a memory breakpoint that has a shadow buffer. But, if we ever decide we want to record a shadow buffer for target-managed breakpoint that ends up reinserted, we'll end up with the same problem again. So might as well go with your patch. >> would be unnecessary. > > But as I noted that breaks mips_breakpoint_from_pc, you must not > overwrite `placed_address' once the instruction shadow has been made. > Indeed. >> I could be missing something else, of course. That's what I was missing... Patch is OK. Please push. Thanks, Pedro Alves
On Mon, 29 Sep 2014, Pedro Alves wrote:
> Patch is OK. Please push.
Applied now, thanks.
Maciej
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); } }