[CodeGen] Emit lifetime.end intrinsic after destructor call in landing pad

[ahatanak]

Summary

This patch fixes a bug in CodeGen where lifetime.end intrinsics were not being inserted after destructor calls in landing pad blocks, which prevented StackColoring from merging stack slots for allocas because it couldn't tell their lifetime ranges were disjoint.

This patch changes the code in CodeGenFunction::EmitAutoVarCleanup to pass NormalAndEHCleanup instead of NormalCleanup to EHScopeStack::pushCleanup so that CallLifetimeEnd runs the cleanup code when a scope is exited using exceptional control flow too. I initially considered adding code to DestroyObject::Emit to emit lifetime.end after the destructor call, but letting CallLifetimeEnd emit lifetime.end seemed like a better approach.

This is the link to the discussion on llvm-dev:

http://lists.llvm.org/pipermail/llvm-dev/2016-March/096233.html

[rjmccall]

You should talk to Reid or someone else involved in MSVC-style EH support to ensure that they generate a reasonable code pattern for this.

You should also check that any back-end peepholes we have in place (null type infos to signify a call-terminate landingpad?) aren't disturbed by the lifetime intrinsics.

[rnk]

In D18196#375997, @rjmccall wrote:

You should talk to Reid or someone else involved in MSVC-style EH support to ensure that they generate a reasonable code pattern for this.

I patched this in and verified it works. Clang makes a separate funclet for the lifetime end, which is suboptimal, but LLVM knows how to simplify it down to one. The stack coloring optimization in the backend doesn't fire yet, though. =/

Can you add a simple test for this? Make something similar to test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp but with optimizations enabled.

[ahatanak]

I can add another RUN line to test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp that is identical to the existing RUN line except that it has "-O3 -disable-llvm-optzns". I confirmed that the test still passes without any changes to the test itself.

Is that what you are looking for?

[rnk]

In D18196#376837, @ahatanak wrote:

I can add another RUN line to test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp that is identical to the existing RUN line except that it has "-O3 -disable-llvm-optzns". I confirmed that the test still passes without any changes to the test itself.

Is that what you are looking for?

I assumed the test wouldn't pass with those flags, but yes, that works. Could you add the obvious CHECK lines for a simple cleanup though? There should be an invoke, two cleanuppads, one calls the dtor, the other ends the lifetime, they chain to each other.

[ahatanak]

Added a test in test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp to check expected patterns are emitted.

[ahatanak]

In D18196#375997, @rjmccall wrote:

You should also check that any back-end peepholes we have in place (null type infos to signify a call-terminate landingpad?) aren't disturbed by the lifetime intrinsics.

I looked at the back-end passes that might be affected by the insertion of lifetime markers. I don't think inserting lifetime markers will have an adverse effect on any of the back-end code-generation.

[ahatanak]

Add "REQUIRES: asserts" to test case to enable checking basic block names.

[rjmccall]

LGTM.

cfe/trunk/lib/CodeGen/CGCleanup.cpp

for (EHScopeStack::iterator it = begin(); stabilize(it) != Old; it++) {
     EHCleanupScope *cleanup = dyn_cast<EHCleanupScope>(&*it);
     if (!cleanup || !cleanup->isLifetimeMarker())
       return false;
   }
 
   return true;
 }
 
