Re: [avr-gcc-list] Potential stack corruption in naked functions at -O0

[Senthil Kumar Selvaraj]

On Tue, Aug 07, 2012 at 07:41:04PM +0200, Georg-Johann Lay wrote:
> Senthil Kumar Selvaraj schrieb:
> >Hi,
> >
> >When tracking down a different (but related) bug, I noticed that,
> >at -O0, code to copy function parameters from registers to the
> >stack is generated at the start of the function. This is done even
> >for naked functions (__attribute__((naked))), which I guess is
> >wrong, as
> >those functions don't have a prologue generated to setup the stack frame.
> 
> The you request to skip prologue/epilogue generation, the compiler will
> skip them, of course.  The code generated for a naked function will be
> the same as for a non-naked function except for pro- and epilogue
> generation, e.g. for tail calls, and TARGET_CANNOT_MODIFY_JUMPS_P.
> 
You're right, and that's what makes the stack copy instructions bad.

> >Note the stores to Y+2 and Y+1 in the example below.
> >
> >[scratch]$ cat test.c
> >void __attribute__((naked)) func(int x)
> >{
> >    __asm volatile ("ret");
> >}
> >[scratch]$ avr-gcc -O0 -S test.c
> >[scratch]$ cat test.s
> >        .file   "test.c"
> >__SREG__ = 0x3f
> >__SP_H__ = 0x3e
> >__SP_L__ = 0x3d
> >__CCP__ = 0x34
> >__tmp_reg__ = 0
> >__zero_reg__ = 1
> >        .global __do_copy_data
> >        .global __do_clear_bss
> >        .text
> >.global func
> >        .type   func, @function
> >func:
> >/* prologue: naked */
> >/* frame size = 2 */
> >/* stack size = 0 */
> >.L__stack_usage = 0
> >        std Y+2,r25
> >        std Y+1,r24
> >/* #APP */
> > ;  3 "test.c" 1
> >        ret
> > ;  0 "" 2
> >/* epilogue start */
> >/* #NOAPP */
> >        .size   func, .-func
> 
> It's not save to use naked with -O0.  naked is a "you must know what you
> are doing" feature.

Shouldn't features work correctly regardless of optimization? Sure,
naked assumes the developer knows what he/she is doing, but generating
wrong code?

> 
> Implementing TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS will improve the
> situation, but as soon as you add some more instructions to the function
> the frame will be used again.  That's what you request per -O0.

Yes, I understand that any C code will most likely cause stack frame
references, but now it's broken for the case it is supposed to work.
> 
> A save use of naked is for functions that only contain inline assembler
> with no arguments; for more complex code there is no guarantee that no
> frame is needed/used.
> 
> If you actually need that feature, you might are better of with
> attribute constructor to call a function before main.
> 
> And I am wondering why a naked function gets a parameter.
> naked is typically used for code in .initN/.finiN or short ISR
> completely in inline assembler.

How about trampolines for functions with arguments? The naked function
could simply do an rjmp/jmp to the actual function.
> 
> >I looked at what other targets have done and created a patch (pasted below). 
> >Should
> >I file a bug first?
> 
> A bug report is nice so that wjen other users hit the problem, they can
> be pointed to the PR and read more about the issue and see if and for
> what version the problem is fixed.  And a source comment can refer to
> it.

Sure, will do it.

> 
> >diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c
> >index e0d2e82..418e4d5 100644
> >--- a/gcc/config/avr/avr.c
> >+++ b/gcc/config/avr/avr.c
> >@@ -2489,6 +2489,13 @@ avr_function_arg_advance (cumulative_args_t cum_v, 
> >enum machine_mode mode,
> >     }
> > }
> >+
> >+static bool
> >+avr_allocate_stack_slots_for_args(void)
> >+{
> >+  return !cfun->machine->is_naked;
> >+}
> >+
> 
> Shouldn't there be a diagnose for the case when a naked touches a
> frame?

You mean when avr_allocate_stack_slots_for_args is called, or in
general? The target hook is called if the function has parameters,
regardless of whether they are actually accessed, so I guess that's not
the right place to emit a diagnostic.
> 
> > /* Implement `TARGET_FUNCTION_OK_FOR_SIBCALL' */
> > /* Decide whether we can make a sibling call to a function.  DECL is the
> >    declaration of the function being targeted by the call and EXP is the
> >@@ -10778,6 +10785,8 @@ avr_fold_builtin (tree fndecl, int n_args 
> >ATTRIBUTE_UNUSED, tree *arg,
> > #define TARGET_FUNCTION_ARG avr_function_arg
> > #undef  TARGET_FUNCTION_ARG_ADVANCE
> > #define TARGET_FUNCTION_ARG_ADVANCE avr_function_arg_advance
> >+#undef  TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS
> >+#define TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS 
> >avr_allocate_stack_slots_for_args
> > #undef  TARGET_SET_CURRENT_FUNCTION
> > #define TARGET_SET_CURRENT_FUNCTION avr_set_current_function
> >diff --git a/gcc/testsuite/gcc.target/avr/naked-nostackpush.c 
> >b/gcc/testsuite/gcc.target/avr/naked-nostackpush.c
> >new file mode 100644
> >index 0000000..c9c8865
> >--- /dev/null
> >+++ b/gcc/testsuite/gcc.target/avr/naked-nostackpush.c
> >@@ -0,0 +1,15 @@
> >+/* { dg-do compile } */
> >+/* { dg-options "-O0 -g3" } */
> >+
> >+/* This testcase verifies that compilation with O0
> >+   for naked functions does not generate code for
> >+   copying arguments into the stack at the beginning
> >+   of the function.
> >+*/
> >+
> >+void __attribute__((naked)) func(int code) +{
> >+    __asm volatile ("ret");
> >+}
> >+
> >+/* { dg-final { scan-assembler-not "\tstd" } } */
> 
> std appears a bit restrictive. You could add -dP and scan against
> "(set (mem" for example or "frame size = [1-9]".

Will do.

> 
> Why does the test case need -g3?

It doesn't, will remove it. It was needed for the other bug I was
talking about and I forgot to remove it.
> 
> If there is a PR, the testcase could have the PR number in the
> file name, and the first line of the test refers to the PR.
> 

Sure, will do.

> Johann
> 

Regards
Senthil