Message ID | alpine.DEB.2.00.1708011707230.29991@tp.orcam.me.uk |
---|---|
State | Superseded |
Headers |
Received: (qmail 84330 invoked by alias); 1 Aug 2017 16:36:41 -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 84321 invoked by uid 89); 1 Aug 2017 16:36:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:2074 X-HELO: mailapp01.imgtec.com Received: from mailapp01.imgtec.com (HELO mailapp01.imgtec.com) (195.59.15.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 01 Aug 2017 16:36:39 +0000 Received: from HHMAIL01.hh.imgtec.org (unknown [10.100.10.19]) by Forcepoint Email with ESMTPS id 36E939E9C98BA; Tue, 1 Aug 2017 17:36:32 +0100 (IST) Received: from [10.20.78.24] (10.20.78.24) by HHMAIL01.hh.imgtec.org (10.100.10.21) with Microsoft SMTP Server id 14.3.294.0; Tue, 1 Aug 2017 17:36:35 +0100 Date: Tue, 1 Aug 2017 17:36:53 +0100 From: "Maciej W. Rozycki" <macro@imgtec.com> To: <gdb-patches@sourceware.org> CC: Yao Qi <yao.qi@linaro.org>, Joel Brobecker <brobecker@adacore.com> Subject: [PATCH] mem-break: Fix breakpoint insertion location Message-ID: <alpine.DEB.2.00.1708011707230.29991@tp.orcam.me.uk> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" |
Commit Message
Maciej W. Rozycki
Aug. 1, 2017, 4:36 p.m. UTC
Fix a commit cd6c3b4ffc4e ("New gdbarch methods breakpoint_kind_from_pc and sw_breakpoint_from_kind") regression and restore the use of ->placed_size rather than ->reqstd_address as the location for a memory breakpoint to be inserted at. Previously `gdbarch_breakpoint_from_pc' was used that made that adjustment in `default_memory_insert_breakpoint' from the preinitialized value, however with the said commit that call is gone, so the passed ->placed_size has to be used for the initialization. The regression manifests itself as the inability to debug any MIPS/Linux compressed ISA dynamic executable as GDB corrupts the dynamic loader with one of its implicit breakpoints, causing the program to crash, as seen for example with the `mips-linux-gnu' target, o32 ABI, MIPS16 code, and the gdb.base/advance.exp test case: (gdb) continue Continuing. Program received signal SIGBUS, Bus error. _dl_debug_initialize (ldbase=0, ns=0) at dl-debug.c:51 51 r = &_r_debug; (gdb) FAIL: gdb.base/advance.exp: Can't run to main gdb/ * mem-break.c (default_memory_insert_breakpoint): Use `->placed_address' rather than `->reqstd_address' for the breakpoint location. --- Hi, No regressions between plain commit cd6c3b4ffc4e^ and commit cd6c3b4ffc4e with this change applied in `mips-linux-gnu', o32, MIPS16 testing. This brings that configuration back to sanity. OK for master and (as a grave regression) for 8.0? Maciej --- gdb/mem-break.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) gdb-mem-break-placed-address.diff
Comments
On Tue, 1 Aug 2017, Maciej W. Rozycki wrote:
> OK for master and (as a grave regression) for 8.0?
This is now PR breakpoints/21886 as per release branch requirements.
Maciej
On 2017-08-01 18:36, Maciej W. Rozycki wrote: > Fix a commit cd6c3b4ffc4e ("New gdbarch methods breakpoint_kind_from_pc > and sw_breakpoint_from_kind") regression and restore the use of > ->placed_size rather than ->reqstd_address as the location for a memory > breakpoint to be inserted at. Previously `gdbarch_breakpoint_from_pc' > was used that made that adjustment in > `default_memory_insert_breakpoint' > from the preinitialized value, however with the said commit that call > is > gone, so the passed ->placed_size has to be used for the > initialization. > > The regression manifests itself as the inability to debug any > MIPS/Linux > compressed ISA dynamic executable as GDB corrupts the dynamic loader > with one of its implicit breakpoints, causing the program to crash, as > seen for example with the `mips-linux-gnu' target, o32 ABI, MIPS16 > code, > and the gdb.base/advance.exp test case: > > (gdb) continue > Continuing. > > Program received signal SIGBUS, Bus error. > _dl_debug_initialize (ldbase=0, ns=0) at dl-debug.c:51 > 51 r = &_r_debug; > (gdb) FAIL: gdb.base/advance.exp: Can't run to main > > gdb/ > * mem-break.c (default_memory_insert_breakpoint): Use > `->placed_address' rather than `->reqstd_address' for the > breakpoint location. > --- > Hi, > > No regressions between plain commit cd6c3b4ffc4e^ and commit > cd6c3b4ffc4e > with this change applied in `mips-linux-gnu', o32, MIPS16 testing. > This > brings that configuration back to sanity. > > OK for master and (as a grave regression) for 8.0? > > Maciej > > --- > gdb/mem-break.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > gdb-mem-break-placed-address.diff > Index: binutils/gdb/mem-break.c > =================================================================== > --- binutils.orig/gdb/mem-break.c 2017-07-30 22:45:34.000000000 +0100 > +++ binutils/gdb/mem-break.c 2017-07-30 23:41:28.595612206 +0100 > @@ -37,7 +37,7 @@ int > default_memory_insert_breakpoint (struct gdbarch *gdbarch, > struct bp_target_info *bp_tgt) > { > - CORE_ADDR addr = bp_tgt->reqstd_address; > + CORE_ADDR addr = bp_tgt->placed_address; > const unsigned char *bp; > gdb_byte *readbuf; > int bplen; IIUC, we end up writing the good breakpoint kind, but at the wrong address? For example, if the requested address is 0x1001, it means that there should be a micro/compressed MIPS breakpoint at address 0x1000, but that bug caused the breakpoint to be written at address 0x1001 instead. Is that right? If so, I think the patch makes sense, I think Yao should have the final say. Thanks, Simon
"Maciej W. Rozycki" <macro@imgtec.com> writes: > Fix a commit cd6c3b4ffc4e ("New gdbarch methods breakpoint_kind_from_pc > and sw_breakpoint_from_kind") regression and restore the use of > ->placed_size rather than ->reqstd_address as the location for a memory s/placed_size/placed_address/ The patch looks good to me, but please give me two or three days to run the tests on an armv7 board. The board is being used for other tests, and I'll start the regression test on Monday next week.
On Fri, 4 Aug 2017, Simon Marchi wrote: > On 2017-08-01 18:36, Maciej W. Rozycki wrote: > > Fix a commit cd6c3b4ffc4e ("New gdbarch methods breakpoint_kind_from_pc > > and sw_breakpoint_from_kind") regression and restore the use of > > ->placed_size rather than ->reqstd_address as the location for a memory > > breakpoint to be inserted at. Previously `gdbarch_breakpoint_from_pc' > > was used that made that adjustment in `default_memory_insert_breakpoint' > > from the preinitialized value, however with the said commit that call is > > gone, so the passed ->placed_size has to be used for the initialization. [...] > IIUC, we end up writing the good breakpoint kind, but at the wrong address? > For example, if the requested address is 0x1001, it means that there should be > a micro/compressed MIPS breakpoint at address 0x1000, but that bug caused the > breakpoint to be written at address 0x1001 instead. Is that right? Exactly! Moreover, as the breakpoint is removed the original instruction bytes will be written back to 0x1000, further corrupting the executable, as `default_memory_remove_breakpoint' already correctly uses `->placed_address'. I can see now that I incorrectly wrote `->placed_size' across the patch description where I meant `->placed_address'. I'll correct that and repost the patch with PR annotation additionally included. Maciej
Index: binutils/gdb/mem-break.c =================================================================== --- binutils.orig/gdb/mem-break.c 2017-07-30 22:45:34.000000000 +0100 +++ binutils/gdb/mem-break.c 2017-07-30 23:41:28.595612206 +0100 @@ -37,7 +37,7 @@ int default_memory_insert_breakpoint (struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { - CORE_ADDR addr = bp_tgt->reqstd_address; + CORE_ADDR addr = bp_tgt->placed_address; const unsigned char *bp; gdb_byte *readbuf; int bplen;