From d5704343ee60c288c87d5b6d08b0aeecf74041da Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Fri, 13 Oct 2017 00:53:02 +0000 Subject: [PATCH] [MS] Don't bail on replacing dllimport vbase dtors with base dtors Fix PR32990 by effectively reverting r283063 and solving it a different way. We want to limit the hack to not replace equivalent available_externally dtors specifically to libc++, which uses always_inline. It seems certain versions of libc++ do not provide all the symbols that an explicit template instantiation is expected to provide. If we get to the code that forms a real alias, only *then* check if this is available_externally, and do that by asking a better question, which is "is this a declaration for the linker?", because *that's* what means we can't form an alias to it. As a follow-on simplification, remove the InEveryTU parameter. Its last use guarded this code for forming aliases, but we should never form aliases to declarations, regardless of what we know about every TU. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@315656 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/CodeGen/CGCXX.cpp | 32 +++++++-------- lib/CodeGen/CodeGenModule.h | 3 +- lib/CodeGen/MicrosoftCXXABI.cpp | 2 +- test/CodeGenCXX/dllimport-dtor-thunks.cpp | 49 +++++++++++++++++++++++ 4 files changed, 66 insertions(+), 20 deletions(-) create mode 100644 test/CodeGenCXX/dllimport-dtor-thunks.cpp diff --git a/lib/CodeGen/CGCXX.cpp b/lib/CodeGen/CGCXX.cpp index 0f3141ab76..5ef4dc45fb 100644 --- a/lib/CodeGen/CGCXX.cpp +++ b/lib/CodeGen/CGCXX.cpp @@ -110,16 +110,14 @@ bool CodeGenModule::TryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) { return true; return TryEmitDefinitionAsAlias(GlobalDecl(D, Dtor_Base), - GlobalDecl(BaseD, Dtor_Base), - false); + GlobalDecl(BaseD, Dtor_Base)); } /// Try to emit a definition as a global alias for another definition. /// If \p InEveryTU is true, we know that an equivalent alias can be produced /// in every translation unit. bool CodeGenModule::TryEmitDefinitionAsAlias(GlobalDecl AliasDecl, - GlobalDecl TargetDecl, - bool InEveryTU) { + GlobalDecl TargetDecl) { if (!getCodeGenOpts().CXXCtorDtorAliases) return true; @@ -134,11 +132,6 @@ bool CodeGenModule::TryEmitDefinitionAsAlias(GlobalDecl AliasDecl, llvm::GlobalValue::LinkageTypes TargetLinkage = getFunctionLinkage(TargetDecl); - // available_externally definitions aren't real definitions, so we cannot - // create an alias to one. - if (TargetLinkage == llvm::GlobalValue::AvailableExternallyLinkage) - return true; - // Check if we have it already. StringRef MangledName = getMangledName(AliasDecl); llvm::GlobalValue *Entry = GetGlobalValue(MangledName); @@ -161,7 +154,14 @@ bool CodeGenModule::TryEmitDefinitionAsAlias(GlobalDecl AliasDecl, // Instead of creating as alias to a linkonce_odr, replace all of the uses // of the aliasee. - if (llvm::GlobalValue::isDiscardableIfUnused(Linkage)) { + if (llvm::GlobalValue::isDiscardableIfUnused(Linkage) && + !(TargetLinkage == llvm::GlobalValue::AvailableExternallyLinkage && + TargetDecl.getDecl()->hasAttr())) { + // FIXME: An extern template instantiation will create functions with + // linkage "AvailableExternally". In libc++, some classes also define + // members with attribute "AlwaysInline" and expect no reference to + // be generated. It is desirable to reenable this optimisation after + // corresponding LLVM changes. addReplacement(MangledName, Aliasee); return false; } @@ -176,13 +176,11 @@ bool CodeGenModule::TryEmitDefinitionAsAlias(GlobalDecl AliasDecl, return true; } - if (!InEveryTU) { - // If we don't have a definition for the destructor yet, don't - // emit. We can't emit aliases to declarations; that's just not - // how aliases work. - if (Ref->isDeclaration()) - return true; - } + // If we don't have a definition for the destructor yet or the definition is + // avaialable_externally, don't emit an alias. We can't emit aliases to + // declarations; that's just not how aliases work. + if (Ref->isDeclarationForLinker()) + return true; // Don't create an alias to a linker weak symbol. This avoids producing // different COMDATs in different TUs. Another option would be to diff --git a/lib/CodeGen/CodeGenModule.h b/lib/CodeGen/CodeGenModule.h index c1a31c9f3d..c28c62e0e6 100644 --- a/lib/CodeGen/CodeGenModule.h +++ b/lib/CodeGen/CodeGenModule.h @@ -1145,8 +1145,7 @@ public: /// are emitted lazily. void EmitGlobal(GlobalDecl D); - bool TryEmitDefinitionAsAlias(GlobalDecl Alias, GlobalDecl Target, - bool InEveryTU); + bool TryEmitDefinitionAsAlias(GlobalDecl Alias, GlobalDecl Target); bool TryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D); /// Set attributes for a global definition. diff --git a/lib/CodeGen/MicrosoftCXXABI.cpp b/lib/CodeGen/MicrosoftCXXABI.cpp index 0a3cc30665..e81dedf14d 100644 --- a/lib/CodeGen/MicrosoftCXXABI.cpp +++ b/lib/CodeGen/MicrosoftCXXABI.cpp @@ -3805,7 +3805,7 @@ static void emitCXXDestructor(CodeGenModule &CGM, const CXXDestructorDecl *dtor, if (!dtor->getParent()->getNumVBases() && (dtorType == StructorType::Complete || dtorType == StructorType::Base)) { bool ProducedAlias = !CGM.TryEmitDefinitionAsAlias( - GlobalDecl(dtor, Dtor_Complete), GlobalDecl(dtor, Dtor_Base), true); + GlobalDecl(dtor, Dtor_Complete), GlobalDecl(dtor, Dtor_Base)); if (ProducedAlias) { if (dtorType == StructorType::Complete) return; diff --git a/test/CodeGenCXX/dllimport-dtor-thunks.cpp b/test/CodeGenCXX/dllimport-dtor-thunks.cpp new file mode 100644 index 0000000000..b381fff450 --- /dev/null +++ b/test/CodeGenCXX/dllimport-dtor-thunks.cpp @@ -0,0 +1,49 @@ +// RUN: %clang_cc1 -mconstructor-aliases %s -triple x86_64-windows-msvc -fms-extensions -emit-llvm -o - | FileCheck %s + +// FIXME: We should really consider removing -mconstructor-aliases for MS C++ +// ABI. The risk of bugs introducing ABI incompatibility under +// -mno-constructor-aliases is too high. + +// PR32990 + +// Introduces the virtual destructor. We should use the base destructor +// directly, no thunk needed. +struct __declspec(dllimport) ImportIntroVDtor { + virtual ~ImportIntroVDtor() {} +}; + +struct BaseClass { + virtual ~BaseClass() {} +}; + +// Non-virtually inherits from a non-dllimport base class. We should again call +// the derived base constructor directly. No need for the complete (aka vbase) +// destructor. +struct __declspec(dllimport) ImportOverrideVDtor : public BaseClass { + virtual ~ImportOverrideVDtor() {} +}; + +// Virtually inherits from a non-dllimport base class. This time we need to call +// the complete destructor and emit it inline. It's not exported from the DLL, +// and it must be emitted. +struct __declspec(dllimport) ImportVBaseOverrideVDtor + : public virtual BaseClass { + virtual ~ImportVBaseOverrideVDtor() {} +}; + +extern "C" void testit() { + ImportIntroVDtor t1; + ImportOverrideVDtor t2; + ImportVBaseOverrideVDtor t3; +} + +// The destructors are called in reverse order of construction. Only the third +// needs the complete destructor (_D). +// CHECK-LABEL: define void @testit() +// CHECK: call void @"\01??_DImportVBaseOverrideVDtor@@QEAAXXZ"(%struct.ImportVBaseOverrideVDtor* %{{.*}}) +// CHECK: call void @"\01??1ImportOverrideVDtor@@UEAA@XZ"(%struct.ImportOverrideVDtor* %{{.*}}) +// CHECK: call void @"\01??1ImportIntroVDtor@@UEAA@XZ"(%struct.ImportIntroVDtor* %{{.*}}) + +// CHECK-LABEL: define linkonce_odr void @"\01??_DImportVBaseOverrideVDtor@@QEAAXXZ" +// CHECK-LABEL: declare dllimport void @"\01??1ImportOverrideVDtor@@UEAA@XZ" +// CHECK-LABEL: declare dllimport void @"\01??1ImportIntroVDtor@@UEAA@XZ" -- 2.40.0