diff mbox

[AVR,trunk,4.7] : Implement PR53256

Message ID 4FA80CF4.3030708@gjlay.de
State New
Headers show

Commit Message

Georg-Johann Lay May 7, 2012, 5:57 p.m. UTC
AVR-LibC switched from using either signal /or/ interrupt function
attribute to using both at the same time.

This was never documented or implemented but worked accidentally for
some time, but results in wrong code for 4.7+

This patch adds better documentation of these attributes and makes
'interrupt' silently override 'signal'.

Besides that, some more sanity checking is done for function attributes.

ASM_DECLARE_FUNCTION_NAME just served to check isr names.
All the checking is done in the new hook TARGET_SET_CURRENT_FUNCTION
now so that ASM_DECLARE_FUNCTION_NAME from defaults.h can be used,
thus some clean-up in elf.h

Ok for trunk and 4.7?

Johann

	PR target/53256
	* config/avr/elf.h (ASM_DECLARE_FUNCTION_NAME): Remove.
	* config/avr/avr-protos.h (avr_asm_declare_function_name): Remove.
	* config/avr/avr.h (struct machine_function): Add attributes_checked_p.
	* config/avr/avr.c (avr_asm_declare_function_name): Remove.
	(expand_prologue): Move initialization of cfun->machine->is_naked,
	is_interrupt, is_signal, is_OS_task, is_OS_main from here to...
	(avr_regs_to_save): Ditto.
	(avr_set_current_function): ...this new static function.
	(TARGET_SET_CURRENT_FUNCTION): New define.
	(avr_function_ok_for_sibcall): Use cfun->machine->is_* instead of
	checking attributes of current_function_decl.
	(signal_function_p): Rename to avr_signal_function_p.
	(interrupt_function_p): Rename to avr_interrupt_function_p.

	* doc/extend.texi (Function Attributes): Better explanation of
	'interrupt' and 'signal for AVR. Move 'ifunc' down for
	alphabetical order.

Comments

Denis Chertykov May 9, 2012, 3:37 p.m. UTC | #1
2012/5/7 Georg-Johann Lay <avr@gjlay.de>:
> AVR-LibC switched from using either signal /or/ interrupt function
> attribute to using both at the same time.
>
> This was never documented or implemented but worked accidentally for
> some time, but results in wrong code for 4.7+
>
> This patch adds better documentation of these attributes and makes
> 'interrupt' silently override 'signal'.
>
> Besides that, some more sanity checking is done for function attributes.
>
> ASM_DECLARE_FUNCTION_NAME just served to check isr names.
> All the checking is done in the new hook TARGET_SET_CURRENT_FUNCTION
> now so that ASM_DECLARE_FUNCTION_NAME from defaults.h can be used,
> thus some clean-up in elf.h
>
> Ok for trunk and 4.7?
>
> Johann
>
>        PR target/53256
>        * config/avr/elf.h (ASM_DECLARE_FUNCTION_NAME): Remove.
>        * config/avr/avr-protos.h (avr_asm_declare_function_name): Remove.
>        * config/avr/avr.h (struct machine_function): Add attributes_checked_p.
>        * config/avr/avr.c (avr_asm_declare_function_name): Remove.
>        (expand_prologue): Move initialization of cfun->machine->is_naked,
>        is_interrupt, is_signal, is_OS_task, is_OS_main from here to...
>        (avr_regs_to_save): Ditto.
>        (avr_set_current_function): ...this new static function.
>        (TARGET_SET_CURRENT_FUNCTION): New define.
>        (avr_function_ok_for_sibcall): Use cfun->machine->is_* instead of
>        checking attributes of current_function_decl.
>        (signal_function_p): Rename to avr_signal_function_p.
>        (interrupt_function_p): Rename to avr_interrupt_function_p.
>
>        * doc/extend.texi (Function Attributes): Better explanation of
>        'interrupt' and 'signal for AVR. Move 'ifunc' down for
>        alphabetical order.
>

Approved.

Denis.
diff mbox

Patch

Index: config/avr/elf.h
===================================================================
--- config/avr/elf.h	(revision 187252)
+++ config/avr/elf.h	(working copy)
@@ -32,11 +32,6 @@ 
 #undef STRING_LIMIT
 #define STRING_LIMIT ((unsigned) 64)
 
