Getting rid of AT_SYMBOL inferior call method

Getting rid of AT_SYMBOL inferior call method

[Joel Brobecker]

Hello,

This is something I promised I'd help with someday on a rainy day:
http://www.sourceware.org/ml/gdb-patches/2011-12/msg00896.html

There are two patches:
  . Patch #1 transitions mips targets to using ON_STACK instead of AT_SYMBOL;
  . Patch #2 removes AT_SYMBOL and AT_SYMBOL support.

The patch series was tested on mips-irix, no regression.

[commit 2/2] Remove AT_SYMBOL

[Joel Brobecker]

Now that this method is no longer used by any architecture,
we can remove its support.

gdb/ChangeLog:

        * infcall.c (call_function_by_hand): Remove AT_SYMBOL handling.
        * inferior.h (AT_SYMBOL): Delete.

Will commit as soon as the mips-tdep patch gets checked in.

---
 gdb/infcall.c  |   27 ---------------------------
 gdb/inferior.h |    1 -
 2 files changed, 0 insertions(+), 28 deletions(-)

diff --git a/gdb/infcall.c b/gdb/infcall.c
index 6c250e3..e0195c9 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -640,33 +640,6 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 	bp_addr = dummy_addr;
 	break;
       }
-    case AT_SYMBOL:
-      /* Some executables define a symbol __CALL_DUMMY_ADDRESS whose
-	 address is the location where the breakpoint should be
-	 placed.  Once all targets are using the overhauled frame code
-	 this can be deleted - ON_STACK is a better option.  */
-      {
-	struct minimal_symbol *sym;
-	CORE_ADDR dummy_addr;
-
-	sym = lookup_minimal_symbol ("__CALL_DUMMY_ADDRESS", NULL, NULL);
-	real_pc = funaddr;
-	if (sym)
-	  {
-	    dummy_addr = SYMBOL_VALUE_ADDRESS (sym);
-	    /* Make certain that the address points at real code, and not
-	       a function descriptor.  */
-	    dummy_addr = gdbarch_convert_from_func_ptr_addr (gdbarch,
-							     dummy_addr,
-							     &current_target);
-	  }
-	else
-	  dummy_addr = entry_point_address ();
-	/* A call dummy always consists of just a single breakpoint,
-	   so it's address is the same as the address of the dummy.  */
-	bp_addr = dummy_addr;
-	break;
-      }
     default:
       internal_error (__FILE__, __LINE__, _("bad switch"));
     }
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 63245a2..8c90f96 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -357,7 +357,6 @@ struct displaced_step_closure *get_displaced_step_closure_by_addr (CORE_ADDR add
 /* Possible values for gdbarch_call_dummy_location.  */
 #define ON_STACK 1
 #define AT_ENTRY_POINT 4
-#define AT_SYMBOL 5
 
 /* If STARTUP_WITH_SHELL is set, GDB's "run"
    will attempts to start up the debugee under a shell.
-- 
1.7.0.4

Re: [commit 2/2] Remove AT_SYMBOL

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * infcall.c (call_function_by_hand): Remove AT_SYMBOL handling.
>         * inferior.h (AT_SYMBOL): Delete.

This patch is now in as well.

-- 
Joel

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

[Joel Brobecker]

This patch switches the mips code to use the ON_STACK method
for function calls instead of AT_SYMBOL, which we want to remove.

The one difficulty came from the fact that we needed to make sure
that the area on the stack just before where we insert the breakpoint
instruction does not look like a branch instruction.  Otherwise,
we get an automatic breakpoint adjustment which breaks everything.

Another little detail on the implementation of mips_push_dummy_code.
It starts by aligning the stack.  AFAIK, the stack is supposed to
always be aligned to at least 4 bytes (4 bytes for mips32, 8 bytes
for mips64). So, the initial alignment shouldn't be necessary, since
that's good enough aligment for our breakpoint instruction.  But
in the end, I chose to keep it, JIC. We could possibly change the
code to align to 4 instead of 16 like mips_frame_align does, if
we want to.

gdb/ChangeLog:

        * mips-tdep.c (mips_push_dummy_code): New function.
        (mips_gdbarch_init): Set the gdbarch call_dummy_location to
        ON_STACK and install mips_push_dummy_code as our gdbarch
        push_dummy_code routine.

Tested on mips-irix.  It might be nice to test on other mips targets
as well, although it should not be necessary. OK to commit?

Thanks,
-- 
Joel

---
 gdb/mips-tdep.c |   36 ++++++++++++++++++++++++++++++++----
 1 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 9a3c7fb..3e6b00b 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -3009,6 +3009,36 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
   return align_down (addr, 16);
 }
 
+/* Implement the push_dummy_code gdbarch method for mips targets.  */
+
+static CORE_ADDR
+mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
+		      CORE_ADDR funaddr, struct value **args,
+		      int nargs, struct type *value_type,
+		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
+		      struct regcache *regcache)
+{
+  int bp_len;
+  gdb_byte null_insn[4] = {0};
+
+  *bp_addr = mips_frame_align (gdbarch, sp);
+  gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);
+
+  /* The breakpoint layer automatically adjusts the address of
+     breakpoints inserted in a branch delay slot.  With enough
+     bad luck, the 4 bytes located just before our breakpoint
+     instruction could look like a branch instruction, and thus
+     trigger the adjustement, and break the function call entirely.
+     So, we reserve those 4 bytes and write a null instruction
+     to prevent that from happening.  */
+  write_memory (*bp_addr - bp_len, null_insn, sizeof (null_insn));
+  sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);
+
+  /* Inferior resumes at the function entry point.  */
+  *real_pc = funaddr;
+
+  return sp;
+}
 static CORE_ADDR
 mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 			   struct regcache *regcache, CORE_ADDR bp_addr,
@@ -6906,10 +6936,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   /* MIPS version of CALL_DUMMY.  */
 
-  /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
-     replaced by a command, and all targets will default to on stack
-     (regardless of the stack's execute status).  */
-  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
   set_gdbarch_frame_align (gdbarch, mips_frame_align);
 
   set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);
-- 
1.7.0.4

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

[Maciej W. Rozycki]

Joel,

> This patch switches the mips code to use the ON_STACK method
> for function calls instead of AT_SYMBOL, which we want to remove.

 Thanks for this work -- can you give me a reference to some background 
information as to why exactly we want to remove the AT_SYMBOL method?

> Another little detail on the implementation of mips_push_dummy_code.
> It starts by aligning the stack.  AFAIK, the stack is supposed to
> always be aligned to at least 4 bytes (4 bytes for mips32, 8 bytes
> for mips64). So, the initial alignment shouldn't be necessary, since
> that's good enough aligment for our breakpoint instruction.  But
> in the end, I chose to keep it, JIC. We could possibly change the
> code to align to 4 instead of 16 like mips_frame_align does, if
> we want to.

 For the record: the respective ABIs mandate that the stack is aligned to 
8 bytes for 32-bit targets and to 16 bytes for 64-bit targets.  However 
the user may have fiddled with SP, so I think it's better to stay safe 
and therefore I agree it's better if we prealign the stack and avoid 
crashing the debuggee in this context.

> gdb/ChangeLog:
> 
>         * mips-tdep.c (mips_push_dummy_code): New function.
>         (mips_gdbarch_init): Set the gdbarch call_dummy_location to
>         ON_STACK and install mips_push_dummy_code as our gdbarch
>         push_dummy_code routine.
> 
> Tested on mips-irix.  It might be nice to test on other mips targets
> as well, although it should not be necessary. OK to commit?

 There's one fundamental problem with this change -- software breakpoints 
must not be used unconditionally, because some configurations do not 
expect or support them.  This affects remote JTAG targets and simulators, 
appropriate `Z0' packets should be used -- this is because JTAG uses SDBBP 
instructions rather than BREAK instructions -- the latters are ordinary 
instructions as far as JTAG debugging is considered and do not trap into 
the debug mode (this is so that you can debug an OS that uses BREAK 
instructions for user debugging); simulators may use yet other means, 
typically they don't use memory breakpoints at all (again, to allow users 
to debug their software within the simulator, e.g. the simulator runs 
Linux and the user debugs some userland app within that instance of 
Linux).

> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 9a3c7fb..3e6b00b 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -3009,6 +3009,36 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>    return align_down (addr, 16);
>  }
>  
> +/* Implement the push_dummy_code gdbarch method for mips targets.  */
> +
> +static CORE_ADDR
> +mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
> +		      CORE_ADDR funaddr, struct value **args,
> +		      int nargs, struct type *value_type,
> +		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
> +		      struct regcache *regcache)
> +{
> +  int bp_len;
> +  gdb_byte null_insn[4] = {0};
> +
> +  *bp_addr = mips_frame_align (gdbarch, sp);
> +  gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);
> +
> +  /* The breakpoint layer automatically adjusts the address of
> +     breakpoints inserted in a branch delay slot.  With enough
> +     bad luck, the 4 bytes located just before our breakpoint
> +     instruction could look like a branch instruction, and thus
> +     trigger the adjustement, and break the function call entirely.
> +     So, we reserve those 4 bytes and write a null instruction
> +     to prevent that from happening.  */
> +  write_memory (*bp_addr - bp_len, null_insn, sizeof (null_insn));
> +  sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);
> +
> +  /* Inferior resumes at the function entry point.  */
> +  *real_pc = funaddr;
> +
> +  return sp;
> +}
>  static CORE_ADDR
>  mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>  			   struct regcache *regcache, CORE_ADDR bp_addr,

 So essentially gdbarch_breakpoint_from_pc may only be used on targets 
known to use software breakpoints.

 Additionally once microMIPS support is in the above must execute as 
microMIPS code where appropriate to support processors that do not have 
the standard MIPS mode implemented -- no such processor currently exists, 
but the architecture spec explicitly permits pure-microMIPS 
implementations and some are expected to happen sooner or later.

 IOW the callee must return to an address that has the ISA bit set 
(that'll be the address of the breakpoint above ORed with 1) or otherwise 
SIGBUS may be thrown (hardware signals an Address Error exception if the 
ISA bit is clear).  Is that going to work with an update to the piece 
above like:

[...]
  if (APPROPRIATE)
    sp = make_compact_addr (sp);
  return sp;
}

(plus a corresponding update to the breakpoint address) or is more surgery 
required, especially to any generic code?  I guess not as I gather the 
return value will not only be used as the return address, but as the stack 
pointer as well, right?

 For simplicity I think:

#define APPROPRIATE is_micromips_addr (gdbarch, funaddr)

will do.

 Finally, I'd be happier if the unpredictability of the microMIPS 
breakpoint kind selection was avoided (there are two software and JTAG 
breakpoints each, BREAK16/BREAK32 and SDBBP16/SDBBP32, used by GDB 
like-for-like with 16-bit and 32-bit microMIPS instructions being 
replaced), so I suggest that the stack frame is preallocated and 
preinitialised along the lines of:

{
  gdb_byte *null_insn;
  CORE_ADDR new_sp;

  sp = mips_frame_align (gdbarch, sp);
  new_sp = mips_frame_align (gdbarch, sp - 2 * MIPS_INSN32_SIZE);
  null_insn = alloca (sp - new_sp);
  memset (null_insn, 0, sp - new_sp);
  write_memory (new_sp, null_insn, sp - new_sp);
[...]

-- that applies to remote targets that use `Z0' too as the expected 
breakpoint length will be encoded by GDB as appropriate in the request.

 I can address all these microMIPS issues if you like as I'm likely more 
familiar with that stuff, but please find a solution to the software 
breakpoint problem.

> @@ -6906,10 +6936,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  
>    /* MIPS version of CALL_DUMMY.  */
>  
> -  /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
> -     replaced by a command, and all targets will default to on stack
> -     (regardless of the stack's execute status).  */
> -  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
> +  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
> +  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
>    set_gdbarch_frame_align (gdbarch, mips_frame_align);
>  
>    set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);

 So what if the stack pages are indeed not executable (their page entries 
have the XI aka Execute Inhibit bit set)?

  Maciej

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

[Joel Brobecker]

Thanks for the detailed feedback :)

>  Thanks for this work -- can you give me a reference to some background 
> information as to why exactly we want to remove the AT_SYMBOL method?

I do not have a specific reference. Only the fact that it's always
been a method that we wanted to deprecate and then remove support for...

>  For the record: the respective ABIs mandate that the stack is aligned
>  to 8 bytes for 32-bit targets and to 16 bytes for 64-bit targets.

Ah, that explains why mips_frame_align aligned to 16 bytes boundaries...
I googled that question instead of looking that information up in my
manuals - my bad.

> There's one fundamental problem with this change -- software breakpoints 
> must not be used unconditionally, because some configurations do not 
> expect or support them.

I think that we will be fine, because the code is not actually inserting
the breakpoint, merely reserving the room for it on the stack. Later
on, the target layer then inserts the breakpoint for us, using the
appropriate method. If you're connected to a board via JTAG, it should
do the right thing.

>  I can address all these microMIPS issues if you like as I'm likely
>  more familiar with that stuff,

I'd feel more comfortable if you did. I think I understand a little
better what you mean by microMIPS, although I thought it was what
the code called mips16. But I realize now that it's something
different, because IIRC you can switch between mips16 and mips32
by simply using odd addresses (or not).

> > @@ -6906,10 +6936,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> >  
> >    /* MIPS version of CALL_DUMMY.  */
> >  
> > -  /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
> > -     replaced by a command, and all targets will default to on stack
> > -     (regardless of the stack's execute status).  */
> > -  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
> > +  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
> > +  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
> >    set_gdbarch_frame_align (gdbarch, mips_frame_align);
> >  
> >    set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);
> 
>  So what if the stack pages are indeed not executable (their page entries 
> have the XI aka Execute Inhibit bit set)?

The only time when we are executing an instruction on the stack is
when the function being called returns and hits the breakpoint.
IIRC, there has been some testing done against this situation,
and things just worked. I suspect that we get a trap, and that trap
correctly gets interpreted. I can see ourselves receiving a signal
instead, but I think this can also be correctly interpreted. Maybe
we'll have to make some adjustement to GDB's core, but I'd rather
fix that if I have a way to reproduce and therefore test...

Cheers,
-- 
Joel

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

[Maciej W. Rozycki]

Joel

> >  Thanks for this work -- can you give me a reference to some background 
> > information as to why exactly we want to remove the AT_SYMBOL method?
> 
> I do not have a specific reference. Only the fact that it's always
> been a method that we wanted to deprecate and then remove support for...

 OK, fair enough.  I'll see if I can find anything relevant myself to feed 
my curiosity.

> I think that we will be fine, because the code is not actually inserting
> the breakpoint, merely reserving the room for it on the stack. Later
> on, the target layer then inserts the breakpoint for us, using the
> appropriate method. If you're connected to a board via JTAG, it should
> do the right thing.

 Indeed, I realised that shortly after posting my reply.  I got tricked 
into thinking this is needed for a reason, but I think it's really not.  
I think it is safe to assume a MIPS software breakpoint will always fit in 
a single machine instruction, so it should be enough to reserve space for 
two 32-bit instructions just as in the snippet I proposed -- one for the 
place to put the breakpoint at and the other for the delay slot avoidance.

> >  I can address all these microMIPS issues if you like as I'm likely
> >  more familiar with that stuff,
> 
> I'd feel more comfortable if you did. I think I understand a little
> better what you mean by microMIPS, although I thought it was what
> the code called mips16. But I realize now that it's something
> different, because IIRC you can switch between mips16 and mips32
> by simply using odd addresses (or not).

 Well, the microMIPS instruction set is a new encoding of all the existing 
MIPS instructions, with some new instructions added.  It is intended to be 
source-compatible with the existing MIPS software, up to handwritten 
assembly (in the macro mode).

 Overall there are four categories of instructions in the microMIPS set:

* newly-added 16-bit instructions,

* newly-added 32-bit instructions,

* reencoded 32-bit instructions,

* newly-added 48-bit instructions.

 Most of 16-bit instructions are shortened forms of frequently used 
instructions that already existed in the original MIPS instruction set.  
Some are genuinely new operations (e.g. MOVEP, move a pair of registers).  
Also most of 16-bit instructions have equivalent 32-bit encodings that 
have a wider range of their immediate argument and/or can address more 
registers.

 There isn't much to say about newly-added 32-bit instructions -- in most 
cases they are widened forms of some 16-bit instructions, though again, 
there are some unique exception (ADDIUPC, add immediate to PC, comes to my 
mind).

 All the reencoded 32-bit instructions already existed in the original 
MIPS instruction set.  Some particularily infrequently used have the range 
of their immediate argument narrowed compared to the original (e.g. DADDI, 
double-word add with overflow detection).  The use of an auxiliary 
register is required to use an immediate outside the range as is with 
values outside the signed 16-bit range in the original MIPS instruction 
set.  GAS handles this peculiarity transparently in the macro mode.

 One 48-bit instruction has been defined, LI32, to load a 32-bit immediate 
into a register with a single instruction.  It's a part of the 64-bit 
architecture spec, you won't get it on 32-bit processors.

 We don't have to care about MIPS16 support here because any processor 
that supports the MIPS16 instruction set will also support the standard 
MIPS set.  This is not going to be true for microMIPS support.

 And for the record, the ISA bit is used for microMIPS code just as it is 
for MIPS16 execution, by using odd addresses you switch the processor into 
"the other" mode, whichever of MIPS16 or microMIPS one it supports.  
However for pure-microMIPS processors the ISA bit is hardwired to 1 -- all 
code addresses are always odd.

 Coincidentally all-zeroes is a 32-bit NOP instruction both in the 
standard MIPS and the microMIPS mode -- there's a 16-bit encoding of NOP 
in the microMIPS mode naturally as well.

> > > @@ -6906,10 +6936,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> > >  
> > >    /* MIPS version of CALL_DUMMY.  */
> > >  
> > > -  /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
> > > -     replaced by a command, and all targets will default to on stack
> > > -     (regardless of the stack's execute status).  */
> > > -  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
> > > +  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
> > > +  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
> > >    set_gdbarch_frame_align (gdbarch, mips_frame_align);
> > >  
> > >    set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);
> > 
> >  So what if the stack pages are indeed not executable (their page entries 
> > have the XI aka Execute Inhibit bit set)?
> 
> The only time when we are executing an instruction on the stack is
> when the function being called returns and hits the breakpoint.
> IIRC, there has been some testing done against this situation,
> and things just worked. I suspect that we get a trap, and that trap
> correctly gets interpreted. I can see ourselves receiving a signal
> instead, but I think this can also be correctly interpreted. Maybe
> we'll have to make some adjustement to GDB's core, but I'd rather
> fix that if I have a way to reproduce and therefore test...

 Understood, but I'd be happier if the comment you're removing or a 
similar stayed in place.  If by trap you mean SIGTRAP, then I think this 
is not going to be the case.

 For the record: if the XI bit is set and execution is attempted, then the 
Execute-Inhibit exception is triggered.  Depending on how the processor 
has been configured, this will map either to the TLBL exception, just as 
with any TLB fault on an instruction fetch, or a newly-defined exception 
(see below).  This will probably be translated to SIGSEGV by any OS; any 
bare-iron application may not be as flexible, but certainly will handle it 
somehow if it set XI in the first place.

 The Execute-Inhibit exception certainly has a precedence over software 
breakpoints (both BREAK and SDBBP) because the respective instruction has 
to execute for the breakpoint exception to trigger.  However hardware 
instruction breakpoints, both EJTAG and CP0 Watch register ones, do take 
precedence over Execute-Inhibit.

 The XI feature itself is a moderately new addition to the MIPS 
architecture, originally introduced with the SmartMIPS ASE (a security 
enhancement intended for smart cards, I'm told) and with revision #3 
merged into the base architecture.  There is a corresponding RI feature, 
intended to read-protect pages holding code.

 The Execute-Inhibit and Read-Inhibit exceptions are new codes (20 and 19, 
respectively) added to the CP0 Cause register with the merge of the 
feature into the base architecture (they were not defined for the original 
SmartMIPS ASE), so there is little if any existing practice as to how they 
are handled by OSes, my SIGSEGV assumption above is how I would handle it 
myself.  Upstream Linux kernel does not handle the new codes for once, but 
does handle the XI bit itself in some configurations.  I don't think I 
have access to any hardware I could test it with though.

 I know that some architectures have supported such fine-grained access to 
virtual memory for quite a while now, so I would have assumed we have got 
it sorted out already somehow in a generic way.

  Maciej

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

Now I know why I was told you are a MIPS expert :-). I never really
had the chance or need to delve into the details of any specific
architecture. Even for the ia64-hpux port, I could do with just
a superficial knowledge of that CPU.

> * newly-added 48-bit instructions.

I am wondering if this addition is going to hurt in terms of our
support...  From what I could tell from my mips64 manual, even
on this CPU the instructions are still 32bit long... But I'm
digressing, sorry.

>  Coincidentally all-zeroes is a 32-bit NOP instruction both in the 
> standard MIPS and the microMIPS mode -- there's a 16-bit encoding of NOP 
> in the microMIPS mode naturally as well.

I'm wondering if you'd like me to rename "null_insn" into "nop_insn"
in my patch. I didn't do it, because I'd expect the instruction size
to depend on the mode. As of today, we know that the breakpoint we
are inserting is always going to be at an even address, so it's always
going to be 4 bytes. So maybe it does make sense to rename it. Let
me know.

>  Understood, but I'd be happier if the comment you're removing or a 
> similar stayed in place.  If by trap you mean SIGTRAP, then I think this 
> is not going to be the case.

I think you refer to the comment from Andrew Cagney? I've put it back
as is.

OK to commit, modulo the possible rename above?

Thanks,
-- 
Joel

[-- Attachment #2: mips-on-stack-v3.diff --]
[-- Type: text/x-diff, Size: 2744 bytes --]

commit b78a75e1442a349531a017036a02f43c4df71427
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Wed May 2 20:39:57 2012 -0400

    mips: Switch inferior function calls to ON_STACK method.
    
    This patch switches the mips code to use the ON_STACK method
    for function calls instead of AT_SYMBOL, which we want to remove.
    
    gdb/ChangeLog:
    
            * mips-tdep.c (mips_push_dummy_code): New function.
            (mips_gdbarch_init): Set the gdbarch call_dummy_location to
            ON_STACK and install mips_push_dummy_code as our gdbarch
            push_dummy_code routine.

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 9a3c7fb..5e9a6ed 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -3009,6 +3009,37 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
   return align_down (addr, 16);
 }
 
+/* Implement the push_dummy_code gdbarch method for mips targets.  */
+
+static CORE_ADDR
+mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
+		      CORE_ADDR funaddr, struct value **args,
+		      int nargs, struct type *value_type,
+		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
+		      struct regcache *regcache)
+{
+  int bp_len;
+  gdb_byte null_insn[4] = { 0 };
+
+  *bp_addr = mips_frame_align (gdbarch, sp);
+  gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);
+
+  /* The breakpoint layer automatically adjusts the address of
+     breakpoints inserted in a branch delay slot.  With enough
+     bad luck, the 4 bytes located just before our breakpoint
+     instruction could look like a branch instruction, and thus
+     trigger the adjustement, and break the function call entirely.
+     So, we reserve those 4 bytes and write a null instruction
+     to prevent that from happening.  */
+  write_memory (*bp_addr - bp_len, null_insn, sizeof (null_insn));
+  sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);
+
+  /* Inferior resumes at the function entry point.  */
+  *real_pc = funaddr;
+
+  return sp;
+}
+
 static CORE_ADDR
 mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 			   struct regcache *regcache, CORE_ADDR bp_addr,
@@ -6909,7 +6940,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
      replaced by a command, and all targets will default to on stack
      (regardless of the stack's execute status).  */
-  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
   set_gdbarch_frame_align (gdbarch, mips_frame_align);
 
   set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);

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

[Mark Kettenis]

> Date: Fri, 4 May 2012 13:58:18 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> >  Understood, but I'd be happier if the comment you're removing or a 
> > similar stayed in place.  If by trap you mean SIGTRAP, then I think this 
> > is not going to be the case.
> 
> I think you refer to the comment from Andrew Cagney? I've put it back
> as is.

I really don't think that comment is helpful anymore.  Almost ten
years have gone by without anyone feeling the need to implement the
command Andrew is alluding to here,

> OK to commit, modulo the possible rename above?

Seems you missed my comment about the the mips_frame_align() call.
It's unecessary since the frame is already properly aligned by the
calles of push_dummy_code().

> commit b78a75e1442a349531a017036a02f43c4df71427
> Author: Joel Brobecker <brobecker@adacore.com>
> Date:   Wed May 2 20:39:57 2012 -0400
> 
>     mips: Switch inferior function calls to ON_STACK method.
>     
>     This patch switches the mips code to use the ON_STACK method
>     for function calls instead of AT_SYMBOL, which we want to remove.
>     
>     gdb/ChangeLog:
>     
>             * mips-tdep.c (mips_push_dummy_code): New function.
>             (mips_gdbarch_init): Set the gdbarch call_dummy_location to
>             ON_STACK and install mips_push_dummy_code as our gdbarch
>             push_dummy_code routine.
> 
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 9a3c7fb..5e9a6ed 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -3009,6 +3009,37 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>    return align_down (addr, 16);
>  }
>  
> +/* Implement the push_dummy_code gdbarch method for mips targets.  */

Perhaps change that comment into:

  /* Implement the "push_dummy_call" gdbarch method.  */

such that it is consistent with the style of comments in rl78-tdep.c
and rx-tdep.c and moxie-tdep.c?  The "for mips targets" isn't really
adding any information (and might end up accidentally being copied).

