Re: RFC: Make dllimport/dllexport imply default visibility

[Mark Mitchell <mark at codesourcery dot com>]

Mark Mitchell wrote:
> Consider:
> 
>   struct __attribute__((vsibility ("hidden"))) S {
>     void __declspec(dllimport) f();
>   };
> 
> At present, we give "f" hidden visibility.  That seems odd since the
> user has explicitly told us that the symbol is coming from another
> shared library.

So, after much discussion of this on the gcc mailing list, here's a
patch which fixes this problem, by ensuring that dllimport'd symbols
have default visibility.

The basic idea is that when we see a dllimport or dllexport attribute we
give a symbol default visibility.  If that conflicts with an explicitly
specified visibility, or if the user later tries to specify a different
visibility, we issue an error.  For example:

  void __declspec(dllimport)
       __attribute__((visibility("hidden")) f();

is an error.  The same goes for dllexport; combining import/export with
non-default doesn't make sense.  I've documented this combination as
invalid in the manual.  (I've not tried to expand the manual to cover
the whole what-does-it-mean-for-a-type-to-have-visibility debate, as I'm
not sure we've reached consensus there, and, anyhow, it's out-of-scope
for this patch.)

It turns out that this patch also fixes visibility-8.C on SymbianOS.
This test has an explicitly dllexport'd class, but one of its members
(with no explicit visibility directive) was not getting default
visibility.  The reason was that we were not processing the dllexport
attribute on the class until after we had already determined the
visibility for the class as a whole.  With this patch, all the
visibility tests pass on SymbianOS.  I also tested on arm-eabi and
x86_64-linux-gnu.

Danny, would you mind testing this patch on Windows?  I don't understand
whether Windows supports the visibility attribute or not.  If it
doesn't, then this patch should have no affect -- but I'd like to be
sure of that.  I'd also like to know if visibility-8.C somehow passes on
Windows without this patch.  (I think that SymbianOS may be the only OS
that has both dllimport/dllexport and visibility, but I'm not sure.)
I'll hold off for a day or so to commit the patch, and if you need
longer than that to test it, please let me know.

I'd also appreciate a Darwin tester, if available.  Eric, is that
something you could try?

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

2007-06-20  Mark Mitchell  <mark@codesourcery.com>

	* doc/extend.texi: Document that dllimport and dllexport imply
	default visibility.
	* tree.c (handle_dll_attribute): Set DECL_VISIBILITY on the
	imported or exported declaration, including type declarations.
	* c-common.c (handle_visibility_attribute): Check for conflicts
	with dllimport/dllexport.
	(c_determine_visibility): Handle dllimport/dllexport as an
	explicit visibility atttribute.

2007-06-20  Mark Mitchell  <mark@codesourcery.com>

	* decl2.c (determine_visibility): Don't look for dllexport here.
	(determine_visibility_from_class): Tidy.

2007-06-20  Mark Mitchell  <mark@codesourcery.com>

	* gcc.dg/visibility-12.c: New test.
	* gcc.dg/visibility-13.c: Likewise.
	* g++.dg/ext/visibility-9.C: Likewise.
	* g++.dg/ext/visibility-10.C: Likewise.

Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 125868)
+++ gcc/doc/extend.texi	(working copy)
@@ -1782,10 +1782,8 @@ You can use @code{__declspec(dllexport)}
 compilers.
 
 On systems that support the @code{visibility} attribute, this
-attribute also implies ``default'' visibility, unless a
-@code{visibility} attribute is explicitly specified.  You should avoid
-the use of @code{dllexport} with ``hidden'' or ``internal''
-visibility; in the future GCC may issue an error for those cases.
+attribute also implies ``default'' visibility.  It is an error to
+explicitly specify any other visibility.
 
 Currently, the @code{dllexport} attribute is ignored for inlined
 functions, unless the @option{-fkeep-inline-functions} flag has been
@@ -1806,14 +1804,18 @@ the @option{--export-all} linker flag.
 On Microsoft Windows and Symbian OS targets, the @code{dllimport}
 attribute causes the compiler to reference a function or variable via
 a global pointer to a pointer that is set up by the DLL exporting the
-symbol.  The attribute implies @code{extern} storage.  On Microsoft
-Windows targets, the pointer name is formed by combining @code{_imp__}
-and the function or variable name.
+symbol.  The attribute implies @code{extern}.  On Microsoft Windows
+targets, the pointer name is formed by combining @code{_imp__} and the
+function or variable name.
 
 You can use @code{__declspec(dllimport)} as a synonym for
 @code{__attribute__ ((dllimport))} for compatibility with other
 compilers.
 
+On systems that support the @code{visibility} attribute, this
+attribute also implies ``default'' visibility.  It is an error to
+explicitly specify any other visibility.
+
 Currently, the attribute is ignored for inlined functions.  If the
 attribute is applied to a symbol @emph{definition}, an error is reported.
 If a symbol previously declared @code{dllimport} is later defined, the
Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 125868)
+++ gcc/tree.c	(working copy)
@@ -4001,18 +4001,25 @@ handle_dll_attribute (tree * pnode, tree
 	  *no_add_attrs = true;
 	  return tree_cons (name, args, NULL_TREE);
 	}
