public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [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

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

Paul Brook wrote:

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

The TARGET_USE_REG_FOR_FUNC name is confusing because, in part, we don't
pass in a function.  Let's just say
TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS.  (That also inverts the return value.)

Then, for documentation:

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

OK with that change,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  2008-05-23 20:41                 ` Mark Mitchell
@ 2008-05-23 20:59                   ` Carlos O'Donell
  0 siblings, 0 replies; 15+ messages in thread
From: Carlos O'Donell @ 2008-05-23 20:59 UTC (permalink / raw)
  To: Mark Mitchell; +Cc: Richard Guenther, Paul Brook, gcc-patches

Mark Mitchell wrote:
> Of course, if you do something inside the naked function that causes an 
> actual need to spill the registers to the stack (like, say, pass them to 
> another function), then you've written invalid code.  The only thing you 
> can really do reliably in a naked function is insert assembly code. 
> (Whether we appropriately diagnose all the invalid things you can do is 
> the other topic of your emails, and a good discussion, but orthgonal.)
> 
> Carlos, please go ahead and commit your patch.

Checked in. Thanks.

I also searched the bugzilla for any outsanding but related bugs, but 
there were none.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  2008-05-23 20:28               ` Richard Guenther
@ 2008-05-23 20:41                 ` Mark Mitchell
  2008-05-23 20:59                   ` Carlos O'Donell
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Mitchell @ 2008-05-23 20:41 UTC (permalink / raw)
  To: Richard Guenther; +Cc: Paul Brook, gcc-patches, Carlos O'Donell

Richard Guenther wrote:

>> Carlos' patch keeps the compiler itself from copying arguments in registers
>> to the stack for naked functions.  That doesn't sound like papering over to
>> me; it sounds like an essential fix.
>>
>> How does Honza's patch relate to this at all?  Even if doing some
>> optimizations would eliminate the copies, etc., depending on the optimizers
>> to avoid using the stack seems fragile.
> 
> I thought the failure mode is that with -O0 we allocate stack slots for
> incoming parameters (why? to eventually spill them or are they passed
> via the stack?) and also access them, even as in the testcases they are
> not used.  But I guess in the "real" testcases the arguments are used?

Sometimes they are used; sometimes not.  The compiler tries to put them 
on the stack at -O0 to make debugging easier; it's not because of any 
actual code-generation requirement.  The whole issue of variables moving 
around during the body of a function, and how to cope with that when 
debugging, is of course another topic of extensive recent discussion. 
It's possible that at -O0 we could eliminate the copy to the stack -- 
but that would be a much more dramatic change, and something to do only 
after the variable-tracking stuff has been resolved.

Of course, if you do something inside the naked function that causes an 
actual need to spill the registers to the stack (like, say, pass them to 
another function), then you've written invalid code.  The only thing you 
can really do reliably in a naked function is insert assembly code. 
(Whether we appropriately diagnose all the invalid things you can do is 
the other topic of your emails, and a good discussion, but orthgonal.)

Carlos, please go ahead and commit your patch.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  2008-05-23 20:23             ` Mark Mitchell
@ 2008-05-23 20:28               ` Richard Guenther
  2008-05-23 20:41                 ` Mark Mitchell
  0 siblings, 1 reply; 15+ messages in thread
From: Richard Guenther @ 2008-05-23 20:28 UTC (permalink / raw)
  To: Mark Mitchell; +Cc: Paul Brook, gcc-patches, Carlos O'Donell

On Fri, May 23, 2008 at 9:18 PM, Mark Mitchell <mark@codesourcery.com> wrote:
> Richard Guenther wrote:
>
>> I merely wanted to raise the concern that user errors will be noticed as
>> ICEs
>> and not errors, which will lead to false bug-reports.  It would be nice if
>> we
>> can emit an error, even if we cannot make sure to not ICE afterwards -
>> that
>> will be a confused after errors, which is way better than an ICE with an
>> instruction to report a bug.
>
> How does this relate to Carlos' patch?
>
> Carlos' patch keeps the compiler itself from copying arguments in registers
> to the stack for naked functions.  That doesn't sound like papering over to
> me; it sounds like an essential fix.
>
> How does Honza's patch relate to this at all?  Even if doing some
> optimizations would eliminate the copies, etc., depending on the optimizers
> to avoid using the stack seems fragile.

I thought the failure mode is that with -O0 we allocate stack slots for
incoming parameters (why? to eventually spill them or are they passed
via the stack?) and also access them, even as in the testcases they are
not used.  But I guess in the "real" testcases the arguments are used?

So the question is what does fix this with -O1?  Why can't we leverage
this same mechanism with -O0?  That is, what happens if we just change
use_register_for_decl to finish returning DECL_REGISTER unconditional
on optimize?  Or just unconditional on optimize if this is a PARM_DECL?

I guess I'm just too much curious, so I don't want to block this patch just
because of my lack of understanding on this matter ;)

