Re: [RFA 1/2] mips: Switch inferior function calls to ON_STACK method.

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Wed, 9 May 2012, Joel Brobecker wrote:

> I have now checked this patch in.

 Thanks.  It looks like a typo has crept in in the review process and 
nobody noticed.  I have now fixed it with the change below.

Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c	2012-05-12 22:50:59.375649586 +0100
+++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c	2012-05-12 22:51:22.565626048 +0100
@@ -3009,7 +3009,7 @@ mips_frame_align (struct gdbarch *gdbarc
   return align_down (addr, 16);
 }
 
-/* Implement the "push_dummy_call" gdbarch method.  */
+/* Implement the "push_dummy_code" gdbarch method.  */
 
 static CORE_ADDR
 mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,

> > Not too long ago, Jan Kratochvil pointed out a problem with
> > AT_ENTRY_POINT.
> 
> Yeah - I thought we had fixed that by switching the x86/amd64 targets
> to using ON_STACK.  But then when I grep'ed around, I didn't find that.
> So I thought perhaps we went a different route, because I was sure we
> had checked a fix in.  But in fact, reviewing the history, I think
> that this issue is still open, probably one of these low-priority
> issues that one looks at when there is free moment.
> 
> In my opinion, the point about not being able to write the breakpoint
> if the program is executed from ROM, combined with the fact that entry
> points may have CFI info, clinches it.

 I gave it yet more thinking and came to the conclusion that at least for 
the MIPS target, where it is safe to use either way, but both have some 
drawbacks, we should really apply both, switching dynamically.  The reason 
is the stack may be unwritable for whatever reason (e.g. not correctly set 
up), so we should try ON_STACK first and if that fails (e.g. SP is NULL or 
writing to the stack has faulted), then fall back to AT_ENTRY_POINT.  
This is another corner case however and I don't feel compelled to 
implement it right now.  Let's leave it for another sunny day in 
Cambridgeshire. ;)

> OK - now that this is out of the way, which comments did we want to add?

 I've lost track, sorry, however here is the promised update I will 
include with the microMIPS change.  I used a local variable to hold the 
address of the breakpoint slot after all lest the result be horrible.  No 
regressions on mips-sde-elf or mips-linux-gnu, checked the usual set of 
standard MIPS, MIPS16 and microMIPS multilibs.

 Since this is recent code and I was changing it anyway I have reordered 
the local variables to follow the "reverse Christmas tree" style -- I find 
the definitions easier to read this way (of course initialisers may 
prevent this style to be fully applied in some cases).  The style has been 
used in the various places of the Linux kernel recently and I believe 
there is nothing said about how local variable definitions are meant to be 
ordered in the GNU Coding Style, so it should be fine to use it in GDB 
(at least in places I am concerned about).  I do not plan to specifically 
revamp existing code though.

2012-05-14  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* mips-tdep.c (mips_push_dummy_code): Handle microMIPS code.

  Maciej

gdb-micromips-on-stack.diff
Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c	2012-05-12 21:21:38.000000000 +0100
+++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c	2012-05-12 22:45:37.355619042 +0100
@@ -4198,11 +4198,18 @@ mips_push_dummy_code (struct gdbarch *gd
 		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
 		      struct regcache *regcache)
 {
-  CORE_ADDR nop_addr;
   static gdb_byte nop_insn[] = { 0, 0, 0, 0 };
+  CORE_ADDR nop_addr;
+  CORE_ADDR bp_slot;
 
   /* Reserve enough room on the stack for our breakpoint instruction.  */
-  *bp_addr = sp - sizeof (nop_insn);
+  bp_slot = sp - sizeof (nop_insn);
+
+  /* Return to microMIPS mode if calling microMIPS code to avoid
+     triggering an address error exception on processors that only
+     support microMIPS execution.  */
+  *bp_addr = (mips_pc_is_micromips (gdbarch, funaddr)
+	      ? make_compact_addr (bp_slot) : bp_slot);
 
   /* The breakpoint layer automatically adjusts the address of
      breakpoints inserted in a branch delay slot.  With enough
@@ -4211,7 +4218,7 @@ mips_push_dummy_code (struct gdbarch *gd
      trigger the adjustement, and break the function call entirely.
      So, we reserve those 4 bytes and write a nop instruction
      to prevent that from happening.  */
-  nop_addr = *bp_addr - sizeof (nop_insn);
+  nop_addr = bp_slot - sizeof (nop_insn);
   write_memory (nop_addr, nop_insn, sizeof (nop_insn));
   sp = mips_frame_align (gdbarch, nop_addr);