Fix vtable not receiving hidden visibility when using push(visibility)

[jakehehrlich]

Summary

This change should resolve https://bugs.llvm.org/show_bug.cgi?id=35022

[rjmccall]

Hmm. We're calling this from getAddrOfVTable, which can be used to get a declaration for an external v-table, but setGlobalVisibility appears to set visibility as if we were emitting a definition. Among other reasons, the difference is important because we generally don't honor implicit visibility when emitting external references. The pragma should be treated as an explicit source of visibility — IIRC we model it by actually adding a visibility attribute — but I think your patch would cause us to use hidden visibility on an external v-table declaration even if the only reason we thought it was hidden was that we were compiling under -fvisibility=hidden.

The logic for declarations is currently defined in the function setLinkageAndVisibilityForGV in CodeGenModule.cpp. That's a static function in that file: we could just add it to CodeGenModule, but I think there's a better solution: we could add a ForDefinition_t flag to CGM::setGlobalVisibility and then unify these two implementations.

[jakehehrlich]

In D39627#915820, @rjmccall wrote:

Hmm. We're calling this from getAddrOfVTable, which can be used to get a declaration for an external v-table, but setGlobalVisibility appears to set visibility as if we were emitting a definition. Among other reasons, the difference is important because we generally don't honor implicit visibility when emitting external references. The pragma should be treated as an explicit source of visibility — IIRC we model it by actually adding a visibility attribute — but I think your patch would cause us to use hidden visibility on an external v-table declaration even if the only reason we thought it was hidden was that we were compiling under -fvisibility=hidden.

The logic for declarations is currently defined in the function setLinkageAndVisibilityForGV in CodeGenModule.cpp. That's a static function in that file: we could just add it to CodeGenModule, but I think there's a better solution: we could add a ForDefinition_t flag to CGM::setGlobalVisibility and then unify these two implementations.

I'm missing some background lingo here. What is the meaning of "a declaration" and "a definition" here? It sounds like setLinkageAndVisibilityForGV will be called if the GlobalValue is synthetically generated (as vtables are) and doesn't have a corresponding deceleration in source? And setGlobalVisibility will be used to set visibility of GlobalValues mentioned in source?

[efriedma]

Adding cfe-commits. (In the future, please make sure to CC that list instead of llvm-commits for clang changes.)

I'm missing some background lingo here. What is the meaning of "a declaration" and "a definition" here?

In most cases, we only emit the vtable for a class in one object file. Informally, we can call that the "definition" of the vtable, as an analogy to variables and functions. See also http://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-vtable .

[rjmccall]

In D39627#917175, @efriedma wrote:

Adding cfe-commits. (In the future, please make sure to CC that list instead of llvm-commits for clang changes.)

I'm missing some background lingo here. What is the meaning of "a declaration" and "a definition" here?

In most cases, we only emit the vtable for a class in one object file. Informally, we can call that the "definition" of the vtable, as an analogy to variables and functions. See also http://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-vtable .

LLVM also distinguishes between a declaration (a Function or GlobalVariable which is presumed to be defined in a different Module) vs. a definition.

[jakehehrlich]

1. Added a ForDefinition parameter to setGlobalVisibility and merged visibility setting behaviors for setGlobalVisibility and setLinkageAndVisibilityForGV
2. Renamed setLinkageAndVisibilityForGV to setLinkageForGV
3. refactored calls to these two functions to match their new form.

[rjmccall]

Looks great, thanks!

[jakehehrlich]

This test was failing on windows machines. I'm hoping this change (adding "_cc1" to "%clang") will resolve this because %clang_cc1 shouldn't behave any differently on windows.

[jakehehrlich]

When I changed the test to add "_cc1" I got rid of the temporary file. Well for some reason on windows we're still getting different output so we need the temporary file to debug things.

[efriedma]

You probably just need to pass a "-triple" flag so we don't use Windows mangling or something like that.

lib/CodeGen/CGDecl.cpp

if (Ty.getAddressSpace() != LangAS::opencl_local)
     Init = EmitNullConstant(Ty);
   else
     Init = llvm::UndefValue::get(LTy);
 
   llvm::GlobalVariable *GV = new llvm::GlobalVariable(
       getModule(), LTy, Ty.isConstant(getContext()), Linkage, Init, Name,
       nullptr, llvm::GlobalVariable::NotThreadLocal, TargetAS);
   GV->setAlignment(getContext().getDeclAlign(&D).getQuantity());
-  setGlobalVisibility(GV, &D);
+  setGlobalVisibility(GV, &D, ForDefinition);
 
   if (supportsCOMDAT() && GV->isWeakForLinker())
     GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));
 
   if (D.getTLSKind())
     setTLSMode(GV, D);
 
   if (D.isExternallyVisible()) {

lib/CodeGen/CGVTT.cpp

CodeGenVTables::EmitVTTDefinition(llvm::GlobalVariable *VTT,
 
   // Set the correct linkage.
   VTT->setLinkage(Linkage);
 
   if (CGM.supportsCOMDAT() && VTT->isWeakForLinker())
     VTT->setComdat(CGM.getModule().getOrInsertComdat(VTT->getName()));
 
   // Set the right visibility.
-  CGM.setGlobalVisibility(VTT, RD);
+  CGM.setGlobalVisibility(VTT, RD, ForDefinition);
 }
 
 llvm::GlobalVariable *CodeGenVTables::GetAddrOfVTT(const CXXRecordDecl *RD) {
   assert(RD->getNumVBases() && "Only classes with virtual bases need a VTT");
 
   SmallString<256> OutName;
   llvm::raw_svector_ostream Out(OutName);
   cast<ItaniumMangleContext>(CGM.getCXXABI().getMangleContext())

lib/CodeGen/CGVTables.cpp

llvm::Constant *CodeGenModule::GetAddrOfThunk(GlobalDecl GD,
 
   llvm::Type *Ty = getTypes().GetFunctionTypeForVTable(GD);
   return GetOrCreateLLVMFunction(Name, Ty, GD, /*ForVTable=*/true,
                                  /*DontDefer=*/true, /*IsThunk=*/true);
 }
 
 static void setThunkVisibility(CodeGenModule &CGM, const CXXMethodDecl *MD,
                                const ThunkInfo &Thunk, llvm::Function *Fn) {
-  CGM.setGlobalVisibility(Fn, MD);
+  CGM.setGlobalVisibility(Fn, MD, ForDefinition);
 }
 
 static void setThunkProperties(CodeGenModule &CGM, const ThunkInfo &Thunk,
                                llvm::Function *ThunkFn, bool ForVTable,
                                GlobalDecl GD) {
   CGM.setFunctionLinkage(GD, ThunkFn);
   CGM.getCXXABI().setThunkLinkage(ThunkFn, ForVTable, GD,
                                   !Thunk.Return.isEmpty());
CodeGenVTables::GenerateConstructionVTable(const CXXRecordDecl *RD,
   // linkage construction vtable. The ABI only requires complete-object vtables
   // to be the same for all instances of a type, not construction vtables.
   if (Linkage == llvm::GlobalVariable::AvailableExternallyLinkage)
     Linkage = llvm::GlobalVariable::InternalLinkage;
 
   // Create the variable that will hold the construction vtable.
   llvm::GlobalVariable *VTable =
     CGM.CreateOrReplaceCXXRuntimeVariable(Name, VTType, Linkage);
-  CGM.setGlobalVisibility(VTable, RD);
+  CGM.setGlobalVisibility(VTable, RD, ForDefinition);
 
   // V-tables are always unnamed_addr.
   VTable->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
 
   llvm::Constant *RTTI = CGM.GetAddrOfRTTIDescriptor(
       CGM.getContext().getTagDeclType(Base.getBase()));
 
   // Create and set the initializer.

lib/CodeGen/CodeGenModule.h

public:
   /// Adds !invariant.barrier !tag to instruction
   void DecorateInstructionWithInvariantGroup(llvm::Instruction *I,
                                              const CXXRecordDecl *RD);
 
   /// Emit the given number of characters as a value of type size_t.
   llvm::ConstantInt *getSize(CharUnits numChars);
 
   /// Set the visibility for the given LLVM GlobalValue.
-  void setGlobalVisibility(llvm::GlobalValue *GV, const NamedDecl *D) const;
+  void setGlobalVisibility(llvm::GlobalValue *GV, const NamedDecl *D,
+                           ForDefinition_t IsForDefinition) const;
 
   /// Set the TLS mode for the given LLVM GlobalValue for the thread-local
   /// variable declaration D.
   void setTLSMode(llvm::GlobalValue *GV, const VarDecl &D) const;
 
   static llvm::GlobalValue::VisibilityTypes GetLLVMVisibility(Visibility V) {
     switch (V) {
     case DefaultVisibility:   return llvm::GlobalValue::DefaultVisibility;

lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::ErrorUnsupported(const Decl *D, const char *Type) {
   getDiags().Report(Context.getFullLoc(D->getLocation()), DiagID) << Msg;
 }
 
 llvm::ConstantInt *CodeGenModule::getSize(CharUnits size) {
   return llvm::ConstantInt::get(SizeTy, size.getQuantity());
 }
 
 void CodeGenModule::setGlobalVisibility(llvm::GlobalValue *GV,
-                                        const NamedDecl *D) const {
+                                        const NamedDecl *D,
+                                        ForDefinition_t IsForDefinition) const {
   // Internal definitions always have default visibility.
   if (GV->hasLocalLinkage()) {
     GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
     return;
   }
 
   // Set visibility for definitions.
   LinkageInfo LV = D->getLinkageAndVisibility();
-  if (LV.isVisibilityExplicit() || !GV->hasAvailableExternallyLinkage())
+  if (LV.isVisibilityExplicit() ||
+      (IsForDefinition && !GV->hasAvailableExternallyLinkage()))
     GV->setVisibility(GetLLVMVisibility(LV.getVisibility()));
 }
 
 static llvm::GlobalVariable::ThreadLocalMode GetLLVMTLSModel(StringRef S) {
   return llvm::StringSwitch<llvm::GlobalVariable::ThreadLocalMode>(S)
       .Case("global-dynamic", llvm::GlobalVariable::GeneralDynamicTLSModel)
       .Case("local-dynamic", llvm::GlobalVariable::LocalDynamicTLSModel)
       .Case("initial-exec", llvm::GlobalVariable::InitialExecTLSModel)
void CodeGenModule::SetLLVMFunctionAttributesForDefinition(const Decl *D,
   if (CodeGenOpts.SanitizeCfiCrossDso)
     if (auto *FD = dyn_cast<FunctionDecl>(D))
       CreateFunctionTypeMetadata(FD, F);
 }
 
 void CodeGenModule::SetCommonAttributes(const Decl *D,
                                         llvm::GlobalValue *GV) {
   if (const auto *ND = dyn_cast_or_null<NamedDecl>(D))
-    setGlobalVisibility(GV, ND);
+    setGlobalVisibility(GV, ND, ForDefinition);
   else
     GV->setVisibility(llvm::GlobalValue::DefaultVisibility);
 
   if (D && D->hasAttr<UsedAttr>())
     addUsedGlobal(GV);
 }
 
 void CodeGenModule::setAliasAttributes(const Decl *D,
void CodeGenModule::SetInternalFunctionAttributes(const Decl *D,
   SetLLVMFunctionAttributes(D, FI, F);
   SetLLVMFunctionAttributesForDefinition(D, F);
 
   F->setLinkage(llvm::Function::InternalLinkage);
 
   setNonAliasAttributes(D, F);
 }
 
-static void setLinkageAndVisibilityForGV(llvm::GlobalValue *GV,
-                                         const NamedDecl *ND) {
+static void setLinkageForGV(llvm::GlobalValue *GV,
   // Set linkage and visibility in case we never see a definition.
   LinkageInfo LV = ND->getLinkageAndVisibility();
   if (!isExternallyVisible(LV.getLinkage())) {
     // Don't set internal linkage on declarations.
   } else {
     if (ND->hasAttr<DLLImportAttr>()) {
       GV->setLinkage(llvm::GlobalValue::ExternalLinkage);
       GV->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);
     } else if (ND->hasAttr<DLLExportAttr>()) {
       GV->setLinkage(llvm::GlobalValue::ExternalLinkage);
     } else if (ND->hasAttr<WeakAttr>() || ND->isWeakImported()) {
       // "extern_weak" is overloaded in LLVM; we probably should have
       // separate linkage types for this.
       GV->setLinkage(llvm::GlobalValue::ExternalWeakLinkage);
     }
-
-    // Set visibility on a declaration only if it's explicit.
-    if (LV.isVisibilityExplicit())
-      GV->setVisibility(CodeGenModule::GetLLVMVisibility(LV.getVisibility()));
   }
 }
 
 void CodeGenModule::CreateFunctionTypeMetadata(const FunctionDecl *FD,
                                                llvm::Function *F) {
   // Only if we are checking indirect calls.
   if (!LangOpts.Sanitize.has(SanitizerKind::CFIICall))
     return;
assert(!F->arg_empty() &&
              ->canLosslesslyBitCastTo(F->getReturnType()) &&
            "unexpected this return");
     F->addAttribute(1, llvm::Attribute::Returned);
   }
 
   // Only a few attributes are set on declarations; these may later be
   // overridden by a definition.
 
-  setLinkageAndVisibilityForGV(F, FD);
+  setLinkageForGV(F, FD);
+  setGlobalVisibility(F, FD, NotForDefinition);
 
   if (FD->getAttr<PragmaClangTextSectionAttr>()) {
     F->addFnAttr("implicit-section-name");
   }
 
   if (const SectionAttr *SA = FD->getAttr<SectionAttr>())
     F->setSection(SA->getName());
 
CodeGenModule::GetOrCreateLLVMGlobal(StringRef MangledName,
   // Handle things which are present even on external declarations.
   if (D) {
     // FIXME: This code is overly simple and should be merged with other global
     // handling.
     GV->setConstant(isTypeConstant(D->getType(), false));
 
     GV->setAlignment(getContext().getDeclAlign(D).getQuantity());
 
-    setLinkageAndVisibilityForGV(GV, D);
+    setLinkageForGV(GV, D);
+    setGlobalVisibility(GV, D, NotForDefinition);
 
     if (D->getTLSKind()) {
       if (D->getTLSKind() == VarDecl::TLS_Dynamic)
         CXXThreadLocals.push_back(D);
       setTLSMode(GV, *D);
     }
 
     // If required by the ABI, treat declarations of static data members with
void CodeGenModule::EmitGlobalFunctionDefinition(GlobalDecl GD,
   // generating code for it because various parts of IR generation
   // want to propagate this information down (e.g. to local static
   // declarations).
   auto *Fn = cast<llvm::Function>(GV);
   setFunctionLinkage(GD, Fn);
   setFunctionDLLStorageClass(GD, Fn);
 
   // FIXME: this is redundant with part of setFunctionDefinitionAttributes
-  setGlobalVisibility(Fn, D);
+  setGlobalVisibility(Fn, D, ForDefinition);
 
   MaybeHandleStaticInExternC(D, Fn);
 
   maybeSetTrivialComdat(*D, *Fn);
 
   CodeGenFunction(*this).GenerateCode(D, Fn, FI);
 
   setFunctionDefinitionAttributes(D, Fn);
if (VD->isStaticDataMember() && VD->getAnyInitializer(InitVD) &&
       Linkage = llvm::GlobalVariable::InternalLinkage;
     }
   }
   auto TargetAS = getContext().getTargetAddressSpace(AddrSpace);
   auto *GV = new llvm::GlobalVariable(
       getModule(), Type, Constant, Linkage, InitialValue, Name.c_str(),
       /*InsertBefore=*/nullptr, llvm::GlobalVariable::NotThreadLocal, TargetAS);
   if (emitter) emitter->finalize(GV);
-  setGlobalVisibility(GV, VD);
+  setGlobalVisibility(GV, VD, ForDefinition);
   GV->setAlignment(Align.getQuantity());
   if (supportsCOMDAT() && GV->isWeakForLinker())
     GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));
   if (VD->getTLSKind())
     setTLSMode(GV, *VD);
   llvm::Constant *CV = GV;
   if (AddrSpace != LangAS::Default)
     CV = getTargetCodeGenInfo().performAddrSpaceCast(

lib/CodeGen/ItaniumCXXABI.cpp

void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables &CGVT,
 
   // Set the correct linkage.
   VTable->setLinkage(Linkage);
 
   if (CGM.supportsCOMDAT() && VTable->isWeakForLinker())
     VTable->setComdat(CGM.getModule().getOrInsertComdat(VTable->getName()));
 
   // Set the right visibility.
-  CGM.setGlobalVisibility(VTable, RD);
+  CGM.setGlobalVisibility(VTable, RD, ForDefinition);
 
   // Use pointer alignment for the vtable. Otherwise we would align them based
   // on the size of the initializer which doesn't make sense as only single
   // values are read.
   unsigned PAlign = CGM.getTarget().getPointerAlign(0);
   VTable->setAlignment(getContext().toCharUnitsFromBits(PAlign).getQuantity());
 
   // If this is the magic class __cxxabiv1::__fundamental_type_info,
llvm::GlobalVariable *ItaniumCXXABI::getAddrOfVTable(const CXXRecordDecl *RD,
 
   const VTableLayout &VTLayout =
       CGM.getItaniumVTableContext().getVTableLayout(RD);
   llvm::Type *VTableType = CGM.getVTables().getVTableType(VTLayout);
 
   VTable = CGM.CreateOrReplaceCXXRuntimeVariable(
       Name, VTableType, llvm::GlobalValue::ExternalLinkage);
   VTable->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+  CGM.setGlobalVisibility(VTable, RD, NotForDefinition);
 
   if (RD->hasAttr<DLLImportAttr>())
     VTable->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);
   else if (RD->hasAttr<DLLExportAttr>())
     VTable->setDLLStorageClass(llvm::GlobalValue::DLLExportStorageClass);
 
   return VTable;
 }

test/CodeGen/push-hidden-visibility-subclass.cpp

+// RUN: %clang_cc1 -emit-llvm %s -o %t
+// RUN: FileCheck --input-file=%t %s
+
+#pragma GCC visibility push(hidden)
+
+struct Base {
+  virtual ~Base() = default;
+  virtual void* Alloc() = 0;
+};
+
+class Child : public Base {
+public:
+  Child() = default;
+  void* Alloc();
+};
+
+void test() {
+  Child x;
+}
+
+// CHECK: @_ZTV5Child = external hidden unnamed_addr constant