[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-26 Thread Akira Hatanaka via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rC331016: [CodeGen] Avoid destructing a callee-destructued 
struct type in a (authored by ahatanak, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D45382

Files:
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGCleanup.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CodeGenFunction.h
  test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
  test/CodeGenObjCXX/arc-special-member-functions.mm
  test/CodeGenObjCXX/lambda-expressions.mm

Index: test/CodeGenObjCXX/lambda-expressions.mm
===
--- test/CodeGenObjCXX/lambda-expressions.mm
+++ test/CodeGenObjCXX/lambda-expressions.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks -fobjc-arc | FileCheck -check-prefix=ARC %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks -fobjc-arc -fobjc-runtime-has-weak -DWEAK_SUPPORTED | FileCheck -check-prefix=ARC %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks | FileCheck -check-prefix=MRC %s
 
 typedef int (^fp)();
@@ -138,5 +138,31 @@
 }
 @end
 
+// Check that the delegating invoke function doesn't destruct the Weak object
+// that is passed.
+
+// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvEN3$_58__invokeENS_4WeakE"(
+// ARC: call void @"_ZZN14LambdaDelegate4testEvENK3$_5clENS_4WeakE"(
+// ARC-NEXT: ret void
+
+// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvENK3$_5clENS_4WeakE"(
+// ARC: call void @_ZN14LambdaDelegate4WeakD1Ev(
+
+#ifdef WEAK_SUPPORTED
+
+namespace LambdaDelegate {
+
+struct Weak {
+  __weak id x;
+};
+
+void test() {
+  void (*p)(Weak) = [](Weak a) { };
+}
+
+};
+
+#endif
+
 // ARC: attributes [[NUW]] = { noinline nounwind{{.*}} }
 // MRC: attributes [[NUW]] = { noinline nounwind{{.*}} }
Index: test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
===
--- test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
+++ test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
@@ -10,6 +10,17 @@
   // CHECK-NEXT: ret i8* [[T2]]
 }
 
+// Check that the delegating block invoke function doesn't destruct the Weak
+// object that is passed.
+
+// CHECK-LABEL: define internal void @___Z8testWeakv_block_invoke(
+// CHECK: call void @"_ZZ8testWeakvENK3$_2clE4Weak"(
+// CHECK-NEXT: ret void
+
+// CHECK-LABEL: define internal void @"_ZZ8testWeakvENK3$_2clE4Weak"(
+// CHECK: call void @_ZN4WeakD1Ev(
+// CHECK-NEXT: ret void
+
 id test1_rv;
 
 void test1() {
@@ -21,3 +32,12 @@
   // CHECK-NEXT: [[T2:%.*]] = tail call i8* @objc_autoreleaseReturnValue(i8* [[T1]])
   // CHECK-NEXT: ret i8* [[T2]]
 }
+
+struct Weak {
+  __weak id x;
+};
+
+void testWeak() {
+  extern void testWeak_helper(void (^)(Weak));
+  testWeak_helper([](Weak){});
+}
Index: test/CodeGenObjCXX/arc-special-member-functions.mm
===
--- test/CodeGenObjCXX/arc-special-member-functions.mm
+++ test/CodeGenObjCXX/arc-special-member-functions.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -fobjc-runtime-has-weak -emit-llvm -o - %s | FileCheck %s
 
 struct ObjCMember {
   id member;
@@ -12,6 +12,59 @@
   int (^bp)(int);
 };
 
+// CHECK: %[[STRUCT_CONTAINSWEAK:.*]] = type { %[[STRUCT_WEAK:.*]] }
+// CHECK: %[[STRUCT_WEAK]] = type { i8* }
+
+// The Weak object that is passed is destructed in this constructor.
+
+// CHECK: define void @_ZN12ContainsWeakC2E4Weak(
+// CHECK: call void @_ZN4WeakC1ERKS_(
+// CHECK: call void @_ZN4WeakD1Ev(
+
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define void @_ZN12ContainsWeakC1E4Weak(
+// CHECK: call void @_ZN12ContainsWeakC2E4Weak(
+// CHECK-NEXT: ret void
+
+struct Weak {
+  Weak(id);
+  __weak id x;
+};
+
+struct ContainsWeak {
+  ContainsWeak(Weak);
+  Weak w;
+};
+
+ContainsWeak::ContainsWeak(Weak a) : w(a) {}
+
+// The Weak object that is passed is destructed in this constructor.
+
+// CHECK: define void @_ZN4BaseC2E4Weak(
+// CHECK: call void @_ZN4WeakD1Ev(
+// CHECK: ret void
+
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define linkonce_odr void @_ZN7DerivedCI14BaseE4Weak(
+// CHECK: call void @_ZN7DerivedCI24BaseE4Weak(
+// CHECK-NEXT: ret void
+
+struct Base {
+  Base(Weak);
+};
+
+Base::Base(Weak a) {}
+
+struct Derived : Base {
+  using Base::Base;
+};
+
+Derived d(Weak(0));
+
 // CHECK-LABEL: define void @_Z42test_ObjCMember_default_construct_destructv(
 void test_ObjCMember_default_construct_destruct() {

[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-26 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.

Thanks, LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D45382



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


[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-26 Thread Akira Hatanaka via Phabricator via cfe-commits
ahatanak updated this revision to Diff 144231.
ahatanak marked an inline comment as done.
ahatanak added a comment.

Rename variable to something OldCleanupScopeDepth.


Repository:
  rC Clang

https://reviews.llvm.org/D45382

Files:
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGCleanup.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CodeGenFunction.h
  test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
  test/CodeGenObjCXX/arc-special-member-functions.mm
  test/CodeGenObjCXX/lambda-expressions.mm

Index: test/CodeGenObjCXX/lambda-expressions.mm
===
--- test/CodeGenObjCXX/lambda-expressions.mm
+++ test/CodeGenObjCXX/lambda-expressions.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks -fobjc-arc | FileCheck -check-prefix=ARC %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks -fobjc-arc -fobjc-runtime-has-weak -DWEAK_SUPPORTED | FileCheck -check-prefix=ARC %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks | FileCheck -check-prefix=MRC %s
 
 typedef int (^fp)();
@@ -138,5 +138,31 @@
 }
 @end
 
+// Check that the delegating invoke function doesn't destruct the Weak object
+// that is passed.
+
+// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvEN3$_58__invokeENS_4WeakE"(
+// ARC: call void @"_ZZN14LambdaDelegate4testEvENK3$_5clENS_4WeakE"(
+// ARC-NEXT: ret void
+
+// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvENK3$_5clENS_4WeakE"(
+// ARC: call void @_ZN14LambdaDelegate4WeakD1Ev(
+
+#ifdef WEAK_SUPPORTED
+
+namespace LambdaDelegate {
+
+struct Weak {
+  __weak id x;
+};
+
+void test() {
+  void (*p)(Weak) = [](Weak a) { };
+}
+
+};
+
+#endif
+
 // ARC: attributes [[NUW]] = { noinline nounwind{{.*}} }
 // MRC: attributes [[NUW]] = { noinline nounwind{{.*}} }
Index: test/CodeGenObjCXX/arc-special-member-functions.mm
===
--- test/CodeGenObjCXX/arc-special-member-functions.mm
+++ test/CodeGenObjCXX/arc-special-member-functions.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -fobjc-runtime-has-weak -emit-llvm -o - %s | FileCheck %s
 
 struct ObjCMember {
   id member;
@@ -12,6 +12,59 @@
   int (^bp)(int);
 };
 
+// CHECK: %[[STRUCT_CONTAINSWEAK:.*]] = type { %[[STRUCT_WEAK:.*]] }
+// CHECK: %[[STRUCT_WEAK]] = type { i8* }
+
+// The Weak object that is passed is destructed in this constructor.
+
+// CHECK: define void @_ZN12ContainsWeakC2E4Weak(
+// CHECK: call void @_ZN4WeakC1ERKS_(
+// CHECK: call void @_ZN4WeakD1Ev(
+
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define void @_ZN12ContainsWeakC1E4Weak(
+// CHECK: call void @_ZN12ContainsWeakC2E4Weak(
+// CHECK-NEXT: ret void
+
+struct Weak {
+  Weak(id);
+  __weak id x;
+};
+
+struct ContainsWeak {
+  ContainsWeak(Weak);
+  Weak w;
+};
+
+ContainsWeak::ContainsWeak(Weak a) : w(a) {}
+
+// The Weak object that is passed is destructed in this constructor.
+
+// CHECK: define void @_ZN4BaseC2E4Weak(
+// CHECK: call void @_ZN4WeakD1Ev(
+// CHECK: ret void
+
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define linkonce_odr void @_ZN7DerivedCI14BaseE4Weak(
+// CHECK: call void @_ZN7DerivedCI24BaseE4Weak(
+// CHECK-NEXT: ret void
+
+struct Base {
+  Base(Weak);
+};
+
+Base::Base(Weak a) {}
+
+struct Derived : Base {
+  using Base::Base;
+};
+
+Derived d(Weak(0));
+
 // CHECK-LABEL: define void @_Z42test_ObjCMember_default_construct_destructv(
 void test_ObjCMember_default_construct_destruct() {
   // CHECK: call void @_ZN10ObjCMemberC1Ev
@@ -111,6 +164,13 @@
 // CHECK-NEXT: call void @objc_release(i8* [[T7]])
 // CHECK-NEXT: ret
 
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define linkonce_odr void @_ZN7DerivedCI24BaseE4Weak(
+// CHECK: call void @_ZN4BaseC2E4Weak(
+// CHECK-NEXT: ret void
+
 // Implicitly-generated default constructor for ObjCMember
 // CHECK-LABEL: define linkonce_odr void @_ZN10ObjCMemberC2Ev
 // CHECK-NOT: objc_release
Index: test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
===
--- test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
+++ test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
@@ -10,6 +10,17 @@
   // CHECK-NEXT: ret i8* [[T2]]
 }
 
+// Check that the delegating block invoke function doesn't destruct the Weak
+// object that is passed.
+
+// CHECK-LABEL: define internal void @___Z8testWeakv_block_invoke(
+// CHECK: call void @

[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:3069
+  if (hasAggregateEvaluationKind(type) &&
+  getContext().isParamDestroyedInCallee(type)) {
+EHScopeStack::stable_iterator cleanup =

ahatanak wrote:
> rjmccall wrote:
> > I wonder if this is something we should be taking from the CGFunctionInfo 
> > instead.  It does seem plausible that it could vary, e.g. according to the 
> > calling convention.  But maybe that's something we can handle in a separate 
> > patch?
> I assume you are talking about the call to isParamDestroyedInCallee? If so, 
> yes, I think we can discuss it in a separate patch. I have plans to clean up 
> the way ParamDestroyedInCallee is handled in Sema and IRGen.
> I assume you are talking about the call to isParamDestroyedInCallee?

Yeah.

> I assume you are talking about the call to isParamDestroyedInCallee? If so, 
> yes, I think we can discuss it in a separate patch. 

Okay, great, thanks.


Repository:
  rC Clang

https://reviews.llvm.org/D45382



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


[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-23 Thread Akira Hatanaka via Phabricator via cfe-commits
ahatanak added a comment.

x.




Comment at: lib/CodeGen/CGCall.cpp:3069
+  if (hasAggregateEvaluationKind(type) &&
+  getContext().isParamDestroyedInCallee(type)) {
+EHScopeStack::stable_iterator cleanup =

rjmccall wrote:
> I wonder if this is something we should be taking from the CGFunctionInfo 
> instead.  It does seem plausible that it could vary, e.g. according to the 
> calling convention.  But maybe that's something we can handle in a separate 
> patch?
I assume you are talking about the call to isParamDestroyedInCallee? If so, 
yes, I think we can discuss it in a separate patch. I have plans to clean up 
the way ParamDestroyedInCallee is handled in Sema and IRGen.


Repository:
  rC Clang

https://reviews.llvm.org/D45382



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


[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-23 Thread Richard Smith - zygoloid via Phabricator via cfe-commits
rsmith added a comment.

In https://reviews.llvm.org/D45382#1060452, @ahatanak wrote:

> In https://reviews.llvm.org/D45382#1060163, @rjmccall wrote:
>
> > Hmm.  I'm not actually sure *why* it's not okay to forward callee-cleanup 
> > arguments.  Do we just not have the necessary logic to disable the cleanup 
> > in the caller?
>
>
> It seems that it would be OK if there was a way to disable the cleanup in the 
> caller when we know that we are delegating to another constructor, possibly 
> by setting the DelegateCXXConstructorCall here too. But maybe there are other 
> reasons for not doing so.
>
> Perhaps Richard (who committed r274049) knows why we can't call the delegated 
> constructor when there are callee-cleanup arguments?


I think the only reason was that we didn't have a good way to handle cleanup 
emission in the thunk (as @rjmccall points out, we do need parameter cleanups 
in the thunk, but we need to disable them at the point of the call to the 
inherited constructor). I cannot think of any correctness reason that prevents 
using a thunk for the callee-cleanup case.


Repository:
  rC Clang

https://reviews.llvm.org/D45382



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


[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:3069
+  if (hasAggregateEvaluationKind(type) &&
+  getContext().isParamDestroyedInCallee(type)) {
+EHScopeStack::stable_iterator cleanup =

I wonder if this is something we should be taking from the CGFunctionInfo 
instead.  It does seem plausible that it could vary, e.g. according to the 
calling convention.  But maybe that's something we can handle in a separate 
patch?



Comment at: lib/CodeGen/CodeGenFunction.h:590
   class RunCleanupsScope {
-EHScopeStack::stable_iterator CleanupStackDepth;
+EHScopeStack::stable_iterator CleanupStackDepth, OldCleanupStackDepth;
 size_t LifetimeExtendedCleanupStackSize;

Please rename this variable to something like `OldCleanupScopeDepth`.


Repository:
  rC Clang

https://reviews.llvm.org/D45382



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


[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-23 Thread Akira Hatanaka via Phabricator via cfe-commits
ahatanak updated this revision to Diff 143624.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.

Address review comments.


Repository:
  rC Clang

https://reviews.llvm.org/D45382

Files:
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGCleanup.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CodeGenFunction.h
  test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
  test/CodeGenObjCXX/arc-special-member-functions.mm
  test/CodeGenObjCXX/lambda-expressions.mm

Index: test/CodeGenObjCXX/lambda-expressions.mm
===
--- test/CodeGenObjCXX/lambda-expressions.mm
+++ test/CodeGenObjCXX/lambda-expressions.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks -fobjc-arc | FileCheck -check-prefix=ARC %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks -fobjc-arc -fobjc-runtime-has-weak -DWEAK_SUPPORTED | FileCheck -check-prefix=ARC %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks | FileCheck -check-prefix=MRC %s
 
 typedef int (^fp)();
@@ -138,5 +138,31 @@
 }
 @end
 
+// Check that the delegating invoke function doesn't destruct the Weak object
+// that is passed.
+
+// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvEN3$_58__invokeENS_4WeakE"(
+// ARC: call void @"_ZZN14LambdaDelegate4testEvENK3$_5clENS_4WeakE"(
+// ARC-NEXT: ret void
+
+// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvENK3$_5clENS_4WeakE"(
+// ARC: call void @_ZN14LambdaDelegate4WeakD1Ev(
+
+#ifdef WEAK_SUPPORTED
+
+namespace LambdaDelegate {
+
+struct Weak {
+  __weak id x;
+};
+
+void test() {
+  void (*p)(Weak) = [](Weak a) { };
+}
+
+};
+
+#endif
+
 // ARC: attributes [[NUW]] = { noinline nounwind{{.*}} }
 // MRC: attributes [[NUW]] = { noinline nounwind{{.*}} }
Index: test/CodeGenObjCXX/arc-special-member-functions.mm
===
--- test/CodeGenObjCXX/arc-special-member-functions.mm
+++ test/CodeGenObjCXX/arc-special-member-functions.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -fobjc-runtime-has-weak -emit-llvm -o - %s | FileCheck %s
 
 struct ObjCMember {
   id member;
@@ -12,6 +12,59 @@
   int (^bp)(int);
 };
 
+// CHECK: %[[STRUCT_CONTAINSWEAK:.*]] = type { %[[STRUCT_WEAK:.*]] }
+// CHECK: %[[STRUCT_WEAK]] = type { i8* }
+
+// The Weak object that is passed is destructed in this constructor.
+
+// CHECK: define void @_ZN12ContainsWeakC2E4Weak(
+// CHECK: call void @_ZN4WeakC1ERKS_(
+// CHECK: call void @_ZN4WeakD1Ev(
+
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define void @_ZN12ContainsWeakC1E4Weak(
+// CHECK: call void @_ZN12ContainsWeakC2E4Weak(
+// CHECK-NEXT: ret void
+
+struct Weak {
+  Weak(id);
+  __weak id x;
+};
+
+struct ContainsWeak {
+  ContainsWeak(Weak);
+  Weak w;
+};
+
+ContainsWeak::ContainsWeak(Weak a) : w(a) {}
+
+// The Weak object that is passed is destructed in this constructor.
+
+// CHECK: define void @_ZN4BaseC2E4Weak(
+// CHECK: call void @_ZN4WeakD1Ev(
+// CHECK: ret void
+
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define linkonce_odr void @_ZN7DerivedCI14BaseE4Weak(
+// CHECK: call void @_ZN7DerivedCI24BaseE4Weak(
+// CHECK-NEXT: ret void
+
+struct Base {
+  Base(Weak);
+};
+
+Base::Base(Weak a) {}
+
+struct Derived : Base {
+  using Base::Base;
+};
+
+Derived d(Weak(0));
+
 // CHECK-LABEL: define void @_Z42test_ObjCMember_default_construct_destructv(
 void test_ObjCMember_default_construct_destruct() {
   // CHECK: call void @_ZN10ObjCMemberC1Ev
@@ -111,6 +164,13 @@
 // CHECK-NEXT: call void @objc_release(i8* [[T7]])
 // CHECK-NEXT: ret
 
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define linkonce_odr void @_ZN7DerivedCI24BaseE4Weak(
+// CHECK: call void @_ZN4BaseC2E4Weak(
+// CHECK-NEXT: ret void
+
 // Implicitly-generated default constructor for ObjCMember
 // CHECK-LABEL: define linkonce_odr void @_ZN10ObjCMemberC2Ev
 // CHECK-NOT: objc_release
Index: test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
===
--- test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
+++ test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
@@ -10,6 +10,17 @@
   // CHECK-NEXT: ret i8* [[T2]]
 }
 
+// Check that the delegating block invoke function doesn't destruct the Weak
+// object that is passed.
+
+// CHECK-LABEL: define internal void @___Z8testWeakv_block_invoke(
+// CHECK: call void @"_ZZ8testWeakvENK3$_2clE4W

[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.
Herald added a reviewer: javed.absar.



Comment at: lib/CodeGen/CodeGenFunction.h:847
+CurrentCleanupStackDepth = C;
+  }
+

You don't need (or want) these accessors, I think; this is just private state 
of the CGF object, and nobody else should be using it.



Comment at: lib/CodeGen/CodeGenFunction.h:1112
+  llvm::DenseMap
+  CalleeDestructedParamCleanups;
+

It's too bad that we need this DenseMap in every CGF when actually only a very 
specific set of thunk functions will actually use it.  But I guess DenseMap is 
at least trivial to construct/destroy when empty, which will be the most common 
case.



Comment at: lib/CodeGen/CodeGenFunction.h:1116
+  EHScopeStack::stable_iterator CurrentCleanupStackDepth =
+  EHScopeStack::stable_end();
+

How about `CurrentCleanupScopeDepth`?  The current name makes it sound like 
it's the active depth of the cleanup stack.


Repository:
  rC Clang

https://reviews.llvm.org/D45382



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


[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-17 Thread Akira Hatanaka via Phabricator via cfe-commits
ahatanak updated this revision to Diff 142792.
ahatanak added a comment.

Deactivate the cleanups for callee-destructed parameters that are being 
forwarded to a delegated constructor.

I also added a new member (CurrentCleanupStackDepth) to CodeGenFunction that 
tracks the stack depth of the most recent RunCleanupsScope. This is needed to 
prevent popping the cleanup at the top of the stack in DeactivateCleanupBlock 
if the cleanup doesn't belong to the current RunCleanupsScope. Without this, 
the current RunCleanupsScope's CleanupStackDepth can point to a cleanup that 
doesn't exist on the stack anymore 
(CleanupStackDepth.encloses(EHStack.stable_begin()) doesn't hold true anymore), 
which can cause CodeGenFunction::PopCleanupBlocks to pop all the cleanups 
before it hits the bottom of the stack and crash.


Repository:
  rC Clang

https://reviews.llvm.org/D45382

Files:
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGCleanup.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CodeGenFunction.h
  test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
  test/CodeGenObjCXX/arc-special-member-functions.mm
  test/CodeGenObjCXX/lambda-expressions.mm

Index: test/CodeGenObjCXX/lambda-expressions.mm
===
--- test/CodeGenObjCXX/lambda-expressions.mm
+++ test/CodeGenObjCXX/lambda-expressions.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks -fobjc-arc | FileCheck -check-prefix=ARC %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks -fobjc-arc -fobjc-runtime-has-weak -DWEAK_SUPPORTED | FileCheck -check-prefix=ARC %s
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s -fexceptions -std=c++11 -fblocks | FileCheck -check-prefix=MRC %s
 
 typedef int (^fp)();
@@ -138,5 +138,31 @@
 }
 @end
 
+// Check that the delegating invoke function doesn't destruct the Weak object
+// that is passed.
+
+// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvEN3$_58__invokeENS_4WeakE"(
+// ARC: call void @"_ZZN14LambdaDelegate4testEvENK3$_5clENS_4WeakE"(
+// ARC-NEXT: ret void
+
+// ARC-LABEL: define internal void @"_ZZN14LambdaDelegate4testEvENK3$_5clENS_4WeakE"(
+// ARC: call void @_ZN14LambdaDelegate4WeakD1Ev(
+
+#ifdef WEAK_SUPPORTED
+
+namespace LambdaDelegate {
+
+struct Weak {
+  __weak id x;
+};
+
+void test() {
+  void (*p)(Weak) = [](Weak a) { };
+}
+
+};
+
+#endif
+
 // ARC: attributes [[NUW]] = { noinline nounwind{{.*}} }
 // MRC: attributes [[NUW]] = { noinline nounwind{{.*}} }
Index: test/CodeGenObjCXX/arc-special-member-functions.mm
===
--- test/CodeGenObjCXX/arc-special-member-functions.mm
+++ test/CodeGenObjCXX/arc-special-member-functions.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -fobjc-runtime-has-weak -emit-llvm -o - %s | FileCheck %s
 
 struct ObjCMember {
   id member;
@@ -12,6 +12,59 @@
   int (^bp)(int);
 };
 
+// CHECK: %[[STRUCT_CONTAINSWEAK:.*]] = type { %[[STRUCT_WEAK:.*]] }
+// CHECK: %[[STRUCT_WEAK]] = type { i8* }
+
+// The Weak object that is passed is destructed in this constructor.
+
+// CHECK: define void @_ZN12ContainsWeakC2E4Weak(
+// CHECK: call void @_ZN4WeakC1ERKS_(
+// CHECK: call void @_ZN4WeakD1Ev(
+
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define void @_ZN12ContainsWeakC1E4Weak(
+// CHECK: call void @_ZN12ContainsWeakC2E4Weak(
+// CHECK-NEXT: ret void
+
+struct Weak {
+  Weak(id);
+  __weak id x;
+};
+
+struct ContainsWeak {
+  ContainsWeak(Weak);
+  Weak w;
+};
+
+ContainsWeak::ContainsWeak(Weak a) : w(a) {}
+
+// The Weak object that is passed is destructed in this constructor.
+
+// CHECK: define void @_ZN4BaseC2E4Weak(
+// CHECK: call void @_ZN4WeakD1Ev(
+// CHECK: ret void
+
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define linkonce_odr void @_ZN7DerivedCI14BaseE4Weak(
+// CHECK: call void @_ZN7DerivedCI24BaseE4Weak(
+// CHECK-NEXT: ret void
+
+struct Base {
+  Base(Weak);
+};
+
+Base::Base(Weak a) {}
+
+struct Derived : Base {
+  using Base::Base;
+};
+
+Derived d(Weak(0));
+
 // CHECK-LABEL: define void @_Z42test_ObjCMember_default_construct_destructv(
 void test_ObjCMember_default_construct_destruct() {
   // CHECK: call void @_ZN10ObjCMemberC1Ev
@@ -111,6 +164,13 @@
 // CHECK-NEXT: call void @objc_release(i8* [[T7]])
 // CHECK-NEXT: ret
 
+// Check that the Weak object passed to this constructor is not destructed after
+// the delegate constructor is called.
+
+// CHECK: define linkonce_odr void @_ZN7DerivedCI24BaseE4Weak(
+// CHECK: call void @_ZN4BaseC2E4Weak(
+// CHE

[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment.

If that's the problem, then I think the right design is for CallArg to include 
an optional cleanup reference for a cleanup that can be deactivated at the 
instant of the call (we should assert that this exists for parameters that are 
destroyed in the callee), and then for forwarding it's just a matter of getting 
that cleanup from parameter emission to forwarding-argument emission.


Repository:
  rC Clang

https://reviews.llvm.org/D45382



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


[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-06 Thread Akira Hatanaka via Phabricator via cfe-commits
ahatanak added a comment.

In https://reviews.llvm.org/D45382#1060163, @rjmccall wrote:

> Hmm.  I'm not actually sure *why* it's not okay to forward callee-cleanup 
> arguments.  Do we just not have the necessary logic to disable the cleanup in 
> the caller?


It seems that it would be OK if there was a way to disable the cleanup in the 
caller when we know that we are delegating to another constructor, possibly by 
setting the DelegateCXXConstructorCall here too. But maybe there are other 
reasons for not doing so.

Perhaps Richard (who committed r274049) knows why we can't call the delegated 
constructor when there are callee-cleanup arguments?


Repository:
  rC Clang

https://reviews.llvm.org/D45382



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


[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment.

Hmm.  I'm not actually sure *why* it's not okay to forward callee-cleanup 
arguments.  Do we just not have the necessary logic to disable the cleanup in 
the caller?


Repository:
  rC Clang

https://reviews.llvm.org/D45382



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


[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-06 Thread Akira Hatanaka via Phabricator via cfe-commits
ahatanak created this revision.
ahatanak added reviewers: rjmccall, rsmith.
Herald added a subscriber: kristof.beyls.

This patch fixes a bug where a struct with an ObjC `__weak` field gets 
destructed after it has already been destructed. This bug was introduced in 
r328731, which made changes to the ABI that caused structs with ObjC pointer 
fields to be destructed in the callee in some cases.

This happens in two cases:

1. C++11 inheriting constructors.
2. When EmitConstructorBody does complete->base constructor delegation 
optimization.

I fixed the first case by making changes to canEmitDelegateCallArgs so that it 
returns false when the constructor has a parameter that is destructed in the 
callee.

For the second case, I made changes so that EmitParmDecl doesn't push the 
destructor cleanup for the struct parameter if the function is a constructor 
that is going to delegate to the base constructor. Alternatively, I think it's 
possible to just disable the optimization in EmitConstructorBody if 
canEmitDelegateCallArgs returns false.

rdar://problem/39194693


Repository:
  rC Clang

https://reviews.llvm.org/D45382

Files:
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  test/CodeGenObjCXX/arc-special-member-functions.mm

Index: test/CodeGenObjCXX/arc-special-member-functions.mm
===
--- test/CodeGenObjCXX/arc-special-member-functions.mm
+++ test/CodeGenObjCXX/arc-special-member-functions.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -fobjc-arc -fblocks -triple x86_64-apple-darwin10.0.0 -fobjc-runtime-has-weak -emit-llvm -o - %s | FileCheck %s
 
 struct ObjCMember {
   id member;
@@ -12,6 +12,54 @@
   int (^bp)(int);
 };
 
+// CHECK: %[[STRUCT_CONTAINSWEAK:.*]] = type { %[[STRUCT_WEAK:.*]] }
+// CHECK: %[[STRUCT_WEAK]] = type { i8* }
+
+// The Weak object that is passed is destructed in this constructor.
+
+// CHECK: define void @_ZN12ContainsWeakC2E4Weak(
+// CHECK: call void @_ZN4WeakC1ERKS_(
+// CHECK: call void @_ZN4WeakD1Ev(
+
+// Check that Weak's destructor is not called after the delegate constructor is
+// called.
+
+// CHECK: define void @_ZN12ContainsWeakC1E4Weak(
+// CHECK: call void @_ZN12ContainsWeakC2E4Weak(
+// CHECK-NEXT: ret void
+
+struct Weak {
+  Weak(id);
+  __weak id x;
+};
+
+struct ContainsWeak {
+  ContainsWeak(Weak);
+  Weak w;
+};
+
+ContainsWeak::ContainsWeak(Weak a) : w(a) {}
+
+// Check that the call to the inheriting constructor is inlined.
+
+// CHECK: define internal void @__cxx_global_var_init{{.*}}()
+// CHECK: call void @_ZN4WeakC1EP11objc_object(
+// CHECK-NOT: call
+// CHECK: call void @_ZN4BaseC2E4Weak(
+// CHECK-NEXT: ret void
+
+struct Base {
+  Base(Weak);
+};
+
+Base::Base(Weak a) {}
+
+struct Derived : Base {
+  using Base::Base;
+};
+
+Derived d(Weak(0));
+
 // CHECK-LABEL: define void @_Z42test_ObjCMember_default_construct_destructv(
 void test_ObjCMember_default_construct_destruct() {
   // CHECK: call void @_ZN10ObjCMemberC1Ev
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -380,6 +380,10 @@
   /// should emit cleanups.
   bool CurFuncIsThunk;
 
+  /// In C++, whether we are code generating a constructor that is calling a
+  /// delegating constructor.
+  bool DelegateCXXConstructorCall;
+
   /// In ARC, whether we should autorelease the return value.
   bool AutoreleaseResult;
 
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -67,7 +67,8 @@
   CGBuilderInserterTy(this)),
   CurFn(nullptr), ReturnValue(Address::invalid()),
   CapturedStmtInfo(nullptr), SanOpts(CGM.getLangOpts().Sanitize),
-  IsSanitizerScope(false), CurFuncIsThunk(false), AutoreleaseResult(false),
+  IsSanitizerScope(false), CurFuncIsThunk(false),
+  DelegateCXXConstructorCall(false), AutoreleaseResult(false),
   SawAsmBlock(false), IsOutlinedSEHHelper(false), BlockInfo(nullptr),
   BlockPointer(nullptr), LambdaThisCaptureField(nullptr),
   NormalCleanupDest(Address::invalid()), NextCleanupDestIndex(1),
@@ -1307,6 +1308,13 @@
   if (Body && ShouldEmitLifetimeMarkers)
 Bypasses.Init(Body);
 
+  if (const auto *Ctor = dyn_cast(CurGD.getDecl())) {
+if (CurGD.getCtorType() == Ctor_Complete &&
+IsConstructorDelegationValid(Ctor) &&
+CGM.getTarget().getCXXABI().hasConstructorVariants())
+  DelegateCXXConstructorCall = true;
+  }
+
   // Emit the standard function prologue.
   StartFunction(GD, ResTy, Fn, FnInfo, Args, Loc, BodyRange.getBegin());
 
Index: lib/CodeGen/CGDecl.cpp
==