[MS] Skip vbase construction in abstract class ctors

As background, when constructing a complete object, virtual bases are
constructed first. If an exception is thrown later in the ctor, those
virtual bases are destroyed, so sema marks the relevant constructors and
destructors of virtual bases as referenced. If necessary, they are
emitted.

However, an abstract class can never be used to construct a complete
object. In the Itanium C++ ABI, this works out nicely, because we never
end up emitting the "complete" constructor variant, only the "base"
constructor variant, which can be called by constructors of derived
classes. Clang's Sema::MarkBaseAndMemberDestructorsReferenced is aware
of this optimization, and it does not mark ctors and dtors of virtual
bases referenced when the constructor of an abstract class is emitted.

In the Microsoft ABI, there are no complete/base variants, so before
this change, the constructor of an abstract class could reference ctors
and dtors of a virtual base without marking them referenced. This could
lead to unresolved symbol errors at link time, as reported in PR41065.

The fix is to implement the same optimization as Sema: If the class is
abstract, don't bother initializing its virtual bases. The "is this
class the most derived class" check in the constructor will never pass,
and the virtual base constructor calls are always dead. Skip them.

I think Richard noticed this missed optimization back in 2016 when he
was implementing inheriting constructors. I wasn't able to find any bugs
or email about it, though.

Fixes PR41065

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@356425 91177308-0d34-0410-b5e6-96231b3b80d8

lib/CodeGen/CGClass.cpp