-/* Take care of `signal' and `interrupt' attributes.  */
-#undef ASM_DECLARE_FUNCTION_NAME
-#define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)     \
-  avr_asm_declare_function_name ((FILE), (NAME), (DECL))
-
 /* Output alignment 2**1 for jump tables.  */
 #undef ASM_OUTPUT_BEFORE_CASE_LABEL
 #define ASM_OUTPUT_BEFORE_CASE_LABEL(FILE, PREFIX, NUM, TABLE) \
Index: config/avr/avr-protos.h
===================================================================
--- config/avr/avr-protos.h	(revision 187252)
+++ config/avr/avr-protos.h	(working copy)
@@ -26,7 +26,6 @@  extern int function_arg_regno_p (int r);
 extern void avr_cpu_cpp_builtins (struct cpp_reader * pfile);
 extern enum reg_class avr_regno_reg_class (int r);
 extern void asm_globalize_label (FILE *file, const char *name);
-extern void avr_asm_declare_function_name (FILE *, const char *, tree);
 extern void order_regs_for_local_alloc (void);
 extern int avr_initial_elimination_offset (int from, int to);
 extern int avr_simple_epilogue (void);
Index: config/avr/avr.c
===================================================================
--- config/avr/avr.c	(revision 187259)
+++ config/avr/avr.c	(working copy)
@@ -138,12 +138,6 @@  static const char* out_movqi_mr_r (rtx,
 static const char* out_movhi_mr_r (rtx, rtx[], int*);
 static const char* out_movsi_mr_r (rtx, rtx[], int*);
 
-static int avr_naked_function_p (tree);
-static int interrupt_function_p (tree);
-static int signal_function_p (tree);
-static int avr_OS_task_function_p (tree);
-static int avr_OS_main_function_p (tree);
-static int avr_regs_to_save (HARD_REG_SET *);
 static int get_sequence_length (rtx insns);
 static int sequent_regs_live (void);
 static const char *ptrreg_to_str (int);
@@ -491,7 +485,7 @@  avr_naked_function_p (tree func)
    by the "interrupt" attribute.  */
 
 static int
-interrupt_function_p (tree func)
+avr_interrupt_function_p (tree func)
 {
   return avr_lookup_function_attribute1 (func, "interrupt");
 }
@@ -500,7 +494,7 @@  interrupt_function_p (tree func)
    by the "signal" attribute.  */
 
 static int
-signal_function_p (tree func)
+avr_signal_function_p (tree func)
 {
   return avr_lookup_function_attribute1 (func, "signal");
 }
@@ -522,6 +516,80 @@  avr_OS_main_function_p (tree func)
 }
 
 
