Re: [patch/rfc] New method, frame_align(), use in MIPS

[Andrew Cagney <ac131313 at ges dot redhat dot com>]

I've checked in the bulk of this fix.  I'll post updated doco separatly.

The MIPS, for a generic dummy frame, will now always use the correct 
return value address.

Andrew


> Hello,
>
> This patch comes direct from the school of ``how did this ever manage to work''.
>
> The function valops.c:hand_function_call() is responsible for most of the work required when performing an inferior function call.  The code starts out with the innocent looking:
>   old_sp = sp = read_sp ();
> And then uses SP for things like allocating space for a call dummy (if on the stack), the return value, and even some arguments.
>
> When space is allocated, the code carefully uses STACK_ALIGN(amount) to ensure that the adjusted stack is still aligned vis:
>   if (struct_return)
>     {
>       int len = TYPE_LENGTH (value_type);
>       if (STACK_ALIGN_P ())
>         len = STACK_ALIGN (len);
>       if (INNER_THAN (1, 2))
>         {
>           /* stack grows downward */
>           sp -= len;
>           struct_addr = sp;
>         }
>       else
>         {
>           /* stack grows upward */
>           struct_addr = sp;
>           sp += len;
>         }
>     }
> (I should note that STACK_ALIGN() is a little wierd, and for reasons I don't understand has ended up always rounding up.)
>
> These allocated values are then used for things like creating the call frame:
>   sp = PUSH_ARGUMENTS (nargs, args, sp, struct_return, struct_addr);
> so it all looks inocent enough.
>
> Just a minor problem.  That initial SP was never aligned according to the target's frame alignment requirements (for MIPS it's typically 16 bytes).  Consequently, every address that was ever computed using that SP is most likely bogus.
>
> However, don't let that get in the way of a ``working'' GDB.
>
> Targets, like the MIPS, ``fix'' this minor problem, by quietly and locally adjusting everything in PUSH_ARGUMENTS().  STRUCT_ADDR is the address that the struct will be stored, right?  Wrong.  Its actually stored at:
>   struct_addr = ROUND_DOWN (struct_addr, 16);
> but whats a few bytes between friends ;-)  There is no way that valops.c:hand_function_call() can know where the return value is going to be stored.
2002-09-18  Andrew Cagney  <ac131313@redhat.com>

	* valops.c (hand_function_call): Align the initial stack pointer
	and STRUCT_ADDR using frame_align.  When STRUCT_RETURN and
	FRAME_ALIGN_P, use STRUCT_ADDR to obtain the called function's
	return value.
	* mips-tdep.c (mips_frame_align): New function.
	(mips_gdbarch_init): Set frame_align.
	* gdbarch.sh (FRAME_ALIGN): New method.
	* gdbarch.h, gdbarch.c: Re-generate.

Index: gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.161
diff -u -r1.161 gdbarch.sh
--- gdbarch.sh	13 Sep 2002 23:21:45 -0000	1.161
+++ gdbarch.sh	18 Sep 2002 15:32:49 -0000
@@ -572,6 +572,7 @@
 f:2:FRAME_NUM_ARGS:int:frame_num_args:struct frame_info *frame:frame::0:0
 #
 F:2:STACK_ALIGN:CORE_ADDR:stack_align:CORE_ADDR sp:sp::0:0
+M:::CORE_ADDR:frame_align:CORE_ADDR address:address
 v:2:EXTRA_STACK_ALIGNMENT_NEEDED:int:extra_stack_alignment_needed::::0:1::0:::
 F:2:REG_STRUCT_HAS_ADDR:int:reg_struct_has_addr:int gcc_p, struct type *type:gcc_p, type::0:0
 F:2:SAVE_DUMMY_FRAME_TOS:void:save_dummy_frame_tos:CORE_ADDR sp:sp::0:0
Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.125
diff -u -r1.125 mips-tdep.c
--- mips-tdep.c	17 Sep 2002 23:26:01 -0000	1.125
+++ mips-tdep.c	18 Sep 2002 15:32:49 -0000
@@ -2584,6 +2584,14 @@
 #define ROUND_DOWN(n,a) ((n) & ~((a)-1))
 #define ROUND_UP(n,a) (((n)+(a)-1) & ~((a)-1))
 
