[avr-gcc-list] RFC: Patch for "signal" argument

[Ron Kreymborg]

This RFC is essentially that presented by Marek Michalkiewicz in 2005 with
some cosmetic changes. It falls into two parts: The changes to gcc and the
supporting changes to AVR header files.

GCC Changes (4.2.2)

The patch provides for an optional numeric argument to the "signal"
attribute. An interrupt function prototype using this system looks like:

void IntName(void) __attribute__ ((signal(4), __INTR_ATTRS));

The signal argument is used to name the interrupt function for later linking
by directly inserting the following two (example) lines into the assembly
output:

      .global __vector_4
      __vector_4:

The given name is still processed according to the language rules. The name
is thus independent of the vector number, but the number is attached to the
name. Note that for C++, by the time the signal argument is being processed
the given name is mangled. 

The vector number is tested to be in the range 1 to MAX_INTERRUPT_NUMBER.
The latter is newly defined in avr.h as 56. This is a barely adequate test.
I initially added two additional variables to the "mcu_type_s" structure
type: an integer setting the maximum interrupt vector for that mcu and a
usually null pointer for those mcus with "holes" in their table. In these
cases the pointer pointed to a zero-terminated list of numbers that matched
the "holes". Together this allowed for an exhaustive test on the signal
argument but was not included because there were mcus for which I had no
data and there may be an easier method I am not aware of. Any comments would
be welcome.

If the signal argument is non-numeric, the effect is that the
TREE_INT_CST_LOW function returns a very large number. This is used in the
error handler as a hint to the programmer.

This patch means the original warnings for misspelled vector names in
avr_handle_fndecl_attribute can now be errors.

AVR Headers

This patch makes no difference to existing macros, in particular the ISR
macro. In C an interrupt handler stands alone and its name need only be
known to the vector table. I can see no reason why C programmers would use
this new method and that is perhaps why Marek's original submission was not
implemented. Although not all AVR C++ programmers consider it necessary
(perhaps only me!), because an interrupt function would almost never be
called from other code provides a excellent reason to make it invisible
within the class managing that peripheral (see the thread on "C++
Interrupts"). This patch allows that method.

The proposed additional macros in interrupt.h are:

#ifdef __cplusplus
  #define ISRN(name, number) \
    void name(void) __attribute__ ((signal(number),__INTR_ATTRS)); \
    void name(void)
  #define CLASS_ISRN(name, number) \
    void name(void) __attribute__ ((signal(number),__INTR_ATTRS))
#else
  #define ISRN(name, number) \
    void name(void) __attribute__ ((signal(number),__INTR_ATTRS)); \
    void name(void)#endif
#endif

The CLASS_ISRN macro would be used in C++ where the implementation is
separate from the class definition, the common C++ paradigm. 

The "number" in these macros can be a defined name. The AVR ioXX header
files already define the vector names using the Atmel name with the text
"_vect" appended. It is a simple task to rewrite these header files so the
interrupt entries look like:

/* USART, Rx Complete */
#define USART_RXC              11
#define USART_RXC_vect         _VECTOR(11)
#define SIG_UART_RECV          _VECTOR(11)

The unadorned name can then be used directly in the proposed new macros with
the added advantage of matching the Atmel name in the respective mcu doco. I
have written a program to do this conversion and could upload the converted
header files.

If accepted I would be happy to add a note to the avr-libc manual.

This is my first RFC so I am not sure I have the protocol correct. If I've
done something incorrectly please let me know.

Thanks
Ron Kreymborg


--- gcc/config/avr/avr-protos_org.h     Sun Sep  2 01:28:30 2007
+++ gcc/config/avr/avr-protos.h Wed Mar 26 21:45:46 2008
@@ -42,6 +42,7 @@
 #ifdef TREE_CODE
 extern void asm_output_external (FILE *file, tree decl, char *name);
 extern int avr_progmem_p (tree decl, tree attributes);
+extern void avr_declare_function_name (FILE *file, const char *name, tree
decl);
 
 #ifdef RTX_CODE /* inside TREE_CODE */
 extern rtx avr_function_value (tree type, tree func);
 
 
 
--- gcc/config/avr/avr_org.h    Tue Sep  4 07:03:50 2007
+++ gcc/config/avr/avr.h        Sat Mar 29 12:11:53 2008
@@ -81,6 +81,8 @@
 
 #define POINTER_SIZE 16
 
+/* Upper boundary of valid interrupt vectors */
+#define MAX_INTERRUPT_NUMBER 56
 
 /* Maximum sized of reasonable data type
    DImode or Dfmode ...  */
@@ -522,10 +524,7 @@
    specific tm.h file (depending upon the particulars of your assembler).
*/
 
 #define ASM_DECLARE_FUNCTION_NAME(FILE, NAME, DECL)            \
-do {                                                           \
-     ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, "function");       \
-     ASM_OUTPUT_LABEL (FILE, NAME);                            \
-} while (0)
+  avr_declare_function_name (FILE, NAME, DECL)
 
 #define ASM_DECLARE_FUNCTION_SIZE(FILE, FNAME, DECL)                   \
   do {                                                                 \
   
   
   
--- gcc/config/avr/avr_org.c    Sun Sep  2 01:28:30 2007
+++ gcc/config/avr/avr.c        Mon Mar 31 18:27:57 2008
@@ -430,6 +430,46 @@
   return a != NULL_TREE;
 }
 