-      if (TREE_CODE (node) != RECORD_TYPE && TREE_CODE (node) != UNION_TYPE)
+      if (TREE_CODE (node) == RECORD_TYPE
+	  || TREE_CODE (node) == UNION_TYPE)
+	{
+	  node = TYPE_NAME (node);
+	  if (!node)
+	    return NULL_TREE;
+	}
+      else
 	{
 	  warning (OPT_Wattributes, "%qs attribute ignored",
 		   IDENTIFIER_POINTER (name));
 	  *no_add_attrs = true;
+	  return NULL_TREE;
 	}
-
-      return NULL_TREE;
     }
 
   if (TREE_CODE (node) != FUNCTION_DECL
-      && TREE_CODE (node) != VAR_DECL)
+      && TREE_CODE (node) != VAR_DECL
+      && TREE_CODE (node) != TYPE_DECL)
     {
       *no_add_attrs = true;
       warning (OPT_Wattributes, "%qs attribute ignored",
@@ -4075,6 +4082,22 @@ handle_dll_attribute (tree * pnode, tree
       *no_add_attrs = true;
     }
 
+  /* A dllexport'd entity must have default visibility so that other
+     program units (shared libraries or the main executable) can see
+     it.  A dllimport'd entity must have default visibility so that
+     the linker knows that undefined references within this program
+     unit can be resolved by the dynamic linker.  */
+  if (!*no_add_attrs)
+    {
+      if (DECL_VISIBILITY_SPECIFIED (node)
+	  && DECL_VISIBILITY (node) != VISIBILITY_DEFAULT)
+	error ("%qs implies default visibility, but %qD has already "
+	       "been declared with a different visibility", 
+	       IDENTIFIER_POINTER (name), node);
+      DECL_VISIBILITY (node) = VISIBILITY_DEFAULT;
+      DECL_VISIBILITY_SPECIFIED (node) = 1;
+    }
+
   return NULL_TREE;
 }
 
Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c	(revision 125868)
+++ gcc/c-common.c	(working copy)
@@ -5419,11 +5419,22 @@ handle_visibility_attribute (tree *node,
     }
 
   if (DECL_VISIBILITY_SPECIFIED (decl)
-      && vis != DECL_VISIBILITY (decl)
-      && lookup_attribute ("visibility", (TYPE_P (*node)
-					  ? TYPE_ATTRIBUTES (*node)
-					  : DECL_ATTRIBUTES (decl))))
-    error ("%qD redeclared with different visibility", decl);
+      && vis != DECL_VISIBILITY (decl))
+    {
+      tree attributes = (TYPE_P (*node)
+			 ? TYPE_ATTRIBUTES (*node)
+			 : DECL_ATTRIBUTES (decl));
+      if (lookup_attribute ("visibility", attributes))
+	error ("%qD redeclared with different visibility", decl);
+      else if (TARGET_DLLIMPORT_DECL_ATTRIBUTES
+	       && lookup_attribute ("dllimport", attributes))
+	error ("%qD was declared %qs which implies default visibility",
+	       decl, "dllimport");
+      else if (TARGET_DLLIMPORT_DECL_ATTRIBUTES
+	       && lookup_attribute ("dllexport", attributes))
+	error ("%qD was declared %qs which implies default visibility",
+	       decl, "dllexport");
+    }
 
   DECL_VISIBILITY (decl) = vis;
   DECL_VISIBILITY_SPECIFIED (decl) = 1;