> +
> +static CORE_ADDR
> +mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
> +		      CORE_ADDR funaddr, struct value **args,
> +		      int nargs, struct type *value_type,
> +		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
> +		      struct regcache *regcache)
> +{
> +  int bp_len;
> +  gdb_byte null_insn[4] = { 0 };
> +
> +  *bp_addr = mips_frame_align (gdbarch, sp);
> +  gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);
> +
> +  /* The breakpoint layer automatically adjusts the address of
> +     breakpoints inserted in a branch delay slot.  With enough
> +     bad luck, the 4 bytes located just before our breakpoint
> +     instruction could look like a branch instruction, and thus
> +     trigger the adjustement, and break the function call entirely.
> +     So, we reserve those 4 bytes and write a null instruction
> +     to prevent that from happening.  */
> +  write_memory (*bp_addr - bp_len, null_insn, sizeof (null_insn));
> +  sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);
> +
> +  /* Inferior resumes at the function entry point.  */
> +  *real_pc = funaddr;
> +
> +  return sp;
> +}
> +
>  static CORE_ADDR
>  mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>  			   struct regcache *regcache, CORE_ADDR bp_addr,
> @@ -6909,7 +6940,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
>       replaced by a command, and all targets will default to on stack
>       (regardless of the stack's execute status).  */
> -  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
> +  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
> +  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
>    set_gdbarch_frame_align (gdbarch, mips_frame_align);
>  
>    set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);
> 
> --fXStkuK2IQBfcDe+--
> 

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

[Maciej W. Rozycki]

On Fri, 4 May 2012, Mark Kettenis wrote:

> > >  Understood, but I'd be happier if the comment you're removing or a 
> > > similar stayed in place.  If by trap you mean SIGTRAP, then I think this 
> > > is not going to be the case.
> > 
> > I think you refer to the comment from Andrew Cagney? I've put it back
> > as is.
> 
> I really don't think that comment is helpful anymore.  Almost ten
> years have gone by without anyone feeling the need to implement the
> command Andrew is alluding to here,

 I referred to the non-executable stack behaviour, not the command wished 
for, sorry if I worded that ambiguously.

> > OK to commit, modulo the possible rename above?
> 
> Seems you missed my comment about the the mips_frame_align() call.
> It's unecessary since the frame is already properly aligned by the
> calles of push_dummy_code().

 Indeed.

> > +/* Implement the push_dummy_code gdbarch method for mips targets.  */
> 
> Perhaps change that comment into:
> 
>   /* Implement the "push_dummy_call" gdbarch method.  */
> 
> such that it is consistent with the style of comments in rl78-tdep.c
> and rx-tdep.c and moxie-tdep.c?  The "for mips targets" isn't really
> adding any information (and might end up accidentally being copied).

 As per my suggestion I think it makes sense to document any peculiarities 
of this specific implementation here as well (in this case the safety to 
use with non-executable stack).

  Maciej

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

[Mark Kettenis]

> Date: Sat, 5 May 2012 00:25:04 +0100
> From: "Maciej W. Rozycki" <macro@codesourcery.com>
>
> > > +/* Implement the push_dummy_code gdbarch method for mips targets.  */
> > 
> > Perhaps change that comment into:
> > 
> >   /* Implement the "push_dummy_call" gdbarch method.  */
> > 
> > such that it is consistent with the style of comments in rl78-tdep.c
> > and rx-tdep.c and moxie-tdep.c?  The "for mips targets" isn't really
> > adding any information (and might end up accidentally being copied).
> 
>  As per my suggestion I think it makes sense to document any peculiarities 
> of this specific implementation here as well (in this case the safety to 
> use with non-executable stack).

The non-executable stack issue really isn't specific to this
implementation though.  On OpenBSD the stack is non-executable on all
architectures where it is possible (including through a clever trick
on i386).  I haven't tried ON_STACK on all of them, but it works on
all of them.

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

[Maciej W. Rozycki]

On Sat, 5 May 2012, Mark Kettenis wrote:

> >  As per my suggestion I think it makes sense to document any peculiarities 
> > of this specific implementation here as well (in this case the safety to 
> > use with non-executable stack).
> 
> The non-executable stack issue really isn't specific to this
> implementation though.  On OpenBSD the stack is non-executable on all
> architectures where it is possible (including through a clever trick
> on i386).  I haven't tried ON_STACK on all of them, but it works on
> all of them.

 After some thinking I realised that the reliance on signal delivery to 
work properly to trap non-executable stack may actually be a problem for 
bare-iron targets.  I'd expect TLB exceptions not to be forwarded up the 
debug stack in a typical JTAG debugging scenario -- it's not obvious how 
they should be mapped to signals even, the low-level hardware has no way 
to classify them and not all have to be fatal; in about the most 
sophisticated scenario (which is not unlikely, I did that many times 
myself) consider debugging Linux (the kernel) through JTAG.

 Instead you'll have the joy of running or stepping through the exception 
handler (from the probe's point of view it's really no different to an 
ordinary jump; in the hardware stepping mode the processor will just trap 
at the exception handler's entry point instead of the intended breakpoint 
location) on the target and your device being debugged may go astray, e.g. 
panic blinking LEDs or trigger hardware reset.

 Given that this feature in the MIPS case comes from the SmartMIPS ASE 
(that as noted earlier has been designed with smart cards in mind) I think 
such a scenario is actually not unlikely, even though, as I say, I have 
never had an opportunity to deal with a MIPS processor that had this 
non-executable stack feature implemented.

  Maciej

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 782 bytes --]

> After some thinking I realised that the reliance on signal delivery to 
> work properly to trap non-executable stack may actually be a problem for 
> bare-iron targets.

IMO, I think we can start worrying about those when we actually
encounter the problem; and I am assuming that this is not going to
be specific to mips.

Are we still good to go with this patch? Attached is the latest
version. For any additional comments that you'd like to be added
(in particular, with respect to what you just pointed out), I suggest
add them as a followup patch.  This way, this one is out of the way,
and we can just focus on the comments themselves.

I am also adding a second patch, which shows the changes I made
in this iteration.

Tested on mips-irix, no regression.

Thanks,
-- 
Joel

[-- Attachment #2: 0001-mips-Switch-inferior-function-calls-to-ON_STACK-meth.patch --]
[-- Type: text/x-diff, Size: 2877 bytes --]

From ddb25412f122ca8180238f188536b3027182cb31 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Wed, 2 May 2012 20:39:57 -0400
Subject: [PATCH] mips: Switch inferior function calls to ON_STACK method.

This patch switches the mips code to use the ON_STACK method
for function calls instead of AT_SYMBOL, which we want to remove.

gdb/ChangeLog:

        * mips-tdep.c (mips_push_dummy_code): New function.
        (mips_gdbarch_init): Set the gdbarch call_dummy_location to
        ON_STACK and install mips_push_dummy_code as our gdbarch
        push_dummy_code routine.
---
 gdb/mips-tdep.c |   37 +++++++++++++++++++++++++++++++++----
 1 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 9a3c7fb..68ac858 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -3009,6 +3009,37 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
   return align_down (addr, 16);
 }
 
+/* Implement the "push_dummy_call" gdbarch method.  */
+
+static CORE_ADDR
+mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
+		      CORE_ADDR funaddr, struct value **args,
+		      int nargs, struct type *value_type,
+		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
+		      struct regcache *regcache)
+{
+  int bp_len;
+  static gdb_byte nop_insn[] = { 0, 0, 0, 0 };
+
+  *bp_addr = sp;
+  gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);
+
+  /* The breakpoint layer automatically adjusts the address of
+     breakpoints inserted in a branch delay slot.  With enough
+     bad luck, the 4 bytes located just before our breakpoint
+     instruction could look like a branch instruction, and thus
+     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.  */
+  write_memory (*bp_addr - bp_len, nop_insn, sizeof (nop_insn));
+  sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);
+
+  /* Inferior resumes at the function entry point.  */
+  *real_pc = funaddr;
+
+  return sp;
+}
+
 static CORE_ADDR
 mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 			   struct regcache *regcache, CORE_ADDR bp_addr,
@@ -6906,10 +6937,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   /* MIPS version of CALL_DUMMY.  */
 
-  /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
-     replaced by a command, and all targets will default to on stack
-     (regardless of the stack's execute status).  */
-  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
   set_gdbarch_frame_align (gdbarch, mips_frame_align);
 
   set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);
-- 
1.7.0.4


[-- Attachment #3: 0001-mips-tdeps-ON_STACK-push_dummy_call-adjustements.patch --]
[-- Type: text/x-diff, Size: 2394 bytes --]

From 758b0c5b1dfab824516badb3f8b238b40905732f Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Mon, 7 May 2012 18:54:27 -0400
Subject: [PATCH] mips-tdeps ON_STACK push_dummy_call adjustements.

---
 gdb/mips-tdep.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 5e9a6ed..68ac858 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -3009,7 +3009,7 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
   return align_down (addr, 16);
 }
 
-/* Implement the push_dummy_code gdbarch method for mips targets.  */
+/* Implement the "push_dummy_call" gdbarch method.  */
 
 static CORE_ADDR
 mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
