diff mbox

Fix inconsistent section flags

Message ID 20170717143547.GA23668@britannica.bec.de
State New
Headers show

Commit Message

Joerg Sonnenberger July 17, 2017, 2:35 p.m. UTC 
Hello,
attached patch fixes inconsistent handling of section flags when using
the section attribute, i.e.:

__attribute__((section("writable1"))) int foo1;
__attribute__((section("readonly1"))) const int foo1c;
__attribute__((section("writable2"))) int foo2 = 42;
__attribute__((section("readonly2"))) const int foo2c = 42;

should give section attributes of "aw", "a", "aw", "a" in that order.
Currently, "foo1c" is classified as BSS though and therefore put into a
writable section.

Joerg

Comments

Jeff Law Aug. 28, 2017, 5:42 p.m. UTC | #1 
On 07/17/2017 08:35 AM, Joerg Sonnenberger wrote:
> Hello,
> attached patch fixes inconsistent handling of section flags when using
> the section attribute, i.e.:
> 
> __attribute__((section("writable1"))) int foo1;
> __attribute__((section("readonly1"))) const int foo1c;
> __attribute__((section("writable2"))) int foo2 = 42;
> __attribute__((section("readonly2"))) const int foo2c = 42;
> 
> should give section attributes of "aw", "a", "aw", "a" in that order.
> Currently, "foo1c" is classified as BSS though and therefore put into a
> writable section.
ISTM the test we need here is whether or not the underlying DECL is
readonly.  If it READONLY, then it shouldn't go into .bss, but should
instead to into a readable section.

Testing based on names seems wrong.

Does the attached (untested) patch work for you?

JEff
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 91680d6..37438c1 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -976,16 +976,16 @@ decode_reg_name (const char *name)
 bool
 bss_initializer_p (const_tree decl)
 {
-  return (DECL_INITIAL (decl) == NULL
-	  /* In LTO we have no errors in program; error_mark_node is used
-	     to mark offlined constructors.  */
-	  || (DECL_INITIAL (decl) == error_mark_node
-	      && !in_lto_p)
-	  || (flag_zero_initialized_in_bss
-	      /* Leave constant zeroes in .rodata so they
-		 can be shared.  */
-	      && !TREE_READONLY (decl)
-	      && initializer_zerop (DECL_INITIAL (decl))));
+  /* Do not put constants into the .bss section, they belong in a readonly
+     section.  */
+  return (!TREE_READONLY (decl)
+	  && (DECL_INITIAL (decl) == NULL
+	      /* In LTO we have no errors in program; error_mark_node is used
+	         to mark offlined constructors.  */
+	      || (DECL_INITIAL (decl) == error_mark_node
+	          && !in_lto_p)
+	      || (flag_zero_initialized_in_bss
+	          && initializer_zerop (DECL_INITIAL (decl)))));
 }
 
 /* Compute the alignment of variable specified by DECL.
@@ -6508,7 +6508,8 @@ categorize_decl_for_section (const_tree decl, int reloc)
 	ret = SECCAT_BSS;
       else if (! TREE_READONLY (decl)
 	       || TREE_SIDE_EFFECTS (decl)
-	       || ! TREE_CONSTANT (DECL_INITIAL (decl)))
+	       || (DECL_INITIAL (decl)
+		   && ! TREE_CONSTANT (DECL_INITIAL (decl))))
 	{
 	  /* Here the reloc_rw_mask is not testing whether the section should
 	     be read-only or not, but whether the dynamic link will have to
@@ -6528,7 +6529,8 @@ categorize_decl_for_section (const_tree decl, int reloc)
 	   location.  -fmerge-all-constants allows even that (at the
 	   expense of not conforming).  */
 	ret = SECCAT_RODATA;
-      else if (TREE_CODE (DECL_INITIAL (decl)) == STRING_CST)
+      else if (DECL_INITIAL (decl)
+	       && TREE_CODE (DECL_INITIAL (decl)) == STRING_CST)
 	ret = SECCAT_RODATA_MERGE_STR_INIT;
       else
 	ret = SECCAT_RODATA_MERGE_CONST;
