public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Stack corruption in naked functions.
@ 2008-05-23  2:28 Carlos O'Donell
  2008-05-23 10:40 ` Richard Guenther
  0 siblings, 1 reply; 15+ messages in thread
From: Carlos O'Donell @ 2008-05-23  2:28 UTC (permalink / raw)
  To: Mark Mitchell, gcc-patches, Paul Brook

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

The patch below fixes a stack corruption bug in __attribute__((naked))
functions.

Mark Mitchell's comments here:
http://gcc.gnu.org/ml/gcc-patches/2007-11/msg00592.html
have been incorporated into this patch.

On supported targets this attribute can be used to suppress the normal 
function prologue/epilogue. This allows a function to be implemented in 
assembly without requiring the user to put everything in a separate .S file.

The problem is that at -O0 the compiler assigns all decls to a local 
stack slot and the value will be copied to this slot even if not used. 
This is undesirable in naked function because we don't allocate a stack 
frame.

The best solution we could come up with is to suppress stack slot 
allocation for these functions.

The user documentation is enhanced to clarify the intended use of 
__attribute__((naked)).

Tested on arm-none-eabi and i686-pc-linux-gnu.

OK to checkin to mainline?

Cheers,
Carlos.
-- 
Carlos O'Donell
CodeSourcery
carlos@codesourcery.com
(650) 331-3385 x716

gcc/

2008-05-22  Paul Brook  <paul@codesourcery.com>
	    Carlos O'Donell  <carlos@codesourcery.com>

	* doc/extend.texi: Clarify use of __attribute__((naked)).
	* doc/tm.texi: Document TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS.
	* target.h (gcc_target): Add allocate_stack_slots_for_args.
	* function.c (use_register_for_decl): Use
	targetm.calls.allocate_stack_slots_for_args.
	* target-def.h (TARGET_CALLS): Add
	TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS.
	* config/arm/arm.c (arm_allocate_stack_slots_for_args):
	New function.
	(TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS): Define.

gcc/testsuite/

2008-05-22  Paul Brook  <paul@codesourcery.com>
	    Carlos O'Donell  <carlos@codesourcery.com>

	* gcc.target/arm/naked-1.c: New test.
	* gcc.target/arm/naked-2.c: New test.


[-- Attachment #2: 2008-05-22-naked.diff --]
[-- Type: text/x-diff, Size: 6664 bytes --]

Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 135776)
+++ gcc/doc/extend.texi	(working copy)
@@ -2512,7 +2512,13 @@ defined by shared libraries.
 @cindex function without a prologue/epilogue code
 Use this attribute on the ARM, AVR, IP2K and SPU ports to indicate that
 the specified function does not need prologue/epilogue sequences generated by
-the compiler.  It is up to the programmer to provide these sequences.
+the compiler.  It is up to the programmer to provide these sequences. The 
+only statements that can be safely included in naked functions are 
+@code{asm} statements that do not have operands.  All other statements,
+including declarations of local variables, @code{if} statements, and so 
+forth, should be avoided.  Naked functions should be used to implement the 
+body of an assembly function, while allowing the compiler to construct
+the requisite function declaration for the assembler.
 
 @item near
 @cindex functions which do not handle memory bank switching on 68HC11/68HC12
Index: gcc/doc/tm.texi
===================================================================
--- gcc/doc/tm.texi	(revision 135776)
+++ gcc/doc/tm.texi	(working copy)
@@ -10465,3 +10465,14 @@ to the functions in @file{libgcc} that p
 call stack unwinding.  It is used in declarations in @file{unwind-generic.h}
 and the associated definitions of those functions.
 @end defmac
+
+@deftypefn {Target Hook} {bool} TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS (void)
+When optimization is disabled, this hook indicates whether or not
+arguments should be allocated to stack slots.  Normally, GCC allocates
+stacks slots for arguments when not optimizing in order to make
+debugging easier.  However, when a function is declared with
+@code{__attribute__((naked))}, there is no stack frame, and the compiler
+cannot safely move arguments from the registers in which they are passed
+to the stack.  Therefore, this hook should return true in general, but
+false for naked functions.  The default implementation always returns true.
+@end deftypefn
Index: gcc/target.h
===================================================================
--- gcc/target.h	(revision 135776)
+++ gcc/target.h	(working copy)
@@ -830,6 +830,11 @@ struct gcc_target
     /* Return an rtx for the argument pointer incoming to the
        current function.  */
     rtx (*internal_arg_pointer) (void);
+
+    /* Return true if all function parameters should be spilled to the
+       stack.  */
+    bool (*allocate_stack_slots_for_args) (void);
+    
   } calls;
 
   /* Return the diagnostic message string if conversion from FROMTYPE
Index: gcc/testsuite/gcc.target/arm/naked-1.c
===================================================================
--- gcc/testsuite/gcc.target/arm/naked-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/naked-1.c	(revision 0)
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O0" } */
+/* Check that function arguments aren't assigned and copied to stack slots
+   in naked functions.  This ususally happens at -O0 (presumably for
+   better debugging), but is highly undesirable if we haven't created
+   a stack frame.  */
+void __attribute__((naked))
+foo(int n)
+{
+  __asm__ volatile ("frob r0\n");
+}
+/* { dg-final { scan-assembler "\tfrob r0" } } */
+/* { dg-final { scan-assembler-not "\tstr" } } */
Index: gcc/testsuite/gcc.target/arm/naked-2.c
===================================================================
--- gcc/testsuite/gcc.target/arm/naked-2.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/naked-2.c	(revision 0)
@@ -0,0 +1,12 @@
+/* Verify that __attribute__((naked)) produces a naked function 
+   that does not use bx to return. Naked functions could be used
+   to implement interrupt routines and must not return using bx.  */
+/* { dg-do compile } */
+/* { dg-options "-O0" } */
+/* Use more arguments than we have argument registers.  */
+int __attribute__((naked)) foo(int a, int b, int c, int d, int e, int f)
+{
+  __asm__ volatile ("@ naked");
+}
+/* { dg-final { scan-assembler "\t@ naked" } } */
+/* { dg-final { scan-assembler-not "\tbx\tlr" } } */
Index: gcc/function.c
===================================================================
--- gcc/function.c	(revision 135776)
+++ gcc/function.c	(working copy)
@@ -1776,6 +1776,9 @@ aggregate_value_p (const_tree exp, const
 bool
 use_register_for_decl (const_tree decl)
 {
+  if (!targetm.calls.allocate_stack_slots_for_args())
+    return true;
+  
   /* Honor volatile.  */
   if (TREE_SIDE_EFFECTS (decl))
     return false;
Index: gcc/target-def.h
===================================================================
--- gcc/target-def.h	(revision 135776)
+++ gcc/target-def.h	(working copy)
@@ -568,6 +568,7 @@
 
 #define TARGET_FUNCTION_VALUE default_function_value
 #define TARGET_INTERNAL_ARG_POINTER default_internal_arg_pointer
+#define TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS hook_bool_void_true
 
 #define TARGET_CALLS {						\
    TARGET_PROMOTE_FUNCTION_ARGS,				\
@@ -587,7 +588,8 @@
    TARGET_ARG_PARTIAL_BYTES,					\
    TARGET_INVALID_ARG_FOR_UNPROTOTYPED_FN,			\
    TARGET_FUNCTION_VALUE,					\
-   TARGET_INTERNAL_ARG_POINTER					\
+   TARGET_INTERNAL_ARG_POINTER,					\
+   TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS				\
    }
 
 #ifndef TARGET_UNWIND_TABLES_DEFAULT
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 135776)
+++ gcc/config/arm/arm.c	(working copy)
@@ -189,6 +189,7 @@ static bool arm_cannot_copy_insn_p (rtx)
 static bool arm_tls_symbol_p (rtx x);
 static int arm_issue_rate (void);
 static void arm_output_dwarf_dtprel (FILE *, int, rtx) ATTRIBUTE_UNUSED;
+static bool arm_allocate_stack_slots_for_args (void);
 
 \f
 /* Initialize the GCC target structure.  */
@@ -289,6 +290,9 @@ static void arm_output_dwarf_dtprel (FIL
 #undef  TARGET_SETUP_INCOMING_VARARGS
 #define TARGET_SETUP_INCOMING_VARARGS arm_setup_incoming_varargs
 
+#undef TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS
+#define TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS arm_allocate_stack_slots_for_args
+
 #undef TARGET_DEFAULT_SHORT_ENUMS
 #define TARGET_DEFAULT_SHORT_ENUMS arm_default_short_enums
 
@@ -1619,6 +1623,14 @@ arm_current_func_type (void)
 
   return cfun->machine->func_type;
 }
+
+bool
+arm_allocate_stack_slots_for_args (void)
+{
+  /* Naked functions should not allocate stack slots for arguments.  */
+  return !IS_NAKED (arm_current_func_type ());
+}
+
 \f
 /* Return 1 if it is possible to return using a single instruction.
    If SIBLING is non-null, this is a test for a return before a sibling

^ permalink raw reply	[flat|nested] 15+ messages in thread
* [patch] Stack corruption in naked functions
@ 2007-11-08 17:14 Paul Brook
  2007-11-12  3:27 ` Mark Mitchell
  0 siblings, 1 reply; 15+ messages in thread