@@ -3019,9 +3019,9 @@ mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
 		      struct regcache *regcache)
 {
   int bp_len;
-  gdb_byte null_insn[4] = { 0 };
+  static gdb_byte nop_insn[] = { 0, 0, 0, 0 };
 
-  *bp_addr = mips_frame_align (gdbarch, sp);
+  *bp_addr = sp;
   gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);
 
   /* The breakpoint layer automatically adjusts the address of
@@ -3029,9 +3029,9 @@ mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
      bad luck, the 4 bytes located just before our breakpoint
      instruction could look like a branch instruction, and thus
      trigger the adjustement, and break the function call entirely.
-     So, we reserve those 4 bytes and write a null instruction
+     So, we reserve those 4 bytes and write a nop instruction
      to prevent that from happening.  */
-  write_memory (*bp_addr - bp_len, null_insn, sizeof (null_insn));
+  write_memory (*bp_addr - bp_len, nop_insn, sizeof (nop_insn));
   sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);
 
   /* Inferior resumes at the function entry point.  */
@@ -6937,9 +6937,6 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   /* MIPS version of CALL_DUMMY.  */
 
-  /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
-     replaced by a command, and all targets will default to on stack
-     (regardless of the stack's execute status).  */
   set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
   set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
   set_gdbarch_frame_align (gdbarch, mips_frame_align);
-- 
1.7.0.4

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

[Maciej W. Rozycki]

On Tue, 8 May 2012, Joel Brobecker wrote:

> > After some thinking I realised that the reliance on signal delivery to 
> > work properly to trap non-executable stack may actually be a problem for 
> > bare-iron targets.
> 
> IMO, I think we can start worrying about those when we actually
> encounter the problem; and I am assuming that this is not going to
> be specific to mips.

 I don't consider this a showstopper for your change either; in particular 
it does not apply to Linux kernel debugging because except from one or two 
obscure corner cases of some very old hardware that did not have memory at 
0 (except from a page worth shadow area for exception handlers) Linux runs 
its own code using unmapped segments so no memory access permissions 
apply.

 But I decided the issue had to be noted.

> Are we still good to go with this patch? Attached is the latest
> version. For any additional comments that you'd like to be added
> (in particular, with respect to what you just pointed out), I suggest
> add them as a followup patch.  This way, this one is out of the way,
> and we can just focus on the comments themselves.

 I agree about the comments.  However it's just struck me there's 
something weird about your code, see below.

> I am also adding a second patch, which shows the changes I made
> in this iteration.

 Thanks, that's useful.

> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 9a3c7fb..68ac858 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -3009,6 +3009,37 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>    return align_down (addr, 16);
>  }
>  
> +/* Implement the "push_dummy_call" gdbarch method.  */
> +
> +static CORE_ADDR
> +mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
> +		      CORE_ADDR funaddr, struct value **args,
> +		      int nargs, struct type *value_type,
> +		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
> +		      struct regcache *regcache)
> +{
> +  int bp_len;
> +  static gdb_byte nop_insn[] = { 0, 0, 0, 0 };
> +
> +  *bp_addr = sp;
> +  gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);

 You set bp_addr to SP here, so you rely on the stack pointer to have been 
implicitly adjusted down below the current frame by call_function_by_hand 
(or otherwise the breakpoint would occupy a valid stack frame location at 
this point -- which is dangerous if software breakpoints are used).  I 
find it a bit fragile, how about putting the breakpoint entirely in space 
allocated here?

 Then I think that you can avoid the call to gdbarch_breakpoint_from_pc, 
just as I noted in the microMIPS consideration.  We know that for an 
aligned stack location it's always going to return 4 in bp_len anyway and 
if we want to go below the top of the stack, then we sort of get into a 
chicken and egg problem -- we'll only know how far to go below SP once 
we've called this function, but to call it we need to know the location to 
ask about first.

 Fortunately getting this value exactly right does not really matter -- we 
just need enough space to cover the worst case.  So I suggest this 
instead:

  CORE_ADDR addr;

  addr = sp - sizeof (nop_insn);

so that the breakpoint itself is below the requested SP...

> +
> +  /* The breakpoint layer automatically adjusts the address of
> +     breakpoints inserted in a branch delay slot.  With enough
> +     bad luck, the 4 bytes located just before our breakpoint
> +     instruction could look like a branch instruction, and thus
> +     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.  */
> +  write_memory (*bp_addr - bp_len, nop_insn, sizeof (nop_insn));
> +  sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);

... and then, consequently:

  write_memory (addr - sizeof (nop_insn), nop_insn, sizeof (nop_insn));
  sp = mips_frame_align (gdbarch, addr - sizeof (nop_insn));

  *bp_addr = addr;

(note that in your variation you write the NOP into space of bp_len bytes 
-- as noted above we know that this is going to be 4 anyway, but strictly 
speaking this is risky as it does not tie the number of bytes written to 
the space available).

 OK with these adjustments and sorry that I missed this previously.

  Maciej

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

[Joel Brobecker]

> You set bp_addr to SP here, so you rely on the stack pointer to have
> been implicitly adjusted down below the current frame [...]

I was actually confused, as I thought that SP pointed to the first
unused slot in the stack.

I will make the changes that you suggest and re-test.

One thing that just occured to me while driving home is why not
also use the AT_ENTRY_POINT approach. I figured that there must
have been a reason why we used AT_SYMBOL instead of AT_ENTRY_POINT.
But then, there is your comment that makes me think that the symbol
isn't usually defined, which means that most of (all?) the time,
we actually end up using AT_ENTRY_POINT. Do we know of any reason
why AT_ENTRY_POINT would not work? I'd assume that as long as the
object format is ELF, we'd have one, and so we could use that as
well.

Geee, are we ever going to reach a conclusion on this discussion? :-/

-- 
Joel

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

Attached is the latest version.

It's very very slightly different from the version you suggested,
in the fact that I didn't create a local variable for the breakpoint
address, and stored it in *bp_addr directly.  I didn't see a real
purpose for having a local variable in this case.  I did create
a local variable for the nop instruction address, however. I found
that it did make things a little clearer for that one.

As before, I'm attaching two patches, the first being the last
version of the patch, and the second being the changes introduced
by this iteration.

Testec on mips-irix with no regression.  If we'd rather go with
AT_ENTRY_POINT instead, at least the patch is available here for
the record.

-- 
Joel

[-- Attachment #2: 0001-mips-Switch-inferior-function-calls-to-ON_STACK-meth.patch --]
[-- Type: text/x-diff, Size: 2944 bytes --]

From 19ebe2e03aab266ddd3771fbd7aeff430c32079a Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Wed, 2 May 2012 20:39:57 -0400
Subject: [PATCH] mips: Switch inferior function calls to ON_STACK method.

This patch switches the mips code to use the ON_STACK method
for function calls instead of AT_SYMBOL, which we want to remove.

gdb/ChangeLog:

        * mips-tdep.c (mips_push_dummy_code): New function.
        (mips_gdbarch_init): Set the gdbarch call_dummy_location to
        ON_STACK and install mips_push_dummy_code as our gdbarch
        push_dummy_code routine.
---
 gdb/mips-tdep.c |   38 ++++++++++++++++++++++++++++++++++----
 1 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 9a3c7fb..ebf7c48 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -3009,6 +3009,38 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
   return align_down (addr, 16);
 }
 
+/* Implement the "push_dummy_call" gdbarch method.  */
+
+static CORE_ADDR
+mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
+		      CORE_ADDR funaddr, struct value **args,
+		      int nargs, struct type *value_type,
+		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
+		      struct regcache *regcache)
+{
+  CORE_ADDR nop_addr;
+  static gdb_byte nop_insn[] = { 0, 0, 0, 0 };
+
+  /* Reserve enough room on the stack for our breakpoint instruction.  */
+  *bp_addr = sp - sizeof (nop_insn);
+
+  /* The breakpoint layer automatically adjusts the address of
+     breakpoints inserted in a branch delay slot.  With enough
+     bad luck, the 4 bytes located just before our breakpoint
+     instruction could look like a branch instruction, and thus
+     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);
+  write_memory (nop_addr, nop_insn, sizeof (nop_insn));
+  sp = mips_frame_align (gdbarch, nop_addr);
+
+  /* Inferior resumes at the function entry point.  */
+  *real_pc = funaddr;
+
+  return sp;
+}
+
 static CORE_ADDR
 mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 			   struct regcache *regcache, CORE_ADDR bp_addr,
@@ -6906,10 +6938,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   /* MIPS version of CALL_DUMMY.  */
 
-  /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
-     replaced by a command, and all targets will default to on stack
-     (regardless of the stack's execute status).  */
-  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
   set_gdbarch_frame_align (gdbarch, mips_frame_align);
 
   set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);
-- 
1.7.0.4


