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

[Georg-Johann Lay]

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.

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

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.

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.

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

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

>  /* 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]".

Why does the test case need -g3?

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.

Johann