@@ -5456,18 +5467,12 @@ c_determine_visibility (tree decl)
      to distinguish the use of an attribute from the use of a "#pragma
      GCC visibility push(...)"; in the latter case we still want other
      considerations to be able to overrule the #pragma.  */
-  if (lookup_attribute ("visibility", DECL_ATTRIBUTES (decl)))
+  if (lookup_attribute ("visibility", DECL_ATTRIBUTES (decl))
+      || (TARGET_DLLIMPORT_DECL_ATTRIBUTES
+	  && (lookup_attribute ("dllimport", DECL_ATTRIBUTES (decl))
+	      || lookup_attribute ("dllexport", DECL_ATTRIBUTES (decl)))))
     return true;
 
-  /* Anything that is exported must have default visibility.  */
-  if (TARGET_DLLIMPORT_DECL_ATTRIBUTES
-      && lookup_attribute ("dllexport", DECL_ATTRIBUTES (decl)))
-    {
-      DECL_VISIBILITY (decl) = VISIBILITY_DEFAULT;
-      DECL_VISIBILITY_SPECIFIED (decl) = 1;
-      return true;
-    }
-
   /* Set default visibility to whatever the user supplied with
      visibility_specified depending on #pragma GCC visibility.  */
   if (!DECL_VISIBILITY_SPECIFIED (decl))
Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 125868)
+++ gcc/cp/decl2.c	(working copy)
@@ -1684,17 +1684,6 @@ determine_visibility (tree decl)
   else
     use_template = 0;
 
-  /* Anything that is exported must have default visibility.  */
-  if (TARGET_DLLIMPORT_DECL_ATTRIBUTES
-      && lookup_attribute ("dllexport",
-			   TREE_CODE (decl) == TYPE_DECL
-			   ? TYPE_ATTRIBUTES (TREE_TYPE (decl))
-			   : DECL_ATTRIBUTES (decl)))
-    {
-      DECL_VISIBILITY (decl) = VISIBILITY_DEFAULT;
-      DECL_VISIBILITY_SPECIFIED (decl) = 1;
-    }
-
   /* If DECL is a member of a class, visibility specifiers on the
      class can influence the visibility of the DECL.  */
   if (DECL_CLASS_SCOPE_P (decl))
@@ -1796,18 +1785,20 @@ determine_visibility (tree decl)
 static void
 determine_visibility_from_class (tree decl, tree class_type)
 {
+  if (DECL_VISIBILITY_SPECIFIED (decl))
+    return;
+
   if (visibility_options.inlines_hidden
       /* Don't do this for inline templates; specializations might not be
 	 inline, and we don't want them to inherit the hidden
 	 visibility.  We'll set it here for all inline instantiations.  */
       && !processing_template_decl
-      && ! DECL_VISIBILITY_SPECIFIED (decl)
       && TREE_CODE (decl) == FUNCTION_DECL
       && DECL_DECLARED_INLINE_P (decl)
       && (! DECL_LANG_SPECIFIC (decl)
 	  || ! DECL_EXPLICIT_INSTANTIATION (decl)))
     DECL_VISIBILITY (decl) = VISIBILITY_HIDDEN;
-  else if (!DECL_VISIBILITY_SPECIFIED (decl))
+  else
     {
       /* Default to the class visibility.  */
       DECL_VISIBILITY (decl) = CLASSTYPE_VISIBILITY (class_type);
@@ -1826,7 +1817,6 @@ determine_visibility_from_class (tree de
 	      && !DECL_CONSTRUCTION_VTABLE_P (decl)))
       && TREE_PUBLIC (decl)
       && !DECL_REALLY_EXTERN (decl)
-      && !DECL_VISIBILITY_SPECIFIED (decl)
       && !CLASSTYPE_VISIBILITY_SPECIFIED (class_type))
     targetm.cxx.determine_class_data_visibility (decl);
 }