[-- Attachment #3: 0001-More-mods-to-mips-ON_STACK-function-call.patch --]
[-- Type: text/x-diff, Size: 1629 bytes --]

From 0e907377fff968693ff42d3cab61cafa4d50521b Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 8 May 2012 17:57:56 -0400
Subject: [PATCH] More mods to mips ON_STACK function call.

---
 gdb/mips-tdep.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 68ac858..ebf7c48 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -3018,11 +3018,11 @@ mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
 		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
 		      struct regcache *regcache)
 {
-  int bp_len;
+  CORE_ADDR nop_addr;
   static gdb_byte nop_insn[] = { 0, 0, 0, 0 };
 
-  *bp_addr = sp;
-  gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);
+  /* Reserve enough room on the stack for our breakpoint instruction.  */
+  *bp_addr = sp - sizeof (nop_insn);
 
   /* The breakpoint layer automatically adjusts the address of
      breakpoints inserted in a branch delay slot.  With enough
@@ -3031,8 +3031,9 @@ mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
      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.  */
-  write_memory (*bp_addr - bp_len, nop_insn, sizeof (nop_insn));
-  sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);
+  nop_addr = *bp_addr - sizeof (nop_insn);
+  write_memory (nop_addr, nop_insn, sizeof (nop_insn));
+  sp = mips_frame_align (gdbarch, nop_addr);
 
   /* Inferior resumes at the function entry point.  */
   *real_pc = funaddr;
-- 
1.7.0.4

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

[Maciej W. Rozycki]

On Tue, 8 May 2012, Joel Brobecker wrote:

> Attached is the latest version.
> 
> It's very very slightly different from the version you suggested,
> in the fact that I didn't create a local variable for the breakpoint
> address, and stored it in *bp_addr directly.  I didn't see a real
> purpose for having a local variable in this case.  I did create
> a local variable for the nop instruction address, however. I found
> that it did make things a little clearer for that one.

 That variable was expected to save some memory accesses.  Your version 
should be equally good, I think -- there's no function call between 
setting *bp_addr and reading it back, it's not volatile, so any sane 
compiler should keep it in a register and do not really make that 
read-back while optimising.  While not optimising that probably does not 
matter.  And given it's your code, you're of course free to write it your 
style as long as it's functionally correct and comprehensible for the 
average GDB developer.

> As before, I'm attaching two patches, the first being the last
> version of the patch, and the second being the changes introduced
> by this iteration.

 I'm fine with this version.

> Testec on mips-irix with no regression.  If we'd rather go with
> AT_ENTRY_POINT instead, at least the patch is available here for
> the record.

 I have no strong opinion either way -- as we discussed both choices work 
equally well for the common cases and both have their corner-case 
advantages and disadvantages, none of which seem to directly hit any one 
of us.  What are the reasons for other targets we support to have chosen 
their particular way?

  Maciej

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

[Mark Kettenis]

> Date: Wed, 9 May 2012 08:31:47 +0100
> From: "Maciej W. Rozycki" <macro@codesourcery.com>
> 
> On Tue, 8 May 2012, Joel Brobecker wrote:
> 
> > Testec on mips-irix with no regression.  If we'd rather go with
> > AT_ENTRY_POINT instead, at least the patch is available here for
> > the record.
> 
>  I have no strong opinion either way -- as we discussed both choices work 
> equally well for the common cases and both have their corner-case 
> advantages and disadvantages, none of which seem to directly hit any one 
> of us.  What are the reasons for other targets we support to have chosen 
> their particular way?

Not too long ago, Jan Kratochvil pointed out a problem with
AT_ENTRY_POINT.  The entry point address might be covered by DWARF CFI
embedded in the binary.  Now if the called function throws an
exception, it will use this CFI to unwind the stack with potential
disastrous consequences.  Now I'm not sure how serious that problem
actually is; calling functions that throw exceptions from within GDB
seems like a really bad idea in the first place (did I ever mention
that C++ code is basically undebuggable? ;)).  But ON_STACK doesn't
have this limitation.

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

[Maciej W. Rozycki]

On Wed, 9 May 2012, Mark Kettenis wrote:

> >  I have no strong opinion either way -- as we discussed both choices work 
> > equally well for the common cases and both have their corner-case 
> > advantages and disadvantages, none of which seem to directly hit any one 
> > of us.  What are the reasons for other targets we support to have chosen 
> > their particular way?
> 
> Not too long ago, Jan Kratochvil pointed out a problem with
> AT_ENTRY_POINT.  The entry point address might be covered by DWARF CFI
> embedded in the binary.  Now if the called function throws an
> exception, it will use this CFI to unwind the stack with potential
> disastrous consequences.  Now I'm not sure how serious that problem
> actually is; calling functions that throw exceptions from within GDB
> seems like a really bad idea in the first place (did I ever mention
> that C++ code is basically undebuggable? ;)).  But ON_STACK doesn't
> have this limitation.

 OK, that looks like an advantage that actually matters in practice.  So 
unless someone comes with a counterargument I think that we should go for 
this change.

 FWIW I don't find it unreasonable to manually call functions that can 
throw exceptions -- that's just a centralised error recovery mechanism 
that's alternative to the standard procedural language's series of IFs 
checking the error status after each relevant subprocedure call (or other 
action that could score as a failure) and bailing out if unsuccessful 
before proceeding with any further actions.

 It would make sense IMHO though if GDB was capable to catch that 
exception and report that the function returned with such rather than 
normally (if it already does not, that is -- I don't deal with C++ or 
other code using exceptions much, so I don't know).

  Maciej

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

[Tom Tromey]

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

Maciej>  It would make sense IMHO though if GDB was capable to catch that 
Maciej> exception and report that the function returned with such rather than 
Maciej> normally (if it already does not, that is -- I don't deal with C++ or 
Maciej> other code using exceptions much, so I don't know).

GDB tries, see "help show unwind-on-terminating-exception".

The problem here is that gdb doesn't try to modify the unwind data when
making an inferior call.  So, if the inferior call reuses memory covered
by the unwind data, then the unwinder will go ahead and try to use this
data, resulting in weird behavior.

It is probably possible to fix the problem other ways, but ON_STACK is
pretty simple in comparison to modifying the unwind data or hooking into
the unwinder.

Tom

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

[Joel Brobecker]

I have now checked this patch in.

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

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

-- 
Joel

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

[Maciej W. Rozycki]

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

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

[Joel Brobecker]

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

Is that something we could detect at gdbarch init? (I don't think we
have a process at init time)

> 2012-05-14  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	gdb/
> 	* mips-tdep.c (mips_push_dummy_code): Handle microMIPS code.

FWIW, the change looks good to me.

-- 
Joel

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

[Maciej W. Rozycki]

On Mon, 14 May 2012, Joel Brobecker wrote:

> >  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. ;)
> 
> Is that something we could detect at gdbarch init? (I don't think we
> have a process at init time)

 That has to be done every time a request for a manual call is made -- 
according to current conditions.  The stack may be in oblivion during 
early startup for example, but get into working state later on.  Of course 
the callee may need to use the stack too, in which case it's not going to 
work anyway.  So it is really leaf functions only that could be called and 
then not even all of them.

 Therefore I'll just reiterate the unimportance of this corner case.  I 
guess nobody will really notice, it looks to me the manual call feature is 
not used by people that often in the first place.  I've looked through our 
bugzilla and the long-lived breakage of MIPS16 manual calls I posted fixes 
for recently (both the FP ABI fix and the ISA bit fix) has never been 
reported by anyone.

> > 2012-05-14  Maciej W. Rozycki  <macro@codesourcery.com>
> > 
> > 	gdb/
> > 	* mips-tdep.c (mips_push_dummy_code): Handle microMIPS code.
> 
> FWIW, the change looks good to me.

 Thanks for checking and confirming.

  Maciej

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

[Maciej W. Rozycki]

On Mon, 14 May 2012, Maciej W. Rozycki wrote:

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

 Hmm, I intended to fold this update into the original microMIPS change 
before committing, but somehow I did not.  I have checked it now instead.

2012-06-11  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);
 

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

[Maciej W. Rozycki]

On Tue, 8 May 2012, Joel Brobecker wrote:

> > You set bp_addr to SP here, so you rely on the stack pointer to have
> > been implicitly adjusted down below the current frame [...]
> 
> I was actually confused, as I thought that SP pointed to the first
> unused slot in the stack.

 Correct, but stack grows downwards.  So SP points to the end of the first 
unused slot.  This is best shown with an illustration, e.g. for 32 bits:

       |           | +12
       +           + 
       |   . . .   |  +8
       +           +
       |  current  |  +4
       +           +
       |   frame   |   0
SP --> +-----------+
       |   free    |  -4
       +           +
       |   . . .   |  -8
       +           +
       |           | -12

For example for a nested o32 function at SP + 0 you'll have the next 
frame's $a0 argument save slot.

 This is really no different to how some architectures with hardware stack 
support interpret the SP register, e.g. Intel pieces like 8080 or x86 or 
DEC VAX.