+/* Implement `TARGET_SET_CURRENT_FUNCTION'.  */
+/* Sanity cheching for above function attributes.  */
+
+static void
+avr_set_current_function (tree decl)
+{
+  location_t loc;
+  const char *isr;
+
+  if (decl == NULL_TREE
+      || current_function_decl == NULL_TREE
+      || current_function_decl == error_mark_node
+      || cfun->machine->attributes_checked_p)
+    return;
+
+  loc = DECL_SOURCE_LOCATION (decl);
+
+  cfun->machine->is_naked = avr_naked_function_p (decl);
+  cfun->machine->is_signal = avr_signal_function_p (decl);
+  cfun->machine->is_interrupt = avr_interrupt_function_p (decl);
+  cfun->machine->is_OS_task = avr_OS_task_function_p (decl);
+  cfun->machine->is_OS_main = avr_OS_main_function_p (decl);
+
+  isr = cfun->machine->is_interrupt ? "interrupt" : "signal";
+
+  /* Too much attributes make no sense as they request conflicting features. */
+
+  if (cfun->machine->is_OS_task + cfun->machine->is_OS_main
+      + (cfun->machine->is_signal || cfun->machine->is_interrupt) > 1)
+    error_at (loc, "function attributes %qs, %qs and %qs are mutually"
+               " exclusive", "OS_task", "OS_main", isr);
+
+  /* 'naked' will hide effects of 'OS_task' and 'OS_main'.  */
+
+  if (cfun->machine->is_naked
+      && (cfun->machine->is_OS_task || cfun->machine->is_OS_main))
+    warning_at (loc, OPT_Wattributes, "function attributes %qs and %qs have"
+                " no effect on %qs function", "OS_task", "OS_main", "naked");
+
+  if (cfun->machine->is_interrupt || cfun->machine->is_signal)
+    {
+      tree args = TYPE_ARG_TYPES (TREE_TYPE (decl));
+      tree ret = TREE_TYPE (TREE_TYPE (decl));
+      const char *name = IDENTIFIER_POINTER (DECL_NAME (decl));
+      
+      /* Silently ignore 'signal' if 'interrupt' is present.  AVR-LibC startet
+         using this when it switched from SIGNAL and INTERRUPT to ISR.  */
+
+      if (cfun->machine->is_interrupt)
+        cfun->machine->is_signal = 0;
+
+      /* Interrupt handlers must be  void __vector (void)  functions.  */
+
+      if (args && TREE_CODE (TREE_VALUE (args)) != VOID_TYPE)
+        error_at (loc, "%qs function cannot have arguments", isr);
+
+      if (TREE_CODE (ret) != VOID_TYPE)
+        error_at (loc, "%qs function cannot return a value", isr);
+
+      /* If the function has the 'signal' or 'interrupt' attribute, ensure
+         that the name of the function is "__vector_NN" so as to catch
+         when the user misspells the vector name.  */
+
+      if (!STR_PREFIX_P (name, "__vector"))
+        warning_at (loc, 0, "%qs appears to be a misspelled %s handler",
+                    name, isr);
+    }
+
+  /* Avoid the above diagnosis to be printed more than once.  */
+  
+  cfun->machine->attributes_checked_p = 1;
+}
+
+
 /* Implement `ACCUMULATE_OUTGOING_ARGS'.  */
 
 int
@@ -570,8 +638,7 @@  static int
 avr_regs_to_save (HARD_REG_SET *set)
 {
   int reg, count;
-  int int_or_sig_p = (interrupt_function_p (current_function_decl)
-                      || signal_function_p (current_function_decl));
+  int int_or_sig_p = cfun->machine->is_interrupt || cfun->machine->is_signal;
 
   if (set)
     CLEAR_HARD_REG_SET (*set);
@@ -683,9 +750,9 @@  avr_simple_epilogue (void)
           && get_frame_size () == 0
           && avr_outgoing_args_size() == 0
           && avr_regs_to_save (NULL) == 0
-          && ! interrupt_function_p (current_function_decl)
-          && ! signal_function_p (current_function_decl)
-          && ! avr_naked_function_p (current_function_decl)
+          && ! cfun->machine->is_interrupt
+          && ! cfun->machine->is_signal
+          && ! cfun->machine->is_naked
           && ! TREE_THIS_VOLATILE (current_function_decl));
 }
 
@@ -1096,12 +1163,6 @@  expand_prologue (void)
 
   size = get_frame_size() + avr_outgoing_args_size();
   
-  /* Init cfun->machine.  */
-  cfun->machine->is_naked = avr_naked_function_p (current_function_decl);
-  cfun->machine->is_interrupt = interrupt_function_p (current_function_decl);
-  cfun->machine->is_signal = signal_function_p (current_function_decl);
-  cfun->machine->is_OS_task = avr_OS_task_function_p (current_function_decl);
-  cfun->machine->is_OS_main = avr_OS_main_function_p (current_function_decl);
   cfun->machine->stack_usage = 0;
   
   /* Prologue: naked.  */
@@ -2458,17 +2519,17 @@  avr_function_ok_for_sibcall (tree decl_c
 
   /* Ensure that caller and callee have compatible epilogues */
   
-  if (interrupt_function_p (current_function_decl)
-      || signal_function_p (current_function_decl)
+  if (cfun->machine->is_interrupt
+      || cfun->machine->is_signal
+      || cfun->machine->is_naked
       || avr_naked_function_p (decl_callee)
-      || avr_naked_function_p (current_function_decl)
       /* FIXME: For OS_task and OS_main, we are over-conservative.
          This is due to missing documentation of these attributes
          and what they actually should do and should not do. */
       || (avr_OS_task_function_p (decl_callee)
-          != avr_OS_task_function_p (current_function_decl))
+          != cfun->machine->is_OS_task)
       || (avr_OS_main_function_p (decl_callee)
-          != avr_OS_main_function_p (current_function_decl)))
+          != cfun->machine->is_OS_main))
     {
       return false;
     }