Joerg Sonnenberger Aug. 28, 2017, 6:34 p.m. UTC | #2 
On Mon, Aug 28, 2017 at 11:42:53AM -0600, Jeff Law wrote:
> On 07/17/2017 08:35 AM, Joerg Sonnenberger wrote:
> > Hello,
> > attached patch fixes inconsistent handling of section flags when using
> > the section attribute, i.e.:
> > 
> > __attribute__((section("writable1"))) int foo1;
> > __attribute__((section("readonly1"))) const int foo1c;
> > __attribute__((section("writable2"))) int foo2 = 42;
> > __attribute__((section("readonly2"))) const int foo2c = 42;
> > 
> > should give section attributes of "aw", "a", "aw", "a" in that order.
> > Currently, "foo1c" is classified as BSS though and therefore put into a
> > writable section.
> ISTM the test we need here is whether or not the underlying DECL is
> readonly.  If it READONLY, then it shouldn't go into .bss, but should
> instead to into a readable section.
> 
> Testing based on names seems wrong.
> 
> Does the attached (untested) patch work for you?

The intention should work, will test it.  The attached patch will likely
also be needed on top.

Joerg
Index: varasm.c
===================================================================
RCS file: /home/joerg/repo/netbsd/src/external/gpl3/gcc/dist/gcc/varasm.c,v
retrieving revision 1.2
retrieving revision 1.3
diff -u -p -r1.2 -r1.3
--- varasm.c	17 Jul 2017 19:53:10 -0000	1.2
+++ varasm.c	22 Jul 2017 20:52:52 -0000	1.3
@@ -6428,7 +6428,8 @@ categorize_decl_for_section (const_tree 
 	   location.  -fmerge-all-constants allows even that (at the
 	   expense of not conforming).  */
 	ret = SECCAT_RODATA;
-      else if (TREE_CODE (DECL_INITIAL (decl)) == STRING_CST)
+      else if (DECL_INITIAL (decl) != NULL
+               && TREE_CODE (DECL_INITIAL (decl)) == STRING_CST)
 	ret = SECCAT_RODATA_MERGE_STR_INIT;
       else
 	ret = SECCAT_RODATA_MERGE_CONST;
Joerg Sonnenberger Aug. 29, 2017, 3:09 p.m. UTC | #3 
On Mon, Aug 28, 2017 at 11:42:53AM -0600, Jeff Law wrote:
> Does the attached (untested) patch work for you?

Works for the cases I care about, yes.

Joerg
Jeff Law Sept. 1, 2017, 4:26 p.m. UTC | #4 
On 08/29/2017 09:09 AM, Joerg Sonnenberger wrote:
> On Mon, Aug 28, 2017 at 11:42:53AM -0600, Jeff Law wrote:
>> Does the attached (untested) patch work for you?
> 
> Works for the cases I care about, yes.
Thanks.   Here's what I'm committing after the usual bootstrap &
regression testing cycle on x86.  This includes a regression test for
the testsuite.

Jeff
commit 371072bf395be11f36ef31bb3cfec06bbfc58597
Author: law <law@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Fri Sep 1 16:26:00 2017 +0000

            * varasm.c (bss_initializer_p): Do not put constants into .bss
            (categorize_decl_for_section): Handle bss_initializer_p returning
            false when DECL_INITIAL is NULL.
    
            * gcc.target/i386/const-in-bss.c: New test.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@251602 138bc75d-0d04-0410-961f-82ee72b054a4

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index e71380e5c9d..f9d9eb74a3a 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@
+2017-09-01  Joerg Sonnenberger  <joerg@bec.de>
+	    Jeff Law  <law@redhat.com>
+
+	* varasm.c (bss_initializer_p): Do not put constants into .bss
+	(categorize_decl_for_section): Handle bss_initializer_p returning
+	false when DECL_INITIAL is NULL.
+
 2017-09-01  Andreas Krebbel  <krebbel@linux.vnet.ibm.com>
 
 	PR target/82012
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index fe6e4301bff..e82751438d6 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -5,6 +5,8 @@
 
 2017-09-01  Jeff Law  <law@redhat.com>
 
+	* gcc.target/i386/const-in-bss.c: New test.
+
 	PR tree-optimization/82052
 	* gcc.c-torture/compile/pr82052.c: New test.
 
diff --git a/gcc/testsuite/gcc.target/i386/const-in-bss.c b/gcc/testsuite/gcc.target/i386/const-in-bss.c
new file mode 100644
index 00000000000..c70aa0bcb4e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/const-in-bss.c
@@ -0,0 +1,6 @@
+/* { dg-do compile { target *-*-linux* } } */
+
+__attribute__((section("readonly1"))) const int foo1c;
+
+/* { dg-final { scan-assembler "readonly1,\"a\"" } } */
+/* { dg-final { scan-assembler-not "readonly1,\"aw\"" } } */
diff --git a/gcc/varasm.c b/gcc/varasm.c
index e5393377a43..d38d2c2721b 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -976,16 +976,16 @@ decode_reg_name (const char *name)
 bool
 bss_initializer_p (const_tree decl)
 {
-  return (DECL_INITIAL (decl) == NULL
-	  /* In LTO we have no errors in program; error_mark_node is used
-	     to mark offlined constructors.  */
-	  || (DECL_INITIAL (decl) == error_mark_node
-	      && !in_lto_p)
-	  || (flag_zero_initialized_in_bss
-	      /* Leave constant zeroes in .rodata so they
-		 can be shared.  */
-	      && !TREE_READONLY (decl)
-	      && initializer_zerop (DECL_INITIAL (decl))));
+  /* Do not put constants into the .bss section, they belong in a readonly
+     section.  */
+  return (!TREE_READONLY (decl)
+	  && (DECL_INITIAL (decl) == NULL
+	      /* In LTO we have no errors in program; error_mark_node is used
+	         to mark offlined constructors.  */
+	      || (DECL_INITIAL (decl) == error_mark_node
+	          && !in_lto_p)
+	      || (flag_zero_initialized_in_bss
+	          && initializer_zerop (DECL_INITIAL (decl)))));
 }
 
 /* Compute the alignment of variable specified by DECL.
@@ -6517,7 +6517,8 @@ categorize_decl_for_section (const_tree decl, int reloc)
 	ret = SECCAT_BSS;
       else if (! TREE_READONLY (decl)
 	       || TREE_SIDE_EFFECTS (decl)
-	       || ! TREE_CONSTANT (DECL_INITIAL (decl)))
+	       || (DECL_INITIAL (decl)
+		   && ! TREE_CONSTANT (DECL_INITIAL (decl))))
 	{
 	  /* Here the reloc_rw_mask is not testing whether the section should
 	     be read-only or not, but whether the dynamic link will have to
@@ -6537,7 +6538,8 @@ categorize_decl_for_section (const_tree decl, int reloc)
 	   location.  -fmerge-all-constants allows even that (at the
 	   expense of not conforming).  */
 	ret = SECCAT_RODATA;
-      else if (TREE_CODE (DECL_INITIAL (decl)) == STRING_CST)
+      else if (DECL_INITIAL (decl)
+	       && TREE_CODE (DECL_INITIAL (decl)) == STRING_CST)
 	ret = SECCAT_RODATA_MERGE_STR_INIT;
       else
 	ret = SECCAT_RODATA_MERGE_CONST;
diff mbox

Patch

diff --git a/gcc/varasm.c b/gcc/varasm.c
index 45611a9a858..eaf78b28bc1 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -969,11 +969,17 @@  decode_reg_name (const char *name)
 }
 
 
