| Message ID | 201011171818.oAHIIeE05221@lucas.cup.hp.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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;