[PATCH][msp430] Don't output __interrupt_vector sections for weak functions

[PATCH][msp430] Don't output __interrupt_vector sections for weak functions

[Joe Seymour]

The msp430 target supports an "interrupt" attribute, which can take an optional
argument specifying the interrupt number. If this argument is provided then the
compiler will output an __interrupt_vector_<number> section that the linker
script can use to create a vector table.

PR target/70713 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70713) reports
that if an __interrupt_vector section is generated for a weak function, this 
section won't be discarded when the linker discards the weak function, which
results in multiple __interrupt_vector sections that the linker is unable
to choose between.

This patch seeks to avoid the issue by not generating __interrupt_vector
sections for weak functions, instead emitting a warning.

I considered implementing the warning in msp430_attr(), but that only works if
the weak attribute is specified before the interrupt attribute. Instead I've
implemented the whole patch in msp430_start_function (aside: there are two
functions named msp430_start_function).

I've moved part of msp430_start_function into a new helper function, largely to
keep line length down. I haven't tried to keep the line that calls warning
under 79 characters, as other parts of msp430.c don't.

Built and tested msp430-elf-gcc without regressions. I haven't done any c++
testing because that doesn't build for msp430-elf at the moment:

https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01256.html

Note that I don't have commit access. If this patch (or some future version of
it) is OK, I'd appreciate it if someone could commit it for me.

Thanks,

2016-08-XX  Joe Seymour  <joe.s@somniumtech.com>

	gcc/
	PR target/70713
	* config/msp430/msp430.c (msp430_start_function): Don't output
	interrupt vectors for weak functions. Issue a warning.
	(msp430_output_interrupt_vector): New helper function.

	gcc/testsuite/
	PR target/70713
	* gcc.target/msp430/function-attributes.c: New test.

---
 gcc/config/msp430/msp430.c                         |   58 +++++++++++++-------
 .../gcc.target/msp430/function-attributes.c        |   17 ++++++
 2 files changed, 55 insertions(+), 20 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/msp430/function-attributes.c

diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index dba4d19..ee9cdf9 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -2094,6 +2094,32 @@ increment_stack (HOST_WIDE_INT amount)
     }
 }
 
+static void
+msp430_output_interrupt_vector (FILE *file, const char *name,
+				tree decl, tree intr_vector)
+{
+  char buf[101];
+
+  intr_vector = TREE_VALUE (intr_vector);
+
+  /* The interrupt attribute has a vector value.  Turn this into a
+     section name, switch to that section and put the address of
+     the current function into that vector slot.  Note msp430_attr()
+     has already verified the vector name for us.  */
+  if (TREE_CODE (intr_vector) == STRING_CST)
+    sprintf (buf, "__interrupt_vector_%.80s",
+	     TREE_STRING_POINTER (intr_vector));
+  else /* TREE_CODE (intr_vector) == INTEGER_CST */
+    sprintf (buf, "__interrupt_vector_%u",
+	     (unsigned int) TREE_INT_CST_LOW (intr_vector));
+
+  switch_to_section (get_section (buf, SECTION_CODE, decl));
+  fputs ("\t.word\t", file);
+  assemble_name (file, name);
+  fputc ('\n', file);
+  fputc ('\t', file);
+}
+
 void
 msp430_start_function (FILE *file, const char *name, tree decl)
 {
@@ -2106,26 +2132,18 @@ msp430_start_function (FILE *file, const char *name, tree decl)
 
       if (intr_vector != NULL_TREE)
 	{
-	  char buf[101];
-
-	  intr_vector = TREE_VALUE (intr_vector);
-
-	  /* The interrupt attribute has a vector value.  Turn this into a
-	     section name, switch to that section and put the address of
-	     the current function into that vector slot.  Note msp430_attr()
-	     has already verified the vector name for us.  */
-	  if (TREE_CODE (intr_vector) == STRING_CST)
-	    sprintf (buf, "__interrupt_vector_%.80s",
-		     TREE_STRING_POINTER (intr_vector));
-	  else /* TREE_CODE (intr_vector) == INTEGER_CST */
-	    sprintf (buf, "__interrupt_vector_%u",
-		     (unsigned int) TREE_INT_CST_LOW (intr_vector));
-
-	  switch_to_section (get_section (buf, SECTION_CODE, decl));
-	  fputs ("\t.word\t", file);
-	  assemble_name (file, name);
-	  fputc ('\n', file);
-	  fputc ('\t', file);
+	  /* Interrupt vector sections should be unique, use of weak functions
+	     implies multiple definitions, so don't generate vector table
+	     entries for weak functions.  */
+	  if (DECL_WEAK (decl))
+	    {
+	    warning (OPT_Wattributes,
+		     "argument to interrupt attribute is ignored for weak functions");
+	    }
+	  else
+	    {
+	      msp430_output_interrupt_vector (file, name, decl, intr_vector);
+	    }
 	}
     }
 
