Patchwork [c++] : PR/47213] New: ICE: SIGSEGV in determine_visibility (decl2.c:2076) with -fvisibility-ms-compat

login
register
mail settings
Submitter Kai Tietz
Date Jan. 12, 2011, 9:07 a.m.
Message ID <AANLkTikX+x1T3067DCqQFnTZJB7mBq4ERMmyi9Ct4bNB@mail.gmail.com>
Download mbox | patch
Permalink /patch/78525/
State New
Headers show

Comments

Kai Tietz - Jan. 12, 2011, 9:07 a.m.
2011/1/9 Jason Merrill <jason@redhat.com>:
> On 01/08/2011 10:19 AM, Kai Tietz wrote:
>>
>>           if (underlying_vis == VISIBILITY_ANON
>> -             || CLASSTYPE_VISIBILITY_SPECIFIED (underlying_type))
>> +             || (TYPE_NAME (underlying_type)
>> +                 &&  CLASSTYPE_VISIBILITY_SPECIFIED (underlying_type)))
>>             constrain_visibility (decl, underlying_vis);
>
> This should check CLASS_TYPE_P rather than TYPE_NAME.  And looking at this I
> realize that CLASSTYPE_VISIBILITY/_SPECIFIED should use TYPE_MAIN_DECL
> rather than TYPE_NAME.
>
> Jason
>
Hello,

I adjusted the patch as discussed on IRC. Additionally I found a regression
in varasm.c's function default_assemble_visibility(). This caused a lot of
testsuite regressions (eg.
c-c++-common/torture/complex-sign-mixed-add.c, & co) as
here internal visibility sets of C++ FE getting warned.

ChangeLog cp/

2011-01-12  Kai Tietz

	PR c++/47213
	* cp-tree.h (CLASSTYPE_VISIBILITY): Use
	TYPE_MAIN_DECL instead of TYPE_NAME.
	(CLASSTYPE_VISIBILITY_SPECIFIED): Likewise.
	* decl2.c (determine_visibility): Add check
	of CLASS_TYPE_P for underlying_type.

ChangeLog testsuite/

2011-01-12  Kai Tietz

	PR c++/47213
	* g++.dg/ext/pr47213.C: New.

ChangeLog gcc/

2011-01-12  Kai Tietz

	PR c++/47213
	* varasm.c (default_assemble_visibility): Warn
	only if attribute was specified by user.

Tested for x86_64-pc-linux-gnu (multilib), and x86_64-w64-mingw32. Ok for apply?

Regards,
Kai
Jason Merrill - Jan. 12, 2011, 6:53 p.m.
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
Kai Tietz - Jan. 12, 2011, 6:56 p.m.
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
Dave Korn - Jan. 12, 2011, 8:06 p.m.
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

Patch

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
 }