Patchwork [pr31490] Fix section type error.

login
register
mail settings
Submitter Steve Ellcey
Date Nov. 17, 2010, 6:18 p.m.
Message ID <201011171818.oAHIIeE05221@lucas.cup.hp.com>
Download mbox | patch
Permalink /patch/71608/
State New
Headers show

Comments

Steve Ellcey - Nov. 17, 2010, 6:18 p.m.
PR 31490 (Compile error section type conflict) has been sitting around
for a while even though there are two patches for it.  I picked one of
those patches (from comment #9) and tested it and would like it check it
in.  This patch seems better then the one in comment #10 because it
avoids the mis-catorization instead of fixing it up later after the
mistake was made.

Also, this patch was approved by Ian, modulo a comment request, back in
2007 (see http://gcc.gnu.org/ml/gcc-patches/2007-05/msg01280.html) and I
have updated the comment to address that issue.  I tested it on my HP-UX
and Linux boxes with no regressions.

OK to checkin?

Steve Ellcey
sje@cup.hp.com


2010-11-17  Dinar Temirbulatov <dtemirbulatov@gmail.com>
	    Steve Ellcey  <sje@cup.hp.com>

	PR middle-end/31490
	* varasm.c (categorize_decl_for_section): Ignore reloc_rw_mask
	if section attribute used.
Ian Taylor - Nov. 17, 2010, 6:40 p.m.
Steve Ellcey <sje@cup.hp.com> writes:

> PR 31490 (Compile error section type conflict) has been sitting around
> for a while even though there are two patches for it.  I picked one of
> those patches (from comment #9) and tested it and would like it check it
> in.  This patch seems better then the one in comment #10 because it
> avoids the mis-catorization instead of fixing it up later after the
> mistake was made.
>
> Also, this patch was approved by Ian, modulo a comment request, back in
> 2007 (see http://gcc.gnu.org/ml/gcc-patches/2007-05/msg01280.html) and I
> have updated the comment to address that issue.  I tested it on my HP-UX
> and Linux boxes with no regressions.
>
> OK to checkin?
>
> Steve Ellcey
> sje@cup.hp.com
>
>
> 2010-11-17  Dinar Temirbulatov <dtemirbulatov@gmail.com>
> 	    Steve Ellcey  <sje@cup.hp.com>
>
> 	PR middle-end/31490
> 	* varasm.c (categorize_decl_for_section): Ignore reloc_rw_mask
> 	if section attribute used.


> Index: varasm.c
> ===================================================================
> --- varasm.c	(revision 166852)
> +++ varasm.c	(working copy)
> @@ -6102,13 +6102,16 @@ categorize_decl_for_section (const_tree 
>  	  /* 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
>  	     do something.  If so, we wish to segregate the data in order to
> -	     minimize cache misses inside the dynamic linker.  */
> -	  if (reloc & targetm.asm_out.reloc_rw_mask ())
> +	     minimize cache misses inside the dynamic linker.  If the data
> +	     has a section attribute, ignore the value of reloc_rw_mask().  */
> +	  if (reloc & targetm.asm_out.reloc_rw_mask ()
> +	      && !lookup_attribute ("section", DECL_ATTRIBUTES (decl)))
>  	    ret = reloc == 1 ? SECCAT_DATA_REL_LOCAL : SECCAT_DATA_REL;
>  	  else
>  	    ret = SECCAT_DATA;
>  	}
> -      else if (reloc & targetm.asm_out.reloc_rw_mask ())
> +      else if (reloc & targetm.asm_out.reloc_rw_mask ()
> +	       && !lookup_attribute ("section", DECL_ATTRIBUTES (decl)))
>  	ret = reloc == 1 ? SECCAT_DATA_REL_RO_LOCAL : SECCAT_DATA_REL_RO;
>        else if (reloc || flag_merge_constants < 2)
>  	/* C and C++ don't allow different variables to share the same


The patch is still OK but I would still like the comment to be extended.
Your extended comment says what the code does.  That is unnecessary--I
can see what it does.  I would like the comment to say why it does that.

Thanks.

Ian
Steve Ellcey - Nov. 17, 2010, 7:24 p.m.
On Wed, 2010-11-17 at 10:40 -0800, Ian Lance Taylor wrote:

> The patch is still OK but I would still like the comment to be extended.
> Your extended comment says what the code does.  That is unnecessary--I
> can see what it does.  I would like the comment to say why it does that.
> 
> Thanks.
> 
> Ian

How about this:

          /* 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
             do something.  If so, we wish to segregate the data in order to
             minimize cache misses inside the dynamic linker.  If the data
             has a section attribute, ignore reloc_rw_mask() so that all data
             in a given named section is catagorized in the same way.  */

Steve Ellcey
sje@cup.hp.com
Ian Taylor - Nov. 17, 2010, 9:07 p.m.
Steve Ellcey <sje@cup.hp.com> writes:

> On Wed, 2010-11-17 at 10:40 -0800, Ian Lance Taylor wrote:
>
>> The patch is still OK but I would still like the comment to be extended.
>> Your extended comment says what the code does.  That is unnecessary--I
>> can see what it does.  I would like the comment to say why it does that.
>> 
>> Thanks.
>> 
>> Ian
>
> How about this:
>
>           /* 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
>              do something.  If so, we wish to segregate the data in order to
>              minimize cache misses inside the dynamic linker.  If the data
>              has a section attribute, ignore reloc_rw_mask() so that all data
>              in a given named section is catagorized in the same way.  */

Works for me.

Thanks.

Ian
Jakub Jelinek - Jan. 27, 2011, 11:30 a.m.
On Wed, Nov 17, 2010 at 10:40:51AM -0800, Ian Lance Taylor wrote:
> > PR 31490 (Compile error section type conflict) has been sitting around
> > for a while even though there are two patches for it.  I picked one of
> > those patches (from comment #9) and tested it and would like it check it
> > in.  This patch seems better then the one in comment #10 because it
> > avoids the mis-catorization instead of fixing it up later after the
> > mistake was made.
> >
> > Also, this patch was approved by Ian, modulo a comment request, back in
> > 2007 (see http://gcc.gnu.org/ml/gcc-patches/2007-05/msg01280.html) and I
> > have updated the comment to address that issue.  I tested it on my HP-UX
> > and Linux boxes with no regressions.
> >
> > 2010-11-17  Dinar Temirbulatov <dtemirbulatov@gmail.com>
> > 	    Steve Ellcey  <sje@cup.hp.com>
> >
> > 	PR middle-end/31490
> > 	* varasm.c (categorize_decl_for_section): Ignore reloc_rw_mask
> > 	if section attribute used.

This patch broke glibc, nscd is now DT_TEXTREL which is rejected by SELinux.
Ignoring reloc_rw_mask is just always wrong IMNSHO, the patch should be
reverted.

nscd uses something like:
static const char *const foo __attribute__((used, section("bar"))) = "baz";
and is compiled with -fpie (but the same problem is with -fpic).

I think PR31490 contains a saner patch which just doesn't complain about
write vs. non-write conflicts in named sections, in that case write should
just win.  Or we could add some extra SECTION_* bit which would describe the
.rodata.rel property of the section, i.e. readonly after relocation
processing, and not to complain about just those for named sections.

> > --- varasm.c	(revision 166852)
> > +++ varasm.c	(working copy)
> > @@ -6102,13 +6102,16 @@ categorize_decl_for_section (const_tree 
> >  	  /* 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
> >  	     do something.  If so, we wish to segregate the data in order to
> > -	     minimize cache misses inside the dynamic linker.  */
> > -	  if (reloc & targetm.asm_out.reloc_rw_mask ())
> > +	     minimize cache misses inside the dynamic linker.  If the data
> > +	     has a section attribute, ignore the value of reloc_rw_mask().  */
> > +	  if (reloc & targetm.asm_out.reloc_rw_mask ()
> > +	      && !lookup_attribute ("section", DECL_ATTRIBUTES (decl)))
> >  	    ret = reloc == 1 ? SECCAT_DATA_REL_LOCAL : SECCAT_DATA_REL;
> >  	  else
> >  	    ret = SECCAT_DATA;
> >  	}
> > -      else if (reloc & targetm.asm_out.reloc_rw_mask ())
> > +      else if (reloc & targetm.asm_out.reloc_rw_mask ()
> > +	       && !lookup_attribute ("section", DECL_ATTRIBUTES (decl)))
> >  	ret = reloc == 1 ? SECCAT_DATA_REL_RO_LOCAL : SECCAT_DATA_REL_RO;
> >        else if (reloc || flag_merge_constants < 2)
> >  	/* C and C++ don't allow different variables to share the same
> 
> 
> The patch is still OK but I would still like the comment to be extended.
> Your extended comment says what the code does.  That is unnecessary--I
> can see what it does.  I would like the comment to say why it does that.

	Jakub

Patch

Index: varasm.c
===================================================================
--- varasm.c	(revision 166852)
+++ varasm.c	(working copy)
@@ -6102,13 +6102,16 @@  categorize_decl_for_section (const_tree 
 	  /* 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
 	     do something.  If so, we wish to segregate the data in order to
-	     minimize cache misses inside the dynamic linker.  */
-	  if (reloc & targetm.asm_out.reloc_rw_mask ())
+	     minimize cache misses inside the dynamic linker.  If the data
+	     has a section attribute, ignore the value of reloc_rw_mask().  */
+	  if (reloc & targetm.asm_out.reloc_rw_mask ()
+	      && !lookup_attribute ("section", DECL_ATTRIBUTES (decl)))
 	    ret = reloc == 1 ? SECCAT_DATA_REL_LOCAL : SECCAT_DATA_REL;
 	  else
 	    ret = SECCAT_DATA;
 	}
-      else if (reloc & targetm.asm_out.reloc_rw_mask ())
+      else if (reloc & targetm.asm_out.reloc_rw_mask ()
+	       && !lookup_attribute ("section", DECL_ATTRIBUTES (decl)))
 	ret = reloc == 1 ? SECCAT_DATA_REL_RO_LOCAL : SECCAT_DATA_REL_RO;
       else if (reloc || flag_merge_constants < 2)
 	/* C and C++ don't allow different variables to share the same



2010-11-17  Steve Ellcey  <sje@cup.hp.com>

	PR middle-end/31490
	* gcc.dg/pr31490.c: New test.


Index: gcc.dg/pr31490.c
===================================================================
--- gcc.dg/pr31490.c	(revision 0)
+++ gcc.dg/pr31490.c	(revision 0)
@@ -0,0 +1,6 @@ 
+/* PR middle-end/31490 */
+/* { dg-do compile } */
+/* { dg-require-named-sections "" } */
+int cpu (void *attr) {}
+const unsigned long x __attribute__((section("foo"))) =  (unsigned long)&cpu;
+const unsigned long g __attribute__((section("foo"))) = 0;