| Message ID | ydd62qtaxg1.fsf@manam.CeBiTec.Uni-Bielefeld.DE |
|---|---|
| State | New |
| Headers | show |
On Tue, 5 Apr 2011, Rainer Orth wrote: > As described in the PR, it seems to make more sense to avoid to use the > visibility attribute on targets that don't support it rather than using > it unconditionally and later (and incompletely) prune the resulting > warning. > > The following patch does exactly that. It now needs to document the > explicit visibility requirement in gcc.dg/lto/20081222_0.c (the main > file of the testcase, rather than in 20081222_1.c that uses it) so the > whole testcase is skipped. > > Bootstrapped on mainline without regressions on i386-pc-solaris2.8 with > Sun as which doesn't support the visibility attribute. > > Ok for mainline and the 4.6 branch after testing? The testsuite change is definitely ok. I'm not sure about the lto.c changes - as we make TU statics global but with hidden visibility those symbols may clash with other global syms at link time (a pre-existing problem it seems), now with your change it also may clash with symbols from shared libs. Honza, why is this promotion to global ok at all? I can deliberately break it by linking in a non-LTO object which clashes with the mangled symbol. Thanks, Richard. > Thanks. > Rainer > > > 2011-03-27 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> > > gcc/lto: > PR lto/47334) > * lto.c (promote_var): Only set VISIBILITY_HIDDEN if > HAVE_GAS_HIDDEN. > (promote_fn): Likewise. > > gcc/testsuite: > PR lto/47334) > * lib/lto.exp (lto_prune_warns): Don't prune visibility warning. > * gcc.dg/lto/20081222_0.c: Add dg-require-visibility. > > diff --git a/gcc/lto/lto.c b/gcc/lto/lto.c > --- a/gcc/lto/lto.c > +++ b/gcc/lto/lto.c > @@ -1,5 +1,5 @@ > /* Top-level LTO routines. > - Copyright 2009, 2010 Free Software Foundation, Inc. > + Copyright 2009, 2010, 2011 Free Software Foundation, Inc. > Contributed by CodeSourcery, Inc. > > This file is part of GCC. > @@ -1271,11 +1271,13 @@ promote_var (struct varpool_node *vnode) > return false; > gcc_assert (flag_wpa); > TREE_PUBLIC (vnode->decl) = 1; > +#ifdef HAVE_GAS_HIDDEN > DECL_VISIBILITY (vnode->decl) = VISIBILITY_HIDDEN; > DECL_VISIBILITY_SPECIFIED (vnode->decl) = true; > if (cgraph_dump_file) > fprintf (cgraph_dump_file, > "Promoting var as hidden: %s\n", varpool_node_name (vnode)); > +#endif > return true; > } > > @@ -1288,8 +1290,10 @@ promote_fn (struct cgraph_node *node) > if (TREE_PUBLIC (node->decl) || DECL_EXTERNAL (node->decl)) > return false; > TREE_PUBLIC (node->decl) = 1; > +#ifdef HAVE_GAS_HIDDEN > DECL_VISIBILITY (node->decl) = VISIBILITY_HIDDEN; > DECL_VISIBILITY_SPECIFIED (node->decl) = true; > +#endif > if (node->same_body) > { > struct cgraph_node *alias; > @@ -1297,14 +1301,18 @@ promote_fn (struct cgraph_node *node) > alias; alias = alias->next) > { > TREE_PUBLIC (alias->decl) = 1; > +#ifdef HAVE_GAS_HIDDEN > DECL_VISIBILITY (alias->decl) = VISIBILITY_HIDDEN; > DECL_VISIBILITY_SPECIFIED (alias->decl) = true; > +#endif > } > } > +#ifdef HAVE_GAS_HIDDEN > if (cgraph_dump_file) > fprintf (cgraph_dump_file, > "Promoting function as hidden: %s/%i\n", > cgraph_node_name (node), node->uid); > +#endif > return true; > } > > diff --git a/gcc/testsuite/gcc.dg/lto/20081222_0.c b/gcc/testsuite/gcc.dg/lto/20081222_0.c > --- a/gcc/testsuite/gcc.dg/lto/20081222_0.c > +++ b/gcc/testsuite/gcc.dg/lto/20081222_0.c > @@ -1,4 +1,6 @@ > /* { dg-require-alias "" } */ > +/* { dg-require-visibility "" } */ > + > #include "20081222_0.h" > > extern void abort (void); > diff --git a/gcc/testsuite/lib/lto.exp b/gcc/testsuite/lib/lto.exp > --- a/gcc/testsuite/lib/lto.exp > +++ b/gcc/testsuite/lib/lto.exp > @@ -1,4 +1,4 @@ > -# Copyright (C) 2009, 2010 Free Software Foundation, Inc. > +# Copyright (C) 2009, 2010, 2011 Free Software Foundation, Inc. > > # This program is free software; you can redistribute it and/or modify > # it under the terms of the GNU General Public License as published by > @@ -22,14 +22,6 @@ proc lto_prune_warns { text } { > > verbose "lto_prune_warns: entry: $text" 2 > > - # Many tests that use visibility will still pass on platforms that don't support it. > - regsub -all "(^|\n)\[^\n\]*: warning: visibility attribute not supported in this configuration; ignored\[^\n\]*" $text "" text > - > - # And any stray location lines. > - regsub -all "(^|\n)\[^\n\]*: In function \[^\n\]*" $text "" text > - regsub -all "(^|\n)In file included from \[^\n\]*" $text "" text > - regsub -all "(^|\n)\[ \t\]*from \[^\n\]*" $text "" text > - > # Sun ld warns about common symbols with differing sizes. Unlike GNU ld > # --warn-common (off by default), they cannot be disabled. > regsub -all "(^|\n)ld: warning: symbol \[`'\]\[^\n\]*' has differing sizes:" $text "" text > > >
On Apr 5, 2011, at 1:56 AM, Rainer Orth wrote: > * lto.c (promote_var): Only set VISIBILITY_HIDDEN if > HAVE_GAS_HIDDEN. This looks wrong, there are more things that have visibility than those things that use GAS and have .hidden. Darwin I think is one of them. ? cygming.h seems to be another.
On Apr 5, 2011, at 1:56 AM, Rainer Orth wrote: > * lto.c (promote_var): Only set VISIBILITY_HIDDEN if > HAVE_GAS_HIDDEN. Oh, at a minimum, if TARGET_ASM_ASSEMBLE_VISIBILITY is set, doing this stuff I think is useful?
Mike Stump <mikestump@comcast.net> writes: > On Apr 5, 2011, at 1:56 AM, Rainer Orth wrote: >> * lto.c (promote_var): Only set VISIBILITY_HIDDEN if >> HAVE_GAS_HIDDEN. > > This looks wrong, there are more things that have visibility than those things that use GAS and have .hidden. Darwin I think is one of them. ? cygming.h seems to be another. As for many of the HAVE_GAS_* macros, this one is a misnomer: it simply describes if the target assembler has visibility support (at least for many targets). Rainer
Mike Stump <mikestump@comcast.net> writes: > On Apr 5, 2011, at 1:56 AM, Rainer Orth wrote: >> * lto.c (promote_var): Only set VISIBILITY_HIDDEN if >> HAVE_GAS_HIDDEN. > > Oh, at a minimum, if TARGET_ASM_ASSEMBLE_VISIBILITY is set, doing this stuff I think is useful? No, this won't work. E.g. on Solaris with an assembler without visibility support, TARGET_ASM_ASSEMBLE_VISIBILITY is set, but just emits a warning. This is similiar to default_assemble_visibility with HAVE_GAS_HIDDEN undefined. Right now, there are four definitions of TARGET_ASM_ASSEMBLE_VISIBILITY: * i386/cygming.h: i386_pe_assemble_visibility only warns about visibility attributes, so no problem here. * darwin.h: darwin_assemble_visibility is the only implementation which can handle VISIBILITY_HIDDEN (only), but doesn't define HAVE_GAS_HIDDEN. Maybe it should, but one would have to check every instance of the macro to make sure there are no ill effects. * rs6000/rs6000.c: protected by HAVE_GAS_HIDDEN. * sol2.h: warns unless HAVE_GAS_HIDDEN. What a mess. Rainer
Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes: > Mike Stump <mikestump@comcast.net> writes: > >> On Apr 5, 2011, at 1:56 AM, Rainer Orth wrote: >>> * lto.c (promote_var): Only set VISIBILITY_HIDDEN if >>> HAVE_GAS_HIDDEN. >> >> Oh, at a minimum, if TARGET_ASM_ASSEMBLE_VISIBILITY is set, doing this stuff I think is useful? > > No, this won't work. E.g. on Solaris with an assembler without > visibility support, TARGET_ASM_ASSEMBLE_VISIBILITY is set, but just > emits a warning. This is similiar to default_assemble_visibility with > HAVE_GAS_HIDDEN undefined. > > Right now, there are four definitions of TARGET_ASM_ASSEMBLE_VISIBILITY: > > * i386/cygming.h: i386_pe_assemble_visibility only warns about > visibility attributes, so no problem here. > > * darwin.h: darwin_assemble_visibility is the only implementation which > can handle VISIBILITY_HIDDEN (only), but doesn't define > HAVE_GAS_HIDDEN. Maybe it should, but one would have to check every > instance of the macro to make sure there are no ill effects. > > * rs6000/rs6000.c: protected by HAVE_GAS_HIDDEN. > > * sol2.h: warns unless HAVE_GAS_HIDDEN. I've had a closer look now and think it's possible (and desirable) to define HAVE_GAS_HIDDEN for Darwin, too. I've now (after lots of trouble, and without success getting Ada to bootstrap on PowerPC Darwin) set up a development environment on Mac OS X 10.5, both i386 and powerpc. My current plan (though this may be slow) is to define HAVE_GAS_HIDDEN for Darwin in gcc/configure.ac and check what else is necessary to make this work. Once that is done, my patch can probably go in. Additionally, one might want to rename HAVE_GAS_HIDDEN to HAVE_AS_VISIBILITY since that's what the macro really means. (Actually, that's a lie: it means HAVE_AS_LD_VISIBILITY, but I don't think we need to become that verbose.) Does this sound reasonable? Thanks. Rainer
On Apr 19, 2011, at 11:12 AM, Rainer Orth wrote: > I've had a closer look now and think it's possible (and desirable) to > define HAVE_GAS_HIDDEN for Darwin, too. But, they don't have the same thing, therefore, either, you loose out on the meaning, or, you must have yet another test that means the rest. In your email, you don't state that you want to loose out, nor do you state that you want an additional test, so, I am left wondering what you want. I'd say, forge ahead, the worst you can do is break everything... You'll either be careful and not break anything, or you will and someone will pipe up with the bit you broke. I'd like to think we have enough tests in the testsuite to ensure you can't silently break the major pieces.
Mike, [Could you please configure your mail client to break lines? It's hard to reply to messages all on a single line. Thanks.] > On Apr 19, 2011, at 11:12 AM, Rainer Orth wrote: >> I've had a closer look now and think it's possible (and desirable) to >> define HAVE_GAS_HIDDEN for Darwin, too. > > But, they don't have the same thing, therefore, either, you loose out on the meaning, or, you must have yet another test that means the rest. In your email, you don't state that you want to loose out, nor do you state that you want an additional test, so, I am left wondering what you want. I'd say, forge ahead, the worst you can do is break everything... You'll either be careful and not break anything, or you will and someone will pipe up with the bit you broke. I'd like to think we have enough tests in the testsuite to ensure you can't silently break the major pieces. True, Darwin only supports a subset of the whole set of visibilites supported by ELF targets, and even amoung ELF targets there are differences. The testsuite already deals with that alright, and the vast majority of uses inside the compiler proper only use VISIBILITY_(DEFAULT|HIDDEN|INTERNAL) that are supported by Darwin, too. I plan both to check if there are problematic cases that assume more from the target, and of course will rely on a bootstrap and regtest to detect problems. It's probably best to wait for the tested patch and judge from there. If all else fails, one could sprinkle TARGET_MACHO over the affected places, but that can only be a very last resort IMO. Rainer
Mike Stump <mikestump@comcast.net> writes: > On Apr 5, 2011, at 1:56 AM, Rainer Orth wrote: >> * lto.c (promote_var): Only set VISIBILITY_HIDDEN if >> HAVE_GAS_HIDDEN. > > This looks wrong, there are more things that have visibility than those things that use GAS and have .hidden. Darwin I think is one of them. ? cygming.h seems to be another. Now that Darwin has been switched to define HAVE_GAS_HIDDEN, is the lto.c part ok? I've re-bootstrapped both patches together on i386-apple-darwin9.8.0, powerpc-apple-darwin9.8.0, i386-pc-solaris2.8 and i386-pc-solaris2.11 without regressions; as expected the failure on Solaris 8/x86 is gone. Thanks. Rainer
On Thu, Jun 9, 2011 at 9:14 AM, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: > Mike Stump <mikestump@comcast.net> writes: > >> On Apr 5, 2011, at 1:56 AM, Rainer Orth wrote: >>> * lto.c (promote_var): Only set VISIBILITY_HIDDEN if >>> HAVE_GAS_HIDDEN. >> >> This looks wrong, there are more things that have visibility than those things that use GAS and have .hidden. Darwin I think is one of them. ? cygming.h seems to be another. > > Now that Darwin has been switched to define HAVE_GAS_HIDDEN, is the > lto.c part ok? Honza? I think if we are not marking the symbols hidden we "break" LTO in the way that we suddenly export local static symbols. So no, I don't think we want to do that - but then we need another way to make it possible to access previously local statics from a different LTO partition. Richard. > I've re-bootstrapped both patches together on i386-apple-darwin9.8.0, > powerpc-apple-darwin9.8.0, i386-pc-solaris2.8 and i386-pc-solaris2.11 > without regressions; as expected the failure on Solaris 8/x86 is gone. > > Thanks. > Rainer > > -- > ----------------------------------------------------------------------------- > Rainer Orth, Center for Biotechnology, Bielefeld University >
> On Thu, Jun 9, 2011 at 9:14 AM, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: > > Mike Stump <mikestump@comcast.net> writes: > > > >> On Apr 5, 2011, at 1:56 AM, Rainer Orth wrote: > >>> * lto.c (promote_var): Only set VISIBILITY_HIDDEN if > >>> HAVE_GAS_HIDDEN. > >> > >> This looks wrong, there are more things that have visibility than those things that use GAS and have .hidden. Darwin I think is one of them. ? cygming.h seems to be another. > > > > Now that Darwin has been switched to define HAVE_GAS_HIDDEN, is the > > lto.c part ok? > > Honza? I think if we are not marking the symbols hidden we "break" > LTO in the way that we suddenly export local static symbols. So no, Yes, on targets that do have concept of hidden symbols we ought to use the flag. > I don't think we want to do that - but then we need another way to > make it possible to access previously local statics from a different > LTO partition. I guess if we want to support targets that has nothing similar to hidden, we can go for the random seed based mangling and simply export the nonsentially named symbols. Honza > > Richard. > > > I've re-bootstrapped both patches together on i386-apple-darwin9.8.0, > > powerpc-apple-darwin9.8.0, i386-pc-solaris2.8 and i386-pc-solaris2.11 > > without regressions; as expected the failure on Solaris 8/x86 is gone. > > > > Thanks. > > Rainer > > > > -- > > ----------------------------------------------------------------------------- > > Rainer Orth, Center for Biotechnology, Bielefeld University > >
On Jun 9, 2011, at 12:14 AM, Rainer Orth wrote: > Mike Stump <mikestump@comcast.net> writes: >> On Apr 5, 2011, at 1:56 AM, Rainer Orth wrote: >>> * lto.c (promote_var): Only set VISIBILITY_HIDDEN if >>> HAVE_GAS_HIDDEN. >> >> This looks wrong, there are more things that have visibility than those things that use GAS and have .hidden. Darwin I think is one of them. ? cygming.h seems to be another. > > Now that Darwin has been switched to define HAVE_GAS_HIDDEN, is the > lto.c part ok? I don't have any lto specific objections related to this.
diff --git a/gcc/lto/lto.c b/gcc/lto/lto.c --- a/gcc/lto/lto.c +++ b/gcc/lto/lto.c @@ -1,5 +1,5 @@ /* Top-level LTO routines. - Copyright 2009, 2010 Free Software Foundation, Inc. + Copyright 2009, 2010, 2011 Free Software Foundation, Inc. Contributed by CodeSourcery, Inc. This file is part of GCC. @@ -1271,11 +1271,13 @@ promote_var (struct varpool_node *vnode) return false; gcc_assert (flag_wpa); TREE_PUBLIC (vnode->decl) = 1; +#ifdef HAVE_GAS_HIDDEN DECL_VISIBILITY (vnode->decl) = VISIBILITY_HIDDEN; DECL_VISIBILITY_SPECIFIED (vnode->decl) = true; if (cgraph_dump_file) fprintf (cgraph_dump_file, "Promoting var as hidden: %s\n", varpool_node_name (vnode)); +#endif return true; } @@ -1288,8 +1290,10 @@ promote_fn (struct cgraph_node *node) if (TREE_PUBLIC (node->decl) || DECL_EXTERNAL (node->decl)) return false; TREE_PUBLIC (node->decl) = 1; +#ifdef HAVE_GAS_HIDDEN DECL_VISIBILITY (node->decl) = VISIBILITY_HIDDEN; DECL_VISIBILITY_SPECIFIED (node->decl) = true; +#endif if (node->same_body) { struct cgraph_node *alias; @@ -1297,14 +1301,18 @@ promote_fn (struct cgraph_node *node) alias; alias = alias->next) { TREE_PUBLIC (alias->decl) = 1; +#ifdef HAVE_GAS_HIDDEN DECL_VISIBILITY (alias->decl) = VISIBILITY_HIDDEN; DECL_VISIBILITY_SPECIFIED (alias->decl) = true; +#endif } } +#ifdef HAVE_GAS_HIDDEN if (cgraph_dump_file) fprintf (cgraph_dump_file, "Promoting function as hidden: %s/%i\n", cgraph_node_name (node), node->uid); +#endif return true; } diff --git a/gcc/testsuite/gcc.dg/lto/20081222_0.c b/gcc/testsuite/gcc.dg/lto/20081222_0.c --- a/gcc/testsuite/gcc.dg/lto/20081222_0.c +++ b/gcc/testsuite/gcc.dg/lto/20081222_0.c @@ -1,4 +1,6 @@ /* { dg-require-alias "" } */ +/* { dg-require-visibility "" } */ + #include "20081222_0.h" extern void abort (void); diff --git a/gcc/testsuite/lib/lto.exp b/gcc/testsuite/lib/lto.exp --- a/gcc/testsuite/lib/lto.exp +++ b/gcc/testsuite/lib/lto.exp @@ -1,4 +1,4 @@ -# Copyright (C) 2009, 2010 Free Software Foundation, Inc. +# Copyright (C) 2009, 2010, 2011 Free Software Foundation, Inc. # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -22,14 +22,6 @@ proc lto_prune_warns { text } { verbose "lto_prune_warns: entry: $text" 2 - # Many tests that use visibility will still pass on platforms that don't support it. - regsub -all "(^|\n)\[^\n\]*: warning: visibility attribute not supported in this configuration; ignored\[^\n\]*" $text "" text - - # And any stray location lines. - regsub -all "(^|\n)\[^\n\]*: In function \[^\n\]*" $text "" text - regsub -all "(^|\n)In file included from \[^\n\]*" $text "" text - regsub -all "(^|\n)\[ \t\]*from \[^\n\]*" $text "" text - # Sun ld warns about common symbols with differing sizes. Unlike GNU ld # --warn-common (off by default), they cannot be disabled. regsub -all "(^|\n)ld: warning: symbol \[`'\]\[^\n\]*' has differing sizes:" $text "" text