subject:"\[PATCH\] D86854\: \[CodeGen\] Make sure the EH cleanup for block captures is conditional when the block literal is in a conditional context"

[PATCH] D86854: [CodeGen] Make sure the EH cleanup for block captures is conditional when the block literal is in a conditional context

2020-08-31 Thread Erik Pilkington via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa9a6e62ddff2: [CodeGen] Make sure the EH cleanup for block 
captures is conditional when the… (authored by erik.pilkington).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D86854?vs=288871=288945#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86854/new/

https://reviews.llvm.org/D86854

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGenObjC/arc-blocks-exceptions.m

Index: clang/test/CodeGenObjC/arc-blocks-exceptions.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/arc-blocks-exceptions.m
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -fblocks -fobjc-arc -fobjc-runtime-has-weak -fexceptions -disable-llvm-passes -o - %s | FileCheck %s
+
+void test1(_Bool c) {
+  void test1_fn(void (^blk)());
+  __weak id weakId = 0;
+  test1_fn(c ? ^{ (void)weakId; } : 0);
+
+  // CHECK: [[CLEANUP_COND:%.*]] = alloca i1
+  // CHECK-NEXT: [[CLEANUP_SAVE:%.*]] = alloca i8**
+
+  // CHECK: store i1 true, i1* [[CLEANUP_COND]]
+  // CHECK-NEXT: store i8** {{.*}}, i8*** [[CLEANUP_SAVE]]
+
+  // CHECK: invoke void @test1_fn(
+  // CHECK-NEXT: to label %[[INVOKE_CONT:.*]] unwind label %[[LANDING_PAD_LAB:.*]]
+
+  // CHECK: [[INVOKE_CONT]]:
+  // CHECK-NEXT: [[LOAD:%.*]] = load i1, i1* [[CLEANUP_COND]]
+  // CHECK-NEXT: br i1 [[LOAD]], label %[[END_OF_SCOPE_LAB:.*]], label
+
+  // CHECK: [[END_OF_SCOPE_LAB]]:
+  // CHECK-NEXT: [[LOAD:%.*]] = load i8**, i8*** [[CLEANUP_SAVE]]
+  // CHECK-NEXT: call void @llvm.objc.destroyWeak(i8** [[LOAD]])
+  // CHECK-NEXT: br label
+
+  // CHECK: [[LANDING_PAD_LAB]]:
+  //   /* some EH stuff */
+  // CHECK:  [[LOAD:%.*]] = load i1, i1* [[CLEANUP_COND]]
+  // CHECK-NEXT: br i1 [[LOAD]], label %[[EH_CLEANUP_LAB:.*]], label
+
+  // CHECK: [[EH_CLEANUP_LAB]]:
+  // CHECK-NEXT: [[LOAD:%.*]] = load i8**, i8*** [[CLEANUP_SAVE]]
+  // CHECK-NEXT: call void @llvm.objc.destroyWeak(i8** [[LOAD]])
+  // CHECK-NEXT: br label
+}
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -672,12 +672,13 @@
 initFullExprCleanup();
   }
 