@@ -526,8 +526,7 @@ static bool BaseInitializerUsesThis(ASTContext &C, const Expr *Init) {
 
 static void EmitBaseInitializer(CodeGenFunction &CGF,
                                 const CXXRecordDecl *ClassDecl,
-                                CXXCtorInitializer *BaseInit,
-                                CXXCtorType CtorType) {
+                                CXXCtorInitializer *BaseInit) {
   assert(BaseInit->isBaseInitializer() &&
          "Must have base initializer!");
 
@@ -539,10 +538,6 @@ static void EmitBaseInitializer(CodeGenFunction &CGF,
 
   bool isBaseVirtual = BaseInit->isBaseVirtual();
 
-  // The base constructor doesn't construct virtual bases.
-  if (CtorType == Ctor_Base && isBaseVirtual)
-    return;
-
   // If the initializer for the base (other than the constructor
   // itself) accesses 'this' in any way, we need to initialize the
   // vtables.
@@ -1264,24 +1259,37 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
   CXXConstructorDecl::init_const_iterator B = CD->init_begin(),
                                           E = CD->init_end();
 
+  // Virtual base initializers first, if any. They aren't needed if:
+  // - This is a base ctor variant
+  // - There are no vbases
+  // - The class is abstract, so a complete object of it cannot be constructed
+  //
+  // The check for an abstract class is necessary because sema may not have
+  // marked virtual base destructors referenced.
+  bool ConstructVBases = CtorType != Ctor_Base &&
+                         ClassDecl->getNumVBases() != 0 &&
+                         !ClassDecl->isAbstract();
+
+  // In the Microsoft C++ ABI, there are no constructor variants. Instead, the
+  // constructor of a class with virtual bases takes an additional parameter to
+  // conditionally construct the virtual bases. Emit that check here.
   llvm::BasicBlock *BaseCtorContinueBB = nullptr;
-  if (ClassDecl->getNumVBases() &&
+  if (ConstructVBases &&
       !CGM.getTarget().getCXXABI().hasConstructorVariants()) {
-    // The ABIs that don't have constructor variants need to put a branch
-    // before the virtual base initialization code.
     BaseCtorContinueBB =
-      CGM.getCXXABI().EmitCtorCompleteObjectHandler(*this, ClassDecl);
+        CGM.getCXXABI().EmitCtorCompleteObjectHandler(*this, ClassDecl);
     assert(BaseCtorContinueBB);
   }
 
   llvm::Value *const OldThis = CXXThisValue;
-  // Virtual base initializers first.
   for (; B != E && (*B)->isBaseInitializer() && (*B)->isBaseVirtual(); B++) {
+    if (!ConstructVBases)
+      continue;
     if (CGM.getCodeGenOpts().StrictVTablePointers &&
         CGM.getCodeGenOpts().OptimizationLevel > 0 &&
         isInitializerOfDynamicClass(*B))
       CXXThisValue = Builder.CreateLaunderInvariantGroup(LoadCXXThis());
-    EmitBaseInitializer(*this, ClassDecl, *B, CtorType);
+    EmitBaseInitializer(*this, ClassDecl, *B);
   }
 
   if (BaseCtorContinueBB) {
@@ -1298,7 +1306,7 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
         CGM.getCodeGenOpts().OptimizationLevel > 0 &&
         isInitializerOfDynamicClass(*B))
       CXXThisValue = Builder.CreateLaunderInvariantGroup(LoadCXXThis());
-    EmitBaseInitializer(*this, ClassDecl, *B, CtorType);
+    EmitBaseInitializer(*this, ClassDecl, *B);
   }
 
   CXXThisValue = OldThis;

test/CodeGenCXX/inheriting-constructor.cpp

@@ -270,15 +270,6 @@ namespace inalloca_virt {
   // WIN32: call void @llvm.stackrestore(
   // WIN32: br
   //
-  // WIN32: store i32 0, i32* %[[IS_MOST_DERIVED_ADDR:.*]]
-  // WIN32: %[[IS_MOST_DERIVED:.*]] = load i32, i32* %[[IS_MOST_DERIVED_ADDR]]
-  // WIN32: %[[IS_MOST_DERIVED_i1:.*]] = icmp ne i32 %[[IS_MOST_DERIVED]], 0
-  // WIN32: br i1 %[[IS_MOST_DERIVED_i1]]
-  //
-  // Note: this block is unreachable.
-  // WIN32: store {{.*}} @"??_8B@inalloca_virt@@7B@"
-  // WIN32: br
-  //
   // WIN32: call {{.*}} @"??0Z@@QAE@XZ"(
   // WIN32: call {{.*}} @"??0Z@@QAE@XZ"(
   // WIN32: call {{.*}} @"??1Q@@QAE@XZ"(
@@ -294,11 +285,6 @@ namespace inalloca_virt {
   // WIN64: br i1
   // WIN64: store {{.*}} @"??_8C@inalloca_virt@@7B@"
   // WIN64: call {{.*}} @"??0A@inalloca_virt@@QEAA@UQ@@H0$$QEAU2@@Z"(%{{.*}}, %{{.*}}* %[[ARG1]], i32 2, %{{.*}}* %[[ARG3]], %{{.*}} %[[TMP]])
-  // WIN64: br
-  // WIN64: br i1
-  // (Unreachable block)
-  // WIN64: store {{.*}} @"??_8B@inalloca_virt@@7B@"
-  // WIN64: br
   // WIN64: call {{.*}} @"??0Z@@QEAA@XZ"(
   // WIN64: call {{.*}} @"??0Z@@QEAA@XZ"(
   // WIN64: call void @"??1Q@@QEAA@XZ"({{.*}}* %[[TMP]])

test/CodeGenCXX/msabi-ctor-abstract-vbase.cpp

@@ -0,0 +1,82 @@
+// RUN: %clang_cc1 -triple x86_64-windows-msvc %s -emit-llvm -fexceptions -o - | FileCheck %s
+
+// PR41065: As background, when constructing a complete object, virtual bases
+// are constructed first. If an exception is thrown while constructing a
+// subobject later, those virtual bases are destroyed, so sema references the
+// relevant constructors and destructors of every base class in case they are
+// needed.
+//
+// However, an abstract class can never be used to construct a complete object.
+// In the Itanium C++ ABI, this works out nicely, because we never end up
+// emitting the "complete" constructor variant, only the "base" constructor
+// variant, which can be called by constructors of derived classes. For various
+// reasons, Sema does not mark ctors and dtors of virtual bases referenced when
+// the constructor of an abstract class is emitted.
+//
+// In the Microsoft ABI, there are no complete/base variants, so before PR41065
+// was fixed, the constructor of an abstract class could reference special
+// members of a virtual base without marking them referenced. This could lead to
+// unresolved symbol errors at link time.
+//
+// The fix is to implement the same optimization as Sema: If the class is
+// abstract, don't bother initializing its virtual bases. The "is this class the
+// most derived class" check in the constructor will never pass, and the virtual
+// base constructor calls are always dead. Skip them.
+
+struct A {
+  A();
+  virtual void f() = 0;
+  virtual ~A();
+};
+
+// B has an implicit inline dtor, but is still abstract.
+struct B : A {
+  B(int n);
+  int n;
+};
+
+// Still abstract
+struct C : virtual B {
+  C(int n);
+  //void f() override;
+};
+
+// Not abstract, D::D calls C::C and B::B.
+struct D : C {
+  D(int n);
+  void f() override;
+};
+
+void may_throw();
+C::C(int n) : B(n) { may_throw(); }
+
+// No branches, no constructor calls before may_throw();
+//
+// CHECK-LABEL: define dso_local %struct.C* @"??0C@@QEAA@H@Z"(%struct.C* returned %this, i32 %n, i32 %is_most_derived)
+// CHECK-NOT: br i1
+// CHECK-NOT: {{call.*@"\?0}}
+// CHECK: call void @"?may_throw@@YAXXZ"()
+// no cleanups
+
+
+D::D(int n) : C(n), B(n) { may_throw(); }
+
+// Conditionally construct (and destroy) vbase B, unconditionally C.
+//
+// CHECK-LABEL: define dso_local %struct.D* @"??0D@@QEAA@H@Z"(%struct.D* returned %this, i32 %n, i32 %is_most_derived)
+// CHECK: icmp ne i32 {{.*}}, 0
+// CHECK: br i1
+// CHECK: call %struct.B* @"??0B@@QEAA@H@Z"
+// CHECK: br label
+// CHECK: invoke %struct.C* @"??0C@@QEAA@H@Z"
+// CHECK: invoke void @"?may_throw@@YAXXZ"()
+// CHECK: cleanuppad
+// CHECK: call void @"??1C@@UEAA@XZ"
+// CHECK: cleanupret
+//
+// CHECK: cleanuppad
+// CHECK: icmp ne i32 {{.*}}, 0
+// CHECK: br i1
+// CHECK: call void @"??1B@@UEAA@XZ"
+// CHECK: br label
+// CHECK: cleanupret