@@ -6656,40 +6717,6 @@  avr_assemble_integer (rtx x, unsigned in
 }
 
 
-/* Worker function for ASM_DECLARE_FUNCTION_NAME.  */
-
-void
-avr_asm_declare_function_name (FILE *file, const char *name, tree decl)
-{
-
-  /* If the function has the 'signal' or 'interrupt' attribute, test to
-     make sure that the name of the function is "__vector_NN" so as to
-     catch when the user misspells the interrupt vector name.  */
-
-  if (cfun->machine->is_interrupt)
-    {
-      if (!STR_PREFIX_P (name, "__vector"))
-        {
-          warning_at (DECL_SOURCE_LOCATION (decl), 0,
-                      "%qs appears to be a misspelled interrupt handler",
-                      name);
-        }
-    }
-  else if (cfun->machine->is_signal)
-    {
-      if (!STR_PREFIX_P (name, "__vector"))
-        {
-           warning_at (DECL_SOURCE_LOCATION (decl), 0,
-                       "%qs appears to be a misspelled signal handler",
-                       name);
-        }
-    }
-
-  ASM_OUTPUT_TYPE_DIRECTIVE (file, name, "function");
-  ASM_OUTPUT_LABEL (file, name);
-}
-
-
 /* Return value is nonzero if pseudos that have been
    assigned to registers of class CLASS would likely be spilled
    because registers of CLASS are needed for spill registers.  */
