[MSP430] Reject -m{code,data}-region options unless large memory model is selected

Message ID 20190723142240.303178df@jozef-kubuntu
State New
Headers show
Series
  • [MSP430] Reject -m{code,data}-region options unless large memory model is selected
Related show

Commit Message

Jozef Lawrynowicz July 23, 2019, 1:22 p.m.
The attached patch adds error messages to be emitted when -mcode/data-region are
used without the accompanying -mlarge option that enables the large memory
model.
Use of the upper/either regions without -mlarge can cause relocation overflows
or stack mismanagement when incorrect call/return instructions are generated.
In most cases, using the lower region without -mlarge has no effect, but it can
affect code generation unexpectedly (msp430_do_not_relax_short_jumps), or
add the ".lower" prefix to some section names.

Similarly, handling of the upper/lower/either attributes has been
modified so the attributes will be ignored, and a warning will be emitted, when
they are used without -mlarge.

I have added the error messages to the driver so that uses of
-mcode/data-region=none can be caught. The compiler cannot catch "none" since
it is the default value that the option variable is initialized to.

So only users calling cc1* directly will see the errors in
msp430_option_override, but I also have updated them to be more similar to the
new errors emitted by the driver.

Successfully regtested on trunk for msp430-elf.

Ok for trunk?

Comments

Jeff Law July 24, 2019, 3:31 p.m. | #1
On 7/23/19 7:22 AM, Jozef Lawrynowicz wrote:
> The attached patch adds error messages to be emitted when -mcode/data-region are

> used without the accompanying -mlarge option that enables the large memory

> model.

> Use of the upper/either regions without -mlarge can cause relocation overflows

> or stack mismanagement when incorrect call/return instructions are generated.

> In most cases, using the lower region without -mlarge has no effect, but it can

> affect code generation unexpectedly (msp430_do_not_relax_short_jumps), or

> add the ".lower" prefix to some section names.

> 

> Similarly, handling of the upper/lower/either attributes has been

> modified so the attributes will be ignored, and a warning will be emitted, when

> they are used without -mlarge.

> 

> I have added the error messages to the driver so that uses of

> -mcode/data-region=none can be caught. The compiler cannot catch "none" since

> it is the default value that the option variable is initialized to.

> 

> So only users calling cc1* directly will see the errors in

> msp430_option_override, but I also have updated them to be more similar to the

> new errors emitted by the driver.

> 

> Successfully regtested on trunk for msp430-elf.

> 

> Ok for trunk?

> 

> 

> 0001-MSP430-Disallow-mcode-data-region-unless-the-large-m.patch

> 

> From 0c37dc30d67229392ae8f834e462dd6336f7ccfc Mon Sep 17 00:00:00 2001

> From: Jozef Lawrynowicz <jozef.l@mittosystems.com>

> Date: Mon, 22 Jul 2019 21:37:45 +0100

> Subject: [PATCH] MSP430: Disallow mcode/data-region unless the large memory

>  model is in use

> 

> gcc/ChangeLog:

> 

> 2019-07-23  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

> 

> 	* config/msp430/msp430.h (DRIVER_SELF_SPECS): Define and emit errors

> 	when -m{code,data}-region are used without -mlarge.

> 	* config/msp430/msp430.c (msp430_option_override): Error when a

> 	non-default code or data region is used without -mlarge.

> 	(msp430_section_attr): Emit a warning and do not add upper/lower/either

> 	attributes when they are used without -mlarge.

> 

> gcc/testsuite/ChangeLog:

> 

> 2019-07-23  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

> 

> 	* gcc.target/msp430/pr78818-data-region.c: Add -mlarge to dg-options.

> 	* gcc.target/msp430/region-misuse-code.c: New test.

> 	* gcc.target/msp430/region-misuse-data.c: Likewise.

> 	* gcc.target/msp430/region-misuse-code-data.c: Likewise.

> 	* gcc.target/msp430/region-attribute-misuse.c: Likewise.