Richard.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  2008-05-23 19:55           ` Richard Guenther
@ 2008-05-23 20:23             ` Mark Mitchell
  2008-05-23 20:28               ` Richard Guenther
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Mitchell @ 2008-05-23 20:23 UTC (permalink / raw)
  To: Richard Guenther; +Cc: Paul Brook, gcc-patches, Carlos O'Donell

Richard Guenther wrote:

> I merely wanted to raise the concern that user errors will be noticed as ICEs
> and not errors, which will lead to false bug-reports.  It would be nice if we
> can emit an error, even if we cannot make sure to not ICE afterwards - that
> will be a confused after errors, which is way better than an ICE with an
> instruction to report a bug.

How does this relate to Carlos' patch?

Carlos' patch keeps the compiler itself from copying arguments in 
registers to the stack for naked functions.  That doesn't sound like 
papering over to me; it sounds like an essential fix.

How does Honza's patch relate to this at all?  Even if doing some 
optimizations would eliminate the copies, etc., depending on the 
optimizers to avoid using the stack seems fragile.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  2008-05-23 19:04         ` Mark Mitchell
@ 2008-05-23 19:55           ` Richard Guenther
  2008-05-23 20:23             ` Mark Mitchell
  0 siblings, 1 reply; 15+ messages in thread
From: Richard Guenther @ 2008-05-23 19:55 UTC (permalink / raw)
  To: Mark Mitchell; +Cc: Paul Brook, gcc-patches, Carlos O'Donell

On Fri, May 23, 2008 at 8:37 PM, Mark Mitchell <mark@codesourcery.com> wrote:
> Paul Brook wrote:
>
>> If the stack slots result from code not permitted by the documentation
>> then this is user error.  Some not-permitted code may happen to "work" at
>> some optimisation levels with some compilers. However when this stops
>> working (e.g. at -O0) it is IMHO not defect in the compiler, it is user
>> error.  If anything the bug is that we allowed the code in the first place.
>
> I agree.
>
> As I see it, Carlos' patch makes things monotonically better.  It makes a
> valid test case work, and it causes no new failures.  Richard's comment is
> about what might happen with invalid code, but Carlos' patch is about valid
> code.  We could perhaps do better to catch more errors in the front ends,
> but that's an orthogonal issue.
>
> Richard, are you objecting to the patch?

I merely wanted to raise the concern that user errors will be noticed as ICEs
and not errors, which will lead to false bug-reports.  It would be nice if we
can emit an error, even if we cannot make sure to not ICE afterwards - that
will be a confused after errors, which is way better than an ICE with an
instruction to report a bug.

It's not clear to me if going this route has been evaluated?  IMHO we
don't need to rush in this change at this point before this was done.
The change itself looks to be papering over the problem - Carlos, can
you try Honzas patch for going into SSA at -O0 and look at what optimizations
you would need to make the valid testcases also work with -O0?

