On 19/05/2017 04:13, Martin Sebor wrote:
On 05/18/2017 10:14 AM, Jozef Lawrynowicz wrote:
A patch for this bug was originally posted here:
https://gcc.gnu.org/ml/gcc-patches/2017-02/msg01054.html
There were some issues with that patch which I've now fixed.
The MSP430 target supports the automatic placement of functions and
data in different memory regions when passing the
"-mdata-region=either" and "-mcode-region=either" options.
MSP430x devices support the large memory model, "-mlarge", which
enables 20 bit pointers, however the vector table across all MSP430
targets only accepts 16-bit pointers. To prevent the use of 20-bit
pointers in the vector table when the large memory model is used, the
MSP430 backend currently places functions specified with the interrupt
attribute in a section called ".lowtext". This section is placed at
the beginning of .text in the MSP430 linker scripts.
PR target/78838 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78838)
reports that a function with the interrupt attribute is placed in a
non-existent section ".either.lowtext" when "-mlarge",
"-mcode-region=either" and "-ffunction-sections" are passed. The
backend has correctly decided to place the function in .lowtext, but
has applied the .either prefix which is undesirable.
No additional .lower/.upper/.either prefixes should be applied to the
section name once it has been placed in .lowtext, the attached patch
implements this, and adds a test.
The patch passed regression testing with "-mcpu=msp430x/-mlarge" for
msp430-elf on the gcc-6-branch (r247086). Trunk doesn't build with C++
support for msp430-elf which is why gcc-6-branch was used.
I don't have write access to the GCC SVN repository, so if this patch
is satisfactory, I would appreciate if someone could commit it for me.
Thanks.
I don't normally review back end patches so forgive me for picking
on two of yours the same day but it seems to me that the first
argument to has_section_name() needs to be const-qualified:
+static bool
+has_section_name (char * name, tree decl = current_function_decl)
...
if the following is to compile without error in C++ where string
literals are const char[]:
+ if (has_section_name (".lowtext", decl))
Martin
No problem. The updated patch is attached.
Thanks,
Jozef
From bc4bda371df3b8bb54614ade700b2c970ef3611a Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <joze...@somniumtech.com>
Date: Thu, 18 May 2017 12:51:12 +0000
Subject: [PATCH] MSP430: Do not add code region prefixes when the section name
is .lowtext (PR 78838)
2017-05-XX Jozef Lawrynowicz <joze...@somniumtech.com>
gcc/
PR target/78838
* config/msp430/msp430.c (gen_prefix): Return NULL when section name is
.lowtext.
(has_section_name): New function.
gcc/testsuite
PR target/78838
* gcc.target/msp430/interrupt_fn_placement.c: New test.
---
gcc/config/msp430/msp430.c | 15 +++++++++++++++
gcc/testsuite/gcc.target/msp430/interrupt_fn_placement.c | 13 +++++++++++++
2 files changed, 28 insertions(+)
create mode 100644 gcc/testsuite/gcc.target/msp430/interrupt_fn_placement.c
diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index 6f63116..067f99f 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -1808,6 +1808,15 @@ is_critical_func (tree decl = current_function_decl)
return has_attr (ATTR_CRIT, decl);
}
+static bool
+has_section_name (const char * name, tree decl = current_function_decl)
+{
+ if (decl == NULL_TREE)
+ return false;
+ return (DECL_SECTION_NAME (decl)
+ && (strcmp (name, DECL_SECTION_NAME (decl)) == 0));
+}
+
#undef TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS
#define TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS
msp430_allocate_stack_slots_for_args
@@ -2146,6 +2155,12 @@ gen_prefix (tree decl)
if (has_attr ("section", decl))
return NULL;
+ /* If the function has been put in the .lowtext section because it is an
+ * interrupt handler, and the large memory model is used, then do not add any
+ * prefixes. */
+ if (has_section_name (".lowtext", decl))
+ return NULL;
+
/* If the object has __attribute__((lower)) then use the ".lower." prefix.
*/
if (has_attr (ATTR_LOWER, decl))
return lower_prefix;
diff --git a/gcc/testsuite/gcc.target/msp430/interrupt_fn_placement.c
b/gcc/testsuite/gcc.target/msp430/interrupt_fn_placement.c
new file mode 100644
index 0000000..c88bfc3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/interrupt_fn_placement.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-mlarge -mcode-region=either -ffunction-sections" } */
+/* { dg-final { scan-assembler-not "\\.either\\.lowtext" } } */
+
+void __attribute__ ((interrupt (2))) ir_1 (void)
+{
+ while (1);
+}
+
+int main (void)
+{
+ while (1);
+}
--
1.8.3.1
