Patchwork C++/c-common PATCH for c++/35688 (wrong visibility of template instantiation)

login
register
mail settings
Submitter Jason Merrill
Date Nov. 7, 2011, 4:31 a.m.
Message ID <4EB75F14.3060400@redhat.com>
Download mbox | patch
Permalink /patch/123996/
State New
Headers show

Comments

Jason Merrill - Nov. 7, 2011, 4:31 a.m.
The function constrain_visibility_for_template tries to set the 
visibility of a template instantiation properly by giving it the minimum 
visibility of the template itself and the template arguments.  But this 
PR points out that we were failing to do that in the case that the 
template is within a namespace with a visibility attribute, because then 
it gets DECL_VISIBILITY_SPECIFIED.  This patch fixes that by re-using 
some of the C front end's visibility code, so that we can further 
constrain visibility on a decl with DECL_VISIBILITY_SPECIFIED so long as 
it doesn't actually have a visibility attribute.

Joseph, I assume you have no objection to the c-common refactoring?

Tested x86_64-pc-linux-gnu, applying to trunk.
Joseph S. Myers - Nov. 7, 2011, 3:56 p.m.
On Sun, 6 Nov 2011, Jason Merrill wrote:

> The function constrain_visibility_for_template tries to set the visibility of
> a template instantiation properly by giving it the minimum visibility of the
> template itself and the template arguments.  But this PR points out that we
> were failing to do that in the case that the template is within a namespace
> with a visibility attribute, because then it gets DECL_VISIBILITY_SPECIFIED.
> This patch fixes that by re-using some of the C front end's visibility code,
> so that we can further constrain visibility on a decl with
> DECL_VISIBILITY_SPECIFIED so long as it doesn't actually have a visibility
> attribute.
> 
> Joseph, I assume you have no objection to the c-common refactoring?

It's fine with me.

Patch

commit 3f9fa74f7ee3f2074316bfb49fa1a09b98f78602
Author: Jason Merrill <jason@redhat.com>
Date:   Sun Nov 6 15:44:49 2011 -0500

    	PR c++/35688
    gcc/c-common/
    	* c-common.c (decl_has_visibility_attr): Split out from...
    	(c_determine_visibility): ...here.
    	* c-common.h: Declare it.
    gcc/cp/
    	* decl2.c (constrain_visibility): Check decl_has_visibility_attr
    	rather than DECL_VISIBILITY_SPECIFIED.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 5627fe1..71b3721 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -7043,6 +7043,22 @@  handle_visibility_attribute (tree *node, tree name, tree args,
   return NULL_TREE;
 }
 
+/* Returns true iff DECL actually has visibility specified by an attribute.
+   We check for an explicit attribute, rather than just checking
+   DECL_VISIBILITY_SPECIFIED, 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.  */
+
+bool
+decl_has_visibility_attr (tree decl)
+{
+  tree attrs = DECL_ATTRIBUTES (decl);
+  return (lookup_attribute ("visibility", attrs)
+	  || (TARGET_DLLIMPORT_DECL_ATTRIBUTES
+	      && (lookup_attribute ("dllimport", attrs)
+		  || lookup_attribute ("dllexport", attrs))));
+}
+
 /* Determine the ELF symbol visibility for DECL, which is either a
    variable or a function.  It is an error to use this function if a
    definition of DECL is not available in this translation unit.
@@ -7058,15 +7074,8 @@  c_determine_visibility (tree decl)
 
   /* If the user explicitly specified the visibility with an
      attribute, honor that.  DECL_VISIBILITY will have been set during
-     the processing of the attribute.  We check for an explicit
-     attribute, rather than just checking DECL_VISIBILITY_SPECIFIED,
-     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))
-      || (TARGET_DLLIMPORT_DECL_ATTRIBUTES
-	  && (lookup_attribute ("dllimport", DECL_ATTRIBUTES (decl))
-	      || lookup_attribute ("dllexport", DECL_ATTRIBUTES (decl)))))
+     the processing of the attribute.  */
+  if (decl_has_visibility_attr (decl))
     return true;
 
   /* Set default visibility to whatever the user supplied with
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 7ecb57e..0b22d3d 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -771,6 +771,7 @@  extern void overflow_warning (location_t, tree);
 extern void warn_logical_operator (location_t, enum tree_code, tree,
 				   enum tree_code, tree, enum tree_code, tree);
 extern void check_main_parameter_types (tree decl);
+extern bool decl_has_visibility_attr (tree);
 extern bool c_determine_visibility (tree);
 extern bool same_scalar_type_ignoring_signedness (tree, tree);
 extern void mark_valid_location_for_stdc_pragma (bool);
diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
index 32b5c7e..4bc02bd 100644
--- a/gcc/cp/decl2.c
+++ b/gcc/cp/decl2.c
@@ -1974,8 +1974,11 @@  constrain_visibility (tree decl, int visibility)
 	    DECL_NOT_REALLY_EXTERN (decl) = 1;
 	}
     }
+  /* We check decl_has_visibility_attr rather than
+     DECL_VISIBILITY_SPECIFIED here because we want other considerations
+     to override visibility from a namespace or #pragma.  */
   else if (visibility > DECL_VISIBILITY (decl)
-	   && !DECL_VISIBILITY_SPECIFIED (decl))
+	   && !decl_has_visibility_attr (decl))
     {
       DECL_VISIBILITY (decl) = (enum symbol_visibility) visibility;
       return true;
diff --git a/gcc/testsuite/g++.dg/ext/visibility/template7.C b/gcc/testsuite/g++.dg/ext/visibility/template7.C
new file mode 100644
index 0000000..5197fb1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/visibility/template7.C
@@ -0,0 +1,29 @@ 
+// PR c++/35688
+// { dg-require-visibility "" }
+// { dg-options "-fvisibility=hidden" }
+
+// { dg-final { scan-hidden "_ZN1s6vectorI1AEC1Ev" } }
+// { dg-final { scan-hidden "_ZN1s3fooI1AEEvT_" } }
+
+namespace s __attribute__((visibility("default"))) {
+  template <class T>
+    class vector {
+  public:
+    vector() { }
+  };
+  template <class T>
+    void foo(T t) {
+  }
+}
+
+class A {
+public:
+  A() { }
+};
+
+s::vector<A> v;
+
+int main() {
+  A a;
+  s::foo(a);
+}