From: Paul Brook @ 2007-11-08 17:14 UTC (permalink / raw)
  To: gcc-patches

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

The patch below fixed a stack corruption bug in __attribute__((naked)) 
functions. On supported targets this attribute can be used to suppress the 
normal function prologue/epilogue. This allows a function to be implemented 
in assembly without requiring the user to put everything in a separate .S 
file.

The problem is that at -O0 the compiler assigns all decls to a local stack 
slot and the value will be copied to this slot even if not used. This is 
undesirable in naked function because we don't allocate a stack frame.

The best solution we could come up with is to suppress stack slot allocation 
for these functions.

I can't find evidence that this is a regression, however it does make this 
attribute effectively useless.

I've also updated the user documentation to clarify the intended use of 
__attribute__((naked)).

Tested on arm-nopne-eabi.
Ok?

Paul

2007-11-08  Paul Brook  <paul@codesourcery.com>

	gcc/
	* doc/extend.texi: Clarify use of __attribute__((naked)).
	* doc/tm.texi: Document TARGET_USE_REG_FOR_FUNC.
	* target.h (gcc_target): Add use_reg_for_func.
	* function.c (use_register_for_decl): Use
	targetm.calls.use_reg_for_func.
	* target-def.h (TARGET_CALLS): Add TARGET_USE_REG_FOR_FUNC.
	* config/arm/arm.c (arm_use_reg_for_func): New function.
	(TARGET_USE_REG_FOR_FUNC): Define.

	gcc/testsuite/
	* gcc.target/arm/naked-1.c: New test.