> I will make the changes that you suggest and re-test.

 Great!

> One thing that just occured to me while driving home is why not
> also use the AT_ENTRY_POINT approach. I figured that there must
> have been a reason why we used AT_SYMBOL instead of AT_ENTRY_POINT.
> But then, there is your comment that makes me think that the symbol
> isn't usually defined, which means that most of (all?) the time,
> we actually end up using AT_ENTRY_POINT. Do we know of any reason
> why AT_ENTRY_POINT would not work? I'd assume that as long as the
> object format is ELF, we'd have one, and so we could use that as
> well.

 I mentioned that in one of the replies -- there was a comment originally 
that stated that AT_ENTRY_POINT wouldn't work if it was located in a ROM.  
With software breakpoints that is, but support for hardware breakpoints is 
not mandatory in MIPS processors (and I think we don't use them for 
internal breakpoints anyway).  The comment was removed with the addition 
of AT_SYMBOL.  Presumably that magic symbol would be arranged by a ROM 
image generation toolkit.  As I say, I've never actually encountered it.

> Geee, are we ever going to reach a conclusion on this discussion? :-/

 Well, we'll die off eventually, so certainly there's some finite limit.

  Maciej

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

[Maciej W. Rozycki]

On Fri, 4 May 2012, Joel Brobecker wrote:

> Now I know why I was told you are a MIPS expert :-). I never really
> had the chance or need to delve into the details of any specific
> architecture. Even for the ia64-hpux port, I could do with just
> a superficial knowledge of that CPU.

 It just started the other way round for me -- I got pulled into toolchain 
hacking as a result of my hopeless attempts to bring up the then current 
version of Linux kernel on my DECstation box (a MIPS R3000 class system).  
Of course you can't really use a kernel without userland and as it turned 
out that meant porting upcoming glibc 2.2 unless I wanted to downgrade 
back to long obsolete 2.0 (which I did not).

 Of course binutils at that point did not support symbol versioning in the 
MIPS port yet (it couldn't use the generic ELF versioning stuff all the 
other ports used because of some peculiarities of the IRIX ELF variation), 
so I had to fix that first and that's how it started rolling...  So I'm 
really a low-level kernel developer who just happens to do something else. 
;)

 I still have that DECstation and can check any original MIPS I ISA stuff 
at the run time should there be such a need.

> > * newly-added 48-bit instructions.
> 
> I am wondering if this addition is going to hurt in terms of our
> support...  From what I could tell from my mips64 manual, even
> on this CPU the instructions are still 32bit long... But I'm
> digressing, sorry.

 My GDB patch handles it just fine as the major opcode for any 48-bit 
instructions has been already allocated by the architecture (you can see 
some parts in my change that classify instructions according to their 
length for example), except there has been no hardware or simulator 
available to test it yet.  QEMU might be closest to getting there.  For 
the purpose of software breakpoints any 48-bit instruction is I reckon 
replaced with a 32-bit breakpoint.  This is safe because 48-bit 
instructions cannot be placed in any fixed-width branch delay slots, so 
the choice of the width of the breakpoint does not matter (not that we put 
any breakpoints in delay slots anyway, but it doesn't hurt to play safe 
anyway).

 Worse, however, that the lack of permission for C99 code in opcodes 
precludes the use of the long long type to hold instruction patterns and 
opcode masks there.  And redefining "struct mips_opcode" to split 
instructions/masks across multiple members (or worse yet, using some hack 
to split them acros multiple opcode table entries) means rewriting lots of 
stuff at least in opcodes itself and in GAS.

 That's sort of discouraging for a single instruction (and no 64-bit 
microMIPS hardware yet), especially given that I think we will eventually 
permit at least the type itself if not other C99 features sometime.  This 
is even more disappointing given these members are defined as the long 
type -- already wasting 32-bits twice per each entry throughout the table 
on 64-bit hosts...

 The end result is binutils do not support the LI32 instruction now, so 
while we do in GDB, there's actually no reasonable way to produce it (of 
course you can still handcode it as data with the .half directive -- note 
that just like MIPS16 extended instructions (and unlike standard MIPS 
ones) 32-bit and 48-bit microMIPS instructions are encoded as sequences of 
16-bit halfwords each of which is in the processor's endianness).

> >  Coincidentally all-zeroes is a 32-bit NOP instruction both in the 
> > standard MIPS and the microMIPS mode -- there's a 16-bit encoding of NOP 
> > in the microMIPS mode naturally as well.
> 
> I'm wondering if you'd like me to rename "null_insn" into "nop_insn"
> in my patch. I didn't do it, because I'd expect the instruction size
> to depend on the mode. As of today, we know that the breakpoint we
> are inserting is always going to be at an even address, so it's always
> going to be 4 bytes. So maybe it does make sense to rename it. Let
> me know.

 I think "nop_insn" might be better; we handle different encodings of the 
NOP intruction in GAS already (used for .align padding among others) but 
from my understanding of this piece this is never going to be necessary 
here as we'll only ever need a standard MIPS and a 32-bit microMIPS 
breakpoint here.  I think however that initialising it like this:

  static gdb_byte nop_insn[] = { 0, 0, 0, 0 };

may express the intent a bit better (this is what we do in 
mips_breakpoint_from_pc where we initialise all array elements explicitly 
even where we could let the compiler default to zeros).

> >  Understood, but I'd be happier if the comment you're removing or a 
> > similar stayed in place.  If by trap you mean SIGTRAP, then I think this 
> > is not going to be the case.
> 
> I think you refer to the comment from Andrew Cagney? I've put it back
> as is.

 Yes, however following Mark's note I think it'll be better to state that 
it's OK if we get SIGSEGV for a non-executable stack and refer to 
handle_inferior_event (look for SIGSEGV there for a detailed explanation).  
Furthermore, given that it is supported I think it should simply be 
documented in the function's description instead.

> OK to commit, modulo the possible rename above?

 OK with these changes; once you've checked your patch in I'll update the 
outstanding microMIPS change according to my earlier notes (and will 
probably come back with issues and haunt everybody here).

  Maciej

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

[Mark Kettenis]

> Date: Thu, 3 May 2012 22:08:58 +0100
> From: "Maciej W. Rozycki" <macro@codesourcery.com>
> 
> Joel,
> 
> > This patch switches the mips code to use the ON_STACK method
> > for function calls instead of AT_SYMBOL, which we want to remove.
> 
>  Thanks for this work -- can you give me a reference to some background 
> information as to why exactly we want to remove the AT_SYMBOL method?

The AT_SYMBOL method relies on a magic symbol being present in the
binarie that's being debugged.  There is no guarantee that that magic
symbol is actually present in your binary.

> > Another little detail on the implementation of mips_push_dummy_code.
> > It starts by aligning the stack.  AFAIK, the stack is supposed to
> > always be aligned to at least 4 bytes (4 bytes for mips32, 8 bytes
> > for mips64). So, the initial alignment shouldn't be necessary, since
> > that's good enough aligment for our breakpoint instruction.  But
> > in the end, I chose to keep it, JIC. We could possibly change the
> > code to align to 4 instead of 16 like mips_frame_align does, if
> > we want to.
> 
>  For the record: the respective ABIs mandate that the stack is aligned to 
> 8 bytes for 32-bit targets and to 16 bytes for 64-bit targets.  However 
> the user may have fiddled with SP, so I think it's better to stay safe 
> and therefore I agree it's better if we prealign the stack and avoid 
> crashing the debuggee in this context.

Like I wrote elsewhere, the generic code that calls push_dummy_code()
already alignes the stack, so it isn't necessary to do it again here.

> >    /* MIPS version of CALL_DUMMY.  */
> >  
> > -  /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
> > -     replaced by a command, and all targets will default to on stack
> > -     (regardless of the stack's execute status).  */
> > -  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
> > +  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
> > +  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
> >    set_gdbarch_frame_align (gdbarch, mips_frame_align);
> >  
> >    set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);
> 
>  So what if the stack pages are indeed not executable (their page entries 
> have the XI aka Execute Inhibit bit set)?

The resulting SIGSEGV will be recognized by GDB and handled
appropriately; see infrun.c:handle_inferior_event().

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

[Maciej W. Rozycki]

Mark,

> >  Thanks for this work -- can you give me a reference to some background 
> > information as to why exactly we want to remove the AT_SYMBOL method?
> 
> The AT_SYMBOL method relies on a magic symbol being present in the
> binarie that's being debugged.  There is no guarantee that that magic
> symbol is actually present in your binary.

 And failing that the executable's entry point.  Actually I have yet to 
see a binary that has that magic __CALL_DUMMY_ADDRESS symbol, so the 
fallback must have worked pretty well.  Interestingly enough it was the 
MIPS target AT_SYMBOL was specifically added for:

http://sourceware.org/ml/gdb-patches/2003-08/msg00076.html