OK.  I didn't check all the formatting directives in the error messages.
 You might consider building a native compiler and using that to then
build your msp cross to ensure those formatting directives pass the
checker.  Reasonable adjustments to the error messages to address any
such issues found should be considered pre-approved.

jeff
Jozef Lawrynowicz July 29, 2019, 8:30 p.m. | #2
On Wed, 24 Jul 2019 09:31:11 -0600
Jeff Law <law@redhat.com> wrote:

> On 7/23/19 7:22 AM, Jozef Lawrynowicz wrote:

> > 

> > gcc/ChangeLog:

> > 

> > 2019-07-23  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

> > 

> > 	* config/msp430/msp430.h (DRIVER_SELF_SPECS): Define and emit errors

> > 	when -m{code,data}-region are used without -mlarge.

> > 	* config/msp430/msp430.c (msp430_option_override): Error when a

> > 	non-default code or data region is used without -mlarge.

> > 	(msp430_section_attr): Emit a warning and do not add upper/lower/either

> > 	attributes when they are used without -mlarge.

> > 

> > gcc/testsuite/ChangeLog:

> > 

> > 2019-07-23  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

> > 

> > 	* gcc.target/msp430/pr78818-data-region.c: Add -mlarge to dg-options.

> > 	* gcc.target/msp430/region-misuse-code.c: New test.

> > 	* gcc.target/msp430/region-misuse-data.c: Likewise.

> > 	* gcc.target/msp430/region-misuse-code-data.c: Likewise.

> > 	* gcc.target/msp430/region-attribute-misuse.c: Likewise.  

> OK.  I didn't check all the formatting directives in the error messages.

>  You might consider building a native compiler and using that to then

> build your msp cross to ensure those formatting directives pass the

> checker.  Reasonable adjustments to the error messages to address any

> such issues found should be considered pre-approved.

> 

> jeff


Thanks for the tip, I checked for any format warnings emitted when building
the cross using native trunk and there weren't any issues with my patch.

There are problems with some of the other strings in the msp430 backend though,
I'll make a note to fix them.

Applied.

Jozef

Patch

From 0c37dc30d67229392ae8f834e462dd6336f7ccfc Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Mon, 22 Jul 2019 21:37:45 +0100
Subject: [PATCH] MSP430: Disallow mcode/data-region unless the large memory
 model is in use

gcc/ChangeLog:

2019-07-23  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	* config/msp430/msp430.h (DRIVER_SELF_SPECS): Define and emit errors
	when -m{code,data}-region are used without -mlarge.
	* config/msp430/msp430.c (msp430_option_override): Error when a
	non-default code or data region is used without -mlarge.
	(msp430_section_attr): Emit a warning and do not add upper/lower/either
	attributes when they are used without -mlarge.

gcc/testsuite/ChangeLog:

2019-07-23  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	* gcc.target/msp430/pr78818-data-region.c: Add -mlarge to dg-options.
	* gcc.target/msp430/region-misuse-code.c: New test.
	* gcc.target/msp430/region-misuse-data.c: Likewise.
	* gcc.target/msp430/region-misuse-code-data.c: Likewise.
	* gcc.target/msp430/region-attribute-misuse.c: Likewise.
---
 gcc/config/msp430/msp430.c                    | 35 ++++++++++++++++---
 gcc/config/msp430/msp430.h                    |  8 +++++
 .../gcc.target/msp430/pr78818-data-region.c   |  3 +-
 .../msp430/region-attribute-misuse.c          | 16 +++++++++
 .../msp430/region-misuse-code-data.c          |  4 +++
 .../gcc.target/msp430/region-misuse-code.c    |  4 +++
 .../gcc.target/msp430/region-misuse-data.c    |  4 +++
 7 files changed, 69 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/msp430/region-attribute-misuse.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/region-misuse-code-data.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/region-misuse-code.c
 create mode 100644 gcc/testsuite/gcc.target/msp430/region-misuse-data.c

diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index 265c2f642d8..c7b774e71a6 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -875,10 +875,26 @@  msp430_option_override (void)
   if (TARGET_LARGE && !msp430x)
     error ("%<-mlarge%> requires a 430X-compatible %<-mmcu=%>");
 
-  if (msp430_code_region == MSP430_REGION_UPPER && ! msp430x)
-    error ("%<-mcode-region=upper%> requires 430X-compatible cpu");
-  if (msp430_data_region == MSP430_REGION_UPPER && ! msp430x)
-    error ("%<-mdata-region=upper%> requires 430X-compatible cpu");
+  if (!TARGET_LARGE && msp430_code_region == MSP430_REGION_EITHER)
+    error ("%<-mcode-region=either%> requires the large memory model "
+	   "(%<-mlarge%>)");
+  else if (!TARGET_LARGE && msp430_code_region == MSP430_REGION_UPPER)
+    error ("%<-mcode-region=upper%> requires the large memory model "
+	   "(%<-mlarge%>)");
+  else if (!TARGET_LARGE && msp430_code_region == MSP430_REGION_LOWER)
+    error ("%<-mcode-region=lower%> requires the large memory model "
+	   "(%<-mlarge%>)");
+
+  if (!TARGET_LARGE && msp430_data_region == MSP430_REGION_EITHER)
+    error ("%<-mdata-region=either%> requires the large memory model "
+	   "(%<-mlarge%>)");
+  else if (!TARGET_LARGE && msp430_data_region == MSP430_REGION_UPPER)
+    error ("%<-mdata-region=upper%> requires the large memory model "
+	   "(%<-mlarge%>)");
+  else if (!TARGET_LARGE && msp430_data_region == MSP430_REGION_LOWER)
+    error ("%<-mdata-region=lower%> requires the large memory model "
+	   "(%<-mlarge%>)");
+
 
   if (flag_exceptions || flag_non_call_exceptions
       || flag_unwind_tables || flag_asynchronous_unwind_tables)
@@ -2038,6 +2054,17 @@  msp430_section_attr (tree * node,
 	message = "already marked with 'upper' attribute";
     }
 