+/* Adjust the address downward (direction of stack growth) so that it
+   is correctly aligned for a new stack frame.  */
+static CORE_ADDR
+mips_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  return ROUND_DOWN (addr, 16);
+}
+
 static CORE_ADDR
 mips_eabi_push_arguments (int nargs,
 			  struct value **args,
@@ -5963,6 +5971,7 @@
   set_gdbarch_call_dummy_words (gdbarch, mips_call_dummy_words);
   set_gdbarch_sizeof_call_dummy_words (gdbarch, sizeof (mips_call_dummy_words));
   set_gdbarch_push_return_address (gdbarch, mips_push_return_address);
+  set_gdbarch_frame_align (gdbarch, mips_frame_align);
   set_gdbarch_register_convertible (gdbarch, mips_register_convertible);
   set_gdbarch_register_convert_to_virtual (gdbarch, 
 					   mips_register_convert_to_virtual);
Index: valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.72
diff -u -r1.72 valops.c
--- valops.c	14 Sep 2002 02:09:39 -0000	1.72
+++ valops.c	18 Sep 2002 15:32:50 -0000
@@ -1351,7 +1351,55 @@
      they are saved on the stack in the inferior.  */
   PUSH_DUMMY_FRAME;
 
-  old_sp = sp = read_sp ();
+  old_sp = read_sp ();
+
+  /* Ensure that the initial SP is correctly aligned.  */
+  if (gdbarch_frame_align_p (current_gdbarch))
+    {
+      /* NOTE: cagney/2002-09-18:
+
+	 On a RISC architecture, a void parameterless generic dummy
+	 frame (i.e., no parameters, no result) typically does not
+	 need to push anything the stack and hence can leave SP and
+	 FP.  Similarly, a framelss (possibly leaf) function does not
+	 push anything on the stack and, hence, that too can leave FP
+	 and SP unchanged.  As a consequence, a sequence of void
+	 parameterless generic dummy frame calls to frameless
+	 functions will create a sequence of effectively identical
+	 frames (SP, FP and TOS and PC the same).  This, not
+	 suprisingly, results in what appears to be a stack in an
+	 infinite loop --- when GDB tries to find a generic dummy
+	 frame on the internal dummy frame stack, it will always find
+	 the first one.
+
+	 To avoid this problem, the code below always grows the stack.
+	 That way, two dummy frames can never be identical.  It does
+	 burn a few bytes of stack but that is a small price to pay
+	 :-).  */
+      sp = gdbarch_frame_align (current_gdbarch, old_sp);
+      if (sp == old_sp)
+	{
+	  if (INNER_THAN (1, 2))
+	    /* Stack grows down.  */
+	    sp = gdbarch_frame_align (current_gdbarch, old_sp - 1);
+	  else
+	    /* Stack grows up.  */
+	    sp = gdbarch_frame_align (current_gdbarch, old_sp + 1);
+	}
+      gdb_assert ((INNER_THAN (1, 2) && sp <= old_sp)
+		  || (INNER_THAN (2, 1) && sp >= old_sp));
+    }
+  else
+    /* FIXME: cagney/2002-09-18: Hey, you loose!  Who knows how badly
+       aligned the SP is!  Further, per comment above, if the generic
+       dummy frame ends up empty (because nothing is pushed) GDB won't
+       be able to correctly perform back traces.  If a target is
+       having trouble with backtraces, first thing to do is add
+       FRAME_ALIGN() to its architecture vector.  After that, try
+       adding SAVE_DUMMY_FRAME_TOS() and modifying FRAME_CHAIN so that
+       when the next outer frame is a generic dummy, it returns the
+       current frame's base.  */
+    sp = old_sp;
 
   if (INNER_THAN (1, 2))
     {
@@ -1366,6 +1414,11 @@
       sp += sizeof_dummy1;
     }
 
+  /* NOTE: cagney/2002-09-10: Don't bother re-adjusting the stack
+     after allocating space for the call dummy.  A target can specify
+     a SIZEOF_DUMMY1 (via SIZEOF_CALL_DUMMY_WORDS) such that all local
+     alignment requirements are met.  */
+
   funaddr = find_function_addr (function, &value_type);
   CHECK_TYPEDEF (value_type);
 
@@ -1562,7 +1615,8 @@
 
 
   /* Reserve space for the return structure to be written on the
-     stack, if necessary */
+     stack, if necessary.  Make certain that the value is correctly
+     aligned. */
 
   if (struct_return)
     {
@@ -1574,15 +1628,23 @@
 	len = STACK_ALIGN (len);
       if (INNER_THAN (1, 2))
 	{
-	  /* stack grows downward */
+	  /* Stack grows downward.  Align STRUCT_ADDR and SP after
+             making space for the return value.  */
 	  sp -= len;
+	  if (gdbarch_frame_align_p (current_gdbarch))
+	    sp = gdbarch_frame_align (current_gdbarch, sp);
 	  struct_addr = sp;
 	}
       else
 	{
-	  /* stack grows upward */
+	  /* Stack grows upward.  Align the frame, allocate space, and
+             then again, re-align the frame??? */
+	  if (gdbarch_frame_align_p (current_gdbarch))
+	    sp = gdbarch_frame_align (current_gdbarch, sp);
 	  struct_addr = sp;
 	  sp += len;
+	  if (gdbarch_frame_align_p (current_gdbarch))
+	    sp = gdbarch_frame_align (current_gdbarch, sp);
 	}
     }
 
@@ -1778,14 +1840,13 @@
     do_cleanups (inf_status_cleanup);
 
     /* Figure out the value returned by the function.  */
-/* elz: I defined this new macro for the hppa architecture only.
-   this gives us a way to get the value returned by the function from the stack,
-   at the same address we told the function to put it.
-   We cannot assume on the pa that r28 still contains the address of the returned
-   structure. Usually this will be overwritten by the callee.
-   I don't know about other architectures, so I defined this macro
- */
-
+    /* elz: I defined this new macro for the hppa architecture only.
+       this gives us a way to get the value returned by the function
+       from the stack, at the same address we told the function to put
+       it.  We cannot assume on the pa that r28 still contains the
+       address of the returned structure. Usually this will be
+       overwritten by the callee.  I don't know about other
+       architectures, so I defined this macro */
 #ifdef VALUE_RETURNED_FROM_STACK
     if (struct_return)
       {
@@ -1793,12 +1854,26 @@
 	return VALUE_RETURNED_FROM_STACK (value_type, struct_addr);
       }
 #endif
-
-    {
-      struct value *retval = value_being_returned (value_type, retbuf, struct_return);
-      do_cleanups (retbuf_cleanup);
-      return retval;
-    }
+    /* NOTE: cagney/2002-09-10: Only when the stack has been correctly
+       aligned (using frame_align()) do we can trust STRUCT_ADDR and
+       fetch the return value direct from the stack.  This lack of
+       trust comes about because legacy targets have a nasty habit of
+       silently, and local to PUSH_ARGUMENTS(), moving STRUCT_ADDR.
+       For such targets, just hope that value_being_returned() can
+       find the adjusted value.  */
+    if (struct_return && gdbarch_frame_align_p (current_gdbarch))
+      {
+        struct value *retval = value_at (value_type, struct_addr, NULL);
+        do_cleanups (retbuf_cleanup);
+        return retval;
+      }
+    else
+      {
+	struct value *retval = value_being_returned (value_type, retbuf,
+						     struct_return);
+	do_cleanups (retbuf_cleanup);
+	return retval;
+      }
   }
 }