| Message ID | CAB=4xhrks=KVuKKBOxwS9iV9==sdhKmu_9RqopiSeKC-+Hb08g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
ping? On Thu, Sep 15, 2016 at 4:09 PM, Roland McGrath <mcgrathr@google.com> wrote: > This fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77609 (which I've > just filed). > > OK for trunk? > > I'm not sure if this kind of fix is appropriate for gcc-6-branch or not, > but I'd like to backport it there too if it is acceptable. > > > Thanks, > Roland > > > gcc/ > 2016-09-15 Roland McGrath <<mcgrathr@google.com> > > PR other/77609 > * varasm.c (default_section_type_flags): Set SECTION_NOTYPE for > any section for which we don't know a specific type it should have, > regardless of name. Previously this was done only for the exact > names ".init_array", ".fini_array", and ".preinit_array". > > diff --git a/gcc/varasm.c b/gcc/varasm.c > index 00a9b30..0d7ea38 100644 > --- a/gcc/varasm.c > +++ b/gcc/varasm.c > @@ -6210,15 +6210,20 @@ default_section_type_flags (tree decl, const > char *name, int reloc) > || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) > flags |= SECTION_TLS | SECTION_BSS; > > - /* These three sections have special ELF types. They are neither > - SHT_PROGBITS nor SHT_NOBITS, so when changing sections we don't > - want to print a section type (@progbits or @nobits). If someone > - is silly enough to emit code or TLS variables to one of these > - sections, then don't handle them specially. */ > - if (!(flags & (SECTION_CODE | SECTION_BSS | SECTION_TLS)) > - && (strcmp (name, ".init_array") == 0 > - || strcmp (name, ".fini_array") == 0 > - || strcmp (name, ".preinit_array") == 0)) > + /* Various sections have special ELF types that the assembler will > + assign by default based on the name. They are neither SHT_PROGBITS > + nor SHT_NOBITS, so when changing sections we don't want to print a > + section type (@progbits or @nobits). Rather than duplicating the > + assembler's knowledge of what those special name patterns are, just > + let the assembler choose the type if we don't know a specific > + reason to set it to something other than the default. SHT_PROGBITS > + is the default for sections whose name is not specially known to > + the assembler, so it does no harm to leave the choice to the > + assembler when @progbits is the best thing we know to use. If > + someone is silly enough to emit code or TLS variables to one of > + these sections, then don't handle them specially. */ > + if (!(flags & (SECTION_CODE | SECTION_BSS | SECTION_TLS | SECTION_ENTSIZE)) > + && !(HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))) > flags |= SECTION_NOTYPE; > > return flags;
Anybody want to look at this? It rebases identically on today's trunk. I'd like to commit it to trunk and gcc-7-branch and gcc-6-branch ideally. Thanks, Roland On Thu, Sep 22, 2016 at 1:15 PM, Roland McGrath <mcgrathr@google.com> wrote: > ping? > > On Thu, Sep 15, 2016 at 4:09 PM, Roland McGrath <mcgrathr@google.com> wrote: >> This fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77609 (which I've >> just filed). >> >> OK for trunk? >> >> I'm not sure if this kind of fix is appropriate for gcc-6-branch or not, >> but I'd like to backport it there too if it is acceptable. >> >> >> Thanks, >> Roland >> >> >> gcc/ >> 2016-09-15 Roland McGrath <<mcgrathr@google.com> >> >> PR other/77609 >> * varasm.c (default_section_type_flags): Set SECTION_NOTYPE for >> any section for which we don't know a specific type it should have, >> regardless of name. Previously this was done only for the exact >> names ".init_array", ".fini_array", and ".preinit_array". >> >> diff --git a/gcc/varasm.c b/gcc/varasm.c >> index 00a9b30..0d7ea38 100644 >> --- a/gcc/varasm.c >> +++ b/gcc/varasm.c >> @@ -6210,15 +6210,20 @@ default_section_type_flags (tree decl, const >> char *name, int reloc) >> || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) >> flags |= SECTION_TLS | SECTION_BSS; >> >> - /* These three sections have special ELF types. They are neither >> - SHT_PROGBITS nor SHT_NOBITS, so when changing sections we don't >> - want to print a section type (@progbits or @nobits). If someone >> - is silly enough to emit code or TLS variables to one of these >> - sections, then don't handle them specially. */ >> - if (!(flags & (SECTION_CODE | SECTION_BSS | SECTION_TLS)) >> - && (strcmp (name, ".init_array") == 0 >> - || strcmp (name, ".fini_array") == 0 >> - || strcmp (name, ".preinit_array") == 0)) >> + /* Various sections have special ELF types that the assembler will >> + assign by default based on the name. They are neither SHT_PROGBITS >> + nor SHT_NOBITS, so when changing sections we don't want to print a >> + section type (@progbits or @nobits). Rather than duplicating the >> + assembler's knowledge of what those special name patterns are, just >> + let the assembler choose the type if we don't know a specific >> + reason to set it to something other than the default. SHT_PROGBITS >> + is the default for sections whose name is not specially known to >> + the assembler, so it does no harm to leave the choice to the >> + assembler when @progbits is the best thing we know to use. If >> + someone is silly enough to emit code or TLS variables to one of >> + these sections, then don't handle them specially. */ >> + if (!(flags & (SECTION_CODE | SECTION_BSS | SECTION_TLS | SECTION_ENTSIZE)) >> + && !(HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))) >> flags |= SECTION_NOTYPE; >> >> return flags;
On Sat, Feb 24, 2018 at 1:54 AM, Roland McGrath via gcc-patches <gcc-patches@gcc.gnu.org> wrote: > Anybody want to look at this? > > It rebases identically on today's trunk. I'd like to commit it to > trunk and gcc-7-branch and gcc-6-branch ideally. You are recreating the conditions used in default_elf_asm_named_section, so I think you ought to have comments referring back and forth between them. This is OK with the two additional comments. Ian > On Thu, Sep 22, 2016 at 1:15 PM, Roland McGrath <mcgrathr@google.com> wrote: >> ping? >> >> On Thu, Sep 15, 2016 at 4:09 PM, Roland McGrath <mcgrathr@google.com> wrote: >>> This fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77609 (which I've >>> just filed). >>> >>> OK for trunk? >>> >>> I'm not sure if this kind of fix is appropriate for gcc-6-branch or not, >>> but I'd like to backport it there too if it is acceptable. >>> >>> >>> Thanks, >>> Roland >>> >>> >>> gcc/ >>> 2016-09-15 Roland McGrath <<mcgrathr@google.com> >>> >>> PR other/77609 >>> * varasm.c (default_section_type_flags): Set SECTION_NOTYPE for >>> any section for which we don't know a specific type it should have, >>> regardless of name. Previously this was done only for the exact >>> names ".init_array", ".fini_array", and ".preinit_array". >>> >>> diff --git a/gcc/varasm.c b/gcc/varasm.c >>> index 00a9b30..0d7ea38 100644 >>> --- a/gcc/varasm.c >>> +++ b/gcc/varasm.c >>> @@ -6210,15 +6210,20 @@ default_section_type_flags (tree decl, const >>> char *name, int reloc) >>> || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) >>> flags |= SECTION_TLS | SECTION_BSS; >>> >>> - /* These three sections have special ELF types. They are neither >>> - SHT_PROGBITS nor SHT_NOBITS, so when changing sections we don't >>> - want to print a section type (@progbits or @nobits). If someone >>> - is silly enough to emit code or TLS variables to one of these >>> - sections, then don't handle them specially. */ >>> - if (!(flags & (SECTION_CODE | SECTION_BSS | SECTION_TLS)) >>> - && (strcmp (name, ".init_array") == 0 >>> - || strcmp (name, ".fini_array") == 0 >>> - || strcmp (name, ".preinit_array") == 0)) >>> + /* Various sections have special ELF types that the assembler will >>> + assign by default based on the name. They are neither SHT_PROGBITS >>> + nor SHT_NOBITS, so when changing sections we don't want to print a >>> + section type (@progbits or @nobits). Rather than duplicating the >>> + assembler's knowledge of what those special name patterns are, just >>> + let the assembler choose the type if we don't know a specific >>> + reason to set it to something other than the default. SHT_PROGBITS >>> + is the default for sections whose name is not specially known to >>> + the assembler, so it does no harm to leave the choice to the >>> + assembler when @progbits is the best thing we know to use. If >>> + someone is silly enough to emit code or TLS variables to one of >>> + these sections, then don't handle them specially. */ >>> + if (!(flags & (SECTION_CODE | SECTION_BSS | SECTION_TLS | SECTION_ENTSIZE)) >>> + && !(HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))) >>> flags |= SECTION_NOTYPE; >>> >>> return flags;
On Mon, Feb 26, 2018 at 8:11 PM, Ian Lance Taylor <iant@google.com> wrote: > You are recreating the conditions used in > default_elf_asm_named_section, so I think you ought to have comments > referring back and forth between them. > > This is OK with the two additional comments. Thanks for the review. I've added those comments. However, in testing on x86_64-linux-gnu it caused a regression in: gcc/testsuite/gcc.target/i386/pr25254.c which got the "section type conflict" error. This is because x86_64_elf_select_section for that case calls: get_section (".lrodata", SECTION_LARGE, NULL) but something else had previously instantiated the section via the section_type_flags logic that now adds in SECTION_NOTYPE. I addressed this by making get_section accept having SECTION_NOTYPE and not as a non-conflict if none of SECTION_BSS et al is present. That seemed like a better bet than finding every get_section caller and making sure they use SECTION_NOTYPE when appropriate. But I'm not sure if there might be some downside to that logic or if there is a third way to resolve this that's better than either of those two. Here's the new patch I'd like to commit. It has no regressions on x86_64-linux-gnu, but I'm not set up to test other configurations. gcc/ 2018-02-27 Roland McGrath <mcgrathr@google.com> PR other/77609 * varasm.c (default_section_type_flags): Set SECTION_NOTYPE for any section for which we don't know a specific type it should have, regardless of name. Previously this was done only for the exact names ".init_array", ".fini_array", and ".preinit_array". (default_elf_asm_named_section): Add comment about relationship with default_section_type_flags and SECTION_NOTYPE. (get_section): Don't consider it a type conflict if one side has SECTION_NOTYPE and the other doesn't, as long as neither has the SECTION_BSS et al used in the default_section_type_flags logic. diff --git a/gcc/varasm.c b/gcc/varasm.c index 6e345d39d31..e488f866011 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -296,6 +296,17 @@ get_section (const char *name, unsigned int flags, tree decl) else { sect = *slot; + /* It is fine if one of the sections has SECTION_NOTYPE as long as + the other has none of the contrary flags (see the logic at the end + of default_section_type_flags, below). */ + if (((sect->common.flags ^ flags) & SECTION_NOTYPE) + && !((sect->common.flags | flags) + & (SECTION_CODE | SECTION_BSS | SECTION_TLS | SECTION_ENTSIZE + | (HAVE_COMDAT_GROUP ? SECTION_LINKONCE : 0)))) + { + sect->common.flags |= SECTION_NOTYPE; + flags |= SECTION_NOTYPE; + } if ((sect->common.flags & ~SECTION_DECLARED) != flags && ((sect->common.flags | flags) & SECTION_OVERRIDE) == 0) { @@ -6361,15 +6372,23 @@ default_section_type_flags (tree decl, const char *name, int reloc) || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) flags |= SECTION_TLS | SECTION_BSS; - /* These three sections have special ELF types. They are neither - SHT_PROGBITS nor SHT_NOBITS, so when changing sections we don't - want to print a section type (@progbits or @nobits). If someone - is silly enough to emit code or TLS variables to one of these - sections, then don't handle them specially. */ - if (!(flags & (SECTION_CODE | SECTION_BSS | SECTION_TLS)) - && (strcmp (name, ".init_array") == 0 - || strcmp (name, ".fini_array") == 0 - || strcmp (name, ".preinit_array") == 0)) + /* Various sections have special ELF types that the assembler will + assign by default based on the name. They are neither SHT_PROGBITS + nor SHT_NOBITS, so when changing sections we don't want to print a + section type (@progbits or @nobits). Rather than duplicating the + assembler's knowledge of what those special name patterns are, just + let the assembler choose the type if we don't know a specific + reason to set it to something other than the default. SHT_PROGBITS + is the default for sections whose name is not specially known to + the assembler, so it does no harm to leave the choice to the + assembler when @progbits is the best thing we know to use. If + someone is silly enough to emit code or TLS variables to one of + these sections, then don't handle them specially. + + default_elf_asm_named_section (below) handles the BSS, TLS, ENTSIZE, and + LINKONCE cases when NOTYPE is not set, so leave those to its logic. */ + if (!(flags & (SECTION_CODE | SECTION_BSS | SECTION_TLS | SECTION_ENTSIZE)) + && !(HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))) flags |= SECTION_NOTYPE; return flags; @@ -6455,6 +6474,10 @@ default_elf_asm_named_section (const char *name, unsigned int flags, fprintf (asm_out_file, "\t.section\t%s,\"%s\"", name, flagchars); + /* default_section_type_flags (above) knows which flags need special + handling here, and sets NOTYPE when none of these apply so that the + assembler's logic for default types can apply to user-chosen + section names. */ if (!(flags & SECTION_NOTYPE)) { const char *type;
On Tue, Feb 27, 2018 at 6:01 PM, Roland McGrath <mcgrathr@google.com> wrote: > On Mon, Feb 26, 2018 at 8:11 PM, Ian Lance Taylor <iant@google.com> wrote: >> You are recreating the conditions used in >> default_elf_asm_named_section, so I think you ought to have comments >> referring back and forth between them. >> >> This is OK with the two additional comments. > > Thanks for the review. I've added those comments. > > However, in testing on x86_64-linux-gnu it caused a regression in: > gcc/testsuite/gcc.target/i386/pr25254.c > which got the "section type conflict" error. > > This is because x86_64_elf_select_section for that case calls: > get_section (".lrodata", SECTION_LARGE, NULL) > but something else had previously instantiated the section via > the section_type_flags logic that now adds in SECTION_NOTYPE. > > I addressed this by making get_section accept having SECTION_NOTYPE and not > as a non-conflict if none of SECTION_BSS et al is present. That seemed > like a better bet than finding every get_section caller and making sure > they use SECTION_NOTYPE when appropriate. But I'm not sure if there might > be some downside to that logic or if there is a third way to resolve this > that's better than either of those two. > > Here's the new patch I'd like to commit. It has no regressions on > x86_64-linux-gnu, but I'm not set up to test other configurations. > > > gcc/ > 2018-02-27 Roland McGrath <mcgrathr@google.com> > > PR other/77609 > * varasm.c (default_section_type_flags): Set SECTION_NOTYPE for > any section for which we don't know a specific type it should have, > regardless of name. Previously this was done only for the exact > names ".init_array", ".fini_array", and ".preinit_array". > (default_elf_asm_named_section): Add comment about > relationship with default_section_type_flags and SECTION_NOTYPE. > (get_section): Don't consider it a type conflict if one side has > SECTION_NOTYPE and the other doesn't, as long as neither has the > SECTION_BSS et al used in the default_section_type_flags logic. Still OK, but it should wait until after the tree is back in stage 1. Ian
On Tue, Feb 27, 2018 at 8:14 PM, Ian Lance Taylor <iant@google.com> wrote:
> Still OK, but it should wait until after the tree is back in stage 1.
I've been hoping to get this fixed on stable branches (6, 7).
Are you saying that this bug can only be fixed in 9?
Or will it be OK to backport to 6, 7, and 8 once stage 1 opens for 9?
Thanks,
Roland
On Wed, Feb 28, 2018 at 3:56 PM, Roland McGrath <mcgrathr@google.com> wrote: > On Tue, Feb 27, 2018 at 8:14 PM, Ian Lance Taylor <iant@google.com> wrote: >> Still OK, but it should wait until after the tree is back in stage 1. > > I've been hoping to get this fixed on stable branches (6, 7). > Are you saying that this bug can only be fixed in 9? > Or will it be OK to backport to 6, 7, and 8 once stage 1 opens for 9? Since you already ran into trouble, I'm worried that will be other trouble. At least, it's hard for me to convince myself that there won't be any other trouble. And I don't think we should introduce trouble at this point in the release cycle. So, yes, I'd prefer to see this fixed in GCC 9, and, once people have had a chance to test it on various systems, backport to earlier versions if appropriate. Ian
OK. I'll come back in stage 1. Thanks, Roland
I'm back for stage 1! The same patch from https://gcc.gnu.org/ml/gcc-patches/2018-02/msg01549.html rebases cleanly and I didn't change anything but the date on the log entry since what I posted there. The fresh rebase is on the roland/pr77609 git branch for your convenience. It has no check-gcc failures on x86_64-linux-gnu. OK to commit to trunk now? When will be the right time to raise the question of backporting it, perhaps shortly after the 8 release? Thanks, Roland
ping On Sat, Apr 28, 2018 at 2:42 AM Roland McGrath <mcgrathr@google.com> wrote: > I'm back for stage 1! > The same patch from https://gcc.gnu.org/ml/gcc-patches/2018-02/msg01549.html > rebases cleanly and I didn't change anything but the date on the log entry > since what I posted there. The fresh rebase is on the roland/pr77609 git > branch for your convenience. > It has no check-gcc failures on x86_64-linux-gnu. > OK to commit to trunk now? > When will be the right time to raise the question of backporting it, > perhaps shortly after the 8 release? > Thanks, > Roland
On Sat, Apr 28, 2018 at 2:42 AM, Roland McGrath <mcgrathr@google.com> wrote: > I'm back for stage 1! > > The same patch from https://gcc.gnu.org/ml/gcc-patches/2018-02/msg01549.html > rebases cleanly and I didn't change anything but the date on the log entry > since what I posted there. The fresh rebase is on the roland/pr77609 git > branch for your convenience. > > It has no check-gcc failures on x86_64-linux-gnu. > > OK to commit to trunk now? > > When will be the right time to raise the question of backporting it, > perhaps shortly after the 8 release? This is OK for trunk. Thanks. I think a backport would be OK after a month. Ian
Committed. Thanks, Roland
A direct cherry-pick of the trunk change (db7548a2771bbf34cf7430712af7ac670b429958 / r259969) applies fine to today's 8 branch and has no check-gcc regressions on x86_64-linux-gnu. OK to commit to 8 branch now? Thanks, Roland
On Wed, Jun 13, 2018 at 5:52 PM, Roland McGrath <mcgrathr@google.com> wrote: > A direct cherry-pick of the trunk change > (db7548a2771bbf34cf7430712af7ac670b429958 / r259969) applies fine to > today's 8 branch and has no check-gcc regressions on x86_64-linux-gnu. > > OK to commit to 8 branch now? Sure. Ian
Committed. Thanks, Roland
diff --git a/gcc/varasm.c b/gcc/varasm.c index 00a9b30..0d7ea38 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -6210,15 +6210,20 @@ default_section_type_flags (tree decl, const char *name, int reloc) || strncmp (name, ".gnu.linkonce.tb.", 17) == 0) flags |= SECTION_TLS | SECTION_BSS; - /* These three sections have special ELF types. They are neither - SHT_PROGBITS nor SHT_NOBITS, so when changing sections we don't - want to print a section type (@progbits or @nobits). If someone - is silly enough to emit code or TLS variables to one of these - sections, then don't handle them specially. */ - if (!(flags & (SECTION_CODE | SECTION_BSS | SECTION_TLS)) - && (strcmp (name, ".init_array") == 0 - || strcmp (name, ".fini_array") == 0 - || strcmp (name, ".preinit_array") == 0)) + /* Various sections have special ELF types that the assembler will + assign by default based on the name. They are neither SHT_PROGBITS + nor SHT_NOBITS, so when changing sections we don't want to print a + section type (@progbits or @nobits). Rather than duplicating the + assembler's knowledge of what those special name patterns are, just + let the assembler choose the type if we don't know a specific + reason to set it to something other than the default. SHT_PROGBITS + is the default for sections whose name is not specially known to + the assembler, so it does no harm to leave the choice to the + assembler when @progbits is the best thing we know to use. If + someone is silly enough to emit code or TLS variables to one of + these sections, then don't handle them specially. */ + if (!(flags & (SECTION_CODE | SECTION_BSS | SECTION_TLS | SECTION_ENTSIZE)) + && !(HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))) flags |= SECTION_NOTYPE;