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

[Simon Marchi <simon.marchi@polymtl.ca>]

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