4 files changed, 213 insertions, 65 deletions

diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h
index b045fef496..405df239e8 100644
--- a/include/clang/AST/Decl.h
+++ b/include/clang/AST/Decl.h
@@ -694,6 +694,10 @@ public:
   /// definition.
   DefinitionKind isThisDeclarationADefinition() const;
 
+  /// \brief Check whether this variable is defined in this
+  /// translation unit.
+  DefinitionKind hasDefinition() const;
+
   /// \brief Get the tentative definition that acts as the real definition in
   /// a TU. Returns null if there is a proper definition available.
   VarDecl *getActingDefinition();
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
index f7fd7b8d5a..dbd14e067a 100644
--- a/lib/AST/Decl.cpp
+++ b/lib/AST/Decl.cpp
@@ -140,16 +140,25 @@ static LVPair getLVForTemplateArgumentList(const TemplateArgumentList &TArgs) {
                                       TArgs.flat_size());
 }
 
+/// Answers whether the given class, or any containing class, has an
+/// explicit visibility attribute.
+static bool HasExplicitVisibilityInHierarchy(const RecordDecl *RD) {
+  if (RD->hasAttr<VisibilityAttr>()) return true;
+  if (const RecordDecl *Parent = dyn_cast<RecordDecl>(RD->getDeclContext()))
+    return HasExplicitVisibilityInHierarchy(Parent);
+  return false;
+}
+
 /// getLVForDecl - Get the cached linkage and visibility for the given
 /// declaration.
 ///
