[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review, John. Committed the change and will watch the buildbots 
on non-Darwin platform.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55543



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

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC349848: [CodeGen] Fix assertion on emitting cleanup for 
object with inlined inherited… (authored by vsapsai, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55543?vs=178921&id=179165#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55543

Files:
  lib/CodeGen/CGClass.cpp
  test/CodeGenCXX/inheriting-constructor-cleanup.cpp
  test/CodeGenObjCXX/inheriting-constructor-cleanup.mm

Index: test/CodeGenCXX/inheriting-constructor-cleanup.cpp
===
--- test/CodeGenCXX/inheriting-constructor-cleanup.cpp
+++ test/CodeGenCXX/inheriting-constructor-cleanup.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s --check-prefix=EXCEPTIONS
+
+// PR36748
+// rdar://problem/45805151
+
+// Classes to verify order of destroying function parameters.
+struct S1 {
+  ~S1();
+};
+struct S2 {
+  ~S2();
+};
+
+struct Base {
+  // Use variadic args to cause inlining the inherited constructor.
+  Base(const S1&, const S2&, const char *fmt, ...) {}
+};
+
+struct NonTrivialDtor {
+  ~NonTrivialDtor() {}
+};
+struct Inheritor : public NonTrivialDtor, public Base {
+  using Base::Base;
+};
+
+void f() {
+  Inheritor(S1(), S2(), "foo");
+  // CHECK-LABEL: define void @_Z1fv
+  // CHECK: %[[TMP1:.*]] = alloca %struct.S1
+  // CHECK: %[[TMP2:.*]] = alloca %struct.S2
+  // CHECK: call void (%struct.Base*, %struct.S1*, %struct.S2*, i8*, ...) @_ZN4BaseC2ERK2S1RK2S2PKcz(%struct.Base* {{.*}}, %struct.S1* dereferenceable(1) %[[TMP1]], %struct.S2* dereferenceable(1) %[[TMP2]], i8* {{.*}})
+  // CHECK-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+  // CHECK-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // CHECK-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+
+  // EXCEPTIONS-LABEL: define void @_Z1fv
+  // EXCEPTIONS: %[[TMP1:.*]] = alloca %struct.S1
+  // EXCEPTIONS: %[[TMP2:.*]] = alloca %struct.S2
+  // EXCEPTIONS: invoke void (%struct.Base*, %struct.S1*, %struct.S2*, i8*, ...) @_ZN4BaseC2ERK2S1RK2S2PKcz(%struct.Base* {{.*}}, %struct.S1* dereferenceable(1) %[[TMP1]], %struct.S2* dereferenceable(1) %[[TMP2]], i8* {{.*}})
+  // EXCEPTIONS-NEXT: to label %[[CONT:.*]] unwind label %[[LPAD:.*]]
+
+  // EXCEPTIONS: [[CONT]]:
+  // EXCEPTIONS-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+  // EXCEPTIONS-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // EXCEPTIONS-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+
+  // EXCEPTIONS: [[LPAD]]:
+  // EXCEPTIONS: call void @_ZN14NonTrivialDtorD2Ev(%struct.NonTrivialDtor* {{.*}})
+  // EXCEPTIONS-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // EXCEPTIONS-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+}
Index: test/CodeGenObjCXX/inheriting-constructor-cleanup.mm
===
--- test/CodeGenObjCXX/inheriting-constructor-cleanup.mm
+++ test/CodeGenObjCXX/inheriting-constructor-cleanup.mm
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -fobjc-arc -emit-llvm -o - %s | FileCheck %s --implicit-check-not "call\ "
+// rdar://problem/45805151
+
+struct Strong {
+  __strong id x;
+};
+
+struct Base {
+  // Use variadic args to cause inlining the inherited constructor.
+  Base(Strong s, ...) {}
+};
+
+struct NonTrivialDtor {
+  ~NonTrivialDtor() {}
+};
+struct Inheritor : public NonTrivialDtor, public Base {
+  using Base::Base;
+};
+
+id g(void);
+void f() {
+  Inheritor({g()});
+}
+// CHECK-LABEL: define void @_Z1fv
+// CHECK:   %[[TMP:.*]] = call i8* @_Z1gv()
+// CHECK:   {{.*}} = call i8* @objc_retainAutoreleasedReturnValue(i8* %[[TMP]])
+// CHECK:   call void (%struct.Base*, i8*, ...) @_ZN4BaseC2E6Strongz(%struct.Base* {{.*}}, i8* {{.*}})
+// CHECK-NEXT:  call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN4BaseC2E6Strongz(%struct.Base* {{.*}}, i8* {{.*}}, ...)
+// CHECK:   call void @_ZN6StrongD1Ev(%struct.Strong* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+// CHECK:   call void @_ZN9InheritorD2Ev(%struct.Inheritor* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN6StrongD1Ev(%struct.Strong* {{.*}})
+// CHECK:   call void @_ZN6StrongD2Ev(%struct.Strong* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN6StrongD2Ev(%struct.Strong* {{.*}})
+// CHECK:   call void @objc_storeStrong(i8** {{.*}}, i8* null)
+
+// CHECK-LABEL: define linkonce_odr void @_ZN9InheritorD2Ev(%struct.Inheritor* {{.*}})
+// CHECK:   call void @_ZN14NonTrivialDtorD2Ev(%struct.NonTrivialDtor* {{.*}})
Index: lib/CodeGen/CGClass.cpp
==

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, LGTM.  Somewhat surprised that we don't have an implicit copy of the 
`Strong` argument, but it's not a bad thing that we don't.


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

https://reviews.llvm.org/D55543



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

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 178921.
vsapsai added a comment.

- [ObjC++] Verify there are no unexpected calls.

`--implicit-check-not` seems reasonable approach in this case. It causes adding
`Inheritor` destructors but gives higher confidence there are no unexpected
calls in unexpected places.

Escaping a space after check-not pattern to avoid matching attribute
`disable-tail-calls`.


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

https://reviews.llvm.org/D55543

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGenCXX/inheriting-constructor-cleanup.cpp
  clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm

Index: clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm
===
--- /dev/null
+++ clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -fobjc-arc -emit-llvm -o - %s | FileCheck %s --implicit-check-not "call\ "
+// rdar://problem/45805151
+
+struct Strong {
+  __strong id x;
+};
+
+struct Base {
+  // Use variadic args to cause inlining the inherited constructor.
+  Base(Strong s, ...) {}
+};
+
+struct NonTrivialDtor {
+  ~NonTrivialDtor() {}
+};
+struct Inheritor : public NonTrivialDtor, public Base {
+  using Base::Base;
+};
+
+id g(void);
+void f() {
+  Inheritor({g()});
+}
+// CHECK-LABEL: define void @_Z1fv
+// CHECK:   %[[TMP:.*]] = call i8* @_Z1gv()
+// CHECK:   {{.*}} = call i8* @objc_retainAutoreleasedReturnValue(i8* %[[TMP]])
+// CHECK:   call void (%struct.Base*, i8*, ...) @_ZN4BaseC2E6Strongz(%struct.Base* {{.*}}, i8* {{.*}})
+// CHECK-NEXT:  call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN4BaseC2E6Strongz(%struct.Base* {{.*}}, i8* {{.*}}, ...)
+// CHECK:   call void @_ZN6StrongD1Ev(%struct.Strong* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+// CHECK:   call void @_ZN9InheritorD2Ev(%struct.Inheritor* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN6StrongD1Ev(%struct.Strong* {{.*}})
+// CHECK:   call void @_ZN6StrongD2Ev(%struct.Strong* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN6StrongD2Ev(%struct.Strong* {{.*}})
+// CHECK:   call void @objc_storeStrong(i8** {{.*}}, i8* null)
+
+// CHECK-LABEL: define linkonce_odr void @_ZN9InheritorD2Ev(%struct.Inheritor* {{.*}})
+// CHECK:   call void @_ZN14NonTrivialDtorD2Ev(%struct.NonTrivialDtor* {{.*}})
Index: clang/test/CodeGenCXX/inheriting-constructor-cleanup.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/inheriting-constructor-cleanup.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s --check-prefix=EXCEPTIONS
+
+// PR36748
+// rdar://problem/45805151
+
+// Classes to verify order of destroying function parameters.
+struct S1 {
+  ~S1();
+};
+struct S2 {
+  ~S2();
+};
+
+struct Base {
+  // Use variadic args to cause inlining the inherited constructor.
+  Base(const S1&, const S2&, const char *fmt, ...) {}
+};
+
+struct NonTrivialDtor {
+  ~NonTrivialDtor() {}
+};
+struct Inheritor : public NonTrivialDtor, public Base {
+  using Base::Base;
+};
+
+void f() {
+  Inheritor(S1(), S2(), "foo");
+  // CHECK-LABEL: define void @_Z1fv
+  // CHECK: %[[TMP1:.*]] = alloca %struct.S1
+  // CHECK: %[[TMP2:.*]] = alloca %struct.S2
+  // CHECK: call void (%struct.Base*, %struct.S1*, %struct.S2*, i8*, ...) @_ZN4BaseC2ERK2S1RK2S2PKcz(%struct.Base* {{.*}}, %struct.S1* dereferenceable(1) %[[TMP1]], %struct.S2* dereferenceable(1) %[[TMP2]], i8* {{.*}})
+  // CHECK-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+  // CHECK-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // CHECK-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+
+  // EXCEPTIONS-LABEL: define void @_Z1fv
+  // EXCEPTIONS: %[[TMP1:.*]] = alloca %struct.S1
+  // EXCEPTIONS: %[[TMP2:.*]] = alloca %struct.S2
+  // EXCEPTIONS: invoke void (%struct.Base*, %struct.S1*, %struct.S2*, i8*, ...) @_ZN4BaseC2ERK2S1RK2S2PKcz(%struct.Base* {{.*}}, %struct.S1* dereferenceable(1) %[[TMP1]], %struct.S2* dereferenceable(1) %[[TMP2]], i8* {{.*}})
+  // EXCEPTIONS-NEXT: to label %[[CONT:.*]] unwind label %[[LPAD:.*]]
+
+  // EXCEPTIONS: [[CONT]]:
+  // EXCEPTIONS-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+  // EXCEPTIONS-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // EXCEPTIONS-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+
+  // EXCEPTIONS: [[LPAD]]:
+  // EXCEPTIONS: call void @_ZN14NonTrivialDtorD2Ev(%struct.NonTrivialDtor* {{.*}})
+  // EXCEPTIONS-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // EXCEPTIONS-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+}
Index: clan

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm:25
+// CHECK-LABEL: define void @_Z1fv
+// CHECK: call void (%struct.Base*, i8*, ...) 
@_ZN4BaseC2E6Strongz(%struct.Base* {{.*}}, i8* {{.*}})
+// CHECK-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})

Can you include checks for all the calls in this function, with CHECK-NOTs to 
prove that there aren't any other calls?


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

https://reviews.llvm.org/D55543



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

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 178746.
vsapsai added a comment.

- [ObjC++] Pass struct with `__strong` field as a value, not as a reference.


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

https://reviews.llvm.org/D55543

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGenCXX/inheriting-constructor-cleanup.cpp
  clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm

Index: clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm
===
--- /dev/null
+++ clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -fobjc-arc -emit-llvm -o - %s | FileCheck %s
+// rdar://problem/45805151
+
+struct Strong {
+  __strong id x;
+};
+
+struct Base {
+  // Use variadic args to cause inlining the inherited constructor.
+  Base(Strong s, ...) {}
+};
+
+struct NonTrivialDtor {
+  ~NonTrivialDtor() {}
+};
+struct Inheritor : public NonTrivialDtor, public Base {
+  using Base::Base;
+};
+
+id g(void);
+void f() {
+  Inheritor({g()});
+}
+// CHECK-LABEL: define void @_Z1fv
+// CHECK: call void (%struct.Base*, i8*, ...) @_ZN4BaseC2E6Strongz(%struct.Base* {{.*}}, i8* {{.*}})
+// CHECK-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN4BaseC2E6Strongz(%struct.Base* {{.*}}, i8* {{.*}}, ...)
+// CHECK: call void @_ZN6StrongD1Ev(%struct.Strong* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN6StrongD1Ev(%struct.Strong* {{.*}})
+// CHECK: call void @_ZN6StrongD2Ev(%struct.Strong* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN6StrongD2Ev(%struct.Strong* {{.*}})
+// CHECK: call void @objc_storeStrong(i8** {{.*}}, i8* null)
Index: clang/test/CodeGenCXX/inheriting-constructor-cleanup.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/inheriting-constructor-cleanup.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s --check-prefix=EXCEPTIONS
+
+// PR36748
+// rdar://problem/45805151
+
+// Classes to verify order of destroying function parameters.
+struct S1 {
+  ~S1();
+};
+struct S2 {
+  ~S2();
+};
+
+struct Base {
+  // Use variadic args to cause inlining the inherited constructor.
+  Base(const S1&, const S2&, const char *fmt, ...) {}
+};
+
+struct NonTrivialDtor {
+  ~NonTrivialDtor() {}
+};
+struct Inheritor : public NonTrivialDtor, public Base {
+  using Base::Base;
+};
+
+void f() {
+  Inheritor(S1(), S2(), "foo");
+  // CHECK-LABEL: define void @_Z1fv
+  // CHECK: %[[TMP1:.*]] = alloca %struct.S1
+  // CHECK: %[[TMP2:.*]] = alloca %struct.S2
+  // CHECK: call void (%struct.Base*, %struct.S1*, %struct.S2*, i8*, ...) @_ZN4BaseC2ERK2S1RK2S2PKcz(%struct.Base* {{.*}}, %struct.S1* dereferenceable(1) %[[TMP1]], %struct.S2* dereferenceable(1) %[[TMP2]], i8* {{.*}})
+  // CHECK-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+  // CHECK-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // CHECK-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+
+  // EXCEPTIONS-LABEL: define void @_Z1fv
+  // EXCEPTIONS: %[[TMP1:.*]] = alloca %struct.S1
+  // EXCEPTIONS: %[[TMP2:.*]] = alloca %struct.S2
+  // EXCEPTIONS: invoke void (%struct.Base*, %struct.S1*, %struct.S2*, i8*, ...) @_ZN4BaseC2ERK2S1RK2S2PKcz(%struct.Base* {{.*}}, %struct.S1* dereferenceable(1) %[[TMP1]], %struct.S2* dereferenceable(1) %[[TMP2]], i8* {{.*}})
+  // EXCEPTIONS-NEXT: to label %[[CONT:.*]] unwind label %[[LPAD:.*]]
+
+  // EXCEPTIONS: [[CONT]]:
+  // EXCEPTIONS-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+  // EXCEPTIONS-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // EXCEPTIONS-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+
+  // EXCEPTIONS: [[LPAD]]:
+  // EXCEPTIONS: call void @_ZN14NonTrivialDtorD2Ev(%struct.NonTrivialDtor* {{.*}})
+  // EXCEPTIONS-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // EXCEPTIONS-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+}
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2196,6 +2196,7 @@
   GlobalDecl GD(Ctor, CtorType);
   InlinedInheritingConstructorScope Scope(*this, GD);
   ApplyInlineDebugLocation DebugScope(*this, GD);
+  RunCleanupsScope RunCleanups(*this);
 
   // Save the arguments to be passed to the inherited constructor.
   CXXInheritedCtorInitExprArgs = Args;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm:10
+  // Use variadic args to force inlining the inherited constructor.
+  Base(const Strong &s, ...) {}
+};

To test what I'd like to see, this needs to take `Strong` by value, not by 
reference.  There won't be a cleanup for the inlined parameter if it's taken by 
reference.


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

https://reviews.llvm.org/D55543



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

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 178563.
vsapsai added a comment.

- Test order of destroying parameters to inlined inherited constructor.
- [ObjC++] Add test using struct with `__strong` field as a parameter for 
inlined inherited constructor.


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

https://reviews.llvm.org/D55543

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGenCXX/inheriting-constructor-cleanup.cpp
  clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm

Index: clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm
===
--- /dev/null
+++ clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -fobjc-arc -emit-llvm -o - %s | FileCheck %s
+// rdar://problem/45805151
+
+struct Strong {
+  __strong id x;
+};
+
+struct Base {
+  // Use variadic args to force inlining the inherited constructor.
+  Base(const Strong &s, ...) {}
+};
+
+struct NonTrivialDtor {
+  ~NonTrivialDtor() {}
+};
+struct Inheritor : public NonTrivialDtor, public Base {
+  using Base::Base;
+};
+
+id g(void);
+void f() {
+  Inheritor({g()});
+}
+// CHECK-LABEL: define void @_Z1fv
+// CHECK: call void (%struct.Base*, %struct.Strong*, ...) @_ZN4BaseC2ERK6Strongz(%struct.Base* {{.*}}, %struct.Strong* dereferenceable(8) {{.*}})
+// CHECK-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+// CHECK-NEXT: call void @_ZN6StrongD1Ev(%struct.Strong* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN6StrongD1Ev(%struct.Strong* {{.*}})
+// CHECK: call void @_ZN6StrongD2Ev(%struct.Strong* {{.*}})
+
+// CHECK-LABEL: define linkonce_odr void @_ZN6StrongD2Ev(%struct.Strong* {{.*}})
+// CHECK: call void @objc_storeStrong(i8** {{.*}}, i8* null)
Index: clang/test/CodeGenCXX/inheriting-constructor-cleanup.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/inheriting-constructor-cleanup.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-darwin -std=c++11 -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s --check-prefix=EXCEPTIONS
+
+// PR36748
+// rdar://problem/45805151
+
+// Classes to verify order of destroying function parameters.
+struct S1 {
+  ~S1();
+};
+struct S2 {
+  ~S2();
+};
+
+struct Base {
+  // Use variadic args to cause inlining the inherited constructor.
+  Base(const S1&, const S2&, const char *fmt, ...) {}
+};
+
+struct NonTrivialDtor {
+  ~NonTrivialDtor() {}
+};
+struct Inheritor : public NonTrivialDtor, public Base {
+  using Base::Base;
+};
+
+void f() {
+  Inheritor(S1(), S2(), "foo");
+  // CHECK-LABEL: define void @_Z1fv
+  // CHECK: %[[TMP1:.*]] = alloca %struct.S1
+  // CHECK: %[[TMP2:.*]] = alloca %struct.S2
+  // CHECK: call void (%struct.Base*, %struct.S1*, %struct.S2*, i8*, ...) @_ZN4BaseC2ERK2S1RK2S2PKcz(%struct.Base* {{.*}}, %struct.S1* dereferenceable(1) %[[TMP1]], %struct.S2* dereferenceable(1) %[[TMP2]], i8* {{.*}})
+  // CHECK-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+  // CHECK-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // CHECK-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+
+  // EXCEPTIONS-LABEL: define void @_Z1fv
+  // EXCEPTIONS: %[[TMP1:.*]] = alloca %struct.S1
+  // EXCEPTIONS: %[[TMP2:.*]] = alloca %struct.S2
+  // EXCEPTIONS: invoke void (%struct.Base*, %struct.S1*, %struct.S2*, i8*, ...) @_ZN4BaseC2ERK2S1RK2S2PKcz(%struct.Base* {{.*}}, %struct.S1* dereferenceable(1) %[[TMP1]], %struct.S2* dereferenceable(1) %[[TMP2]], i8* {{.*}})
+  // EXCEPTIONS-NEXT: to label %[[CONT:.*]] unwind label %[[LPAD:.*]]
+
+  // EXCEPTIONS: [[CONT]]:
+  // EXCEPTIONS-NEXT: call void @_ZN9InheritorD1Ev(%struct.Inheritor* {{.*}})
+  // EXCEPTIONS-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // EXCEPTIONS-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+
+  // EXCEPTIONS: [[LPAD]]:
+  // EXCEPTIONS: call void @_ZN14NonTrivialDtorD2Ev(%struct.NonTrivialDtor* {{.*}})
+  // EXCEPTIONS-NEXT: call void @_ZN2S2D1Ev(%struct.S2* %[[TMP2]])
+  // EXCEPTIONS-NEXT: call void @_ZN2S1D1Ev(%struct.S1* %[[TMP1]])
+}
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2196,6 +2196,7 @@
   GlobalDecl GD(Ctor, CtorType);
   InlinedInheritingConstructorScope Scope(*this, GD);
   ApplyInlineDebugLocation DebugScope(*this, GD);
+  RunCleanupsScope RunCleanups(*this);
 
   // Save the arguments to be passed to the inherited constructor.
   CXXInheritedCtorInitExprArgs = Args;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D55543#1329288 , @vsapsai wrote:

> In D55543#1328025 , @rjmccall wrote:
>
> > Nice catch.  This also fixes problems where there might be cleanups entered 
> > by `EmitParmDecl`, e.g. in ObjC++ with a parameter of type `struct A { 
> > __strong id x; }`; could you please add a test for that and verify that the 
> > parameter is destroyed at an appropriate point in the inlined call?
>
>
> Aha, I see we have at least `EHStack.pushCleanup` in 
> `EmitParmDecl`. Will make sure to add a test that executes this code path.


The way I understand the comment, we cannot make a parameter of type `struct A` 
to have a cleanup specific to 
`CodeGenFunction::EmitInlinedInheritingCXXConstructorCall`. It calls 
`EmitParmDecl` but does it only for implicit parameters. And I am struggling to 
make `struct A` an implicit parameter.

Nevertheless, my change causes different exception-handling behaviour. Need to 
check further if it is ARC-related or inherited constructor-related.


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

https://reviews.llvm.org/D55543



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

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D55543#1328034 , @smeenai wrote:

> I'd tried this exact same patch back in https://reviews.llvm.org/D44619, but 
> I was running into a bunch of check-clang failures with it, and I was never 
> able to figure them out. It looks like it works now though, so I'm glad :) 
> Richard's comment from that diff might still be relevant:
>
> > Please add a test to ensure that we still destroy function parameters in 
> > the right order and at the right times (for both the exceptional and 
> > non-exceptional cleanup cases).
>
> This will also resolve https://bugs.llvm.org/show_bug.cgi?id=36748


Thanks, Shoaib, for extra information. Maybe I don't see any failing tests 
because I'm not testing on Windows. At least I want to try a sample you've 
provided in https://reviews.llvm.org/D44619#1056458


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

https://reviews.llvm.org/D55543



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

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D55543#1328025 , @rjmccall wrote:

> Nice catch.  This also fixes problems where there might be cleanups entered 
> by `EmitParmDecl`, e.g. in ObjC++ with a parameter of type `struct A { 
> __strong id x; }`; could you please add a test for that and verify that the 
> parameter is destroyed at an appropriate point in the inlined call?


Aha, I see we have at least `EHStack.pushCleanup` in 
`EmitParmDecl`. Will make sure to add a test that executes this code path.


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

https://reviews.llvm.org/D55543



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

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

I'd tried this exact same patch back in https://reviews.llvm.org/D44619, but I 
was running into a bunch of check-clang failures with it, and I was never able 
to figure them out. It looks like it works now though, so I'm glad :) Richard's 
comment from that diff might still be relevant:

> Please add a test to ensure that we still destroy function parameters in the 
> right order and at the right times (for both the exceptional and 
> non-exceptional cleanup cases).

This will also resolve https://bugs.llvm.org/show_bug.cgi?id=36748


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

https://reviews.llvm.org/D55543



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

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Nice catch.  This also fixes problems where there might be cleanups entered by 
`EmitParmDecl`, e.g. in ObjC++ with a parameter of type `struct A { __strong id 
x; }`; could you please add a test for that and verify that the parameter is 
destroyed at an appropriate point in the inlined call?


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

https://reviews.llvm.org/D55543



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

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: rsmith, rjmccall.
Herald added subscribers: dexonsmith, jkorous.

Fixes assertion

> Assertion failed: (isa(Val) && "cast() argument of incompatible 
> type!"), function cast, file llvm/Support/Casting.h, line 255.

It was triggered by trying to cast `FunctionDecl` to `CXXMethodDecl` as
`CGF.CurCodeDecl` in `CallBaseDtor::Emit`. It was happening because
cleanups were emitted in `ScalarExprEmitter::VisitExprWithCleanups`
after destroying `InlinedInheritingConstructorScope`, so
`CodeGenFunction.CurCodeDecl` didn't correspond to expected cleanup decl.

Fix the assertion by emitting cleanups before leaving
`InlinedInheritingConstructorScope` and changing `CurCodeDecl`.

rdar://problem/45805151


https://reviews.llvm.org/D55543

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGenCXX/throw-expressions.cpp


Index: clang/test/CodeGenCXX/throw-expressions.cpp
===
--- clang/test/CodeGenCXX/throw-expressions.cpp
+++ clang/test/CodeGenCXX/throw-expressions.cpp
@@ -112,3 +112,20 @@
   // CHECK: ret i32* @val
   return cond ? val : ((throw "foo"));
 }
+
+// Test throwing object with inlined inherited constructor and non-trivial 
cleanup.
+namespace rdar45805151 {
+  struct BaseException {
+// Use variadic args to force inlining the inherited constructor.
+BaseException(const char *format, ...) {}
+// Add explicit destructor to make it non-trivial.
+~BaseException() {}
+  };
+  struct BadException : public BaseException {
+using BaseException::BaseException;
+  };
+
+  void test9() {
+throw BadException("foo");
+  }
+}
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2196,6 +2196,7 @@
   GlobalDecl GD(Ctor, CtorType);
   InlinedInheritingConstructorScope Scope(*this, GD);
   ApplyInlineDebugLocation DebugScope(*this, GD);
+  RunCleanupsScope RunCleanups(*this);
 
   // Save the arguments to be passed to the inherited constructor.
   CXXInheritedCtorInitExprArgs = Args;


Index: clang/test/CodeGenCXX/throw-expressions.cpp
===
--- clang/test/CodeGenCXX/throw-expressions.cpp
+++ clang/test/CodeGenCXX/throw-expressions.cpp
@@ -112,3 +112,20 @@
   // CHECK: ret i32* @val
   return cond ? val : ((throw "foo"));
 }
+
+// Test throwing object with inlined inherited constructor and non-trivial cleanup.
+namespace rdar45805151 {
+  struct BaseException {
+// Use variadic args to force inlining the inherited constructor.
+BaseException(const char *format, ...) {}
+// Add explicit destructor to make it non-trivial.
+~BaseException() {}
+  };
+  struct BadException : public BaseException {
+using BaseException::BaseException;
+  };
+
+  void test9() {
+throw BadException("foo");
+  }
+}
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -2196,6 +2196,7 @@
   GlobalDecl GD(Ctor, CtorType);
   InlinedInheritingConstructorScope Scope(*this, GD);
   ApplyInlineDebugLocation DebugScope(*this, GD);
+  RunCleanupsScope RunCleanups(*this);
 
   // Save the arguments to be passed to the inherited constructor.
   CXXInheritedCtorInitExprArgs = Args;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits