Fix for clang memcpyizer bugs 23911 and 23924

[• d.zobnin.bugzilla]

Summary

The fix is to remove duplicate copy-initialization of the only memcpy-able struct member and to correct the address of aggregately initialized members in destructors' calls during stack unwinding (in order to obtain address of struct member by using GEP instead of 'bitcast').

[rjmccall]

lib/CodeGen/CGClass.cpp

1105 ↗

(On Diff #29161)

Wow, the old code here was really impressively broken. Saved by the rule of 3/5, I guess.

Okay, two code organization requests. Please add a method on CXXCtorInitializer to get the type of the initialized member. Then, please extract a function in this file to drill down to the initialized member of a CXXCtorInitializer.

With that combination, the code here should end up looking really straightforward.

Please also update EmitMemberInitializer to use your new functions.

[• d.zobnin.bugzilla]

A function to emit lvalue for any field initialization was added (it also drills down to the field of anonymous unions). The code was adapted to work with this new function.
In previous review there was a request to add a method to CXXCtorInitializer to get the type of the initialized member. I realized that this can be done simply by getAnyMember()->getType() and decided not to add this method, because the new code looks pretty straightforward, I think.

[rjmccall]

LGTM, thanks!

cfe/trunk/lib/CodeGen/CGClass.cpp

static bool isMemcpyEquivalentSpecialMember(const CXXMethodDecl *D) {
 
   // We *must* emit a memcpy for a defaulted union copy or move op.
   if (D->getParent()->isUnion() && D->isDefaulted())
     return true;
 
   return false;
 }
 
+static void EmitLValueForAnyFieldInitialization(CodeGenFunction &CGF,
+                                                CXXCtorInitializer *MemberInit,
+                                                LValue &LHS) {
+  FieldDecl *Field = MemberInit->getAnyMember();
+  if (MemberInit->isIndirectMemberInitializer()) {
+    // If we are initializing an anonymous union field, drill down to the field.
+    IndirectFieldDecl *IndirectField = MemberInit->getIndirectMember();
+    for (const auto *I : IndirectField->chain())
+      LHS = CGF.EmitLValueForFieldInitialization(LHS, cast<FieldDecl>(I));
+  } else {
+    LHS = CGF.EmitLValueForFieldInitialization(LHS, Field);
+  }
+}
+
 static void EmitMemberInitializer(CodeGenFunction &CGF,
                                   const CXXRecordDecl *ClassDecl,
                                   CXXCtorInitializer *MemberInit,
                                   const CXXConstructorDecl *Constructor,
                                   FunctionArgList &Args) {
   ApplyDebugLocation Loc(CGF, MemberInit->getSourceLocation());
   assert(MemberInit->isAnyMemberInitializer() &&
          "Must have member initializer!");
   assert(MemberInit->getInit() && "Must have initializer!");
 
   // non-static data member initializers.
   FieldDecl *Field = MemberInit->getAnyMember();
   QualType FieldType = Field->getType();
 
   llvm::Value *ThisPtr = CGF.LoadCXXThis();
   QualType RecordTy = CGF.getContext().getTypeDeclType(ClassDecl);
   LValue LHS = CGF.MakeNaturalAlignAddrLValue(ThisPtr, RecordTy);
 
-  if (MemberInit->isIndirectMemberInitializer()) {
-    // If we are initializing an anonymous union field, drill down to
-    // the field.
-    IndirectFieldDecl *IndirectField = MemberInit->getIndirectMember();
-    for (const auto *I : IndirectField->chain())
-      LHS = CGF.EmitLValueForFieldInitialization(LHS, cast<FieldDecl>(I));
-    FieldType = MemberInit->getIndirectMember()->getAnonField()->getType();
-  } else {
-    LHS = CGF.EmitLValueForFieldInitialization(LHS, Field);
-  }
+  EmitLValueForAnyFieldInitialization(CGF, MemberInit, LHS);
 
   // Special case: if we are in a copy or move constructor, and we are copying
   // an array of PODs or classes with trivial copy constructors, ignore the
   // AST and perform the copy we know is equivalent.
   // FIXME: This is hacky at best... if we had a bit more explicit information
   // in the AST, we could generalize it more easily.
   const ConstantArrayType *Array
     = CGF.getContext().getAsConstantArrayType(FieldType);
public:
     void emitAggregatedInits() {
       if (AggregatedInits.size() <= 1) {
         // This memcpy is too small to be worthwhile. Fall back on default
         // codegen.
         if (!AggregatedInits.empty()) {
           CopyingValueRepresentation CVR(CGF);
           EmitMemberInitializer(CGF, ConstructorDecl->getParent(),
                                 AggregatedInits[0], ConstructorDecl, Args);
+          AggregatedInits.clear();
         }
         reset();
         return;
       }
 
       pushEHDestructors();
       emitMemcpy();
       AggregatedInits.clear();
     }
 
     void pushEHDestructors() {
       llvm::Value *ThisPtr = CGF.LoadCXXThis();
       QualType RecordTy = CGF.getContext().getTypeDeclType(ClassDecl);
       LValue LHS = CGF.MakeNaturalAlignAddrLValue(ThisPtr, RecordTy);
 
       for (unsigned i = 0; i < AggregatedInits.size(); ++i) {
-        QualType FieldType = AggregatedInits[i]->getMember()->getType();
+        CXXCtorInitializer *MemberInit = AggregatedInits[i];
+        QualType FieldType = MemberInit->getAnyMember()->getType();
         QualType::DestructionKind dtorKind = FieldType.isDestructedType();
-        if (CGF.needsEHCleanup(dtorKind))
-          CGF.pushEHDestroy(dtorKind, LHS.getAddress(), FieldType);
+        if (!CGF.needsEHCleanup(dtorKind))
+          continue;
+        LValue FieldLHS = LHS;
+        EmitLValueForAnyFieldInitialization(CGF, MemberInit, FieldLHS);
+        CGF.pushEHDestroy(dtorKind, FieldLHS.getAddress(), FieldType);
       }
     }
 
     void finish() {
       emitAggregatedInits();
     }
 
   private:

cfe/trunk/test/CodeGenCXX/eh-aggregated-inits-unwind.cpp

+// Check that destructors of memcpy-able struct members are called properly
+// during stack unwinding after an exception.
+//
+// Check that destructor's argument (address of member to be destroyed) is
+// obtained by taking offset from struct, not by bitcasting pointers.
+//
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -O0 -fno-elide-constructors -emit-llvm %s -o - | FileCheck %s
+
+struct ImplicitCopy {
+  int id;
+  ImplicitCopy() { id = 10; }
+  ~ImplicitCopy() { id = 20; }
+};
+
+struct ThrowCopy {
+  int id;
+  ThrowCopy() { id = 15; }
+  ThrowCopy(const ThrowCopy &x) {
+    id = 25;
+    throw 1;
+  }
+  ~ThrowCopy() { id = 35; }
+};
+
+struct Container {
+  int id;
+  ImplicitCopy o1;
+  ThrowCopy o2;
+
+  Container() { id = 1000; }
+  ~Container() { id = 2000; }
+};
+
+int main() {
+  try {
+    Container c1;
+    // CHECK-LABEL: main
+    // CHECK: %{{.+}} = getelementptr inbounds %struct.Container, %struct.Container* %{{.+}}, i32 0, i32 1
+    // CHECK-NOT: %{{.+}} = bitcast %struct.Container* %{{.+}} to %struct.ImplicitCopy*
+    Container c2(c1);
+
+    return 2;
+  } catch (...) {
+    return 1;
+  }
+  return 0;
+}

cfe/trunk/test/CodeGenCXX/eh-aggregated-inits.cpp

+// Check that initialization of the only one memcpy-able struct member will not
+// be performed twice after successful non-trivial initializtion of the second
+// member.
+//
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fexceptions -fcxx-exceptions -O0 -fno-elide-constructors -emit-llvm %s -o - | FileCheck %s
+
+int globId = 0;
+
+struct ImplicitCopy {
+  int id;
+
+  ImplicitCopy() { id = 10; }
+  ~ImplicitCopy() { id = 20; }
+};
+
+struct ExplicitCopy {
+  int id;
+
+  ExplicitCopy() { id = 15; }
+  ExplicitCopy(const ExplicitCopy &x) { id = 25; }
+  ~ExplicitCopy() { id = 35; }
+};
+
+struct Container {
+  ImplicitCopy o1; // memcpy-able member.
+  ExplicitCopy o2; // non-trivial initialization.
+
+  Container() { globId = 1000; }
+  ~Container() { globId = 2000; }
+};
+
+int main() {
+  try {
+    Container c1;
+    // CHECK-DAG: call void @llvm.memcpy
+    // CHECK-DAG: declare void @llvm.memcpy
+    // CHECK-NOT: @llvm.memcpy
+    Container c2(c1);
+
+    return 2;
+  }
+  catch (...) {
+    return 1;
+  }
+  return 0;
+}