Thanks,
Richard.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  2008-05-23 14:34       ` Paul Brook
@ 2008-05-23 19:04         ` Mark Mitchell
  2008-05-23 19:55           ` Richard Guenther
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Mitchell @ 2008-05-23 19:04 UTC (permalink / raw)
  To: Paul Brook; +Cc: Richard Guenther, gcc-patches, Carlos O'Donell

Paul Brook wrote:

> If the stack slots result from code not permitted by the documentation then 
> this is user error.  Some not-permitted code may happen to "work" at some 
> optimisation levels with some compilers. However when this stops working 
> (e.g. at -O0) it is IMHO not defect in the compiler, it is user error.  If 
> anything the bug is that we allowed the code in the first place.

I agree.

As I see it, Carlos' patch makes things monotonically better.  It makes 
a valid test case work, and it causes no new failures.  Richard's 
comment is about what might happen with invalid code, but Carlos' patch 
is about valid code.  We could perhaps do better to catch more errors in 
the front ends, but that's an orthogonal issue.

Richard, are you objecting to the patch?

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  2008-05-23 13:48     ` Richard Guenther
@ 2008-05-23 14:34       ` Paul Brook
  2008-05-23 19:04         ` Mark Mitchell
  0 siblings, 1 reply; 15+ messages in thread
From: Paul Brook @ 2008-05-23 14:34 UTC (permalink / raw)
  To: Richard Guenther; +Cc: gcc-patches, Carlos O'Donell, Mark Mitchell

On Friday 23 May 2008, Richard Guenther wrote:
> On Fri, May 23, 2008 at 3:23 PM, Paul Brook <paul@codesourcery.com> wrote:
> >> I wonder if you start to get ICEs all over the place if you use naked
> >> on a function with addressable local variables or with BLKmode
> >> parameters?  IMHO it would be better to sorry () as soon as a
> >> stack slot is allocated for such a function.
> >
> > I'd make it a proper error.  sorry() is for things that should work, but
> > we haven't got round to implementing.
> > In this case the documentation explicitly says this won't work. Any stack
> > slots are either user error or a compiler bug (like the one we're
> > fixing).
>
> But if it only doesn't work because at -O0 we're not optimizing then this
> case is a sorry () IMHO - of course we probably cannot easily distinguish
> our from the user errors here, so I won't mind using error either.

What are you referring to when you say "it doesn't work"?

The only supported use is that mentioned in the documentation. If that results 
in stack slots then this is an internal compiler error.  

If the stack slots result from code not permitted by the documentation then 
this is user error.  Some not-permitted code may happen to "work" at some 
optimisation levels with some compilers. However when this stops working 
(e.g. at -O0) it is IMHO not defect in the compiler, it is user error.  If 
anything the bug is that we allowed the code in the first place.