And that very patch regrettably removed the explanation of the actual 
reason of the whole arrangement -- the need to be able to debug a program 
in ROM that does not have an entry point (or I think, probably more 
accurately, rather has a read-only entry point such as the reset vector, 
so a breakpoint is not guaranteed to work there).

 Of course ON_STACK solves that in a more general manner, so that's the 
actual benefit from this change.  That removes any doubt I might have had.

 Thanks for your input, including your other comments.

  Maciej

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

[Mark Kettenis]

> From: Joel Brobecker <brobecker@adacore.com>
> Date: Thu,  3 May 2012 15:03:21 -0400
> 
> This patch switches the mips code to use the ON_STACK method
> for function calls instead of AT_SYMBOL, which we want to remove.
> 
> The one difficulty came from the fact that we needed to make sure
> that the area on the stack just before where we insert the breakpoint
> instruction does not look like a branch instruction.  Otherwise,
> we get an automatic breakpoint adjustment which breaks everything.
> 
> Another little detail on the implementation of mips_push_dummy_code.
> It starts by aligning the stack.  AFAIK, the stack is supposed to
> always be aligned to at least 4 bytes (4 bytes for mips32, 8 bytes
> for mips64). So, the initial alignment shouldn't be necessary, since
> that's good enough aligment for our breakpoint instruction.  But
> in the end, I chose to keep it, JIC. We could possibly change the
> code to align to 4 instead of 16 like mips_frame_align does, if
> we want to.
> 
> gdb/ChangeLog:
> 
>         * mips-tdep.c (mips_push_dummy_code): New function.
>         (mips_gdbarch_init): Set the gdbarch call_dummy_location to
>         ON_STACK and install mips_push_dummy_code as our gdbarch
>         push_dummy_code routine.
> 
> Tested on mips-irix.  It might be nice to test on other mips targets
> as well, although it should not be necessary. OK to commit?
> 
> Thanks,
> -- 
> Joel
> 
> ---
>  gdb/mips-tdep.c |   36 ++++++++++++++++++++++++++++++++----
>  1 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 9a3c7fb..3e6b00b 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -3009,6 +3009,36 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>    return align_down (addr, 16);
>  }
>  
> +/* Implement the push_dummy_code gdbarch method for mips targets.  */

I notice people have been adding this style of comment in some other
newly contributed targets.  Do people really feel that having these is
useful?  If so, can we at least settle on a consitent style?

> +
> +static CORE_ADDR
> +mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
> +		      CORE_ADDR funaddr, struct value **args,
> +		      int nargs, struct type *value_type,
> +		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
> +		      struct regcache *regcache)
> +{
> +  int bp_len;
> +  gdb_byte null_insn[4] = {0};

Missing spaces around the 0.

> +  *bp_addr = mips_frame_align (gdbarch, sp);

SP is guaranteed to be properly aligned here (see
infcall.c:call_function_by_hand()).

> +  gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);
> +
> +  /* The breakpoint layer automatically adjusts the address of
> +     breakpoints inserted in a branch delay slot.  With enough
> +     bad luck, the 4 bytes located just before our breakpoint
> +     instruction could look like a branch instruction, and thus
> +     trigger the adjustement, and break the function call entirely.
> +     So, we reserve those 4 bytes and write a null instruction
> +     to prevent that from happening.  */
> +  write_memory (*bp_addr - bp_len, null_insn, sizeof (null_insn));
> +  sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);
> +
> +  /* Inferior resumes at the function entry point.  */
> +  *real_pc = funaddr;
> +
> +  return sp;
> +}

Please add a blank line here.

>  static CORE_ADDR
>  mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>  			   struct regcache *regcache, CORE_ADDR bp_addr,
> @@ -6906,10 +6936,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  
>    /* MIPS version of CALL_DUMMY.  */
>  
> -  /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
> -     replaced by a command, and all targets will default to on stack
> -     (regardless of the stack's execute status).  */
> -  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
> +  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
> +  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
>    set_gdbarch_frame_align (gdbarch, mips_frame_align);
>  
>    set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);
> -- 
> 1.7.0.4
> 
> 

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

[Joel Brobecker]

> > +/* Implement the push_dummy_code gdbarch method for mips targets.  */
> 
> I notice people have been adding this style of comment in some other
> newly contributed targets.  Do people really feel that having these is
> useful?  If so, can we at least settle on a consitent style?

I think they are useful, because they allow us to tell people that
new functions should ALL be documented, even the obvious ones
that are used to implement a given hook.  And since we do not
want to repeat the hook's documentation, this tells the reader
where to look.

I am happy to standardize on any format, as long as we all agree.

I will adjust the two style issues that you pointed out.

Thank Mark.
-- 
Joel

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

[Yao Qi]

On 05/04/2012 05:57 AM, Joel Brobecker wrote:
>>> > > +/* Implement the push_dummy_code gdbarch method for mips targets.  */
>> > 
>> > I notice people have been adding this style of comment in some other
>> > newly contributed targets.  Do people really feel that having these is
>> > useful?  If so, can we at least settle on a consitent style?
> I think they are useful, because they allow us to tell people that
> new functions should ALL be documented, even the obvious ones
> that are used to implement a given hook.  And since we do not
> want to repeat the hook's documentation, this tells the reader
> where to look.

Yes, they are useful.  Beyond Joel's description, it still quite useful
when  hook method's comment or parameter is changed, we don't have to
change in every *-tdep.c files.

> 
> I am happy to standardize on any format, as long as we all agree.

+1.

If hook is ABI or target variant specific, such as push_dummy_call in
mips-tdep.c, BAR is the name of ABI or target variant,

  /* Implement the FOO gdbarch method for BAR.  */

If hook is for the whole port, we can simply say this,

  /* Implement the FOO gdbarch method.  */

-- 
Yao (齐尧)

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

> gdb/ChangeLog:
> 
>         * mips-tdep.c (mips_push_dummy_code): New function.
>         (mips_gdbarch_init): Set the gdbarch call_dummy_location to
>         ON_STACK and install mips_push_dummy_code as our gdbarch
>         push_dummy_code routine.

Second version of this patch with Mark's style comments addressed.

-- 
Joel

[-- Attachment #2: mips-on-stack-v2.diff --]
[-- Type: text/x-diff, Size: 2787 bytes --]

commit d7f93872c27075342da02ab2ee57062d8b9511c9
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Wed May 2 20:39:57 2012 -0400

    mips: Switch inferior function calls to ON_STACK method.
    
    This patch switches the mips code to use the ON_STACK method
    for function calls instead of AT_SYMBOL, which we want to remove.
    
    gdb/ChangeLog:
    
            * mips-tdep.c (mips_push_dummy_code): New function.
            (mips_gdbarch_init): Set the gdbarch call_dummy_location to
            ON_STACK and install mips_push_dummy_code as our gdbarch
            push_dummy_code routine.

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 9a3c7fb..5faf114 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -3009,6 +3009,37 @@ mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
   return align_down (addr, 16);
 }
 
+/* Implement the push_dummy_code gdbarch method for mips targets.  */
+
+static CORE_ADDR
+mips_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp,
+		      CORE_ADDR funaddr, struct value **args,
+		      int nargs, struct type *value_type,
+		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
+		      struct regcache *regcache)
+{
+  int bp_len;
+  gdb_byte null_insn[4] = { 0 };
+
+  *bp_addr = mips_frame_align (gdbarch, sp);
+  gdbarch_breakpoint_from_pc (gdbarch, bp_addr, &bp_len);
+
+  /* The breakpoint layer automatically adjusts the address of
+     breakpoints inserted in a branch delay slot.  With enough
+     bad luck, the 4 bytes located just before our breakpoint
+     instruction could look like a branch instruction, and thus
+     trigger the adjustement, and break the function call entirely.
+     So, we reserve those 4 bytes and write a null instruction
+     to prevent that from happening.  */
+  write_memory (*bp_addr - bp_len, null_insn, sizeof (null_insn));
+  sp = mips_frame_align (gdbarch, *bp_addr - 2 * bp_len);
+
+  /* Inferior resumes at the function entry point.  */
+  *real_pc = funaddr;
+
+  return sp;
+}
+
 static CORE_ADDR
 mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 			   struct regcache *regcache, CORE_ADDR bp_addr,
@@ -6906,10 +6937,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   /* MIPS version of CALL_DUMMY.  */
 
-  /* NOTE: cagney/2003-08-05: Eventually call dummy location will be
-     replaced by a command, and all targets will default to on stack
-     (regardless of the stack's execute status).  */
-  set_gdbarch_call_dummy_location (gdbarch, AT_SYMBOL);
+  set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+  set_gdbarch_push_dummy_code (gdbarch, mips_push_dummy_code);
   set_gdbarch_frame_align (gdbarch, mips_frame_align);
 
   set_gdbarch_convert_register_p (gdbarch, mips_convert_register_p);