Index: gcc/testsuite/gcc.dg/visibility-13.c
===================================================================
--- gcc/testsuite/gcc.dg/visibility-13.c	(revision 0)
+++ gcc/testsuite/gcc.dg/visibility-13.c	(revision 0)
@@ -0,0 +1,19 @@
+/* { dg-require-visibility "" } */
+/* { dg-require-dll "" } */
+
+extern void __attribute__((dllimport, visibility("hidden"))) 
+  f1(); /* { dg-error "visibility" } */
+extern void __attribute__((visibility("hidden"), dllimport)) 
+  f2(); /* { dg-error "visibility" } */
+extern void __attribute__((dllexport, visibility("hidden"))) 
+  f3(); /* { dg-error "visibility" } */
+extern void __attribute__((visibility("hidden"), dllexport)) 
+  f4(); /* { dg-error "visibility" } */
+extern void __attribute__((visibility("default"), dllimport))
+  f5();
+extern void __attribute__((dllimport, visibility("default")))
+  f6();
+extern void __attribute__((visibility("default"), dllexport))
+  f7();
+extern void __attribute__((dllexport, visibility("default")))
+  f8();
Index: gcc/testsuite/gcc.dg/visibility-12.c
===================================================================
--- gcc/testsuite/gcc.dg/visibility-12.c	(revision 0)
+++ gcc/testsuite/gcc.dg/visibility-12.c	(revision 0)
@@ -0,0 +1,10 @@
+/* Test that dllimport'd functions have default visibility.  */
+/* { dg-require-visibility "" } */
+/* { dg-require-dll "" } */
+/* { dg-options "-fvisibility=hidden" } */
+/* { dg-final { scan-not-hidden "f1" } } */
+
+extern void  __attribute__((dllimport)) f1();
+void f2() {
+  f1();
+}
Index: gcc/testsuite/g++.dg/ext/visibility/visibility-10.C
===================================================================
--- gcc/testsuite/g++.dg/ext/visibility/visibility-10.C	(revision 0)
+++ gcc/testsuite/g++.dg/ext/visibility/visibility-10.C	(revision 0)
@@ -0,0 +1,19 @@
+// { dg-require-visibility "" }
+// { dg-require-dll "" }
+
+extern void __attribute__((dllimport, visibility("hidden"))) 
+  f1(); // { dg-error "visibility" }
+extern void __attribute__((visibility("hidden"), dllimport)) 
+  f2(); // { dg-error "visibility" }
+extern void __attribute__((dllexport, visibility("hidden"))) 
+  f3(); // { dg-error "visibility" }
+extern void __attribute__((visibility("hidden"), dllexport)) 
+  f4(); // { dg-error "visibility" }
+extern void __attribute__((visibility("default"), dllimport))
+  f5();
+extern void __attribute__((dllimport, visibility("default")))
+  f6();
+extern void __attribute__((visibility("default"), dllexport))
+  f7();
+extern void __attribute__((dllexport, visibility("default")))
+  f8();
Index: gcc/testsuite/g++.dg/ext/visibility/visibility-9.C
===================================================================
--- gcc/testsuite/g++.dg/ext/visibility/visibility-9.C	(revision 0)
+++ gcc/testsuite/g++.dg/ext/visibility/visibility-9.C	(revision 0)
@@ -0,0 +1,29 @@
+// Test that dllimport'd functions have default visibility.
+// { dg-require-visibility "" }
+// { dg-require-dll "" }
+// { dg-options "-fvisibility=hidden" }
+// { dg-final { scan-not-hidden "_Z2f1v" } }
+// { dg-final { scan-not-hidden "_ZN1S2f3Ev" } }
+
+extern void  __attribute__((dllimport)) f1();
+void f2() {
+  f1();
+}
+
+struct __attribute__((visibility("hidden")) S1 {
+  __attribute__((dllimport)) void f3();
+};
+
+void f4() {
+  S1 s1;
+  s1.f3();
+}
+
+struct S2 {
+  __attribute__((dllimport)) void f5();
+};
+
+void f6() {
+  S2 s2;
+  s2.f5();
+}