This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] SPARC psABI fix, callee returns struct checking.
On Wed, Mar 22, 2006 at 08:14:36AM +0100, Eric Botcazou wrote:
> Thanks. I leave the bulk of the review to Richard, but a few remarks:
Thank you for the review. This patch is much simpler and less intrusive
than the previous patch.
> @@ -827,6 +827,9 @@ sparc_override_options (void)
> if (!(target_flags_explicit & MASK_LONG_DOUBLE_128))
> target_flags |= MASK_LONG_DOUBLE_128;
> #endif
> +
> + if (TARGET_ARCH64 && sparc_std_struct_return)
> + error ("-mstd-struct-return is not valid for 64 bit systems");
> }
>
> Any particular reason for not silently accepting it with -m64? After all,
> structure return is already standard in 64-bit mode. Just add !TARGET_ARCH64
> around the new code in sparc_struct_value_rtx.
No reason. Removed.
> +;; Adjust the return address conditionally. If the value of op1 is equal
> +;; to all zero then adjust the return address e.g. op0 = op0 + 4.
>
> I think you want "i.e.", not "e.g." here.
That is correct, changed to "i.e."
> +;; Adjust the return address conditionally. If the value of op1 is equal
> +;; to all zero then adjust the return address e.g. op0 = op0 + 4.
> +;; This is technically *half* the check required by the 32-bit SPARCI
>
> Typo in last word.
Corrected.
> +;; regular rtl because the compiler kills the add thinking that the return
> +;; address should always be constant.
>
> RTL.
Corrected.
> +(define_insn "adjust_register"
> + [(unspec:SI [(match_operand:SI 0 "register_operand" "r")
> + (match_operand:SI 1 "small_int_operand" "I")]
> UNSPEC_ADJUST_REGISTER)]
> + ""
> +{
> + return "add\t%0, %1, %0\n\t";
> +}
> + [(set (attr "length") (const_int 1))])
>
> Add !TARGET_ARCH64 for extra-safety.
Added.
> + /* Calcualte the return object size */
> + tree size = TYPE_SIZE_UNIT (TREE_TYPE (fndecl
>
> Typo.
Corrected.
> +/* Test that GCC follows the SPARC 32-bit psABI with regards to
> + structure return checking in a callee. When -mstd-struct-return
> + is specificed then gcc will emit code to skip the unimp insn. */
> +
> +/* Origin: Carlos O'Donell <carlos@codesourcery.com> */
> +/* { dg-do run { target sparc-*-* } } */
> +/* { dg-options "-mstd-struct-return" } */
>
> This won't pass with -m64, add "dg-require-effective-target ilp32".
That is correct, it would fail with -m64, thanks for spotting that.
New patch attached.
- Removed "emit_barrier ()" in sparc_struct_value_rtx.
No regressions on i686-pc-linux-gnu.
No regressions on sparc-sun-solaris2.8.
Cheers,
Carlos.
--
Carlos O'Donell
CodeSourcery
carlos@codesourcery.com
(650) 331-3385 x716
gcc/
2006-03-20 Carlos O'Donell <carlos@codesourcery.com>
* doc/tm.texi (TARGET_STRUCT_VALUE_RTX): Document
new value 2 for incoming.
* function.c (expand_function_start): Call struct_value_rtx
with incoming as 2.
* config/sparc/sparc.md: Add constant UNSPEC_ADJUST_REGISTER.
Define adjust_register. Comment updated_return.
* config/sparc/sparc.opt: Add -mstd-struct-return option.
* config/sparc/sparc.c (sparc_struct_value_rtx): Use standard
struct return if sparc_std_struct_return and incoming is 2.
(print_operand): Do not adjust return if
sparc_std_struct_return.
gcc/testsuite/
2006-03-20 Carlos O'Donell <carlos@codesourcery.com>
* gcc.dg/sparc-struct-ret-check.c: New test.
Index: gcc/doc/tm.texi
===================================================================
--- gcc/doc/tm.texi (revision 112293)
+++ gcc/doc/tm.texi (working copy)
@@ -4169,12 +4169,15 @@ On some architectures the place where th
is found by the called function is not the same place that the
caller put it. This can be due to register windows, or it could
be because the function prologue moves it to a different place.
-@var{incoming} is @code{true} when the location is needed in
-the context of the called function, and @code{false} in the context of
+@var{incoming} is @code{1} or @code{2} when the location is needed in
+the context of the called function, and @code{0} in the context of
the caller.
-If @var{incoming} is @code{true} and the address is to be found on the
-stack, return a @code{mem} which refers to the frame pointer.
+If @var{incoming} is non-zero and the address is to be found on the
+stack, return a @code{mem} which refers to the frame pointer. If
+@var{incoming} is @code{2}, the result is being used to fetch the
+structure value address at the beginning of a function. If you need
+to emit adjusting code, you should do it at this point.
@end deftypefn
@defmac PCC_STATIC_STRUCT_RETURN
Index: gcc/testsuite/gcc.dg/sparc-struct-ret-check.c
===================================================================
--- gcc/testsuite/gcc.dg/sparc-struct-ret-check.c (revision 0)
+++ gcc/testsuite/gcc.dg/sparc-struct-ret-check.c (revision 0)
@@ -0,0 +1,120 @@
+/* Copyright (C) 2006 Free Software Foundation, Inc. */
+/* Contributed by Carlos O'Donell on 2006-03-14 */
+
+/* Test that GCC follows the SPARC 32-bit psABI with regards to
+ structure return checking in a callee. When -mstd-struct-return
+ is specificed then gcc will emit code to skip the unimp insn. */
+
+/* Origin: Carlos O'Donell <carlos@codesourcery.com> */
+/* { dg-do run { target sparc-*-* } } */
+/* { dg-options "-mstd-struct-return" } */
+/* { dg-require-effective-target ilp32 } */
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+
+/* Global check variable used by signal handler */
+int check = 1;
+div_t dcheck;
+
+div_t foo (void)
+{
+ div_t bar;
+ bar.rem = 3.0;
+ bar.quot = 4.0;
+ return bar;
+}
+
+void handle_sigill (int signum)
+{
+ if (signum == SIGILL && check == 2)
+ {
+ /* We expected a SIGILL due to a mismatch in unimp size
+ and div_t size */
+ exit (0);
+ }
+ else
+ abort ();
+}
+
+/* Implement 3 checks to validate SPARC 32-bit psABI callee
+ returns struct
+
+ Test1: Save area is valid. unimp size is valid.
+ Success: Save area modified correctly.
+ Failure: Save area unmodified.
+
+ Test2: Save area is valid. unimp size is invalid (invalid insn).
+ Success: Save area unmodified. check == 2.
+ Failure: Save area modified or check == 1.
+
+ Test3: Save area is invalid. unimp size is invalid (invalid size).
+ Success: Will raise a SIGILL.
+ Failure: SIGSEGV caused by write to invalid save area. */
+
+int main (void)
+{
+ dcheck.rem = 1;
+ dcheck.quot = 2;
+
+ /*** Test1 ***/
+ /* Insert a call, insert unimp by hand */
+ __asm__ ("st %1, [ %%sp + 0x40 ]\n\t"
+ "call foo\n\t"
+ " nop\n\t"
+ "unimp %2\n\t"
+ : "=m" (dcheck)
+ : "r" (&dcheck), "i" (sizeof(div_t))
+ : "memory");
+
+ /* If the caller doesn't adjust the return, then it crashes.
+ Check the result too. */
+
+ if ((dcheck.rem != 3) || (dcheck.quot !=4))
+ abort ();
+
+
+ /*** Test 2 ***/
+ dcheck.rem = 1;
+ dcheck.quot = 2;
+
+ /* Ignore the return of the function */
+ __asm__ ("st %3, [ %%sp + 0x40 ]\n\t"
+ "call foo\n\t"
+ " nop\n\t"
+ "mov %2, %0\n\t"
+ : "+r" (check), "=m" (dcheck)
+ : "i" (0x2), "r" (&dcheck)
+ : "memory");
+
+ /* If the caller does an unconditional adjustment it will skip
+ the mov, and then we can fail the test based on check's value
+ We pass a valid pointer to a save area in order to check if
+ caller incorrectly wrote to the save area aswell. There may
+ be a case where the unimp check and skip is correct, but the
+ write to the save area still occurs. */
+
+ if (check != 2)
+ abort ();
+
+ if ((dcheck.rem != 1) || (dcheck.quot != 2))
+ abort ();
+
+ /*** Test 3 ***/
+ /* Prepare a test that must SIGILL. According to the spec
+ if the sizes of the save area and return don't match then
+ the copy is ignored and we return to the unimp. */
+
+ signal (SIGILL, handle_sigill);
+
+ __asm__ ("st %%g0, [ %%sp + 0x40 ]\n\t"
+ "call foo\n\t"
+ " nop\n\t"
+ "unimp %0\n\t"
+ : /* No outputs */
+ : "i" (sizeof(div_t)-1)
+ : "memory");
+
+ /* NEVER REACHED */
+ exit (0);
+}
Index: gcc/function.c
===================================================================
--- gcc/function.c (revision 112293)
+++ gcc/function.c (working copy)
@@ -4120,7 +4120,7 @@ expand_function_start (tree subr)
else
#endif
{
- rtx sv = targetm.calls.struct_value_rtx (TREE_TYPE (subr), 1);
+ rtx sv = targetm.calls.struct_value_rtx (TREE_TYPE (subr), 2);
/* Expect to be passed the address of a place to store the value.
If it is passed as an argument, assign_parms will take care of
it. */
Index: gcc/config/sparc/sparc.md
===================================================================
--- gcc/config/sparc/sparc.md (revision 112293)
+++ gcc/config/sparc/sparc.md (working copy)
@@ -60,6 +60,7 @@ (define_constants
(UNSPEC_SP_SET 60)
(UNSPEC_SP_TEST 61)
+ (UNSPEC_ADJUST_REGISTER 62)
])
(define_constants
@@ -7094,8 +7095,15 @@ (define_expand "untyped_return"
DONE;
})
-;; This is a bit of a hack. We're incrementing a fixed register (%i7),
-;; and parts of the compiler don't want to believe that the add is needed.
+;; Adjust the return address conditionally. If the value of op1 is equal
+;; to all zero then adjust the return address i.e. op0 = op0 + 4.
+;; This is technically *half* the check required by the 32-bit SPARC
+;; psABI. This check only ensures that an "unimp" insn was written by
+;; the caller, but doesn't check to see if the expected size matches
+;; (this is encoded in the 12 lower bits). This check is obsolete and
+;; only used by the above code "untyped_return". This cannot be done in
+;; regular RTL because the compiler kills the add thinking that the return
+;; address should always be constant.
(define_insn "update_return"
[(unspec:SI [(match_operand:SI 0 "register_operand" "r")
@@ -7112,6 +7120,25 @@ (define_insn "update_return"
(if_then_else (eq_attr "delayed_branch" "true")
(const_int 3)
(const_int 4)))])
+
+;; This pattern generates an unconditional register adjustment by a small
+;; immediate value. This adjustment is hidden from the compiler.
+;;
+;; It is used in situations where the caller expects a struct return from the
+;; callee, and according to the 32-bit SPARC psABI it has placed an unimp in
+;; the insn stream. The return address needs to be adjusted in these situations.
+;; This cannot be done in regular rtl because the compiler kills the load
+;; thinking that the return address should always be constant.
+;; This code is used by sparc.c (sparc_return_value_rtx).
+
+(define_insn "adjust_register"
+ [(unspec:SI [(match_operand:SI 0 "register_operand" "r")
+ (match_operand:SI 1 "small_int_operand" "I")] UNSPEC_ADJUST_REGISTER)]
+ "! TARGET_ARCH64"
+{
+ return "add\t%0, %1, %0\n\t";
+}
+ [(set (attr "length") (const_int 1))])
(define_insn "nop"
[(const_int 0)]
Index: gcc/config/sparc/sparc.opt
===================================================================
--- gcc/config/sparc/sparc.opt (revision 112293)
+++ gcc/config/sparc/sparc.opt (working copy)
@@ -99,6 +99,9 @@ mcmodel=
Target RejectNegative Joined Var(sparc_cmodel_string)
Use given SPARC-V9 code model
+mstd-struct-return
+Target Report RejectNegative Var(sparc_std_struct_return)
+Enable strict 32-bit psABI struct return checking.
Mask(LITTLE_ENDIAN)
;; Generate code for little-endian
Index: gcc/config/sparc/sparc.c
===================================================================
--- gcc/config/sparc/sparc.c (revision 112293)
+++ gcc/config/sparc/sparc.c (working copy)
@@ -5425,7 +5425,7 @@ sparc_return_in_memory (tree type, tree
Return where to find the structure return value address. */
static rtx
-sparc_struct_value_rtx (tree fndecl ATTRIBUTE_UNUSED, int incoming)
+sparc_struct_value_rtx (tree fndecl, int incoming)
{
if (TARGET_ARCH64)
return 0;
@@ -5440,6 +5440,46 @@ sparc_struct_value_rtx (tree fndecl ATTR
mem = gen_rtx_MEM (Pmode, plus_constant (stack_pointer_rtx,
STRUCT_VALUE_OFFSET));
+ /* Only follow the SPARC ABI for fixed-size structure returns.
+ Variable size structure returns are handled per the normal
+ procedures in GCC. This is enabled by -mstd-struct-return */
+ if (incoming == 2
+ && sparc_std_struct_return
+ && TYPE_SIZE_UNIT (TREE_TYPE (fndecl))
+ && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (fndecl))) == INTEGER_CST)
+ {
+ /* We must check and adjust the return address, as it is
+ optional as to whether the return object is really
+ provided. */
+ rtx ret_rtx = gen_rtx_REG (Pmode, 31);
+ rtx scratch = gen_reg_rtx (SImode);
+ rtx endlab = gen_label_rtx ();
+
+ /* Calculate the return object size */
+ tree size = TYPE_SIZE_UNIT (TREE_TYPE (fndecl));
+ rtx size_rtx = GEN_INT (TREE_INT_CST_LOW (size) & 0xfff);
+ /* Construct a temporary return value */
+ rtx temp_val = assign_stack_local (Pmode, TREE_INT_CST_LOW (size), 0);
+
+ /* Implement SPARC 32-bit psABI callee returns struck checking
+ requirements:
+
+ Fetch the instruction where we will return to and see if
+ it's an unimp instruction (the most significant 10 bits
+ will be zero). */
+ emit_move_insn (scratch, gen_rtx_MEM (SImode,
+ plus_constant (ret_rtx, 8)));
+ /* Assume the size is valid and pre-adjust */
+ emit_insn (gen_adjust_register (ret_rtx, GEN_INT (4)));
+ emit_cmp_and_jump_insns (scratch, size_rtx, EQ, const0_rtx, SImode, 0, endlab);
+ emit_insn (gen_adjust_register (ret_rtx, GEN_INT (-4)));
+ /* Assign stack temp:
+ Write the address of the memory pointed to by temp_val into
+ the memory pointed to by mem */
+ emit_move_insn (mem, XEXP (temp_val, 0));
+ emit_label (endlab);
+ }
+
set_mem_alias_set (mem, struct_value_alias_set);
return mem;
}
@@ -6639,9 +6679,14 @@ print_operand (FILE *file, rtx x, int co
so we have to account for it. This insn is used in the 32-bit ABI
when calling a function that returns a non zero-sized structure. The
64-bit ABI doesn't have it. Be careful to have this test be the same
- as that used on the call. */
+ as that used on the call. The exception here is that when
+ sparc_std_struct_return is enabled, the psABI is followed exactly
+ and the adjustment is made by the code in sparc_struct_value_rtx.
+ The call emitted is the same when sparc_std_struct_return is
+ present. */
if (! TARGET_ARCH64
&& current_function_returns_struct
+ && ! sparc_std_struct_return
&& (TREE_CODE (DECL_SIZE (DECL_RESULT (current_function_decl)))
== INTEGER_CST)
&& ! integer_zerop (DECL_SIZE (DECL_RESULT (current_function_decl))))