| Message ID | AANLkTikX+x1T3067DCqQFnTZJB7mBq4ERMmyi9Ct4bNB@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 01/12/2011 04:07 AM, Kai Tietz wrote: > - warning (OPT_Wattributes, "visibility attribute not supported " > - "in this configuration; ignored"); > + if (decl && lookup_attribute ("dllexport", DECL_ATTRIBUTES (decl))) > + warning (OPT_Wattributes, "visibility attribute not supported " > + "in this configuration; ignored"); This change is wrong. If we don't support visibility on a target, trying to apply it should give a warning by default. It certainly doesn't make any sense to condition that warning on the dllexport attribute in target-independent code. Perhaps we want a target-specific assemble_visibility hook for PE, or to just ignore -fvisibility-ms-compat. Jason
2011/1/12 Jason Merrill <jason@redhat.com>: > On 01/12/2011 04:07 AM, Kai Tietz wrote: >> >> - warning (OPT_Wattributes, "visibility attribute not supported " >> - "in this configuration; ignored"); >> + if (decl && lookup_attribute ("dllexport", DECL_ATTRIBUTES (decl))) >> + warning (OPT_Wattributes, "visibility attribute not supported " >> + "in this configuration; ignored"); > > This change is wrong. If we don't support visibility on a target, trying to > apply it should give a warning by default. It certainly doesn't make any > sense to condition that warning on the dllexport attribute in > target-independent code. > > Perhaps we want a target-specific assemble_visibility hook for PE, or to > just ignore -fvisibility-ms-compat. > > Jason > Yes, thought about that, too. I add Dave to CC. So he can comment on that. PS, first patch accidentially shown "dllexport" as attribute, second one (I missed to update my patch) has correct "visibility" attribute name. Nevertheless, a target specific hook is ok for me. Regards, Kai
On 12/01/2011 18:56, Kai Tietz wrote: > 2011/1/12 Jason Merrill <jason@redhat.com>: >> On 01/12/2011 04:07 AM, Kai Tietz wrote: >>> - warning (OPT_Wattributes, "visibility attribute not supported " >>> - "in this configuration; ignored"); >>> + if (decl && lookup_attribute ("dllexport", DECL_ATTRIBUTES (decl))) >>> + warning (OPT_Wattributes, "visibility attribute not supported " >>> + "in this configuration; ignored"); >> This change is wrong. If we don't support visibility on a target, trying to >> apply it should give a warning by default. It certainly doesn't make any >> sense to condition that warning on the dllexport attribute in >> target-independent code. >> >> Perhaps we want a target-specific assemble_visibility hook for PE, or to >> just ignore -fvisibility-ms-compat. >> >> Jason >> > > Yes, thought about that, too. I add Dave to CC. So he can comment on that. I believe LTO is able to make optimisations based on inferences from the visibility attributes even when the target does not support them. So, we don't want to ban it altogether. Kai, re the varasm.c change: I don't understand how the original patch could lead to visibility being set on any more decls than it would otherwise have been, so how come there were regressions? Is this just to do with -fvisibility-ms-compat? It looks like the change might have the side-effect of trimming down the millions of visibility-not-supported warnings that I get during an lto-bootstrap, although I haven't checked. That would definitely be nice. The concept of only warning for explicitly user-specified attributes seems sensible to me. cheers, DaveK
Index: gcc/gcc/cp/cp-tree.h =================================================================== --- gcc.orig/gcc/cp/cp-tree.h 2010-12-15 14:28:07.000000000 +0100 +++ gcc/gcc/cp/cp-tree.h 2011-01-11 12:36:58.410171200 +0100 @@ -1221,9 +1221,9 @@ enum languages { lang_c, lang_cplusplus, /* Gives the visibility specification for a class type. */ #define CLASSTYPE_VISIBILITY(TYPE) \ - DECL_VISIBILITY (TYPE_NAME (TYPE)) + DECL_VISIBILITY (TYPE_MAIN_DECL (TYPE)) #define CLASSTYPE_VISIBILITY_SPECIFIED(TYPE) \ - DECL_VISIBILITY_SPECIFIED (TYPE_NAME (TYPE)) + DECL_VISIBILITY_SPECIFIED (TYPE_MAIN_DECL (TYPE)) typedef struct GTY (()) tree_pair_s { tree purpose; Index: gcc/gcc/cp/decl2.c =================================================================== --- gcc.orig/gcc/cp/decl2.c 2010-12-15 14:28:07.000000000 +0100 +++ gcc/gcc/cp/decl2.c 2011-01-11 12:35:08.738296200 +0100 @@ -2073,7 +2073,8 @@ determine_visibility (tree decl) tree underlying_type = TREE_TYPE (DECL_NAME (decl)); int underlying_vis = type_visibility (underlying_type); if (underlying_vis == VISIBILITY_ANON - || CLASSTYPE_VISIBILITY_SPECIFIED (underlying_type)) + || (CLASS_TYPE_P (underlying_type) + && CLASSTYPE_VISIBILITY_SPECIFIED (underlying_type))) constrain_visibility (decl, underlying_vis); else DECL_VISIBILITY (decl) = VISIBILITY_DEFAULT; Index: gcc/gcc/testsuite/g++.dg/ext/pr47213.C =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ gcc/gcc/testsuite/g++.dg/ext/pr47213.C 2011-01-12 09:45:44.864673100 +0100 @@ -0,0 +1,15 @@ +// { dg-do compile } +// { dg-options "-fvisibility-ms-compat" } +#include <typeinfo> + +template < typename T > void +bar () +{ + typeid (T); +} + +void +foo () +{ + bar < int () > (); +} Index: gcc/gcc/varasm.c =================================================================== --- gcc.orig/gcc/varasm.c 2010-12-22 09:23:11.000000000 +0100 +++ gcc/gcc/varasm.c 2011-01-12 09:53:21.055968300 +0100 @@ -5799,8 +5799,9 @@ default_assemble_visibility (tree decl A assemble_name (asm_out_file, name); fprintf (asm_out_file, "\n"); #else - warning (OPT_Wattributes, "visibility attribute not supported " - "in this configuration; ignored"); + if (decl && lookup_attribute ("dllexport", DECL_ATTRIBUTES (decl))) + warning (OPT_Wattributes, "visibility attribute not supported " + "in this configuration; ignored"); #endif }