This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

--

The attached patch fixes this:

- it introduces a new methods FRAME_ALIGN() that aligns an address (not length) so that it meets the frame's alignment requirements. It always adjusts the address in the direction of stack growth (no confusion :-).

- it modifies valops.c:hand_funtion_call() so that both the inital SP and the STRUCT_ADDR are alligned according to the targets frame alignment requirements.

- when there is a FRAME_ALIGN() method and STRUCT_RETURN, it trusts that STRUCT_ADDR really is the address of the return value and uses that to obtain the returned value. Not sure if this should be enabled using a separate architecture method rather than frame_align_p(). Anyone?

- for the MIPS, it enables this.

Hence, for the MIPS, when an inferior function call cleanly returns, GDB will always find the correct return value. Even if the register that contains the struct address has been trashed.

Follow on changes would include:

- convert the mips to generic dummy frames

- store these struct return values on the generic dummy frame stack so that multi-level returns work.

- enable the same code for HP/UX (and eliminate a macro that needs to be multi-arched). Anyone know if this will work on hp/ux?

MIPS -o32 -64 and -n32 show no change in test results. I'm suprised that this didn't fix a few failures.

Thoughts?
Andrew
2002-09-10  Andrew Cagney  <cagney@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: doc/ChangeLog
2002-09-10  Andrew Cagney  <cagney@redhat.com>

	* gdbint.texinfo (Target Architecture Definition): Revise
	description of STACK_ALIGN.  Add description of FRAME_ALIGN.

Index: gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.159
diff -u -r1.159 gdbarch.sh
--- gdbarch.sh	6 Sep 2002 20:17:40 -0000	1.159
+++ gdbarch.sh	11 Sep 2002 00:01:47 -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.120
diff -u -r1.120 mips-tdep.c
--- mips-tdep.c	5 Sep 2002 18:31:07 -0000	1.120
+++ mips-tdep.c	11 Sep 2002 00:01:48 -0000
@@ -2576,6 +2576,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,
@@ -5958,6 +5966,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.69
diff -u -r1.69 valops.c
--- valops.c	21 Aug 2002 17:24:31 -0000	1.69
+++ valops.c	11 Sep 2002 00:02:01 -0000
@@ -1353,6 +1353,14 @@
 
   old_sp = sp = read_sp ();
 
+  /* Grow the stack slightly so that the inital SP is correctly
+     aligned for the start of a new frame.  Ensure that any stack
+     growth is in the correct direction  */
+  if (gdbarch_frame_align_p (current_gdbarch))
+    sp = gdbarch_frame_align (current_gdbarch, old_sp);
+  gdb_assert ((INNER_THAN (1, 2) && sp <= old_sp)
+	      || (INNER_THAN (2, 1) && sp >= old_sp));
+
   if (INNER_THAN (1, 2))
     {
       /* Stack grows down */
@@ -1366,6 +1374,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 +1575,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 +1588,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 +1800,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 +1814,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;
+      }
   }
 }
 
Index: doc/gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.100
diff -u -r1.100 gdbint.texinfo
--- doc/gdbint.texinfo	24 Aug 2002 00:21:37 -0000	1.100
+++ doc/gdbint.texinfo	11 Sep 2002 00:02:05 -0000
@@ -3175,6 +3175,23 @@
 represented by @var{fi} does not have a stack frame associated with it.
 Otherwise return 0.
 
+@item frame_align (@var{address})
+@anchor frame_align
+@findex frame_align
+Define this to adjust @var{address} so that it meets the alignment
+requirements for the start of a new stack frame.  A stack frame's
+alignment requirements are typically stronger than a target processors
+stack alignment requirements (@pxref{STACK_ALIGN}).
+
+This function is used to ensure that, when creating a dummy frame, both
+the initial stack pointer and (if needed) the address of the return
+value are correctly aligned.
+
+Unlike @ref{STACK_ALIGN}, this function always adjusts the address in
+the direction of stack growth.
+
+By default, no frame based stack alignment is performed.
+
 @item FRAME_ARGS_ADDRESS_CORRECT
 @findex FRAME_ARGS_ADDRESS_CORRECT
 See @file{stack.c}.
@@ -3690,9 +3707,15 @@
 done.
 
 @item STACK_ALIGN (@var{addr})
+@anchor{STACK_ALIGN}
 @findex STACK_ALIGN
-Define this to adjust the address to the alignment required for the
-processor's stack.
+Define this to increase @var{addr} so that it meets the alignment
+requirements for the processor's stack.
+
+Unlike @ref{frame_align}, this function always adjusts @var{addr}
+upwards.
+
+By default, no stack alignment is performed.
 
 @item STEP_SKIPS_DELAY (@var{addr})
 @findex STEP_SKIPS_DELAY

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]