The "persistent" and "noinit" data attributes for MSP430 assign the variable
they are applied to to the .persistent and .noinit sections, respectively.

The following patch extends the handler for these attributes to check for
misuse.

The testsuite updates add tests for these attributes, and some other MSP430
attributes, which previously did not have tests.

Ok for trunk?

>From 5c56509cfa0f285549eceefb457dc59134d30d0e Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <joze...@mittosystems.com>
Date: Thu, 18 Oct 2018 22:16:22 +0100
Subject: [PATCH] Extend MSP430 data attribute handling

2018-10-19  Jozef Lawrynowicz  <joze...@mittosystems.com>

	* gcc/config/msp430/msp430.c (msp430_data_attr): Warn if data marked
	with the persistent attribute is not intialized.
	(gen_prefix): Do not add prefixes to data to be placed in the 
	.noinit or .persistent section.
	(msp430_section_type_flags): Use global const string to reference
	.noinit and .persistent sections.

	gcc/testsuite
	* gcc.target/msp430/data-attributes.c: Extend test to check behaviour
	of static noinit/persistent data.
	* gcc.target/msp430/function-attributes-4.c: Extend test to check
	behaviour of noinit/persistent attributes when applied to functions.
	* gcc.target/msp430/attr-critical.c: New test.
	* gcc.target/msp430/attr-naked.c: Likewise.
	* gcc.target/msp430/attr-reentrant.c: Likewise.
	* gcc.target/msp430/attr-wakeup.c: Likewise.
	* gcc.target/msp430/data-attributes-2.c: Likewise.
	* gcc.target/msp430/data-attributes-3.c: Likewise.
---
 gcc/config/msp430/msp430.c                         | 93 ++++++++++++++++------
 gcc/testsuite/gcc.target/msp430/attr-critical.c    | 11 +++
 gcc/testsuite/gcc.target/msp430/attr-naked.c       | 11 +++
 gcc/testsuite/gcc.target/msp430/attr-reentrant.c   | 11 +++
 gcc/testsuite/gcc.target/msp430/attr-wakeup.c      | 19 +++++
 .../gcc.target/msp430/data-attributes-2.c          | 15 ++++
 .../gcc.target/msp430/data-attributes-3.c          | 11 +++
 gcc/testsuite/gcc.target/msp430/data-attributes.c  | 28 ++++---
 .../gcc.target/msp430/function-attributes-4.c      | 10 +++
 9 files changed, 174 insertions(+), 35 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/msp430/attr-critical.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/attr-naked.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/attr-reentrant.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/attr-wakeup.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/data-attributes-2.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/data-attributes-3.c

diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index 7d305b1..73a693e 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -1804,6 +1804,8 @@ const char * const  ATTR_UPPER  = "upper";
 const char * const  ATTR_EITHER = "either";
 const char * const  ATTR_NOINIT = "noinit";
 const char * const  ATTR_PERSIST = "persistent";
+const char * const  SEC_NOINIT = ".noinit";
+const char * const  SEC_PERSIST = ".persistent";
 
 static inline bool
 has_attr (const char * attr, tree decl)