+
+void
+avr_declare_function_name (FILE *file, const char *name, tree decl)
+{
+  tree a = NULL_TREE;
+  int vector = -1;
+
+  if (TREE_CODE (decl) == FUNCTION_DECL)
+    a = lookup_attribute ("signal", DECL_ATTRIBUTES (decl));
+
+  if (a)
+    a = TREE_VALUE (a);
+
+  if (a)
+    a = TREE_VALUE (a);
+
+  if (a)
+    vector = TREE_INT_CST_LOW (a);
+
+  ASM_OUTPUT_TYPE_DIRECTIVE (file, name, "function");
+  ASM_OUTPUT_LABEL (file, name);
+
+  if (vector != -1)
+  {
+    /* Minimul sanity check for valid interrupt vector number. */
+    if (vector > 0 && vector <= MAX_INTERRUPT_NUMBER)
+    {
+      fprintf (file, ".global __vector_%d\n", vector);
+      fprintf (file, "__vector_%d:\n", vector);
+    }
+    else
+    {
+      if (vector > 100000)
+        error ("Interrupt vector number %d is outside valid range.  Was it
text?", vector);
+      else
+        error ("Interrupt vector number %d is outside valid range.",
vector);
+    }
+  }
+}
+
 /* Return the number of hard registers to push/pop in the prologue/epilogue
    of the current function, and optionally store these registers in SET.
*/
 
@@ -4650,7 +4690,7 @@
 {
   /* { name, min_len, max_len, decl_req, type_req, fn_type_req, handler }
*/
   { "progmem",   0, 0, false, false, false,  avr_handle_progmem_attribute
},
-  { "signal",    0, 0, true,  false, false,  avr_handle_fndecl_attribute },
+  { "signal",    0, 1, true,  false, false,  avr_handle_fndecl_attribute },
   { "interrupt", 0, 0, true,  false, false,  avr_handle_fndecl_attribute },
   { "naked",     0, 0, false, true,  true,   avr_handle_fntype_attribute },
   { NULL,        0, 0, false, false, false, NULL }
@@ -4704,7 +4744,7 @@
 
 static tree
 avr_handle_fndecl_attribute (tree *node, tree name,
-                            tree args ATTRIBUTE_UNUSED,
+                            tree args,
                             int flags ATTRIBUTE_UNUSED,
                             bool *no_add_attrs)
 {
@@ -4714,7 +4754,7 @@
               IDENTIFIER_POINTER (name));
       *no_add_attrs = true;
     }
-  else
+  else if (args == NULL_TREE)
     {
       const char *func_name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME
(*node));
       const char *attr = IDENTIFIER_POINTER (name);
@@ -4727,16 +4767,14 @@
         {
           if (strncmp (func_name, "__vector", strlen ("__vector")) != 0)
             {
-              warning (0, "%qs appears to be a misspelled interrupt
handler",
-                       func_name);
+              error ("%qs appears to be a misspelled interrupt handler",
func_name);
             }
         }
       else if (strncmp (attr, "signal", strlen ("signal")) == 0)
         {
           if (strncmp (func_name, "__vector", strlen ("__vector")) != 0)
             {
-              warning (0, "%qs appears to be a misspelled signal handler",
-                       func_name);
+              error ("%qs appears to be a misspelled signal handler",
func_name);
             }
         }
     }