[-- Attachment #2: patch.naked_reg --]
[-- Type: text/x-diff, Size: 5671 bytes --]

Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 129981)
+++ gcc/doc/extend.texi	(working copy)
@@ -2477,7 +2477,13 @@ defined by shared libraries.
 @cindex function without a prologue/epilogue code
 Use this attribute on the ARM, AVR, C4x, IP2K and SPU ports to indicate that
 the specified function does not need prologue/epilogue sequences generated by
-the compiler.  It is up to the programmer to provide these sequences.
+the compiler.  It is up to the programmer to provide these sequences.  The
+only statements that can be safely included in naked functions are 
+@code{asm} statements that do not have operands.  All other statements,
+including declarations of local variables, @code{if} statements, and so 
+forth, should be avoided.  Naked functions should be used to implement the 
+body of an assembly function, while allowing the compiler to construct
+the requisite function declaration for the assembler.
 
 @item near
 @cindex functions which do not handle memory bank switching on 68HC11/68HC12
Index: gcc/doc/tm.texi
===================================================================
--- gcc/doc/tm.texi	(revision 129981)
+++ gcc/doc/tm.texi	(working copy)
@@ -4297,6 +4297,18 @@ or 3-byte structure is returned at the m
 @code{SImode} rtx.
 @end deftypefn
 
+@deftypefn {Target Hook} bool TARGET_USE_REG_FOR_FUNC (void)
+When optimization is disabled most decls (including function arguments) 
+will be allocated local stack slots.
+
+This hook should return true if they should be allocated pseudo registers
+for the current function.  Typically this is desirable for functions
+annotated with @code{__attribute__((naked))}.  This avoids generating
+stores to a non-existant stack frame.
+
+Under normal curcumstances this hook should return false.
+@end deftypefn
+
 @node Aggregate Return
 @subsection How Large Values Are Returned
 @cindex aggregates as return values
Index: gcc/target.h
===================================================================
--- gcc/target.h	(revision 129981)
+++ gcc/target.h	(working copy)
@@ -827,6 +827,11 @@ struct gcc_target
     /* Return an rtx for the argument pointer incoming to the
        current function.  */
     rtx (*internal_arg_pointer) (void);
