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
_______________________________________________
AVR-GCC-list mailing list
AVR-GCC-list@nongnu.org
https://lists.nongnu.org/mailman/listinfo/avr-gcc-list
