[PATCH] mem-break: Fix breakpoint insertion location

[PATCH] mem-break: Fix breakpoint insertion location

[Maciej W. Rozycki]

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;

Re: [PATCH] mem-break: Fix breakpoint insertion location

[Maciej W. Rozycki]

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

Re: [PATCH] mem-break: Fix breakpoint insertion location

[Simon Marchi]

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

Re: [PATCH] mem-break: Fix breakpoint insertion location

[Maciej W. Rozycki]

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

Re: [PATCH] mem-break: Fix breakpoint insertion location

[Yao Qi]

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

-- 
Yao (齐尧)

[PATCH v2] PR breakpoints/21886: mem-break: Fix breakpoint insertion location

[Maciej W. Rozycki]

Fix a commit cd6c3b4ffc4e ("New gdbarch methods breakpoint_kind_from_pc 
and sw_breakpoint_from_kind") regression and restore the use of 
`->placed_address' 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_address' 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/
	PR breakpoints/21886
	* mem-break.c (default_memory_insert_breakpoint): Use 
	`->placed_address' rather than `->reqstd_address' for the 
	breakpoint location.
---
On Fri, 4 Aug 2017, Yao Qi wrote:

> "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/

 Oh, I see you've noticed it too! :)

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

 Sure.  Here's v2 with an updated description.  No changes to the patch 
itself.

  Maciej

Changes from v1:

- Description corrected, s/->placed_size/->placed_address/,

- Quotation made consistent across the description, e.g.
  `->reqstd_address' vs previous ->reqstd_address,

- PR annotation added.

---
 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;

Re: [PATCH v2] PR breakpoints/21886: mem-break: Fix breakpoint insertion location

[Yao Qi]

"Maciej W. Rozycki" <macro@imgtec.com> writes:

>> 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.
>
>  Sure.  Here's v2 with an updated description.  No changes to the patch 
> itself.

My test is done.  No regression.  Could you please push it in?

-- 
Yao (齐尧)

Re: [PATCH v2] PR breakpoints/21886: mem-break: Fix breakpoint insertion location

[Maciej W. Rozycki]

On Mon, 7 Aug 2017, Yao Qi wrote:

> >  Sure.  Here's v2 with an updated description.  No changes to the patch 
> > itself.
> 
> My test is done.  No regression.  Could you please push it in?

 Thanks for your review and testing.  I have committed it to master now.  
How about the 8.0 branch?

  Maciej

Re: [PATCH v2] PR breakpoints/21886: mem-break: Fix breakpoint insertion location

[Yao Qi]

"Maciej W. Rozycki" <macro@imgtec.com> writes:

>  Thanks for your review and testing.  I have committed it to master now.  
> How about the 8.0 branch?

Yes, please.  Are you able to edit page
https://sourceware.org/gdb/wiki/GDB_8.0_Release and add this PR in 8.0.1
release?  If you don't have the permission to edit, I can do it for you.

-- 
Yao (齐尧)

Re: [PATCH v2] PR breakpoints/21886: mem-break: Fix breakpoint insertion location

[Maciej W. Rozycki]

On Wed, 9 Aug 2017, Yao Qi wrote:

> >  Thanks for your review and testing.  I have committed it to master now.  
> > How about the 8.0 branch?
> 
> Yes, please.  Are you able to edit page
> https://sourceware.org/gdb/wiki/GDB_8.0_Release and add this PR in 8.0.1
> release?  If you don't have the permission to edit, I can do it for you.

 I have backported the change now and, after a hiccup, managed to create a 
wiki account.  However as you suspected, I seem unable to edit the page as 
it shows as immutable, at least to me.  Would you be able to do anything 
about my editing permission?  FWIW I have no issues with the sibling GLIBC 
wiki.  I'm user `macro' in both wikis.

 Thanks for your review.

  Maciej

Re: [PATCH v2] PR breakpoints/21886: mem-break: Fix breakpoint insertion location

[Yao Qi]

On 17-08-11 15:31:25, Maciej W. Rozycki wrote:
> On Wed, 9 Aug 2017, Yao Qi wrote:
> 
> > >  Thanks for your review and testing.  I have committed it to master now.  
> > > How about the 8.0 branch?
> > 
> > Yes, please.  Are you able to edit page
> > https://sourceware.org/gdb/wiki/GDB_8.0_Release and add this PR in 8.0.1
> > release?  If you don't have the permission to edit, I can do it for you.
> 
>  I have backported the change now and, after a hiccup, managed to create a 
> wiki account.  However as you suspected, I seem unable to edit the page as 
> it shows as immutable, at least to me.  Would you be able to do anything 
> about my editing permission?  FWIW I have no issues with the sibling GLIBC 
> wiki.  I'm user `macro' in both wikis.

AFAIK, users on https://sourceware.org/gdb/wiki/EditorGroup have the
permission to edit the wiki.  I add `macro' in the list.  Does it work?

-- 
Yao (齐尧)

Re: [PATCH v2] PR breakpoints/21886: mem-break: Fix breakpoint insertion location

[Maciej W. Rozycki]

On Fri, 11 Aug 2017, Yao Qi wrote:

> >  I have backported the change now and, after a hiccup, managed to create a 
> > wiki account.  However as you suspected, I seem unable to edit the page as 
> > it shows as immutable, at least to me.  Would you be able to do anything 
> > about my editing permission?  FWIW I have no issues with the sibling GLIBC 
> > wiki.  I'm user `macro' in both wikis.
> 
> AFAIK, users on https://sourceware.org/gdb/wiki/EditorGroup have the
> permission to edit the wiki.  I add `macro' in the list.  Does it work?

 It does, thank you!  Page updated.

 BTW, PR 21555 still has to be closed it would seem.  I'm not sure if it's 
bugzilla or my browser (or myself), but I had troubles closing PR 21886 as 
well, as the intended status flipped from RESOLVED to WAITING as I was 
typing in the comment.

  Maciej