+
+    /* Return true if all function parameters should remain in registers,
+       rather than being spilled to the stack.  */
+    bool (*use_reg_for_func) (void);
+    
   } calls;
 
   /* Return the diagnostic message string if conversion from FROMTYPE
Index: gcc/testsuite/gcc.target/arm/naked-1.c
===================================================================
--- gcc/testsuite/gcc.target/arm/naked-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/naked-1.c	(revision 0)
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* Check that function arguments aren't assigned and copied to stack slots
+   in naked functions.  This ususally happens at -O0 (presumably for
+   better debugging), but is highly undesirable if we haven't created
+   a stack frame.  */
+void __attribute__((naked))
+foo(int n)
+{
+  __asm__ volatile ("frob r0\n");
+}
+
+/* { dg-final { scan-assembler-not "\tstr" } } */
Index: gcc/function.c
===================================================================
--- gcc/function.c	(revision 129981)
+++ gcc/function.c	(working copy)
@@ -1850,7 +1850,8 @@ use_register_for_decl (const_tree decl)
   if (DECL_IGNORED_P (decl))
     return true;
 
-  return (optimize || DECL_REGISTER (decl));
+  return (optimize || DECL_REGISTER (decl)
+	  || targetm.calls.use_reg_for_func ());
 }
 
 /* Return true if TYPE should be passed by invisible reference.  */
Index: gcc/target-def.h
===================================================================
--- gcc/target-def.h	(revision 129981)
+++ gcc/target-def.h	(working copy)
@@ -565,6 +565,7 @@
 
 #define TARGET_FUNCTION_VALUE default_function_value
 #define TARGET_INTERNAL_ARG_POINTER default_internal_arg_pointer
+#define TARGET_USE_REG_FOR_FUNC hook_bool_void_false
 
 #define TARGET_CALLS {						\
    TARGET_PROMOTE_FUNCTION_ARGS,				\
@@ -584,7 +585,8 @@
    TARGET_ARG_PARTIAL_BYTES,					\
    TARGET_INVALID_ARG_FOR_UNPROTOTYPED_FN,			\
    TARGET_FUNCTION_VALUE,					\
-   TARGET_INTERNAL_ARG_POINTER					\
+   TARGET_INTERNAL_ARG_POINTER,					\
+   TARGET_USE_REG_FOR_FUNC					\
    }
 
 #ifndef TARGET_UNWIND_TABLES_DEFAULT
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 129981)
+++ gcc/config/arm/arm.c	(working copy)
@@ -189,6 +189,7 @@ static unsigned HOST_WIDE_INT arm_shift_
 static bool arm_cannot_copy_insn_p (rtx);
 static bool arm_tls_symbol_p (rtx x);
 static void arm_output_dwarf_dtprel (FILE *, int, rtx) ATTRIBUTE_UNUSED;
+static bool arm_use_reg_for_func (void);
 
 \f
 /* Initialize the GCC target structure.  */
@@ -289,6 +290,9 @@ static void arm_output_dwarf_dtprel (FIL
 #undef  TARGET_SETUP_INCOMING_VARARGS
 #define TARGET_SETUP_INCOMING_VARARGS arm_setup_incoming_varargs
 
+#undef TARGET_USE_REG_FOR_FUNC
+#define TARGET_USE_REG_FOR_FUNC arm_use_reg_for_func
+
 #undef TARGET_DEFAULT_SHORT_ENUMS
 #define TARGET_DEFAULT_SHORT_ENUMS arm_default_short_enums
 
@@ -1614,6 +1618,15 @@ arm_current_func_type (void)
 
   return cfun->machine->func_type;
 }
+
+bool
+arm_use_reg_for_func (void)
+{
+  /* Never spill function parameters to the stack if the function
+     is naked.  */
+  return IS_NAKED (arm_current_func_type ());
+}
+
 \f
 /* Return 1 if it is possible to return using a single instruction.
    If SIBLING is non-null, this is a test for a return before a sibling

^ permalink raw reply	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2008-05-23 20:41 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-05-23  2:28 [PATCH] Stack corruption in naked functions Carlos O'Donell
2008-05-23 10:40 ` Richard Guenther
2008-05-23 13:27   ` Carlos O'Donell
2008-05-23 13:30     ` Richard Guenther
2008-05-23 13:31   ` Paul Brook
2008-05-23 13:48     ` Richard Guenther
2008-05-23 14:34       ` Paul Brook
2008-05-23 19:04         ` Mark Mitchell
2008-05-23 19:55           ` Richard Guenther
2008-05-23 20:23             ` Mark Mitchell
2008-05-23 20:28               ` Richard Guenther
2008-05-23 20:41                 ` Mark Mitchell
2008-05-23 20:59                   ` Carlos O'Donell
  -- strict thread matches above, loose matches on Subject: below --
2007-11-08 17:14 [patch] " Paul Brook
2007-11-12  3:27 ` Mark Mitchell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).