public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] [MSP430] PR78838: Do not add section name prefixes when section name is .lowtext
@ 2017-05-18 16:15 Jozef Lawrynowicz
  2017-05-19  3:15 ` Martin Sebor
  0 siblings, 1 reply; 6+ messages in thread
From: Jozef Lawrynowicz @ 2017-05-18 16:15 UTC (permalink / raw)
  To: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

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.

[-- Attachment #2: 0001-MSP430-Do-not-add-code-region-prefixes-when-the-sect.patch --]
[-- Type: application/octet-stream, Size: 2536 bytes --]

From c37408725fa8e304545d578ad70df8ac00101925 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@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  <jozef.l@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..3087ba5 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 (char * name, tree decl = current_function_decl)
+{
+  if (decl == NULL_TREE)
+    return false;
+  const char * decl_sec_name = DECL_SECTION_NAME (decl);
+  return decl_sec_name && (strcmp (name, decl_sec_name) == 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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] [MSP430] PR78838: Do not add section name prefixes when section name is .lowtext
@ 2017-05-30 11:06 Nick Clifton
  0 siblings, 0 replies; 6+ messages in thread
From: Nick Clifton @ 2017-05-30 11:06 UTC (permalink / raw)
  To: jozef.l; +Cc: gcc-patches

Hi Josef,

  Thanks for reporting this problem, and providing a patch to fix it.

  I have checked your patch in, along with one, very very minor change
  to the formatting of the comment in gen_prefix().

Cheers
  Nick

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] [MSP430] PR78838: Do not add section name prefixes when section name is .lowtext
@ 2017-03-03 17:43 Jozef Lawrynowicz
  0 siblings, 0 replies; 6+ messages in thread
From: Jozef Lawrynowicz @ 2017-03-03 17:43 UTC (permalink / raw)
  To: gcc-patches

ping


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 patch below
implements this.

I've built and tested successfully with no regressions reported:
  Target is msp430-unknown-elf
  Host   is x86_64-unknown-linux-gnu

Testsuite variations:
msp430-sim/-mcpu=msp430x/-mlarge/-mdata-region=either/-mcode-region=either

The section .lowtext will only be utilised if the large memory model
is used, which is why I have only tested with this testsuite
variation.
I haven't run the g++ testsuite because msp430-elf doesn't build with
C++ support at the moment:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79242

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.

---

2017-02-XX  Jozef Lawrynowicz  <jozef.l@somniumtech.com>

gcc/
PR target/78838
* config/msp430/msp430.c (gen_prefix): Return NULL when section name is .lowtext
(msp430_section_is_lowtext): New function.

gcc/testsuite
PR target/78838
* gcc.target/msp430/interrupt_fn_placement.c: New test

From: Jozef Lawrynowicz <jozef.l@somniumtech.com>
Date: Wed, 15 Feb 2017 13:03:40 +0000
Subject: [PATCH] [MSP430] PR78838: Do not add section name prefixes
 when the section name is .lowtext

---
 gcc/config/msp430/msp430.c                               | 14 ++++++++++++++
 gcc/testsuite/gcc.target/msp430/interrupt_fn_placement.c |  9 +++++++++
 2 files changed, 23 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..c9dffb1 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
+msp430_section_is_lowtext (tree decl = current_function_decl)
+{
+  if (decl == NULL_TREE)
+    return false;
+  const char * dec_name = DECL_SECTION_NAME (decl);
+  return dec_name && strcmp (".lowtext", dec_name) == 0;
+}
+
 #undef  TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS
 #define TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS
msp430_allocate_stack_slots_for_args

@@ -2146,6 +2155,11 @@ 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 (msp430_section_is_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..d5d2113
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/interrupt_fn_placement.c
@@ -0,0 +1,9 @@
+/* { dg-do link } */
+/* { dg-options "-mlarge -mcode-region=either -ffunction-sections" } */
+
+void __attribute__((interrupt(2))) ir_1(void) {
+}
+
+int main(void) {
+  while(1);
+}
--
1.8.3.1

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] [MSP430] PR78838: Do not add section name prefixes when section name is .lowtext
@ 2017-02-16 17:46 Jozef Lawrynowicz
  0 siblings, 0 replies; 6+ messages in thread
From: Jozef Lawrynowicz @ 2017-02-16 17:46 UTC (permalink / raw)
  To: gcc-patches

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 patch below
implements this.

I've built and tested successfully with no regressions reported:
  Target is msp430-unknown-elf
  Host   is x86_64-unknown-linux-gnu

Testsuite variations:
msp430-sim/-mcpu=msp430x/-mlarge/-mdata-region=either/-mcode-region=either

The section .lowtext will only be utilised if the large memory model
is used, which is why I have only tested with this testsuite
variation.
I haven't run the g++ testsuite because msp430-elf doesn't build with
C++ support at the moment:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79242

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.

---

2017-02-XX  Jozef Lawrynowicz  <jozef.l@somniumtech.com>

gcc/
PR target/78838
* config/msp430/msp430.c (gen_prefix): Return NULL when section name is .lowtext
(msp430_section_is_lowtext): New function.

gcc/testsuite
PR target/78838
* gcc.target/msp430/interrupt_fn_placement.c: New test

From: Jozef Lawrynowicz <jozef.l@somniumtech.com>
Date: Wed, 15 Feb 2017 13:03:40 +0000
Subject: [PATCH] [MSP430] PR78838: Do not add section name prefixes
 when the section name is .lowtext

---
 gcc/config/msp430/msp430.c                               | 14 ++++++++++++++
 gcc/testsuite/gcc.target/msp430/interrupt_fn_placement.c |  9 +++++++++
 2 files changed, 23 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..c9dffb1 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
+msp430_section_is_lowtext (tree decl = current_function_decl)
+{
+  if (decl == NULL_TREE)
+    return false;
+  const char * dec_name = DECL_SECTION_NAME (decl);
+  return dec_name && strcmp (".lowtext", dec_name) == 0;
+}
+
 #undef  TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS
 #define TARGET_ALLOCATE_STACK_SLOTS_FOR_ARGS
msp430_allocate_stack_slots_for_args

@@ -2146,6 +2155,11 @@ 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 (msp430_section_is_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..d5d2113
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/interrupt_fn_placement.c
@@ -0,0 +1,9 @@
+/* { dg-do link } */
+/* { dg-options "-mlarge -mcode-region=either -ffunction-sections" } */
+
+void __attribute__((interrupt(2))) ir_1(void) {
+}
+
+int main(void) {
+  while(1);
+}
--
1.8.3.1

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-30 10:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-18 16:15 [PATCH] [MSP430] PR78838: Do not add section name prefixes when section name is .lowtext Jozef Lawrynowicz
2017-05-19  3:15 ` Martin Sebor
2017-05-22 15:58   ` Jozef Lawrynowicz
  -- strict thread matches above, loose matches on Subject: below --
2017-05-30 11:06 Nick Clifton
2017-03-03 17:43 Jozef Lawrynowicz
2017-02-16 17:46 Jozef Lawrynowicz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).