<div dir="ltr">(-old cfe-commits, +new cfe-commits)</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jan 18, 2019 at 8:21 AM Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">I found this comment useful, but from your reply it sounds like it's no longer true:<div><br><div>- // We must indicate which VFTable is larger to support linking between</div><div>- // translation units which do and do not have RTTI data. The largest</div><div>- // VFTable contains the RTTI data; translation units which reference</div><div>- // the smaller VFTable always reference it relative to the first</div><div>- // virtual method.</div><div><br></div><div>If it's no longer true, why do we still call setSelectionKind(llvm::Comdat::Largest)?</div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 17, 2019 at 5:09 PM David Majnemer <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Yes, the comments don't look like they were relevant after this change.</div><br><div class="gmail_quote"><div dir="ltr">On Sat, Jan 12, 2019 at 4:52 PM Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">This removed two of the comments you had added in <a href="https://reviews.llvm.org/rL212142" target="_blank">https://reviews.llvm.org/rL212142</a> -- was that intentional?</div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Mar 18, 2015 at 6:08 PM David Majnemer <<a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: majnemer<br>
Date: Wed Mar 18 17:04:43 2015<br>
New Revision: 232680<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=232680&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=232680&view=rev</a><br>
Log:<br>
MS ABI: Don't try to emit VF/VB-Tables for extern class templates<br>
<br>
There will be an explicit template instantiation in another translation<br>
unit which will provide the definition of the VF/VB-Tables.<br>
<br>
This fixes PR22932.<br>
<br>
Modified:<br>
cfe/trunk/lib/AST/VTableBuilder.cpp<br>
cfe/trunk/lib/CodeGen/CGVTables.cpp<br>
cfe/trunk/lib/CodeGen/CodeGenModule.cpp<br>
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp<br>
cfe/trunk/test/CodeGenCXX/microsoft-abi-vbtables.cpp<br>
cfe/trunk/test/CodeGenCXX/microsoft-abi-vftables.cpp<br>
<br>
Modified: cfe/trunk/lib/AST/VTableBuilder.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/VTableBuilder.cpp?rev=232680&r1=232679&r2=232680&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/VTableBuilder.cpp?rev=232680&r1=232679&r2=232680&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/AST/VTableBuilder.cpp (original)<br>
+++ cfe/trunk/lib/AST/VTableBuilder.cpp Wed Mar 18 17:04:43 2015<br>
@@ -2589,7 +2589,9 @@ public:<br>
// Only include the RTTI component if we know that we will provide a<br>
// definition of the vftable.<br>
HasRTTIComponent = Context.getLangOpts().RTTIData &&<br>
- !MostDerivedClass->hasAttr<DLLImportAttr>();<br>
+ !MostDerivedClass->hasAttr<DLLImportAttr>() &&<br>
+ MostDerivedClass->getTemplateSpecializationKind() !=<br>
+ TSK_ExplicitInstantiationDeclaration;<br>
<br>
LayoutVFTable();<br>
<br>
<br>
Modified: cfe/trunk/lib/CodeGen/CGVTables.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGVTables.cpp?rev=232680&r1=232679&r2=232680&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGVTables.cpp?rev=232680&r1=232679&r2=232680&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/CodeGen/CGVTables.cpp (original)<br>
+++ cfe/trunk/lib/CodeGen/CGVTables.cpp Wed Mar 18 17:04:43 2015<br>
@@ -743,7 +743,7 @@ CodeGenModule::getVTableLinkage(const CX<br>
return DiscardableODRLinkage;<br>
<br>
case TSK_ExplicitInstantiationDeclaration:<br>
- llvm_unreachable("Should not have been asked to emit this");<br>
+ return llvm::GlobalVariable::ExternalLinkage;<br>
<br>
case TSK_ExplicitInstantiationDefinition:<br>
return NonDiscardableODRLinkage;<br>
<br>
Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=232680&r1=232679&r2=232680&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=232680&r1=232679&r2=232680&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)<br>
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Wed Mar 18 17:04:43 2015<br>
@@ -1851,7 +1851,8 @@ CodeGenModule::CreateOrReplaceCXXRuntime<br>
OldGV->eraseFromParent();<br>
}<br>
<br>
- if (supportsCOMDAT() && GV->isWeakForLinker())<br>
+ if (supportsCOMDAT() && GV->isWeakForLinker() &&<br>
+ !GV->hasAvailableExternallyLinkage())<br>
GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));<br>
<br>
return GV;<br>
<br>
Modified: cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp?rev=232680&r1=232679&r2=232680&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp?rev=232680&r1=232679&r2=232680&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp (original)<br>
+++ cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp Wed Mar 18 17:04:43 2015<br>
@@ -1487,102 +1487,97 @@ llvm::GlobalVariable *MicrosoftCXXABI::g<br>
#endif<br>
}<br>
<br>
- for (size_t J = 0, F = VFPtrs.size(); J != F; ++J) {<br>
- if (VFPtrs[J]->FullOffsetInMDC != VPtrOffset)<br>
- continue;<br>
- SmallString<256> VFTableName;<br>
- mangleVFTableName(getMangleContext(), RD, VFPtrs[J], VFTableName);<br>
- StringRef VTableName = VFTableName;<br>
-<br>
- uint64_t NumVTableSlots =<br>
- VTContext.getVFTableLayout(RD, VFPtrs[J]->FullOffsetInMDC)<br>
- .getNumVTableComponents();<br>
- llvm::GlobalValue::LinkageTypes VTableLinkage =<br>
- llvm::GlobalValue::ExternalLinkage;<br>
- llvm::ArrayType *VTableType =<br>
- llvm::ArrayType::get(CGM.Int8PtrTy, NumVTableSlots);<br>
- if (getContext().getLangOpts().RTTIData) {<br>
- VTableLinkage = llvm::GlobalValue::PrivateLinkage;<br>
- VTableName = "";<br>
- }<br>
+ VPtrInfo *const *VFPtrI =<br>
+ std::find_if(VFPtrs.begin(), VFPtrs.end(), [&](VPtrInfo *VPI) {<br>
+ return VPI->FullOffsetInMDC == VPtrOffset;<br>
+ });<br>
+ if (VFPtrI == VFPtrs.end()) {<br>
+ VFTablesMap[ID] = nullptr;<br>
+ return nullptr;<br>
+ }<br>
+ VPtrInfo *VFPtr = *VFPtrI;<br>
+<br>
+ SmallString<256> VFTableName;<br>
+ mangleVFTableName(getMangleContext(), RD, VFPtr, VFTableName);<br>
+<br>
+ llvm::GlobalValue::LinkageTypes VFTableLinkage = CGM.getVTableLinkage(RD);<br>
+ bool VFTableComesFromAnotherTU =<br>
+ llvm::GlobalValue::isAvailableExternallyLinkage(VFTableLinkage) ||<br>
+ llvm::GlobalValue::isExternalLinkage(VFTableLinkage);<br>
+ bool VTableAliasIsRequred =<br>
+ !VFTableComesFromAnotherTU && getContext().getLangOpts().RTTIData;<br>
+<br>
+ if (llvm::GlobalValue *VFTable =<br>
+ CGM.getModule().getNamedGlobal(VFTableName)) {<br>
+ VFTablesMap[ID] = VFTable;<br>
+ return VTableAliasIsRequred<br>
+ ? cast<llvm::GlobalVariable>(<br>
+ cast<llvm::GlobalAlias>(VFTable)->getBaseObject())<br>
+ : cast<llvm::GlobalVariable>(VFTable);<br>
+ }<br>
<br>
- VTable = CGM.getModule().getNamedGlobal(VFTableName);<br>
- if (!VTable) {<br>
- // Create a backing variable for the contents of VTable. The VTable may<br>
- // or may not include space for a pointer to RTTI data.<br>
- llvm::GlobalValue *VFTable = VTable = new llvm::GlobalVariable(<br>
- CGM.getModule(), VTableType, /*isConstant=*/true, VTableLinkage,<br>
- /*Initializer=*/nullptr, VTableName);<br>
- VTable->setUnnamedAddr(true);<br>
-<br>
- // Only insert a pointer into the VFTable for RTTI data if we are not<br>
- // importing it. We never reference the RTTI data directly so there is no<br>
- // need to make room for it.<br>
- if (getContext().getLangOpts().RTTIData &&<br>
- !RD->hasAttr<DLLImportAttr>()) {<br>
- llvm::Value *GEPIndices[] = {llvm::ConstantInt::get(CGM.IntTy, 0),<br>
- llvm::ConstantInt::get(CGM.IntTy, 1)};<br>
- // Create a GEP which points just after the first entry in the VFTable,<br>
- // this should be the location of the first virtual method.<br>
- llvm::Constant *VTableGEP =<br>
- llvm::ConstantExpr::getInBoundsGetElementPtr(VTable, GEPIndices);<br>
- // The symbol for the VFTable is an alias to the GEP. It is<br>
- // transparent, to other modules, what the nature of this symbol is; all<br>
- // that matters is that the alias be the address of the first virtual<br>
- // method.<br>
- VFTable = llvm::GlobalAlias::create(<br>
- cast<llvm::SequentialType>(VTableGEP->getType())->getElementType(),<br>
- /*AddressSpace=*/0, llvm::GlobalValue::ExternalLinkage,<br>
- VFTableName.str(), VTableGEP, &CGM.getModule());<br>
- } else {<br>
- // We don't need a GlobalAlias to be a symbol for the VTable if we won't<br>
- // be referencing any RTTI data. The GlobalVariable will end up being<br>
- // an appropriate definition of the VFTable.<br>
- VTable->setName(VFTableName.str());<br>
- }<br>
-<br>
- VFTable->setUnnamedAddr(true);<br>
- if (RD->hasAttr<DLLImportAttr>())<br>
- VFTable->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);<br>
- else if (RD->hasAttr<DLLExportAttr>())<br>
- VFTable->setDLLStorageClass(llvm::GlobalValue::DLLExportStorageClass);<br>
-<br>
- llvm::GlobalValue::LinkageTypes VFTableLinkage = CGM.getVTableLinkage(RD);<br>
- if (VFTable != VTable) {<br>
- if (llvm::GlobalValue::isAvailableExternallyLinkage(VFTableLinkage)) {<br>
- // AvailableExternally implies that we grabbed the data from another<br>
- // executable. No need to stick the alias in a Comdat.<br>
- } else if (llvm::GlobalValue::isInternalLinkage(VFTableLinkage) ||<br>
- llvm::GlobalValue::isWeakODRLinkage(VFTableLinkage) ||<br>
- llvm::GlobalValue::isLinkOnceODRLinkage(VFTableLinkage)) {<br>
- // The alias is going to be dropped into a Comdat, no need to make it<br>
- // weak.<br>
- if (!llvm::GlobalValue::isInternalLinkage(VFTableLinkage))<br>
- VFTableLinkage = llvm::GlobalValue::ExternalLinkage;<br>
- llvm::Comdat *C =<br>
- CGM.getModule().getOrInsertComdat(VFTable->getName());<br>
- // We must indicate which VFTable is larger to support linking between<br>
- // translation units which do and do not have RTTI data. The largest<br>
- // VFTable contains the RTTI data; translation units which reference<br>
- // the smaller VFTable always reference it relative to the first<br>
- // virtual method.<br>
- C->setSelectionKind(llvm::Comdat::Largest);<br>
- VTable->setComdat(C);<br>
- } else {<br>
- llvm_unreachable("unexpected linkage for vftable!");<br>
- }<br>
- } else {<br>
- if (llvm::GlobalValue::isWeakForLinker(VFTableLinkage))<br>
- VTable->setComdat(<br>
- CGM.getModule().getOrInsertComdat(VTable->getName()));<br>
- }<br>
- VFTable->setLinkage(VFTableLinkage);<br>
- CGM.setGlobalVisibility(VFTable, RD);<br>
- VFTablesMap[ID] = VFTable;<br>
+ uint64_t NumVTableSlots =<br>
+ VTContext.getVFTableLayout(RD, VFPtr->FullOffsetInMDC)<br>
+ .getNumVTableComponents();<br>
+ llvm::GlobalValue::LinkageTypes VTableLinkage =<br>
+ VTableAliasIsRequred ? llvm::GlobalValue::PrivateLinkage : VFTableLinkage;<br>
+<br>
+ StringRef VTableName = VTableAliasIsRequred ? StringRef() : VFTableName.str();<br>
+<br>
+ llvm::ArrayType *VTableType =<br>
+ llvm::ArrayType::get(CGM.Int8PtrTy, NumVTableSlots);<br>
+<br>
+ // Create a backing variable for the contents of VTable. The VTable may<br>
+ // or may not include space for a pointer to RTTI data.<br>
+ llvm::GlobalValue *VFTable;<br>
+ VTable = new llvm::GlobalVariable(CGM.getModule(), VTableType,<br>
+ /*isConstant=*/true, VTableLinkage,<br>
+ /*Initializer=*/nullptr, VTableName);<br>
+ VTable->setUnnamedAddr(true);<br>
+<br>
+ llvm::Comdat *C = nullptr;<br>
+ if (!VFTableComesFromAnotherTU &&<br>
+ (llvm::GlobalValue::isWeakForLinker(VFTableLinkage) ||<br>
+ (llvm::GlobalValue::isLocalLinkage(VFTableLinkage) &&<br>
+ VTableAliasIsRequred)))<br>
+ C = CGM.getModule().getOrInsertComdat(VFTableName.str());<br>
+<br>
+ // Only insert a pointer into the VFTable for RTTI data if we are not<br>
+ // importing it. We never reference the RTTI data directly so there is no<br>
+ // need to make room for it.<br>
+ if (VTableAliasIsRequred) {<br>
+ llvm::Value *GEPIndices[] = {llvm::ConstantInt::get(CGM.IntTy, 0),<br>
+ llvm::ConstantInt::get(CGM.IntTy, 1)};<br>
+ // Create a GEP which points just after the first entry in the VFTable,<br>
+ // this should be the location of the first virtual method.<br>
+ llvm::Constant *VTableGEP =<br>
+ llvm::ConstantExpr::getInBoundsGetElementPtr(VTable, GEPIndices);<br>
+ if (llvm::GlobalValue::isWeakForLinker(VFTableLinkage)) {<br>
+ VFTableLinkage = llvm::GlobalValue::ExternalLinkage;<br>
+ if (C)<br>
+ C->setSelectionKind(llvm::Comdat::Largest);<br>
}<br>
- break;<br>
+ VFTable = llvm::GlobalAlias::create(<br>
+ cast<llvm::SequentialType>(VTableGEP->getType())->getElementType(),<br>
+ /*AddressSpace=*/0, VFTableLinkage, VFTableName.str(), VTableGEP,<br>
+ &CGM.getModule());<br>
+ VFTable->setUnnamedAddr(true);<br>
+ } else {<br>
+ // We don't need a GlobalAlias to be a symbol for the VTable if we won't<br>
+ // be referencing any RTTI data.<br>
+ // The GlobalVariable will end up being an appropriate definition of the<br>
+ // VFTable.<br>
+ VFTable = VTable;<br>
}<br>
+ if (C)<br>
+ VTable->setComdat(C);<br>
<br>
+ if (RD->hasAttr<DLLImportAttr>())<br>
+ VFTable->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);<br>
+ else if (RD->hasAttr<DLLExportAttr>())<br>
+ VFTable->setDLLStorageClass(llvm::GlobalValue::DLLExportStorageClass);<br>
+<br>
+ VFTablesMap[ID] = VFTable;<br>
return VTable;<br>
}<br>
<br>
@@ -1811,9 +1806,6 @@ void MicrosoftCXXABI::emitVBTableDefinit<br>
llvm::ArrayType::get(CGM.IntTy, Offsets.size());<br>
llvm::Constant *Init = llvm::ConstantArray::get(VBTableType, Offsets);<br>
GV->setInitializer(Init);<br>
-<br>
- // Set the right visibility.<br>
- CGM.setGlobalVisibility(GV, RD);<br>
}<br>
<br>
llvm::Value *MicrosoftCXXABI::performThisAdjustment(CodeGenFunction &CGF,<br>
<br>
Modified: cfe/trunk/test/CodeGenCXX/microsoft-abi-vbtables.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/microsoft-abi-vbtables.cpp?rev=232680&r1=232679&r2=232680&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/microsoft-abi-vbtables.cpp?rev=232680&r1=232679&r2=232680&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeGenCXX/microsoft-abi-vbtables.cpp (original)<br>
+++ cfe/trunk/test/CodeGenCXX/microsoft-abi-vbtables.cpp Wed Mar 18 17:04:43 2015<br>
@@ -528,3 +528,14 @@ D d;<br>
<br>
// CHECK-DAG: @"\01??_8D@Test29@@7BB@1@@" = linkonce_odr unnamed_addr constant [2 x i32] zeroinitializer<br>
}<br>
+<br>
+namespace Test30 {<br>
+struct A {};<br>
+template <class> struct B : virtual A {<br>
+ B() {}<br>
+};<br>
+<br>
+extern template class B<int>;<br>
+template B<int>::B();<br>
+// CHECK-DAG: @"\01??_8?$B@H@Test30@@7B@" = external unnamed_addr constant [2 x i32]{{$}}<br>
+}<br>
<br>
Modified: cfe/trunk/test/CodeGenCXX/microsoft-abi-vftables.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/microsoft-abi-vftables.cpp?rev=232680&r1=232679&r2=232680&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/microsoft-abi-vftables.cpp?rev=232680&r1=232679&r2=232680&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeGenCXX/microsoft-abi-vftables.cpp (original)<br>
+++ cfe/trunk/test/CodeGenCXX/microsoft-abi-vftables.cpp Wed Mar 18 17:04:43 2015<br>
@@ -3,7 +3,6 @@<br>
<br>
// RTTI-DAG: $"\01??_7S@@6B@" = comdat largest<br>
// RTTI-DAG: $"\01??_7V@@6B@" = comdat largest<br>
-// RTTI-DAG: $"\01??_7W@?A@@6B@" = comdat largest<br>
<br>
struct S {<br>
virtual ~S();<br>
@@ -36,7 +35,18 @@ struct W {<br>
virtual ~W();<br>
} w;<br>
}<br>
-// RTTI-DAG: [[VTABLE_W:@.*]] = private unnamed_addr constant [2 x i8*] [i8* bitcast ({{.*}} @"\01??_R4W@?A@@6B@" to i8*), i8* bitcast ({{.*}} @"\01??_GW@?A@@UAEPAXI@Z" to i8*)], comdat($"\01??_7W@?A@@6B@")<br>
-// RTTI-DAG: @"\01??_7W@?A@@6B@" = internal unnamed_addr alias getelementptr inbounds ([2 x i8*], [2 x i8*]* @2, i32 0, i32 1)<br>
+// RTTI-DAG: [[VTABLE_W:@.*]] = private unnamed_addr constant [2 x i8*] [i8* bitcast ({{.*}} @"\01??_R4W@?A@@6B@" to i8*), i8* bitcast ({{.*}} @"\01??_GW@?A@@UAEPAXI@Z" to i8*)]<br>
+// RTTI-DAG: @"\01??_7W@?A@@6B@" = internal unnamed_addr alias getelementptr inbounds ([2 x i8*], [2 x i8*]* [[VTABLE_W]], i32 0, i32 1)<br>
<br>
// NO-RTTI-DAG: @"\01??_7W@?A@@6B@" = internal unnamed_addr constant [1 x i8*] [i8* bitcast ({{.*}} @"\01??_GW@?A@@UAEPAXI@Z" to i8*)]<br>
+<br>
+struct X {};<br>
+template <class> struct Y : virtual X {<br>
+ Y() {}<br>
+ virtual ~Y();<br>
+};<br>
+<br>
+extern template class Y<int>;<br>
+template Y<int>::Y();<br>
+// RTTI-DAG: @"\01??_7?$Y@H@@6B@" = external unnamed_addr constant [1 x i8*]<br>
+// NO-RTTI-DAG: @"\01??_7?$Y@H@@6B@" = external unnamed_addr constant [1 x i8*]<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>
</blockquote></div>
</blockquote></div></blockquote></div>