Paul

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  2008-05-23 13:31   ` Paul Brook
@ 2008-05-23 13:48     ` Richard Guenther
  2008-05-23 14:34       ` Paul Brook
  0 siblings, 1 reply; 15+ messages in thread
From: Richard Guenther @ 2008-05-23 13:48 UTC (permalink / raw)
  To: Paul Brook; +Cc: gcc-patches, Carlos O'Donell, Mark Mitchell

On Fri, May 23, 2008 at 3:23 PM, Paul Brook <paul@codesourcery.com> wrote:
>> I wonder if you start to get ICEs all over the place if you use naked
>> on a function with addressable local variables or with BLKmode
>> parameters?  IMHO it would be better to sorry () as soon as a
>> stack slot is allocated for such a function.
>
> I'd make it a proper error.  sorry() is for things that should work, but we
> haven't got round to implementing.
> In this case the documentation explicitly says this won't work. Any stack
> slots are either user error or a compiler bug (like the one we're fixing).

But if it only doesn't work because at -O0 we're not optimizing then this
case is a sorry () IMHO - of course we probably cannot easily distinguish
our from the user errors here, so I won't mind using error either.

Richard.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  2008-05-23 10:40 ` Richard Guenther
  2008-05-23 13:27   ` Carlos O'Donell
@ 2008-05-23 13:31   ` Paul Brook
  2008-05-23 13:48     ` Richard Guenther
  1 sibling, 1 reply; 15+ messages in thread
From: Paul Brook @ 2008-05-23 13:31 UTC (permalink / raw)
  To: gcc-patches; +Cc: Richard Guenther, Carlos O'Donell, Mark Mitchell

> I wonder if you start to get ICEs all over the place if you use naked
> on a function with addressable local variables or with BLKmode
> parameters?  IMHO it would be better to sorry () as soon as a
> stack slot is allocated for such a function.

I'd make it a proper error.  sorry() is for things that should work, but we 
haven't got round to implementing.
In this case the documentation explicitly says this won't work. Any stack 
slots are either user error or a compiler bug (like the one we're fixing).

Paul

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  2008-05-23 13:27   ` Carlos O'Donell
@ 2008-05-23 13:30     ` Richard Guenther
  0 siblings, 0 replies; 15+ messages in thread
From: Richard Guenther @ 2008-05-23 13:30 UTC (permalink / raw)
  To: Carlos O'Donell; +Cc: Mark Mitchell, gcc-patches, Paul Brook

On Fri, May 23, 2008 at 3:10 PM, Carlos O'Donell
<carlos@codesourcery.com> wrote:
> Richard Guenther wrote:
>>>
>>> Tested on arm-none-eabi and i686-pc-linux-gnu.
>>>
>>> OK to checkin to mainline?
>>
>> I wonder if you start to get ICEs all over the place if you use naked
>> on a function with addressable local variables or with BLKmode
>> parameters?  IMHO it would be better to sorry () as soon as a
>> stack slot is allocated for such a function.
>
> You will ICE in expand_expr_addr_expr_1 if you start creating addressable
> local variables, but the documentation for the naked attribute told you not
> to do that?
>
> While working on this fix it wasn't clear to me that there was an easy way
> to sorry() if the user created addressable local variables *and* used the
> naked attribute.

I don't know if there is an easy way - but IMHO opening the door to
simply emit diagnostics via ICEs should be only the last resort ...

Richard.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  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
  1 sibling, 1 reply; 15+ messages in thread
From: Carlos O'Donell @ 2008-05-23 13:27 UTC (permalink / raw)
  To: Richard Guenther; +Cc: Mark Mitchell, gcc-patches, Paul Brook

Richard Guenther wrote:
>> Tested on arm-none-eabi and i686-pc-linux-gnu.
>>
>> OK to checkin to mainline?
> 
> I wonder if you start to get ICEs all over the place if you use naked
> on a function with addressable local variables or with BLKmode
> parameters?  IMHO it would be better to sorry () as soon as a
> stack slot is allocated for such a function.

You will ICE in expand_expr_addr_expr_1 if you start creating 
addressable local variables, but the documentation for the naked 
attribute told you not to do that?

While working on this fix it wasn't clear to me that there was an easy 
way to sorry() if the user created addressable local variables *and* 
used the naked attribute.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Stack corruption in naked functions.
  2008-05-23  2:28 [PATCH] " Carlos O'Donell
@ 2008-05-23 10:40 ` Richard Guenther
  2008-05-23 13:27   ` Carlos O'Donell
  2008-05-23 13:31   ` Paul Brook
  0 siblings, 2 replies; 15+ messages in thread
From: Richard Guenther @ 2008-05-23 10:40 UTC (permalink / raw)
  To: Carlos O'Donell; +Cc: Mark Mitchell, gcc-patches, Paul Brook

On Fri, May 23, 2008 at 3:18 AM, Carlos O'Donell
<carlos@codesourcery.com> wrote:
> 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?

I wonder if you start to get ICEs all over the place if you use naked
on a function with addressable local variables or with BLKmode
parameters?  IMHO it would be better to sorry () as soon as a
stack slot is allocated for such a function.

Richard.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [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

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 --
2007-11-08 17:14 [patch] Stack corruption in naked functions Paul Brook
2007-11-12  3:27 ` Mark Mitchell
2008-05-23  2:28 [PATCH] " 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

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