-/* Return true if DECL's initializer is suitable for a BSS section.  */
+/*
+ * Return true if DECL's initializer is suitable for a BSS section.
+ * If there is an explicit section name attribute, assume that it is not
+ * for a BSS section, independent of the name.
+ */
 
 bool
 bss_initializer_p (const_tree decl)
 {
+  if (DECL_SECTION_NAME (decl) != NULL)
+    return false;
   return (DECL_INITIAL (decl) == NULL
 	  /* In LTO we have no errors in program; error_mark_node is used
 	     to mark offlined constructors.  */
@@ -6460,7 +6466,7 @@  categorize_decl_for_section (const_tree decl, int reloc)
 	ret = SECCAT_BSS;
       else if (! TREE_READONLY (decl)
 	       || TREE_SIDE_EFFECTS (decl)
-	       || ! TREE_CONSTANT (DECL_INITIAL (decl)))
+	       || (DECL_INITIAL(decl) != NULL && ! TREE_CONSTANT (DECL_INITIAL (decl))))
 	{
 	  /* Here the reloc_rw_mask is not testing whether the section should
 	     be read-only or not, but whether the dynamic link will have to
@@ -6504,6 +6510,7 @@  categorize_decl_for_section (const_tree decl, int reloc)
 	 no concept of a read-only thread-local-data section.  */
       if (ret == SECCAT_BSS
 	       || (flag_zero_initialized_in_bss
+		   && DECL_INITIAL(decl) != NULL
 		   && initializer_zerop (DECL_INITIAL (decl))))
 	ret = SECCAT_TBSS;
       else