-  /// Queue a cleanup to be pushed after finishing the current
-  /// full-expression.
+  /// Queue a cleanup to be pushed after finishing the current full-expression,
+  /// potentially with an active flag.
   template 
   void pushCleanupAfterFullExpr(CleanupKind Kind, As... A) {
 if (!isInConditionalBranch())
-  return pushCleanupAfterFullExprImpl(Kind, Address::invalid(), A...);
+  return pushCleanupAfterFullExprWithActiveFlag(Kind, Address::invalid(),
+   A...);
 
 Address ActiveFlag = createCleanupActiveFlag();
 assert(!DominatingValue::needsSaving(ActiveFlag) &&
@@ -687,12 +688,12 @@
 SavedTuple Saved{saveValueInCond(A)...};
 
 typedef EHScopeStack::ConditionalCleanup CleanupType;
-pushCleanupAfterFullExprImpl(Kind, ActiveFlag, Saved);
+pushCleanupAfterFullExprWithActiveFlag(Kind, ActiveFlag, Saved);
   }
 
   template 
-  void pushCleanupAfterFullExprImpl(CleanupKind Kind, Address ActiveFlag,
-As... A) {
+  void pushCleanupAfterFullExprWithActiveFlag(CleanupKind Kind,
+  Address ActiveFlag, As... A) {
 LifetimeExtendedCleanupHeader Header = {sizeof(T), Kind,
 ActiveFlag.isValid()};
 
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -2095,21 +2095,47 @@
   EHStack.pushCleanup(Kind, SPMem);
 }
 
-void CodeGenFunction::pushLifetimeExtendedDestroy(
-CleanupKind cleanupKind, Address addr, QualType type,
-Destroyer *destroyer, bool useEHCleanupForArray) {
-  // Push an EH-only cleanup for the object now.
-  // FIXME: When popping normal cleanups, we need to keep this EH cleanup
-  // around in case a temporary's destructor throws an exception.
-  if (cleanupKind & EHCleanup)
-EHStack.pushCleanup(
-static_cast(cleanupKind & ~NormalCleanup), addr, type,
+void CodeGenFunction::pushLifetimeExtendedDestroy(CleanupKind cleanupKind,
+  Address addr, QualType type,
+  Destroyer *destroyer,
+  bool useEHCleanupForArray) {
+  // If we're not in a conditional branch, we don't need to bother generating a
+  // conditional cleanup.
+  if (!isInConditionalBranch()) {
+// Push an EH-only cleanup for the object now.
+// FIXME: When

[PATCH] D86854: [CodeGen] Make sure the EH cleanup for block captures is conditional when the block literal is in a conditional context

2020-08-30 Thread John McCall via Phabricator via cfe-commits

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM modulo the minor fix




Comment at: clang/lib/CodeGen/CGDecl.cpp:2107
+// FIXME: When popping normal cleanups, we need to keep this EH cleanup
+// around in case a temporary's destructor throws an exception.
+if (cleanupKind & EHCleanup)

Oh, yuck, that's unfortunate.



Comment at: clang/lib/CodeGen/CGDecl.cpp:2117
+
+  // Otherwise, we should only destroy the object if its been initialized.
+  // Re-use the active flag and saved address across both the EH and end of

"it's"


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86854/new/

https://reviews.llvm.org/D86854

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86854: [CodeGen] Make sure the EH cleanup for block captures is conditional when the block literal is in a conditional context

2020-08-30 Thread Erik Pilkington via Phabricator via cfe-commits

erik.pilkington created this revision.
erik.pilkington added reviewers: ahatanak, rjmccall.
Herald added subscribers: danielkiss, ributzka, dexonsmith, jkorous.
erik.pilkington requested review of this revision.

Previously, clang was crashing on the attached test because the EH cleanup for 
the block capture was incorrectly emitted under the assumption that the 
expression wasn't conditionally evaluated. This was because before D81624 
, `pushLifetimeExtendedDestroy` was mainly 
used with C++ automatic lifetime extension (i.e. `const T  = tmp();`), where 
a conditionally evaluated expression wasn't possible. Now that we're using this 
path for block captures, we need to handle this case.

rdar://66250047


https://reviews.llvm.org/D86854

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGenObjC/arc-blocks-exceptions.m

Index: clang/test/CodeGenObjC/arc-blocks-exceptions.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/arc-blocks-exceptions.m
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -fblocks -fobjc-arc -fobjc-runtime-has-weak -fexceptions -disable-llvm-passes -o - %s | FileCheck %s
+
+void test1(_Bool c) {
+  void test1_fn(void (^blk)());
+  __weak id weakId = 0;
+  test1_fn(c ? ^{ (void)weakId; } : 0);
+
+  // CHECK: [[CLEANUP_COND:%.*]] = alloca i1
+  // CHECK-NEXT: [[CLEANUP_SAVE:%.*]] = alloca i8**
+
+  // CHECK: store i1 true, i1* [[CLEANUP_COND]]
+  // CHECK-NEXT: store i8** {{.*}}, i8*** [[CLEANUP_SAVE]]
+
+  // CHECK: invoke void @test1_fn(
+  // CHECK-NEXT: to label %[[INVOKE_CONT:.*]] unwind label %[[LANDING_PAD_LAB:.*]]
+
+  // CHECK: [[INVOKE_CONT]]:
+  // CHECK-NEXT: [[LOAD:%.*]] = load i1, i1* [[CLEANUP_COND]]
+  // CHECK-NEXT: br i1 [[LOAD]], label %[[END_OF_SCOPE_LAB:.*]], label
+
+  // CHECK: [[END_OF_SCOPE_LAB]]:
+  // CHECK-NEXT: [[LOAD:%.*]] = load i8**, i8*** [[CLEANUP_SAVE]]
+  // CHECK-NEXT: call void @llvm.objc.destroyWeak(i8** [[LOAD]])
+  // CHECK-NEXT: br label
+
+  // CHECK: [[LANDING_PAD_LAB]]:
+  //   /* some EH stuff */
+  // CHECK:  [[LOAD:%.*]] = load i1, i1* [[CLEANUP_COND]]
+  // CHECK-NEXT: br i1 [[LOAD]], label %[[EH_CLEANUP_LAB:.*]], label
+
+  // CHECK: [[EH_CLEANUP_LAB]]:
+  // CHECK-NEXT: [[LOAD:%.*]] = load i8**, i8*** [[CLEANUP_SAVE]]
+  // CHECK-NEXT: call void @llvm.objc.destroyWeak(i8** [[LOAD]])
+  // CHECK-NEXT: br label
+}
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -672,12 +672,13 @@
 initFullExprCleanup();
   }
 
-  /// Queue a cleanup to be pushed after finishing the current
-  /// full-expression.
+  /// Queue a cleanup to be pushed after finishing the current full-expression,
+  /// potentially with an active flag.
   template 
   void pushCleanupAfterFullExpr(CleanupKind Kind, As... A) {
 if (!isInConditionalBranch())
-  return pushCleanupAfterFullExprImpl(Kind, Address::invalid(), A...);
+  return pushCleanupAfterFullExprWithActiveFlag(Kind, Address::invalid(),
+   A...);
 
 Address ActiveFlag = createCleanupActiveFlag();
 assert(!DominatingValue::needsSaving(ActiveFlag) &&
@@ -687,12 +688,12 @@
 SavedTuple Saved{saveValueInCond(A)...};
 
 typedef EHScopeStack::ConditionalCleanup CleanupType;
-pushCleanupAfterFullExprImpl(Kind, ActiveFlag, Saved);
+pushCleanupAfterFullExprWithActiveFlag(Kind, ActiveFlag, Saved);
   }
 
   template 
-  void pushCleanupAfterFullExprImpl(CleanupKind Kind, Address ActiveFlag,
-As... A) {
+  void pushCleanupAfterFullExprWithActiveFlag(CleanupKind Kind,
+  Address ActiveFlag, As... A) {
 LifetimeExtendedCleanupHeader Header = {sizeof(T), Kind,
 ActiveFlag.isValid()};
 
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -2095,21 +2095,47 @@
   EHStack.pushCleanup(Kind, SPMem);
 }
 
-void CodeGenFunction::pushLifetimeExtendedDestroy(
-CleanupKind cleanupKind, Address addr, QualType type,
-Destroyer *destroyer, bool useEHCleanupForArray) {
-  // Push an EH-only cleanup for the object now.
-  // FIXME: When popping normal cleanups, we need to keep this EH cleanup
-  // around in case a temporary's destructor throws an exception.
-  if (cleanupKind & EHCleanup)
-EHStack.pushCleanup(
-static_cast(cleanupKind & ~NormalCleanup), addr, type,
+void CodeGenFunction::pushLifetimeExtendedDestroy(CleanupKind cleanupKind,
+  Address addr, QualType type,
+

[PATCH] D86854: [CodeGen] Make sure the EH cleanup for block captures is conditional when the block literal is in a conditional context

[PATCH] D86854: [CodeGen] Make sure the EH cleanup for block captures is conditional when the block literal is in a conditional context

[PATCH] D86854: [CodeGen] Make sure the EH cleanup for block captures is conditional when the block literal is in a conditional context

3 matches

Site Navigation

Mail list logo

Footer information