+  /* It does not make sense to use upper/lower/either attributes without
+     -mlarge.
+     Without -mlarge, "lower" is the default and only region, so is redundant.
+     Without -mlarge, "upper" will (and "either" might) place code/data in the
+     upper region, which for data could result in relocation overflows, and for
+     code could result in stack mismanagement and incorrect call/return
+     instructions.  */
+  if (!TARGET_LARGE)
+    message = G_("%qE attribute ignored. large memory model (%<-mlarge%>) "
+		 "is required");
+
   if (message)
     {
       warning (OPT_Wattributes, message, name);
diff --git a/gcc/config/msp430/msp430.h b/gcc/config/msp430/msp430.h
index 1288b1a263d..8d2354824a0 100644
--- a/gcc/config/msp430/msp430.h
+++ b/gcc/config/msp430/msp430.h
@@ -67,6 +67,14 @@  extern bool msp430x;
 #define LINK_SPEC "%{mrelax:--relax} %{mlarge:%{!r:%{!g:--gc-sections}}} " \
   "%{mcode-region=*:--code-region=%*} %{mdata-region=*:--data-region=%*}"
 
+#define DRIVER_SELF_SPECS \
+  " %{!mlarge:%{mcode-region=*:%{mdata-region=*:%e-mcode-region and "	\
+    "-mdata-region require the large memory model (-mlarge)}}}" \
+  " %{!mlarge:%{mcode-region=*:"	\
+    "%e-mcode-region requires the large memory model (-mlarge)}}"	\
+  " %{!mlarge:%{mdata-region=*:"	\
+    "%e-mdata-region requires the large memory model (-mlarge)}}"
+
 extern const char * msp430_select_hwmult_lib (int, const char **);
 # define EXTRA_SPEC_FUNCTIONS				\
   { "msp430_hwmult_lib", msp430_select_hwmult_lib },
diff --git a/gcc/testsuite/gcc.target/msp430/pr78818-data-region.c b/gcc/testsuite/gcc.target/msp430/pr78818-data-region.c
index 3244c0ad85d..5b0721e728a 100644
--- a/gcc/testsuite/gcc.target/msp430/pr78818-data-region.c
+++ b/gcc/testsuite/gcc.target/msp430/pr78818-data-region.c
@@ -1,5 +1,6 @@ 
 /* { dg-do compile } */
-/* { dg-options "-mdata-region=either" } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430" } { "" } } */
+/* { dg-options "-mlarge -mdata-region=either" } */
 
 /* { dg-final { scan-assembler-not "\\.either\\.data" } } */
 /* { dg-final { scan-assembler-not "\\.either\\.bss" } } */
diff --git a/gcc/testsuite/gcc.target/msp430/region-attribute-misuse.c b/gcc/testsuite/gcc.target/msp430/region-attribute-misuse.c
new file mode 100644
index 00000000000..fe4617b917c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/region-attribute-misuse.c
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430" "-mlarge" "-mcode-region=*" "-mdata-region=*" } { "" } } */
+/* { dg-final { scan-assembler-not ".section.*bss" } } */
+/* { dg-final { scan-assembler ".section.*upper.data" } } */
+/* { dg-final { scan-assembler ".section.*lower.data" } } */
+/* { dg-final { scan-assembler ".section.*either.data" } } */
+
+int __attribute__((upper)) upper_bss; /* { dg-warning "'upper' attribute ignored. large memory model .'-mlarge'. is required" } */
+int __attribute__((lower)) lower_bss; /* { dg-warning "'lower' attribute ignored. large memory model .'-mlarge'. is required" } */
+int __attribute__((either)) either_bss; /* { dg-warning "'either' attribute ignored. large memory model .'-mlarge'. is required" } */
+
+/* Verify that even without -mlarge, objects can still be placed in
+   upper/lower/either regions manually.  */
+int __attribute__((section(".upper.data"))) upper_data = 1;
+int __attribute__((section(".lower.data"))) lower_data = 2;
+int __attribute__((section(".either.data"))) either_data = 3;
diff --git a/gcc/testsuite/gcc.target/msp430/region-misuse-code-data.c b/gcc/testsuite/gcc.target/msp430/region-misuse-code-data.c
new file mode 100644
index 00000000000..72a160d0bda
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/region-misuse-code-data.c
@@ -0,0 +1,4 @@ 
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-mlarge" } { "" } } */
+/* { dg-options "-mcode-region=either -mdata-region=none" } */
+/* { dg-error "-mcode-region and -mdata-region require the large memory model .-mlarge." "" { target *-*-* } 0 } */
diff --git a/gcc/testsuite/gcc.target/msp430/region-misuse-code.c b/gcc/testsuite/gcc.target/msp430/region-misuse-code.c
new file mode 100644
index 00000000000..6441b773d6e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/region-misuse-code.c
@@ -0,0 +1,4 @@ 
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-mlarge" "-mdata-region=*" } { "" } } */
+/* { dg-options "-mcode-region=lower" } */
+/* { dg-error "-mcode-region requires the large memory model .-mlarge." "" { target *-*-* } 0 } */
diff --git a/gcc/testsuite/gcc.target/msp430/region-misuse-data.c b/gcc/testsuite/gcc.target/msp430/region-misuse-data.c
new file mode 100644
index 00000000000..07523b6042d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/region-misuse-data.c
@@ -0,0 +1,4 @@ 
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-mlarge" "-mdata-region=*" } { "" } } */
+/* { dg-options "-mdata-region=upper" } */
+/* { dg-error "-mdata-region requires the large memory model .-mlarge." "" { target *-*-* } 0 } */
-- 
2.17.1