@@ -2037,37 +2039,71 @@ msp430_data_attr (tree * node,
   gcc_assert (DECL_P (* node));
   gcc_assert (args == NULL);
 
-  if (TREE_CODE (* node) != VAR_DECL)
-    message = G_("%qE attribute only applies to variables");
+  /* Variable name to use in warning string, defaults to attribute name.  */
+  tree var = name;
+  tree decl = *node;
 
-  /* Check that it's possible for the variable to have a section.  */
-  if ((TREE_STATIC (* node) || DECL_EXTERNAL (* node) || in_lto_p)
-      && DECL_SECTION_NAME (* node))
-    message = G_("%qE attribute cannot be applied to variables with specific sections");
+  if (TREE_CODE (decl) != VAR_DECL)
+    {
+      message = G_("%qE attribute only applies to variables");
+      goto fail;
+    }
 
-  if (!message && TREE_NAME_EQ (name, ATTR_PERSIST) && !TREE_STATIC (* node)
-      && !TREE_PUBLIC (* node) && !DECL_EXTERNAL (* node))
-    message = G_("%qE attribute has no effect on automatic variables");
+  if (!(TREE_STATIC (decl) || TREE_PUBLIC (decl) || DECL_EXTERNAL (decl)
+	|| in_lto_p))
+    {
+      message = G_("%qE attribute has no effect on automatic variables");
+      goto fail;
+    }
 
-  /* It's not clear if there is anything that can be set here to prevent the
-     front end placing the variable before the back end can handle it, in a
-     similar way to how DECL_COMMON is used below.
-     So just place the variable in the .persistent section now.  */
-  if ((TREE_STATIC (* node) || DECL_EXTERNAL (* node) || in_lto_p)
-      && TREE_NAME_EQ (name, ATTR_PERSIST))
-    set_decl_section_name (* node, ".persistent");
+  if ((TREE_NAME_EQ (name, ATTR_PERSIST)
+       && has_attr (ATTR_NOINIT, decl))
+      || (TREE_NAME_EQ (name, ATTR_NOINIT)
+	  && has_attr (ATTR_PERSIST, decl)))
+    {
+      message
+	= G_("variable %qE cannot have both noinit and persistent attributes");
+      var = decl;
+      goto fail;
+    }
 
-  /* If this var is thought to be common, then change this.  Common variables
-     are assigned to sections before the backend has a chance to process them.  */
-  if (DECL_COMMON (* node))
-    DECL_COMMON (* node) = 0;
+  if (DECL_SECTION_NAME (decl) != NULL)
+    {
+      message = G_("%qE attribute cannot be applied to variables with "
+		   "specific sections");
+      goto fail;
+    }
 
-  if (message)
+  if (TREE_NAME_EQ (name, ATTR_PERSIST))
     {
-      warning (OPT_Wattributes, message, name);
-      * no_add_attrs = true;
+      if (DECL_INITIAL (decl) == NULL
+	  || initializer_zerop (DECL_INITIAL (decl)))
+	{
+	  message = G_("variable %qE was declared persistent and should be "
+		       "explicitly initialized");
+	  var = decl;
+	  goto fail;
+	}
     }
-    
+
+  /* In some cases the front-end will place data before msp430_select_section
+     gets a chance, so just put noinit and persistent variables in the correct
+     section now.  */
+  if (TREE_NAME_EQ (name, ATTR_PERSIST))
+    set_decl_section_name (decl, SEC_PERSIST);
+  else if (TREE_NAME_EQ (name, ATTR_NOINIT))
+    set_decl_section_name (decl, SEC_NOINIT);
+  else
+    /* Only persistent and noinit attributes are supported in this handler.  */
+    gcc_unreachable ();
+
+  goto done;
+
+fail:
+  warning (OPT_Wattributes, message, var);
+  * no_add_attrs = true;
+
+done:
   return NULL_TREE;
 }
 
@@ -2254,6 +2290,11 @@ gen_prefix (tree decl)
   if (! msp430x)
     return NULL;
 
+  /* Do not add prefixes to data to be placed in the .noinit or .persistent
+     section.  */
+  if (has_attr (ATTR_NOINIT, decl) || has_attr (ATTR_PERSIST, decl))
+    return NULL;
+
   if (has_attr (ATTR_UPPER, decl))
     return upper_prefix;
 
@@ -2396,9 +2437,9 @@ msp430_section_type_flags (tree decl, const char * name, int reloc)
     name += strlen (upper_prefix);
   else if (strncmp (name, either_prefix, strlen (either_prefix)) == 0)
     name += strlen (either_prefix);
