Fix inconsistent section flags

Fix inconsistent section flags

[Joerg Sonnenberger]

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

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

[-- Attachment #2: varasm.c.diff --]
[-- Type: text/plain, Size: 1497 bytes --]

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)
 }
 
 \f
-/* 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

Re: Fix inconsistent section flags

[Joerg Sonnenberger]

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

One more patch needed for another edge condition.

Joerg

[-- Attachment #2: varasm.c.diff2 --]
[-- Type: text/plain, Size: 688 bytes --]

Index: src/external/gpl3/gcc/dist/gcc/varasm.c
diff -u src/external/gpl3/gcc/dist/gcc/varasm.c:1.2 src/external/gpl3/gcc/dist/gcc/varasm.c:1.3
--- src/external/gpl3/gcc/dist/gcc/varasm.c:1.2	Mon Jul 17 19:53:10 2017
+++ src/external/gpl3/gcc/dist/gcc/varasm.c	Sat Jul 22 20:52:52 2017
@@ -6428,7 +6428,8 @@
 	   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;

Re: Fix inconsistent section flags

[coypu]

ping

Re: Fix inconsistent section flags

[Jeff Law]

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

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

[-- Attachment #2: Q --]
[-- Type: text/plain, Size: 2100 bytes --]

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;

Re: Fix inconsistent section flags

[Joerg Sonnenberger]

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

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

[-- Attachment #2: varasm.c.diff --]
[-- Type: text/plain, Size: 763 bytes --]

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;

Re: Fix inconsistent section flags

[Joerg Sonnenberger]

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

Re: Fix inconsistent section flags

[Jeff Law]

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

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

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 3841 bytes --]

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;