-/// \param ConsiderGlobalSettings - Whether to honor global visibility
-///   settings.  This is false when computing the visibility of the
-///   context of a declaration with an explicit visibility attribute.
-static LVPair getLVForDecl(const NamedDecl *D, bool ConsiderGlobalSettings);
+/// \param ConsiderGlobalVisibility - Whether to honor global visibility
+///   settings.  This is generally false when computing the visibility
+///   of the context of a declaration.
+static LVPair getLVForDecl(const NamedDecl *D, bool ConsiderGlobalVisibility);
 
 static LVPair getLVForNamespaceScopeDecl(const NamedDecl *D,
-                                         bool ConsiderGlobalSettings) {
+                                         bool ConsiderGlobalVisibility) {
   assert(D->getDeclContext()->getRedeclContext()->isFileContext() &&
          "Not a name having namespace scope");
   ASTContext &Context = D->getASTContext();
@@ -216,10 +225,6 @@ static LVPair getLVForNamespaceScopeDecl(const NamedDecl *D,
   //   external.
   LVPair LV(ExternalLinkage, DefaultVisibility);
 
-  // We ignore -fvisibility on non-definitions and explicit
-  // instantiation declarations.
-  bool ConsiderDashFVisibility = ConsiderGlobalSettings;
-
   // C++ [basic.link]p4:
 
   //   A name having namespace scope has external linkage if it is the
@@ -227,6 +232,11 @@ static LVPair getLVForNamespaceScopeDecl(const NamedDecl *D,
   //
   //     - an object or reference, unless it has internal linkage; or
   if (const VarDecl *Var = dyn_cast<VarDecl>(D)) {
+    bool isDeclaration = (Var->hasDefinition() == VarDecl::DeclarationOnly);
+
+    // GCC applies the following optimization to variables and static
+    // data members, but not to functions:
+    //
     // Modify the variable's LV by the LV of its type unless this is
     // C or extern "C".  This follows from [basic.link]p9:
     //   A type without linkage shall not be used as the type of a
@@ -245,12 +255,20 @@ static LVPair getLVForNamespaceScopeDecl(const NamedDecl *D,
     //
     // Note that we don't want to make the variable non-external
     // because of this, but unique-external linkage suits us.
-    if (Context.getLangOptions().CPlusPlus && !Var->isExternC() &&
-        !ExplicitVisibility) {
+    if (Context.getLangOptions().CPlusPlus && !ExplicitVisibility &&
+        !Var->isExternC()) {
       LVPair TypeLV = Var->getType()->getLinkageAndVisibility();
       if (TypeLV.first != ExternalLinkage)
         return LVPair(UniqueExternalLinkage, DefaultVisibility);
-      LV.second = minVisibility(LV.second, TypeLV.second);
+
+      // Otherwise, ignore type visibility for declarations.
+      if (!isDeclaration)
+        LV.second = minVisibility(LV.second, TypeLV.second);
+    }
+
+    // Don't consider -fvisibility for pure declarations.
+    if (isDeclaration) {
+      ConsiderGlobalVisibility = false;
     }
 
     if (!Context.getLangOptions().CPlusPlus &&
@@ -315,11 +333,12 @@ static LVPair getLVForNamespaceScopeDecl(const NamedDecl *D,
 
       if (SpecInfo->getTemplateSpecializationKind()
             == TSK_ExplicitInstantiationDeclaration)
-        ConsiderDashFVisibility = false;
+        ConsiderGlobalVisibility = false;
     }
 
-    if (ConsiderDashFVisibility)
-      ConsiderDashFVisibility = Function->hasBody();
+    // -fvisibility only applies to function definitions.
+    if (ConsiderGlobalVisibility)
+      ConsiderGlobalVisibility = Function->hasBody();
 
   //     - a named class (Clause 9), or an unnamed class defined in a
   //       typedef declaration in which the class has the typedef name
@@ -346,12 +365,12 @@ static LVPair getLVForNamespaceScopeDecl(const NamedDecl *D,
 
       if (Spec->getTemplateSpecializationKind()
             == TSK_ExplicitInstantiationDeclaration)
-        ConsiderDashFVisibility = false;
+        ConsiderGlobalVisibility = false;
     }
 
     // Consider -fvisibility unless the type has C linkage.
-    if (ConsiderDashFVisibility)
-      ConsiderDashFVisibility =
+    if (ConsiderGlobalVisibility)
+      ConsiderGlobalVisibility =
         (Context.getLangOptions().CPlusPlus &&
          !Tag->getDeclContext()->isExternCContext());
 
@@ -401,7 +420,7 @@ static LVPair getLVForNamespaceScopeDecl(const NamedDecl *D,
     // If we have an explicit visibility attribute, merge that in.
     if (ExplicitVisibility)
       StandardV = GetVisibilityFromAttr(ExplicitVisibility);
-    else if (ConsiderDashFVisibility)
+    else if (ConsiderGlobalVisibility)
       StandardV = Context.getLangOptions().getVisibilityMode();
     else
       StandardV = DefaultVisibility; // no-op
@@ -413,7 +432,7 @@ static LVPair getLVForNamespaceScopeDecl(const NamedDecl *D,
 }
 
 static LVPair getLVForClassMember(const NamedDecl *D,
-                                  bool ConsiderGlobalSettings) {
+                                  bool ConsiderGlobalVisibility) {
   // Only certain class members have linkage.  Note that fields don't
   // really have linkage, but it's convenient to say they do for the
   // purposes of calculating linkage of pointer-to-data-member
@@ -425,12 +444,9 @@ static LVPair getLVForClassMember(const NamedDecl *D,
          (D->getDeclName() || cast<TagDecl>(D)->getTypedefForAnonDecl()))))
     return LVPair(NoLinkage, DefaultVisibility);
 
-  // If we have an explicit visibility attribute, merge that in.
-  const VisibilityAttr *VA = GetExplicitVisibility(D);
-
   // Class members only have linkage if their class has external linkage.
-  LVPair ClassLV = getLVForDecl(cast<RecordDecl>(D->getDeclContext()),
-                                ConsiderGlobalSettings && !VA);
+  // Always ignore global visibility settings during this.
+  LVPair ClassLV = getLVForDecl(cast<RecordDecl>(D->getDeclContext()), false);
   if (!isExternalLinkage(ClassLV.first))
     return LVPair(NoLinkage, DefaultVisibility);
 
@@ -440,19 +456,21 @@ static LVPair getLVForClassMember(const NamedDecl *D,
 
   // Start with the class's linkage and visibility.
   LVPair LV = ClassLV;
-  if (VA) LV.second = minVisibility(LV.second, GetVisibilityFromAttr(VA));
-
-  // If it's a variable declaration and we don't have an explicit
-  // visibility attribute, apply the LV from its type.
-  // See the comment about namespace-scope variable decls above.
-  if (!VA && isa<VarDecl>(D)) {
-    LVPair TypeLV = cast<VarDecl>(D)->getType()->getLinkageAndVisibility();
-    if (TypeLV.first != ExternalLinkage)
-      LV.first = minLinkage(LV.first, UniqueExternalLinkage);
-    LV.second = minVisibility(LV.second, TypeLV.second);
+
+  // If we have an explicit visibility attribute, merge that in and
+  // ignore global visibility settings.
+  const VisibilityAttr *VA = GetExplicitVisibility(D);
+  if (VA) {
+    LV.second = minVisibility(LV.second, GetVisibilityFromAttr(VA));
+    ConsiderGlobalVisibility = false;
   }
 
+  bool HasExplicitVisibility = (VA ||
+    HasExplicitVisibilityInHierarchy(cast<RecordDecl>(D->getDeclContext())));
+
   if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D)) {
+    TemplateSpecializationKind TSK = TSK_Undeclared;
+
     // If this is a method template specialization, use the linkage for
     // the template parameters and arguments.
     if (FunctionTemplateSpecializationInfo *Spec
@@ -460,24 +478,80 @@ static LVPair getLVForClassMember(const NamedDecl *D,
       LV = merge(LV, getLVForTemplateArgumentList(*Spec->TemplateArguments));
       LV = merge(LV, getLVForTemplateParameterList(
                               Spec->getTemplate()->getTemplateParameters()));
+
+      TSK = Spec->getTemplateSpecializationKind();
+    } else if (MemberSpecializationInfo *MSI =
+                 MD->getMemberSpecializationInfo()) {
+      TSK = MSI->getTemplateSpecializationKind();
     }
 
-    // If -fvisibility-inlines-hidden was provided, then inline C++
-    // member functions get "hidden" visibility if they don't have an
-    // explicit visibility attribute.
-    if (ConsiderGlobalSettings && !VA && MD->isInlined() &&
-        LV.second > HiddenVisibility &&
-        D->getASTContext().getLangOptions().InlineVisibilityHidden &&
-        MD->getTemplateSpecializationKind()
-          != TSK_ExplicitInstantiationDeclaration)
+    // Ignore global visibility if it's an extern template.
+    if (ConsiderGlobalVisibility)
+      ConsiderGlobalVisibility = (TSK != TSK_ExplicitInstantiationDeclaration);
+
+    // If we're paying attention to global visibility, apply
+    // -finline-visibility-hidden if this is an inline method.
+    //
+    // Note that we ignore the existence of visibility attributes
+    // on containing classes when deciding whether to do this.
+    if (ConsiderGlobalVisibility && MD->isInlined() &&
+        MD->getASTContext().getLangOptions().InlineVisibilityHidden)
       LV.second = HiddenVisibility;
 
-  // Similarly for member class template specializations.
-  } else if (const ClassTemplateSpecializationDecl *Spec
-               = dyn_cast<ClassTemplateSpecializationDecl>(D)) {
-    LV = merge(LV, getLVForTemplateArgumentList(Spec->getTemplateArgs()));
-    LV = merge(LV, getLVForTemplateParameterList(
+    // Note that in contrast to basically every other situation, we
+    // *do* apply -fvisibility to method declarations.
+
+  } else if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(D)) {
+    TemplateSpecializationKind TSK = TSK_Undeclared;
+
+    if (const ClassTemplateSpecializationDecl *Spec
+        = dyn_cast<ClassTemplateSpecializationDecl>(RD)) {
+      // Merge template argument/parameter information for member
+      // class template specializations.
+      LV = merge(LV, getLVForTemplateArgumentList(Spec->getTemplateArgs()));
+      LV = merge(LV, getLVForTemplateParameterList(
                     Spec->getSpecializedTemplate()->getTemplateParameters()));
+
+      TSK = Spec->getTemplateSpecializationKind();
+    } else if (MemberSpecializationInfo *MSI =
+                 RD->getMemberSpecializationInfo()) {
+      TSK = MSI->getTemplateSpecializationKind();
+    }
+
+    // Ignore global visibility if it's an extern template.
+    if (ConsiderGlobalVisibility)
+      ConsiderGlobalVisibility = (TSK != TSK_ExplicitInstantiationDeclaration);
+
+  // Static data members.
+  } else if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
+    // If we don't have explicit visibility information in the
+    // hierarchy, apply the LV from its type.  See the comment about
+    // namespace-scope variables for justification for this
+    // optimization.
+    if (!HasExplicitVisibility) {
+      LVPair TypeLV = VD->getType()->getLinkageAndVisibility();
+      if (TypeLV.first != ExternalLinkage)
+        LV.first = minLinkage(LV.first, UniqueExternalLinkage);
+      LV.second = minVisibility(LV.second, TypeLV.second);
+    }
+
+    // Ignore global visibility if it's an extern template or
+    // just a declaration.
+    if (ConsiderGlobalVisibility)
+      ConsiderGlobalVisibility =
+        (VD->getDefinition() &&
+         VD->getTemplateSpecializationKind()
+           != TSK_ExplicitInstantiationDeclaration);
+  }
+
+  // Suppress -fvisibility if we have explicit visibility on any of
+  // our ancestors.
+  ConsiderGlobalVisibility &= !HasExplicitVisibility;
+
+  // Apply -fvisibility if desired.
+  if (ConsiderGlobalVisibility && LV.second != HiddenVisibility) {
+    LV.second = minVisibility(LV.second,
+                    D->getASTContext().getLangOptions().getVisibilityMode());
   }
 
   return LV;
@@ -962,6 +1036,17 @@ VarDecl *VarDecl::getDefinition() {
   return 0;
 }
 
+VarDecl::DefinitionKind VarDecl::hasDefinition() const {
+  DefinitionKind Kind = DeclarationOnly;
+  
+  const VarDecl *First = getFirstDeclaration();
+  for (redecl_iterator I = First->redecls_begin(), E = First->redecls_end();
+       I != E; ++I)
+    Kind = std::max(Kind, (*I)->isThisDeclarationADefinition());
+
+  return Kind;
+}
+
 const Expr *VarDecl::getAnyInitializer(const VarDecl *&D) const {
   redecl_iterator I = redecls_begin(), E = redecls_end();
   while (I != E && !I->getInit())
diff --git a/lib/CodeGen/CodeGenModule.cpp b/lib/CodeGen/CodeGenModule.cpp
index efcdce70a0..558a8a3cf8 100644
--- a/lib/CodeGen/CodeGenModule.cpp
+++ b/lib/CodeGen/CodeGenModule.cpp
@@ -164,6 +164,17 @@ void CodeGenModule::ErrorUnsupported(const Decl *D, const char *Type,
   getDiags().Report(Context.getFullLoc(D->getLocation()), DiagID) << Msg;
 }
 
+static llvm::GlobalValue::VisibilityTypes GetLLVMVisibility(Visibility V) {
+  switch (V) {
+  case DefaultVisibility:   return llvm::GlobalValue::DefaultVisibility;
+  case HiddenVisibility:    return llvm::GlobalValue::HiddenVisibility;
+  case ProtectedVisibility: return llvm::GlobalValue::ProtectedVisibility;
+  }
+  llvm_unreachable("unknown visibility!");
+  return llvm::GlobalValue::DefaultVisibility;
+}
+
+
 void CodeGenModule::setGlobalVisibility(llvm::GlobalValue *GV,
                                         const NamedDecl *D) const {
   // Internal definitions always have default visibility.
@@ -172,15 +183,7 @@ void CodeGenModule::setGlobalVisibility(llvm::GlobalValue *GV,
     return;
   }
 
-  switch (D->getVisibility()) {
-  case DefaultVisibility:
-    return GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
-  case HiddenVisibility:
-    return GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
-  case ProtectedVisibility:
-    return GV->setVisibility(llvm::GlobalValue::ProtectedVisibility);
-  }
-  llvm_unreachable("unknown visibility!");
+  GV->setVisibility(GetLLVMVisibility(D->getVisibility()));
 }
 
 /// Set the symbol visibility of type information (vtable and RTTI)
@@ -934,15 +937,18 @@ CodeGenModule::GetOrCreateLLVMGlobal(llvm::StringRef MangledName,
     // handling.
     GV->setConstant(DeclIsConstantGlobal(Context, D));
 
-    // FIXME: Merge with other attribute handling code.
-    if (D->getStorageClass() == SC_PrivateExtern)
-      GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
+    // Set linkage and visibility in case we never see a definition.
+    std::pair<Linkage,Visibility> LV = D->getLinkageAndVisibility();
+    if (LV.first != ExternalLinkage) {
+      GV->setLinkage(llvm::GlobalValue::InternalLinkage);
+    } else {
+      if (D->hasAttr<DLLImportAttr>())
+        GV->setLinkage(llvm::GlobalValue::DLLImportLinkage);
+      else if (D->hasAttr<WeakAttr>() || D->hasAttr<WeakImportAttr>())
+        GV->setLinkage(llvm::GlobalValue::ExternalWeakLinkage);
 
-    if (D->hasAttr<DLLImportAttr>())
-      GV->setLinkage(llvm::GlobalValue::DLLImportLinkage);
-    else if (D->hasAttr<WeakAttr>() ||
-        D->hasAttr<WeakImportAttr>())
-      GV->setLinkage(llvm::GlobalValue::ExternalWeakLinkage);
+      GV->setVisibility(GetLLVMVisibility(LV.second));
+    }
 
     GV->setThreadLocal(D->isThreadSpecified());
   }
diff --git a/test/CodeGenCXX/visibility.cpp b/test/CodeGenCXX/visibility.cpp
index 5a6ded88b1..a55544573d 100644
--- a/test/CodeGenCXX/visibility.cpp
+++ b/test/CodeGenCXX/visibility.cpp
@@ -10,6 +10,16 @@
 // CHECK: @_ZN5Test71bE = global
 // CHECK: @test9_var = global
 // CHECK-HIDDEN: @test9_var = global
+// CHECK: @_ZN6Test121A6hiddenE = external hidden global
+// CHECK: @_ZN6Test121A7visibleE = external global
+// CHECK-HIDDEN: @_ZN6Test121A6hiddenE = external hidden global
+// CHECK-HIDDEN: @_ZN6Test121A7visibleE = external global
+// CHECK: @_ZN6Test131B1aE = hidden global
+// CHECK: @_ZN6Test131C1aE = global
+// CHECK-HIDDEN: @_ZN6Test131B1aE = hidden global
+// CHECK-HIDDEN: @_ZN6Test131C1aE = global
+// CHECK: @_ZN6Test143varE = external global
+// CHECK-HIDDEN: @_ZN6Test143varE = external global
 // CHECK: @_ZTVN5Test63fooE = weak_odr hidden constant 
 
 namespace Test1 {
@@ -165,3 +175,46 @@ namespace Test11 {
   // CHECK-HIDDEN: define linkonce_odr hidden void @_ZN6Test111A3fooEv(
   // CHECK-HIDDEN: define linkonce_odr void @_ZN6Test111A3barEv(
 }
+
+// Tested at top of file.
+namespace Test12 {
+  struct A {
+    // This is hidden in all cases: the explicit attribute takes
+    // priority over -fvisibility on the parent.
+    static int hidden HIDDEN;
+
+    // This is default in all cases because it's only a declaration.
+    static int visible;
+  };
+
+  void test() {
+    A::hidden = 0;
+    A::visible = 0;
+  }
+}
+
+// Tested at top of file.
+namespace Test13 {
+  struct HIDDEN A {};
+
+  // Should be hidden in all cases.
+  struct B {
+    static A a;
+  };
+  A B::a;
+
+  // Should be default in all cases.
+  struct DEFAULT C {
+    static A a;
+  };
+  A C::a;
+};
+
+// Tested at top of file.
+namespace Test14 {
+  // Neither the visibility of the type nor -fvisibility=hidden should
+  // apply to declarations.
+  extern struct A *var;
+
+  struct A *test() { return var; }
+}