diff mbox

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

Message ID AANLkTimJ1HCX+GsmY1Ah0Xf-0dNH6GxGjtobkQ5AkaJ7@mail.gmail.com
State New
Headers show

Commit Message

Kai Tietz Jan. 13, 2011, 12:09 p.m. UTC 
2011/1/12 Dave Korn <dave.korn.cygwin@gmail.com>:
> 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.

Yes, agreed.

>  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?

I saw this kind of warning for PR's test-case. Therefore I want to
handle it within that patch.
>  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.

Yes, those warnings I see too, and indeed it helps ;)

So I removed from patch the varasm.c change and instead added to i386
windows-targets an specific assemble-visibility-hook.

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
       * config/i386/cygming.h (TARGET_ASM_ASSEMBLE_VISIBILITY):
       PE specific hook.
       * config/i386/i386-protos.h (i386_pe_assemble_visibility):
       New function prototype.
       * config/i386/winnt.c (i386_pe_assemble_visibility):
       Warn only if attribute was specified by user.

Tested for x86_64-w64-mingw32, i686-pc-cygwin, and
x86_64-pc-linux-gnu. Ok for apply?

Regards,
Kai

Comments

Dave Korn Jan. 13, 2011, 5:15 p.m. UTC | #1 
On 13/01/2011 12:09, Kai Tietz wrote:
> 2011/1/12 Dave Korn <dave.korn.cygwin@gmail.com>:

>>  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?
> 
> I saw this kind of warning for PR's test-case. Therefore I want to
> handle it within that patch.

  Righto.

>>  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.
> 
> Yes, those warnings I see too, and indeed it helps ;)
> 
> So I removed from patch the varasm.c change and instead added to i386
> windows-targets an specific assemble-visibility-hook.

  That makes sense to me.

> 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
>        * config/i386/cygming.h (TARGET_ASM_ASSEMBLE_VISIBILITY):
>        PE specific hook.
>        * config/i386/i386-protos.h (i386_pe_assemble_visibility):
>        New function prototype.
>        * config/i386/winnt.c (i386_pe_assemble_visibility):
>        Warn only if attribute was specified by user.
> 
> Tested for x86_64-w64-mingw32, i686-pc-cygwin, and
> x86_64-pc-linux-gnu. Ok for apply?

  Well, the windows stuff is OK.  (And IIUC makes sense even stand-alone.)

    cheers,
      DaveK
Jason Merrill Jan. 13, 2011, 7:36 p.m. UTC | #2 
On 01/13/2011 12:15 PM, Dave Korn wrote:
>    Well, the windows stuff is OK.  (And IIUC makes sense even stand-alone.)

The rest of the patch is OK too.

Jason
Kai Tietz Jan. 13, 2011, 8:02 p.m. UTC | #3 
2011/1/13 Jason Merrill <jason@redhat.com>:
> On 01/13/2011 12:15 PM, Dave Korn wrote:
>>
>>   Well, the windows stuff is OK.  (And IIUC makes sense even stand-alone.)
>
> The rest of the patch is OK too.
>
> Jason
>

Committed at revision 168763 to trunk.

Thanks,
Kai
diff mbox

Patch

Index: gcc/gcc/cp/cp-tree.h
===================================================================
--- gcc.orig/gcc/cp/cp-tree.h	2011-01-13 11:59:19.540648100 +0100
+++ gcc/gcc/cp/cp-tree.h	2011-01-13 12:02:31.323125500 +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	2011-01-13 11:56:09.576177000 +0100
+++ gcc/gcc/cp/decl2.c	2011-01-13 12:02:31.323125500 +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-13 12:02:31.323125500 +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/config/i386/cygming.h
===================================================================
--- gcc.orig/gcc/config/i386/cygming.h	2011-01-13 11:56:09.569177000 +0100
+++ gcc/gcc/config/i386/cygming.h	2011-01-13 12:02:31.323125500 +0100
@@ -465,6 +465,9 @@  do {						\
 #define TARGET_CXX_ADJUST_CLASS_AT_DEFINITION i386_pe_adjust_class_at_definition
 #define TARGET_MANGLE_DECL_ASSEMBLER_NAME i386_pe_mangle_decl_assembler_name
 
+#undef TARGET_ASM_ASSEMBLE_VISIBILITY
+#define TARGET_ASM_ASSEMBLE_VISIBILITY i386_pe_assemble_visibility
+
 /* Static stack checking is supported by means of probes.  */
 #define STACK_CHECK_STATIC_BUILTIN 1
 
Index: gcc/gcc/config/i386/i386-protos.h
===================================================================
--- gcc.orig/gcc/config/i386/i386-protos.h	2011-01-13 11:56:09.570177000 +0100
+++ gcc/gcc/config/i386/i386-protos.h	2011-01-13 12:02:31.338750600 +0100
@@ -225,6 +225,7 @@  extern void i386_pe_asm_output_aligned_d
 extern void i386_pe_file_end (void);
 extern void i386_pe_start_function (FILE *, const char *, tree);
 extern void i386_pe_end_function (FILE *, const char *, tree);
+extern void i386_pe_assemble_visibility (tree, int);
 extern tree i386_pe_mangle_decl_assembler_name (tree, tree);
 extern tree i386_pe_mangle_assembler_name (const char *);
 
Index: gcc/gcc/config/i386/winnt.c
===================================================================
--- gcc.orig/gcc/config/i386/winnt.c	2011-01-13 11:56:09.572177000 +0100
+++ gcc/gcc/config/i386/winnt.c	2011-01-13 12:02:31.338750600 +0100
@@ -232,6 +232,22 @@  i386_pe_maybe_mangle_decl_assembler_name
   return new_id;
 }
 
+/* Emit an assembler directive to set symbol for DECL visibility to
+   the visibility type VIS, which must not be VISIBILITY_DEFAULT.
+   As for PE there is no hidden support in gas, we just warn for
+   user-specified visibility attributes.  */
+
+void
+i386_pe_assemble_visibility (tree decl,
+			     int vis ATTRIBUTE_UNUSED)
+{
+  if (!decl
+      || !lookup_attribute ("visibility", DECL_ATTRIBUTES (decl)))
+    return;
+  warning (OPT_Wattributes, "visibility attribute not supported "
+	   "in this configuration; ignored");
+}
+
 /* This is used as a target hook to modify the DECL_ASSEMBLER_NAME
    in the language-independent default hook
    langhooks,c:lhd_set_decl_assembler_name ()