[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.

llvm-svn: 315656

clang/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<AlwaysInlineAttr>())) {
+    // 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

clang/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.

clang/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;

clang/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"