@@ -10865,6 +10892,9 @@  avr_fold_builtin (tree fndecl, int n_arg
 #undef  TARGET_FUNCTION_ARG_ADVANCE
 #define TARGET_FUNCTION_ARG_ADVANCE avr_function_arg_advance
 
+#undef  TARGET_SET_CURRENT_FUNCTION
+#define TARGET_SET_CURRENT_FUNCTION avr_set_current_function
+
 #undef  TARGET_RETURN_IN_MEMORY
 #define TARGET_RETURN_IN_MEMORY avr_return_in_memory
 
Index: config/avr/avr.h
===================================================================
--- config/avr/avr.h	(revision 187252)
+++ config/avr/avr.h	(working copy)
@@ -699,6 +699,10 @@  struct GTY(()) machine_function
 
   /* 'true' if a callee might be tail called */
   int sibcall_fails;
+
+  /* 'true' if the above is_foo predicates are sanity-checked to avoid
+     multiple diagnose for the same function.  */
+  int attributes_checked_p;
 };
 
 /* AVR does not round pushes, but the existance of this macro is
Index: doc/extend.texi
===================================================================
--- doc/extend.texi	(revision 187252)
+++ doc/extend.texi	(working copy)
@@ -2714,6 +2714,51 @@  then be sure to write this declaration i
 
 This attribute is ignored for R8C target.
 
+@item ifunc ("@var{resolver}")
+@cindex @code{ifunc} attribute
+The @code{ifunc} attribute is used to mark a function as an indirect
+function using the STT_GNU_IFUNC symbol type extension to the ELF
+standard.  This allows the resolution of the symbol value to be
+determined dynamically at load time, and an optimized version of the
+routine can be selected for the particular processor or other system
+characteristics determined then.  To use this attribute, first define
+the implementation functions available, and a resolver function that
+returns a pointer to the selected implementation function.  The
+implementation functions' declarations must match the API of the
+function being implemented, the resolver's declaration is be a
+function returning pointer to void function returning void:
+
+@smallexample
+void *my_memcpy (void *dst, const void *src, size_t len)
+@{
+  @dots{}
+@}
+
+static void (*resolve_memcpy (void)) (void)
+@{
+  return my_memcpy; // we'll just always select this routine
+@}
+@end smallexample
+
+The exported header file declaring the function the user calls would
+contain:
+
+@smallexample
+extern void *memcpy (void *, const void *, size_t);
+@end smallexample
+
+allowing the user to call this as a regular function, unaware of the
+implementation.  Finally, the indirect function needs to be defined in
+the same translation unit as the resolver function:
+
+@smallexample
+void *memcpy (void *, const void *, size_t)
+     __attribute__ ((ifunc ("resolve_memcpy")));
+@end smallexample
+
+Indirect functions cannot be weak, and require a recent binutils (at
+least version 2.20.1), and GNU C library (at least version 2.11.1).
+
 @item interrupt
 @cindex interrupt handler functions
 Use this attribute on the ARM, AVR, CR16, Epiphany, M32C, M32R/D, m68k, MeP, MIPS,
@@ -2726,7 +2771,13 @@  code to initialize the interrupt vector
 Note, interrupt handlers for the Blackfin, H8/300, H8/300H, H8S, MicroBlaze,
 and SH processors can be specified via the @code{interrupt_handler} attribute.
 
-Note, on the AVR, interrupts will be enabled inside the function.
+Note, on the AVR, the hardware globally disables interrupts when an
+interrupt is executed.  The first instruction of an interrupt handler
+declared with this attribute will be a @code{SEI} instruction to
+re-enable interrupts.  See also the @code{signal} function attribute
+that does not insert a @code{SEI} instuction.  If both @code{signal} and
+@code{interrupt} are specified for the same function, @code{signal}
+will be silently ignored.
 
 Note, for the ARM, you can specify the kind of interrupt to be handled by
 adding an optional parameter to the interrupt attribute like this:
@@ -2822,51 +2873,6 @@  On RL78, use @code{brk_interrupt} instea
 handlers intended to be used with the @code{BRK} opcode (i.e.  those
 that must end with @code{RETB} instead of @code{RETI}).
 
-@item ifunc ("@var{resolver}")
-@cindex @code{ifunc} attribute
-The @code{ifunc} attribute is used to mark a function as an indirect
-function using the STT_GNU_IFUNC symbol type extension to the ELF
-standard.  This allows the resolution of the symbol value to be
-determined dynamically at load time, and an optimized version of the
-routine can be selected for the particular processor or other system
-characteristics determined then.  To use this attribute, first define
-the implementation functions available, and a resolver function that
-returns a pointer to the selected implementation function.  The
-implementation functions' declarations must match the API of the
-function being implemented, the resolver's declaration is be a
-function returning pointer to void function returning void:
-
-@smallexample
-void *my_memcpy (void *dst, const void *src, size_t len)
-@{
-  @dots{}
-@}
-
-static void (*resolve_memcpy (void)) (void)
-@{
-  return my_memcpy; // we'll just always select this routine
-@}
-@end smallexample
-
-The exported header file declaring the function the user calls would
-contain:
-
-@smallexample
-extern void *memcpy (void *, const void *, size_t);
-@end smallexample
-
-allowing the user to call this as a regular function, unaware of the
-implementation.  Finally, the indirect function needs to be defined in
-the same translation unit as the resolver function:
-
-@smallexample
-void *memcpy (void *, const void *, size_t)
-     __attribute__ ((ifunc ("resolve_memcpy")));
-@end smallexample
-
-Indirect functions cannot be weak, and require a recent binutils (at
-least version 2.20.1), and GNU C library (at least version 2.11.1).
-
 @item interrupt_handler
 @cindex interrupt handler functions on the Blackfin, m68k, H8/300 and SH processors
 Use this attribute on the Blackfin, m68k, H8/300, H8/300H, H8S, and SH to
@@ -3471,11 +3477,23 @@  See long_call/short_call.
 See longcall/shortcall.
 
 @item signal
-@cindex signal handler functions on the AVR processors
+@cindex interrupt handler functions on the AVR processors
 Use this attribute on the AVR to indicate that the specified
-function is a signal handler.  The compiler will generate function
-entry and exit sequences suitable for use in a signal handler when this
-attribute is present.  Interrupts will be disabled inside the function.
+function is an interrupt handler.  The compiler will generate function
+entry and exit sequences suitable for use in an interrupt handler when this
+attribute is present.
+
+See also the @code{interrupt} function attribute. 
+
+The AVR hardware globally disables interrupts when an interrupt is executed.
+Interrupt handler functions defined with the @code{signal} attribute
+do not re-enable interrupts.  It is save to enable interrupts in a
+@code{signal} handler.  This ``save'' only applies to the code
+generated by the compiler and not to the IRQ-layout of the
+application which is responsibility of the application.
+
+If both @code{signal} and @code{interrupt} are specified for the same
+function, @code{signal} will be silently ignored.
 
 @item sp_switch
 Use this attribute on the SH to indicate an @code{interrupt_handler}