-  else if (strcmp (name, ".noinit") == 0)
+  else if (strcmp (name, SEC_NOINIT) == 0)
     return SECTION_WRITE | SECTION_BSS | SECTION_NOTYPE;
-  else if (strcmp (name, ".persistent") == 0)
+  else if (strcmp (name, SEC_PERSIST) == 0)
     return SECTION_WRITE | SECTION_NOTYPE;
   
   return default_section_type_flags (decl, name, reloc);
diff --git a/gcc/testsuite/gcc.target/msp430/attr-critical.c b/gcc/testsuite/gcc.target/msp430/attr-critical.c
new file mode 100644
index 0000000..bf9a0d0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/attr-critical.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-final { scan-assembler-times "start of prologue.*PUSH\\s+SR.*DINT.*end of prologue" 1 } } */
+/* { dg-final { scan-assembler-times "start of epilogue.*POP\\s+SR.*RET" 1 } } */
+
+extern int a;
+
+void __attribute__((critical))
+critical_fn(void)
+{
+  while(a);
+}
diff --git a/gcc/testsuite/gcc.target/msp430/attr-naked.c b/gcc/testsuite/gcc.target/msp430/attr-naked.c
new file mode 100644
index 0000000..a12f36e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/attr-naked.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-final { scan-assembler-not "prologue" } } */
+/* { dg-final { scan-assembler-not "epilogue" } } */
+
+extern int a;
+
+void __attribute__((naked))
+naked_fn(void)
+{
+  while(a);
+}
diff --git a/gcc/testsuite/gcc.target/msp430/attr-reentrant.c b/gcc/testsuite/gcc.target/msp430/attr-reentrant.c
new file mode 100644
index 0000000..72e8d55
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/attr-reentrant.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-final { scan-assembler-times "start of prologue.*DINT.*end of prologue" 1 } } */
+/* { dg-final { scan-assembler-times "start of epilogue.*EINT.*RET" 1 } } */
+
+extern int a;
+
+void __attribute__((reentrant))
+reentrant_fn(void)
+{
+  while(a);
+}
diff --git a/gcc/testsuite/gcc.target/msp430/attr-wakeup.c b/gcc/testsuite/gcc.target/msp430/attr-wakeup.c
new file mode 100644
index 0000000..63ac3b4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/attr-wakeup.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-final { scan-assembler-times "BIC.W\\s+#240, \\S+SP" 1 } } */
+
+extern int a;
+
+/* The SR is TOS on entry to an interrupt function. Clear bits 0xF0 (240) in
+   the SR to exit out of low power mode.  */
+void __attribute__((wakeup,interrupt))
+wake_interrupt_fn(void)
+{
+  while(a);
+}
+
+/* wakeup attribute is silently ignored for non-interrupt functions.  */
+void __attribute__((wakeup))
+wake_fn(void)
+{
+  while(a);
+}
diff --git a/gcc/testsuite/gcc.target/msp430/data-attributes-2.c b/gcc/testsuite/gcc.target/msp430/data-attributes-2.c
new file mode 100644
index 0000000..5889108
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/data-attributes-2.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+
+int __attribute__((persistent,noinit)) a = 22; /* { dg-warning "variable 'a' cannot have both noinit and persistent attributes" } */
+int __attribute__((noinit,persistent)) b; /* { dg-warning "variable 'b' cannot have both noinit and persistent attributes" } */
+int __attribute__((noinit,section(".foo"))) c; /* { dg-error "section of 'c' conflicts with" } */
+int __attribute__((persistent,section(".foo"))) d = 10; /* { dg-error "section of 'd' conflicts with" } */
+int __attribute__((section(".foo"),noinit)) e; /* { dg-warning "cannot be applied to variables with specific" } */
+int __attribute__((section(".foo"),persistent)) f = 10; /* { dg-warning "cannot be applied to variables with specific" } */
+
+int main (void)
+{
+  int __attribute__((noinit)) la; /* { dg-warning "'noinit' attribute has no effect on automatic variables" } */
+  int __attribute__((persistent)) lb; /* { dg-warning "'persistent' attribute has no effect on automatic variables" } */
+  return la;
+}
diff --git a/gcc/testsuite/gcc.target/msp430/data-attributes-3.c b/gcc/testsuite/gcc.target/msp430/data-attributes-3.c
new file mode 100644
index 0000000..b7b6560
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/data-attributes-3.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+
+int __attribute__((noinit)) a = 5; /* { dg-error "only zero initializers are allowed in section '.noinit'" } */
+int __attribute__((persistent)) b; /* { dg-warning "variable 'b' was declared persistent and should be explicitly initialized" } */
+
+int main (void)
+{
+  static int __attribute__((noinit)) c;
+  return c;
+}
+
diff --git a/gcc/testsuite/gcc.target/msp430/data-attributes.c b/gcc/testsuite/gcc.target/msp430/data-attributes.c
index 10dd171..06e88cb 100644
--- a/gcc/testsuite/gcc.target/msp430/data-attributes.c
+++ b/gcc/testsuite/gcc.target/msp430/data-attributes.c
@@ -10,12 +10,17 @@ extern void exit (int) __attribute__ ((noreturn));
 int a;
 int b = 0;
 int c = 1;
-int __attribute__((noinit)) d;
-int __attribute__((persistent)) e = 2;
+int __attribute__((noinit)) gd;
+int __attribute__((persistent)) ge = 2;
 
 int
 main (void)
 {
+  /* Persistent/noinit static local variables should behave the same as their
+     global counterparts.  */
+  static int __attribute__((noinit)) ld;
+  static int __attribute__((persistent)) le = 2;
+
   /* Make sure that the C startup code has correctly initialised the ordinary variables.  */
   if (a != 0)
     abort ();
@@ -25,26 +30,31 @@ main (void)
      This does not support FLASH, and as a side effect it does not support
      reinitialising initialised data.  Hence we only test b and c if this
      is the first time through this test, or large support has been enabled.  */
-  if (e == 2)
+  if (ge == 2)
 #endif
   if (b != 0 || c != 1)
     abort ();
-  
-  switch (e)
+
+  /* The local and global persistent variables should always have the same
+     value.  */
+  if (ge != le)
+    abort();
+  switch (ge)
     {
     case 2:
       /* First time through - change all the values.  */
-      a = b = c = d = e = 3;
+      a = b = c = gd = ge = ld = le = 3;
       break;
 
     case 3:
-      /* Second time through - make sure that d has not been reset.  */
-      if (d != 3)
+      /* Second time through - make sure that gd and ld have not been reset,
+	 and that they have the same value.  */
+      if (gd != 3 || gd != ld)
 	abort ();
       exit (0);
 
     default:
-      /* Any other value for e is an error.  */
+      /* Any other value for ge is an error.  */
       abort ();
     }
 
diff --git a/gcc/testsuite/gcc.target/msp430/function-attributes-4.c b/gcc/testsuite/gcc.target/msp430/function-attributes-4.c
index 07d13c9..4a41282 100644
--- a/gcc/testsuite/gcc.target/msp430/function-attributes-4.c
+++ b/gcc/testsuite/gcc.target/msp430/function-attributes-4.c
@@ -66,6 +66,16 @@ fn12(void)
 { /* { dg-warning "naked functions cannot be critical" } */
 }
 
+void __attribute__((noinit))
+fn13(void)
+{ /* { dg-warning "'noinit' attribute only applies to variables" } */
+}
+
+void __attribute__((persistent))
+fn14(void)
+{ /* { dg-warning "'persistent' attribute only applies to variables" } */
+}
+
 int __attribute__((interrupt))
 isr1 (void)
 { /* { dg-warning "interrupt handlers must be void" } */
-- 
2.7.4

Reply via email to