diff --git a/gcc/testsuite/gcc.target/msp430/function-attributes.c b/gcc/testsuite/gcc.target/msp430/function-attributes.c
new file mode 100644
index 0000000..1c166af
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/function-attributes.c
@@ -0,0 +1,17 @@
+void __attribute__((weak, interrupt(10)))
+weak_interrupt_number (void) {
+} /* { dg-warning "argument to interrupt attribute is ignored for weak functions" } */
+
+void __attribute__((interrupt("nmi"))) __attribute__((weak))
+interrupt_name_weak (void) {
+} /* { dg-warning "argument to interrupt attribute is ignored for weak functions" } */
+
+void __attribute__((weak, interrupt))
+weak_interrupt (void) {
+}
+
+void __attribute__((interrupt(11)))
+interrupt_number (void) {
+}
+
+/* { dg-final { scan-assembler-times "__interrupt_vector_" 1 } } */
-- 
1.7.1

Re: [PATCH][msp430] Don't output __interrupt_vector sections for weak functions

[DJ Delorie]

Which results in a more user-obvious case, ignoring the interrupt
attribute or ignoring the weak attribute?  I would think that we never
want to compile and link successfully if we can't do what the user
wants, and omitting an interrupt handler is... bad.

I think this should either be a hard error, so the user must fix it
right away, or ignore the weak, which results in a linker error
(somehow? maybe?) if the user specifies two handlers for the same
interrupt.

Re: [PATCH][msp430] Don't output __interrupt_vector sections for weak functions

[Joe Seymour]

On 29/08/2016 22:09, DJ Delorie wrote:
> 
> Which results in a more user-obvious case, ignoring the interrupt
> attribute or ignoring the weak attribute?  I would think that we never
> want to compile and link successfully if we can't do what the user
> wants, and omitting an interrupt handler is... bad.
> 
> I think this should either be a hard error, so the user must fix it
> right away, or ignore the weak, which results in a linker error
> (somehow? maybe?) if the user specifies two handlers for the same
> interrupt.

Thanks for the review.  My original intention was to make it an error, but
msp430_attr() seemed to set the precedent of emitting warnings for invalid
attributes, so I tried to follow that convention.

Here's a patch that produces an error instead:

2016-09-XX  Joe Seymour  <joe.s@somniumtech.com>

	gcc/
	PR target/70713
	* config/msp430/msp430.c (msp430_start_function): Emit an error
	if a function is both weak and specifies an interrupt number.

	gcc/testsuite/
	PR target/70713
	* gcc.target/msp430/function-attributes-1.c: New test.
	* gcc.target/msp430/function-attributes-2.c: New test.
	* gcc.target/msp430/function-attributes-3.c: New test.

diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index dba4d19..c40d2da 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -2108,6 +2108,13 @@ msp430_start_function (FILE *file, const char *name, tree decl)
 	{
 	  char buf[101];
 
+	  /* Interrupt vector sections should be unique, but use of weak
+	     functions implies multiple definitions.  */
+	  if (DECL_WEAK (decl))
+	    {
+	      error ("argument to interrupt attribute is unsupported for weak functions");
+	    }
+
 	  intr_vector = TREE_VALUE (intr_vector);
 
 	  /* The interrupt attribute has a vector value.  Turn this into a
diff --git a/gcc/testsuite/gcc.target/msp430/function-attributes-1.c b/gcc/testsuite/gcc.target/msp430/function-attributes-1.c
new file mode 100644
index 0000000..7a3b7be
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/function-attributes-1.c
@@ -0,0 +1,9 @@
+void __attribute__((weak, interrupt))
+weak_interrupt (void) {
+}
+
+void __attribute__((interrupt(11)))
+interrupt_number (void) {
+}
+
+/* { dg-final { scan-assembler-times "__interrupt_vector_" 1 } } */
diff --git a/gcc/testsuite/gcc.target/msp430/function-attributes-2.c b/gcc/testsuite/gcc.target/msp430/function-attributes-2.c
new file mode 100644
index 0000000..fcb2fb2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/function-attributes-2.c
@@ -0,0 +1,3 @@
+void __attribute__((weak, interrupt(10)))
+weak_interrupt_number (void) {
+} /* { dg-error "argument to interrupt attribute is unsupported for weak functions" } */
diff --git a/gcc/testsuite/gcc.target/msp430/function-attributes-3.c b/gcc/testsuite/gcc.target/msp430/function-attributes-3.c
new file mode 100644
index 0000000..b0acf4a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/function-attributes-3.c
@@ -0,0 +1,3 @@
+void __attribute__((interrupt("nmi"))) __attribute__((weak))
+interrupt_name_weak (void) {
+} /* { dg-error "argument to interrupt attribute is unsupported for weak functions" } */

[PING][PATCH][msp430] Don't output __interrupt_vector sections for weak functions

[Joe Seymour]

Is the revised patch acceptable?

If it is, I don't have commit access, so I'd apprecite it if someone
could commit it for me. Similarly, I can comment on the Bugzilla
entry but can't change it's state - assuming that would be appropriate
if the patch is accepted.

Thanks,

Joe

On 01/09/2016 17:18, Joe Seymour wrote:
> On 29/08/2016 22:09, DJ Delorie wrote:
>>
>> Which results in a more user-obvious case, ignoring the interrupt
>> attribute or ignoring the weak attribute?  I would think that we never
>> want to compile and link successfully if we can't do what the user
>> wants, and omitting an interrupt handler is... bad.
>>
>> I think this should either be a hard error, so the user must fix it
>> right away, or ignore the weak, which results in a linker error
>> (somehow? maybe?) if the user specifies two handlers for the same
>> interrupt.
> 
> Thanks for the review.  My original intention was to make it an error, but
> msp430_attr() seemed to set the precedent of emitting warnings for invalid
> attributes, so I tried to follow that convention.
> 
> Here's a patch that produces an error instead:
> 
> 2016-09-XX  Joe Seymour  <joe.s@somniumtech.com>
> 
> 	gcc/
> 	PR target/70713
> 	* config/msp430/msp430.c (msp430_start_function): Emit an error
> 	if a function is both weak and specifies an interrupt number.
> 
> 	gcc/testsuite/
> 	PR target/70713
> 	* gcc.target/msp430/function-attributes-1.c: New test.
> 	* gcc.target/msp430/function-attributes-2.c: New test.
> 	* gcc.target/msp430/function-attributes-3.c: New test.
> 
> diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
> index dba4d19..c40d2da 100644
> --- a/gcc/config/msp430/msp430.c
> +++ b/gcc/config/msp430/msp430.c
> @@ -2108,6 +2108,13 @@ msp430_start_function (FILE *file, const char *name, tree decl)
>  	{
>  	  char buf[101];
>  
> +	  /* Interrupt vector sections should be unique, but use of weak
> +	     functions implies multiple definitions.  */
> +	  if (DECL_WEAK (decl))
> +	    {
> +	      error ("argument to interrupt attribute is unsupported for weak functions");
> +	    }
> +
>  	  intr_vector = TREE_VALUE (intr_vector);
>  
>  	  /* The interrupt attribute has a vector value.  Turn this into a
> diff --git a/gcc/testsuite/gcc.target/msp430/function-attributes-1.c b/gcc/testsuite/gcc.target/msp430/function-attributes-1.c
> new file mode 100644
> index 0000000..7a3b7be
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/msp430/function-attributes-1.c
> @@ -0,0 +1,9 @@
> +void __attribute__((weak, interrupt))
> +weak_interrupt (void) {
> +}
> +
> +void __attribute__((interrupt(11)))
> +interrupt_number (void) {
> +}
> +
> +/* { dg-final { scan-assembler-times "__interrupt_vector_" 1 } } */
> diff --git a/gcc/testsuite/gcc.target/msp430/function-attributes-2.c b/gcc/testsuite/gcc.target/msp430/function-attributes-2.c
> new file mode 100644
> index 0000000..fcb2fb2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/msp430/function-attributes-2.c
> @@ -0,0 +1,3 @@
> +void __attribute__((weak, interrupt(10)))
> +weak_interrupt_number (void) {
> +} /* { dg-error "argument to interrupt attribute is unsupported for weak functions" } */
> diff --git a/gcc/testsuite/gcc.target/msp430/function-attributes-3.c b/gcc/testsuite/gcc.target/msp430/function-attributes-3.c
> new file mode 100644
> index 0000000..b0acf4a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/msp430/function-attributes-3.c
> @@ -0,0 +1,3 @@
> +void __attribute__((interrupt("nmi"))) __attribute__((weak))
> +interrupt_name_weak (void) {
> +} /* { dg-error "argument to interrupt attribute is unsupported for weak functions" } */
> 
> 

Re: [PATCH][msp430] Don't output __interrupt_vector sections for weak functions

[DJ Delorie]

Approved and committed.  Thanks!  Sorry for the delay, I was away for
the holiday weekend and it slipped through the cracks when I returned.