+bool EHScopeStack::requiresLandingPad() const {
+  for (stable_iterator si = getInnermostEHScope(); si != stable_end(); ) {
+    // Skip lifetime markers.
+    if (auto *cleanup = dyn_cast<EHCleanupScope>(&*find(si)))
+      if (cleanup->isLifetimeMarker()) {
+        si = cleanup->getEnclosingEHScope();
+        continue;
+      }
+    return true;
+  }
+
+  return false;
+}
+
 EHScopeStack::stable_iterator
 EHScopeStack::getInnermostActiveNormalCleanup() const {
   for (stable_iterator si = getInnermostNormalCleanup(), se = stable_end();
          si != se; ) {
     EHCleanupScope &cleanup = cast<EHCleanupScope>(*find(si));
     if (cleanup.isActive()) return si;
     si = cleanup.getEnclosingNormalCleanup();
   }

cfe/trunk/lib/CodeGen/CGDecl.cpp

void CodeGenFunction::EmitAutoVarCleanups(const AutoVarEmission &emission) {
   // us from jumping into any of these scopes anyway.
   if (!HaveInsertPoint()) return;
 
   const VarDecl &D = *emission.Variable;
 
   // Make sure we call @llvm.lifetime.end.  This needs to happen
   // *last*, so the cleanup needs to be pushed *first*.
   if (emission.useLifetimeMarkers()) {
-    EHStack.pushCleanup<CallLifetimeEnd>(NormalCleanup,
+    EHStack.pushCleanup<CallLifetimeEnd>(NormalAndEHCleanup,
                                          emission.getAllocatedAddress(),
                                          emission.getSizeForLifetimeMarkers());
     EHCleanupScope &cleanup = cast<EHCleanupScope>(*EHStack.begin());
     cleanup.setLifetimeMarker();
   }
 
   // Check the type for a cleanup.
   if (QualType::DestructionKind dtorKind = D.getType().isDestructedType())

cfe/trunk/lib/CodeGen/EHScopeStack.h

public:
 
   // Returns true iff the current scope is either empty or contains only
   // lifetime markers, i.e. no real cleanup code
   bool containsOnlyLifetimeMarkers(stable_iterator Old) const;
 
   /// Determines whether the exception-scopes stack is empty.
   bool empty() const { return StartOfData == EndOfBuffer; }
 
-  bool requiresLandingPad() const {
-    return InnermostEHScope != stable_end();
-  }
+  bool requiresLandingPad() const;
 
   /// Determines whether there are any normal cleanups on the stack.
   bool hasNormalCleanups() const {
     return InnermostNormalCleanup != stable_end();
   }
 
   /// Returns the innermost normal cleanup on the stack, or
   /// stable_end() if there are no normal cleanups.

cfe/trunk/test/CodeGenCXX/destructors.cpp

 // RUN: %clang_cc1 %s -triple x86_64-apple-darwin10 -emit-llvm -o - -mconstructor-aliases -fcxx-exceptions -fexceptions -O1 -disable-llvm-optzns > %t
 // RUN: FileCheck --check-prefix=CHECK1 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK2 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK3 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK4 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK5 --input-file=%t %s
+// RUN: %clang_cc1 %s -triple x86_64-apple-darwin10 -emit-llvm -o - -fcxx-exceptions -fexceptions -O1 -disable-llvm-optzns -std=c++11 > %t2
+// RUN: FileCheck --check-prefix=CHECK6 --input-file=%t2 %s
+// REQUIRES: asserts
 
 struct A {
   int a;
 
   ~A();
 };
 
 // Base with non-trivial destructor
namespace test10 {
   template class opt<int>;
   // CHECK5-LABEL: define zeroext i1 @_ZN6test1016handleOccurrenceEv(
   bool handleOccurrence() {
     // CHECK5: call void @_ZN6test106OptionD2Ev(
     Option x;
     return true;
   }
 }
+
+#if __cplusplus >= 201103L
+namespace test11 {
+
+// Check that lifetime.end is emitted in the landing pad.
+
+// CHECK6-LABEL: define void @_ZN6test1115testLifetimeEndEi(
+// CHECK6: entry:
+// CHECK6: [[T1:%[a-z0-9]+]] = alloca %"struct.test11::S1"
+// CHECK6: [[T2:%[a-z0-9]+]] = alloca %"struct.test11::S1"
+// CHECK6: [[T3:%[a-z0-9]+]] = alloca %"struct.test11::S1"
+
+// CHECK6: {{^}}invoke.cont
+// CHECK6: call void @_ZN6test112S1D1Ev(%"struct.test11::S1"* [[T1]])
+// CHECK6: [[BC1:%[a-z0-9]+]] = bitcast %"struct.test11::S1"* [[T1]] to i8*
+// CHECK6: call void @llvm.lifetime.end(i64 32, i8* [[BC1]])
+// CHECK6: {{^}}lpad
+// CHECK6: call void @_ZN6test112S1D1Ev(%"struct.test11::S1"* [[T1]])
+// CHECK6: [[BC2:%[a-z0-9]+]] = bitcast %"struct.test11::S1"* [[T1]] to i8*
+// CHECK6: call void @llvm.lifetime.end(i64 32, i8* [[BC2]])
+
+// CHECK6: {{^}}invoke.cont
+// CHECK6: call void @_ZN6test112S1D1Ev(%"struct.test11::S1"* [[T2]])
+// CHECK6: [[BC3:%[a-z0-9]+]] = bitcast %"struct.test11::S1"* [[T2]] to i8*
+// CHECK6: call void @llvm.lifetime.end(i64 32, i8* [[BC3]])
+// CHECK6: {{^}}lpad
+// CHECK6: call void @_ZN6test112S1D1Ev(%"struct.test11::S1"* [[T2]])
+// CHECK6: [[BC4:%[a-z0-9]+]] = bitcast %"struct.test11::S1"* [[T2]] to i8*
+// CHECK6: call void @llvm.lifetime.end(i64 32, i8* [[BC4]])
+
+// CHECK6: {{^}}invoke.cont
+// CHECK6: call void @_ZN6test112S1D1Ev(%"struct.test11::S1"* [[T3]])
+// CHECK6: [[BC5:%[a-z0-9]+]] = bitcast %"struct.test11::S1"* [[T3]] to i8*
+// CHECK6: call void @llvm.lifetime.end(i64 32, i8* [[BC5]])
+// CHECK6: {{^}}lpad
+// CHECK6: call void @_ZN6test112S1D1Ev(%"struct.test11::S1"* [[T3]])
+// CHECK6: [[BC6:%[a-z0-9]+]] = bitcast %"struct.test11::S1"* [[T3]] to i8*
+// CHECK6: call void @llvm.lifetime.end(i64 32, i8* [[BC6]])
+
+  struct S1 {
+    ~S1();
+    int a[8];
+  };
+
+  void func1(S1 &) noexcept(false);
+
+  void testLifetimeEnd(int n) {
+    if (n < 10) {
+      S1 t1;
+      func1(t1);
+    } else if (n < 100) {
+      S1 t2;
+      func1(t2);
+    } else if (n < 1000) {
+      S1 t3;
+      func1(t3);
+    }
+  }
+
+}
+#endif

cfe/trunk/test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp

 // RUN: %clang_cc1 -std=c++11 -emit-llvm %s -o - -triple=i386-pc-win32 -mconstructor-aliases -fexceptions -fcxx-exceptions -fno-rtti | FileCheck -check-prefix WIN32 %s
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -O3 -disable-llvm-optzns %s -o - -triple=i386-pc-win32 -mconstructor-aliases -fexceptions -fcxx-exceptions -fno-rtti | FileCheck -check-prefix WIN32 -check-prefix WIN32-LIFETIME %s
 
 struct A {
   A();
   ~A();
   int a;
 };
 
 A getA();

 }
 
 // WIN32-LABEL: define void @"\01?f@noexcept_false_dtor@@YAXXZ"()
 // WIN32: invoke i32 @"\01?CouldThrow@@YAHXZ"()
 // WIN32: call x86_thiscallcc void @"\01??1D@noexcept_false_dtor@@QAE@XZ"(%"struct.noexcept_false_dtor::D"* %{{.*}})
 // WIN32: cleanuppad
 // WIN32: call x86_thiscallcc void @"\01??1D@noexcept_false_dtor@@QAE@XZ"(%"struct.noexcept_false_dtor::D"* %{{.*}})
 // WIN32: cleanupret
+
+namespace lifetime_marker {
+struct C {
+  ~C();
+};
+void g();
+void f() {
+  C c;
+  g();
+}
+
+// WIN32-LIFETIME-LABEL: define void @"\01?f@lifetime_marker@@YAXXZ"()
+// WIN32-LIFETIME: %[[c:.*]] = alloca %"struct.lifetime_marker::C"
+// WIN32-LIFETIME: %[[bc0:.*]] = bitcast %"struct.lifetime_marker::C"* %c to i8*
+// WIN32-LIFETIME: call void @llvm.lifetime.start(i64 1, i8* %[[bc0]])
+// WIN32-LIFETIME: invoke void @"\01?g@lifetime_marker@@YAXXZ"()
+// WIN32-LIFETIME-NEXT: to label %[[cont:[^ ]*]] unwind label %[[lpad0:[^ ]*]]
+//
+// WIN32-LIFETIME: [[cont]]:
+// WIN32-LIFETIME: call x86_thiscallcc void @"\01??1C@lifetime_marker@@QAE@XZ"({{.*}})
+// WIN32-LIFETIME: %[[bc1:.*]] = bitcast %"struct.lifetime_marker::C"* %[[c]] to i8*
+// WIN32-LIFETIME: call void @llvm.lifetime.end(i64 1, i8* %[[bc1]])
+//
+// WIN32-LIFETIME: [[lpad0]]:
+// WIN32-LIFETIME-NEXT: cleanuppad
+// WIN32-LIFETIME: call x86_thiscallcc void @"\01??1C@lifetime_marker@@QAE@XZ"({{.*}})
+// WIN32-LIFETIME: cleanupret {{.*}} unwind label %[[lpad1:[^ ]*]]
+//
+// WIN32-LIFETIME: [[lpad1]]:
+// WIN32-LIFETIME-NEXT: cleanuppad
+// WIN32-LIFETIME: %[[bc2:.*]] = bitcast %"struct.lifetime_marker::C"* %[[c]] to i8*
+// WIN32-LIFETIME: call void @llvm.lifetime.end(i64 1, i8* %[[bc2]])
+}