[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-27 Thread Øystein Dale via Phabricator via cfe-commits 
oydale abandoned this revision.
oydale added a comment.

compnerd added a triple to the test case and re-committed the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-25 Thread JF Bastien via Phabricator via cfe-commits 
jfb reopened this revision.
jfb added a comment.
This revision is now accepted and ready to land.

Reverted in r367051, it's causing failures: 
http://lab.llvm.org:8011/builders/clang-cmake-armv8-full/builds/13658/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Apr40771-ctad-with-lambda-copy-capture.cpp

  
/home/buildslave/buildslave/clang-cmake-armv8-full/llvm/tools/clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:20:16:
 error: CHECK-NEXT: expected string not found in input
  // CHECK-NEXT: call void @_ZN1RC1E1Q(%struct.R* [[TMP_R]])
 ^
  :37:2: note: scanning from here
   %8 = call %struct.R* @_ZN1RC1E1Q(%struct.R* %1)
   ^
  :37:2: note: with "TMP_R" equal to "%1"
   %8 = call %struct.R* @_ZN1RC1E1Q(%struct.R* %1)
   ^
  :37:17: note: possible intended match here
   %8 = call %struct.R* @_ZN1RC1E1Q(%struct.R* %1)
  ^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-25 Thread Saleem Abdulrasool via Phabricator via cfe-commits 
compnerd closed this revision.
compnerd added a comment.

SVN r367042


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-18 Thread Øystein Dale via Phabricator via cfe-commits 
oydale added a comment.

@rsmith, @rjmccall, I don't have access to commit this myself, could one of you 
commit it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-13 Thread Øystein Dale via Phabricator via cfe-commits 
oydale accepted this revision.
oydale added a comment.

Feel free to commit this patch on my behalf.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-13 Thread Øystein Dale via Phabricator via cfe-commits 
oydale updated this revision to Diff 209701.
oydale added a comment.

Changed test to use CHECK-NEXT, -discard-value-names, and pattern matching


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp

Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -emit-llvm --std=c++17 -fcxx-exceptions -fexceptions \
+// RUN:   -discard-value-names %s -o - | FileCheck %s
+
+struct Q { Q(); };
+struct R { R(Q); ~R(); };
+struct S { S(Q); ~S(); };
+struct T : R, S {};
+
+Q q;
+T t { R{q}, S{q} };
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init.1() {{.*}} {
+// CHECK-NEXT: [[TMP_R:%[a-z0-9.]+]] = alloca %struct.R, align 1
+// CHECK-NEXT: [[TMP_Q1:%[a-z0-9.]+]] = alloca %struct.Q, align 1
+// CHECK-NEXT: [[TMP_S:%[a-z0-9.]+]] = alloca %struct.S, align 1
+// CHECK-NEXT: [[TMP_Q2:%[a-z0-9.]+]] = alloca %struct.Q, align 1
+// CHECK-NEXT: [[XPT:%[a-z0-9.]+]] = alloca i8*
+// CHECK-NEXT: [[SLOT:%[a-z0-9.]+]] = alloca i32
+// CHECK-NEXT: [[ACTIVE:%[a-z0-9.]+]] = alloca i1, align 1
+// CHECK-NEXT: call void @_ZN1RC1E1Q(%struct.R* [[TMP_R]])
+// CHECK-NEXT: store i1 true, i1* [[ACTIVE]], align 1
+// CHECK-NEXT: invoke void @_ZN1SC1E1Q(%struct.S* [[TMP_S]])
+// CHECK-NEXT:   to label %[[L1:[a-z0-9.]+]] unwind label %[[L2:[a-z0-9.]+]]
+// CHECK-EMPTY:
+// CHECK-NEXT: [[L1]]:
+// CHECK-NEXT: store i1 false, i1* [[ACTIVE]], align 1
+// CHECK-NEXT: call void @_ZN1SD1Ev(%struct.S*
+// CHECK-NEXT: call void @_ZN1RD1Ev(%struct.R*
+// CHECK-NEXT: [[EXIT:%[a-z0-9.]+]] = call i32 @__cxa_atexit(
+// CHECK-NEXT: ret void
+// CHECK-EMPTY:
+// CHECK-NEXT: [[L2]]:
+// CHECK-NEXT: [[LP:%[a-z0-9.]+]] = landingpad { i8*, i32 }
+// CHECK-NEXT:  cleanup
+// CHECK-NEXT: [[X1:%[a-z0-9.]+]] = extractvalue { i8*, i32 } [[LP]], 0
+// CHECK-NEXT: store i8* [[X1]], i8** [[XPT]], align 8
+// CHECK-NEXT: [[X2:%[a-z0-9.]+]] = extractvalue { i8*, i32 } [[LP]], 1
+// CHECK-NEXT: store i32 [[X2]], i32* [[SLOT]], align 4
+// CHECK-NEXT: [[IS_ACT:%[a-z0-9.]+]] = load i1, i1* [[ACTIVE]], align 1
+// CHECK-NEXT: br i1 [[IS_ACT]], label %[[L3:[a-z0-9.]+]], label %[[L4:[a-z0-9.]+]]
+// CHECK-EMPTY:
+// CHECK-NEXT: [[L3]]:
+// CHECK-NEXT: call void @_ZN1RD1Ev(%struct.R*
+// CHECK-NEXT: br label %[[L4]]
+// CHECK-EMPTY:
+// CHECK-NEXT: [[L4]]:
+// CHECK-NEXT: call void @_ZN1RD1Ev(%struct.R* [[TMP_R]])
+// CHECK-NEXT: br label %[[L5:[a-z0-9.]+]]
+// CHECK-EMPTY:
+// CHECK-NEXT: [[L5]]:
+// CHECK-NEXT: [[EXN:%[a-z0-9.]+]] = load i8*, i8** [[XPT]], align 8
+// CHECK-NEXT: [[SEL:%[a-z0-9.]+]] = load i32, i32* [[SLOT]], align 4
+// CHECK-NEXT: [[LV1:%[a-z0-9.]+]] = insertvalue { i8*, i32 } undef, i8* [[EXN]], 0
+// CHECK-NEXT: [[LV2:%[a-z0-9.]+]] = insertvalue { i8*, i32 } [[LV1]], i32 [[SEL]], 1
+// CHECK-NEXT: resume { i8*, i32 } [[LV2]]
+// CHECK-NEXT: }
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1495,6 +1495,13 @@
   // initializers throws an exception.
   SmallVector cleanups;
   llvm::Instruction *cleanupDominator = nullptr;
+  auto addCleanup = [&](const EHScopeStack::stable_iterator ) {
+cleanups.push_back(cleanup);
+if (!cleanupDominator) // create placeholder once needed
+  cleanupDominator = CGF.Builder.CreateAlignedLoad(
+  CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
+  CharUnits::One());
+  };
 
   unsigned curInitIndex = 0;
 
@@ -1519,7 +1526,7 @@
   if (QualType::DestructionKind dtorKind =
   Base.getType().isDestructedType()) {
 CGF.pushDestroy(dtorKind, V, Base.getType());
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
   }
 }
   }
@@ -1596,15 +1603,9 @@
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
   if (CGF.needsEHCleanup(dtorKind)) {
-if (!cleanupDominator)
-  cleanupDominator = CGF.Builder.CreateAlignedLoad(
-  CGF.Int8Ty,
-  llvm::Constant::getNullValue(CGF.Int8PtrTy),
-  CharUnits::One()); // placeholder
-
 CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(),
 CGF.getDestroyer(dtorKind), false);
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
 pushedCleanup = true;
   }
 }
@@ -1620,6 +1621,8 @@
 
   // Deactivate all the partial cleanups in reverse order, which
   // generally means popping them.
+  assert((cleanupDominator || cleanups.empty()) &&
+

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-13 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

In D64656#1584437 , @oydale wrote:

> My understanding of the issue is that clang emits incorrect IR. Without my 
> fix and when disabling the assertion mentioned in the commit message by 
> commenting it out, llvm-lit gives the following output when executed against 
> the minimal test case in the current version of the commit:
>
>   + /home/maestro/llvm/llvm-project/build/bin/clang -cc1 -internal-isystem 
> /home/maestro/llvm/llvm-project/build/lib/clang/9.0.0/include -nostdsysteminc 
> -emit-obj --std=c++17 -fcxx-exceptions -fexceptions 
> /home/maestro/llvm/llvm-project/clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
>   Instruction referencing instruction not embedded in a basic block!
> %cleanup.isactive = alloca i1, align 1
> store i1 true, i1* %cleanup.isactive, align 1
>   in function __cxx_global_var_init.1
>   fatal error: error in backend: Broken function found, compilation aborted!
>
>
> This is what makes me assume that the IR output is incorrect.


Aha, this i didn't see before, thank you.
Then i agree the clang fix is needed.

> ...

Since some other assertion (in `-verify` pass?) triggers before the SROA crash 
can be reached, i think may be no middle-end issue here after all.




Comment at: 
clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:14-18
+// CHECK: %cleanup.isactive = alloca i1, align 1
+// CHECK: call void @_ZN1RC1E1Q(%struct.R* %ref.tmp)
+// CHECK: store i1 true, i1* %cleanup.isactive, align 1
+// CHECK: invoke void @_ZN1SC1E1Q(%struct.S* %ref.tmp1)
+// CHECK:   to label %invoke.cont unwind label %lpad

2 things:
1. These should be `CHECK-NEXT:`.
2. This will immediately break in release build mode, since the value names 
will be discarded.
   You want to follow 
https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-string-substitution-blocks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-13 Thread Øystein Dale via Phabricator via cfe-commits 
oydale added a comment.

My understanding of the issue is that clang emits incorrect IR. Without my fix 
and when disabling the assertion mentioned in the commit message by commenting 
it out, llvm-lit gives the following output when executed against the minimal 
test case in the current version of the commit:

  + /home/maestro/llvm/llvm-project/build/bin/clang -cc1 -internal-isystem 
/home/maestro/llvm/llvm-project/build/lib/clang/9.0.0/include -nostdsysteminc 
-emit-obj --std=c++17 -fcxx-exceptions -fexceptions 
/home/maestro/llvm/llvm-project/clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
  Instruction referencing instruction not embedded in a basic block!
%cleanup.isactive = alloca i1, align 1
store i1 true, i1* %cleanup.isactive, align 1
  in function __cxx_global_var_init.1
  fatal error: error in backend: Broken function found, compilation aborted!

This is what makes me assume that the IR output is incorrect.

Without my fix and with the assertion still commented out, disabling the LLVM 
verification, and enabling optimizations with -O1, the debug build of clang 
yields a similar crash as what can be observed in release builds such as on 
godbolt:

  ~/llvm/llvm-project/build/bin/clang-9 -cc1 -triple x86_64-pc-linux-gnu 
-emit-obj -disable-free -disable-llvm-verifier -std=c++17 -fexceptions -O1 
-fcxx-exceptions minimal.cpp
  Stack dump:
  0.Program arguments: /home/maestro/llvm/llvm-project/build/bin/clang-9 
-cc1 -triple x86_64-pc-linux-gnu -emit-obj -disable-free -disable-llvm-verifier 
-std=c++17 -fexceptions -O1 -fcxx-exceptions minimal.cpp 
  1. parser at end of file
  2.Per-function optimization
  3.Running pass 'SROA' on function '@__cxx_global_var_init.1'
   #0 0x7f2309f7b7a7 llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
/home/maestro/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:494:22
   #1 0x7f2309f7b83a PrintStackTraceSignalHandler(void*) 
/home/maestro/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:555:1
   #2 0x7f2309f79834 llvm::sys::RunSignalHandlers() 
/home/maestro/llvm/llvm-project/llvm/lib/Support/Signals.cpp:68:20
   #3 0x7f2309f7b1fd SignalHandler(int) 
/home/maestro/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:357:1
   #4 0x7f230929e4d0 __restore_rt (/usr/lib/libpthread.so.0+0x124d0)
   #5 0x7f230be28fda llvm::PointerIntPair*, 1u, 
unsigned int, llvm::PointerLikeTypeTraits*>, 
llvm::PointerIntPairInfo*, 1u, 
llvm::PointerLikeTypeTraits*> > 
>::setPointer(llvm::ilist_node_base*) 
/home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:63:32
   #6 0x7f230be267cb 
llvm::ilist_node_base::setPrev(llvm::ilist_node_base*) 
/home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist_node_base.h:40:75
   #7 0x7f230be3748b 
llvm::ilist_base::removeImpl(llvm::ilist_node_base&) 
/home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist_base.h:34:5
   #8 0x7f230be37725 void 
llvm::ilist_base::remove > 
>(llvm::ilist_node_impl >&) 
/home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist_base.h:80:64
   #9 0x7f230be37166 
llvm::simple_ilist::remove(llvm::Instruction&) 
/home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/simple_ilist.h:183:77
  #10 0x7f230be36aa2 
llvm::iplist_impl, 
llvm::SymbolTableListTraits 
>::remove(llvm::ilist_iterator, false, false>&) 
/home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist.h:253:12
  #11 0x7f230be35bf1 
llvm::iplist_impl, 
llvm::SymbolTableListTraits 
>::erase(llvm::ilist_iterator, false, false>) 
/home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist.h:266:5
  #12 0x7f230bf7bb5d llvm::Instruction::eraseFromParent() 
/home/maestro/llvm/llvm-project/llvm/lib/IR/Instruction.cpp:69:1
  #13 0x7f230a611a47 
llvm::SROA::deleteDeadInstructions(llvm::SmallPtrSetImpl&) 
/home/maestro/llvm/llvm-project/llvm/lib/Transforms/Scalar/SROA.cpp:4520:13
  #14 0x7f230a611df1 llvm::SROA::runImpl(llvm::Function&, 
llvm::DominatorTree&, llvm::AssumptionCache&) 
/home/maestro/llvm/llvm-project/llvm/lib/Transforms/Scalar/SROA.cpp:4564:15
  #15 0x7f230a62979e 
llvm::sroa::SROALegacyPass::runOnFunction(llvm::Function&) 
/home/maestro/llvm/llvm-project/llvm/lib/Transforms/Scalar/SROA.cpp:4620:31
  #16 0x7f230bfc820a llvm::FPPassManager::runOnFunction(llvm::Function&) 
/home/maestro/llvm/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1648:20
  #17 0x7f230bfc7e2c 
llvm::legacy::FunctionPassManagerImpl::run(llvm::Function&) 
/home/maestro/llvm/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1585:13
  #18 0x7f230bfc7a14 
llvm::legacy::FunctionPassManager::run(llvm::Function&) 
/home/maestro/llvm/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1511:1
  #19 0x7f2307ff5d67 (anonymous 
namespace)::EmitAssemblyHelper::EmitAssembly(clang::BackendAction, 
std::unique_ptr >) 
/home/maestro/llvm/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:885:25
  #20 0x7f2307ffa457

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-13 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

In D64656#1584424 , @lebedev.ri wrote:

> Hm, i have a question about this fix.
>  As it can be seen the C++17 code is successfully codegened by clang to LLVM 
> IR, and the actual failure is in LLVM middle-end optimization pass:
>  https://godbolt.org/z/P3RB23
>
> 1. Please file a bug about that pass crash, include that link. It most 
> definitively should not crash.


As i can now see, there is one in https://bugs.llvm.org/show_bug.cgi?id=40771

> 2. Is this fix just workarounding that crash, or is the clang producing 
> incorrect IR without this fix, miscompiling it?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-13 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

Hm, i have a question about this fix.
As it can be seen the C++17 code is successfully codegened by clang to LLVM IR, 
and the actual failure is in LLVM middle-end optimization pass:
https://godbolt.org/z/P3RB23

1. Please file a bug about that pass crash, include that link. It most 
definitively should not crash.
2. Is this fix just workarounding that crash, or is the clang producing 
incorrect IR without this fix, miscompiling it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-13 Thread Øystein Dale via Phabricator via cfe-commits 
oydale added a comment.

I have added my initial attempt at verifying the cleanup code generated in this 
scenario, it's probably going to require modifications. The original IR can be 
found at https://reviews.llvm.org/P8156.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-13 Thread Øystein Dale via Phabricator via cfe-commits 
oydale updated this revision to Diff 209690.
oydale added a comment.

Initial attempt at IR output checks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp

Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -emit-llvm --std=c++17 -fcxx-exceptions -fexceptions \
+// RUN:   %s -o - | FileCheck %s
+
+struct Q { Q(); };
+struct R { R(Q); ~R(); };
+struct S { S(Q); ~S(); };
+struct T : R, S {};
+
+Q q;
+T t { R{q}, S{q} };
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init.1() {{.*}} {
+// CHECK-LABEL: entry:
+// CHECK: %cleanup.isactive = alloca i1, align 1
+// CHECK: call void @_ZN1RC1E1Q(%struct.R* %ref.tmp)
+// CHECK: store i1 true, i1* %cleanup.isactive, align 1
+// CHECK: invoke void @_ZN1SC1E1Q(%struct.S* %ref.tmp1)
+// CHECK:   to label %invoke.cont unwind label %lpad
+
+// CHECK-LABEL: invoke.cont:
+// CHECK: store i1 false, i1* %cleanup.isactive, align 1
+// CHECK: call void @_ZN1SD1Ev(%struct.S*
+// CHECK: call void @_ZN1RD1Ev(%struct.R*
+// CHECK: %0 = call i32 @__cxa_atexit(
+// CHECK: ret void
+
+// CHECK-LABEL: lpad:
+// CHECK: %1 = landingpad { i8*, i32 }
+// CHECK:cleanup
+// CHECK: %2 = extractvalue { i8*, i32 } %1, 0
+// CHECK: store i8* %2, i8** %exn.slot, align 8
+// CHECK: %3 = extractvalue { i8*, i32 } %1, 1
+// CHECK: store i32 %3, i32* %ehselector.slot, align 4
+// CHECK: %cleanup.is_active = load i1, i1* %cleanup.isactive, align 1
+// CHECK: br i1 %cleanup.is_active, label %cleanup.action, label %cleanup.done
+
+// CHECK-LABEL: cleanup.action:
+// CHECK: call void @_ZN1RD1Ev(%struct.R*
+// CHECK: br label %cleanup.done
+
+// CHECK-LABEL: cleanup.done:
+// CHECK: call void @_ZN1RD1Ev(%struct.R* %ref.tmp)
+// CHECK: br label %eh.resume
+
+// CHECK-LABEL: eh.resume:
+// CHECK: %exn = load i8*, i8** %exn.slot, align 8
+// CHECK: %sel = load i32, i32* %ehselector.slot, align 4
+// CHECK: %lpad.val = insertvalue { i8*, i32 } undef, i8* %exn, 0
+// CHECK: %lpad.val3 = insertvalue { i8*, i32 } %lpad.val, i32 %sel, 1
+// CHECK: resume { i8*, i32 } %lpad.val3
+// CHECK: }
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1495,6 +1495,13 @@
   // initializers throws an exception.
   SmallVector cleanups;
   llvm::Instruction *cleanupDominator = nullptr;
+  auto addCleanup = [&](const EHScopeStack::stable_iterator ) {
+cleanups.push_back(cleanup);
+if (!cleanupDominator) // create placeholder once needed
+  cleanupDominator = CGF.Builder.CreateAlignedLoad(
+  CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
+  CharUnits::One());
+  };
 
   unsigned curInitIndex = 0;
 
@@ -1519,7 +1526,7 @@
   if (QualType::DestructionKind dtorKind =
   Base.getType().isDestructedType()) {
 CGF.pushDestroy(dtorKind, V, Base.getType());
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
   }
 }
   }
@@ -1596,15 +1603,9 @@
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
   if (CGF.needsEHCleanup(dtorKind)) {
-if (!cleanupDominator)
-  cleanupDominator = CGF.Builder.CreateAlignedLoad(
-  CGF.Int8Ty,
-  llvm::Constant::getNullValue(CGF.Int8PtrTy),
-  CharUnits::One()); // placeholder
-
 CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(),
 CGF.getDestroyer(dtorKind), false);
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
 pushedCleanup = true;
   }
 }
@@ -1620,6 +1621,8 @@
 
   // Deactivate all the partial cleanups in reverse order, which
   // generally means popping them.
+  assert((cleanupDominator || cleanups.empty()) &&
+ "Missing cleanupDominator before deactivating cleanup blocks");
   for (unsigned i = cleanups.size(); i != 0; --i)
 CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-13 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Sorry, i still don't like the test.
You want to check the produced IR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-13 Thread Øystein Dale via Phabricator via cfe-commits 
oydale updated this revision to Diff 209683.
oydale added a comment.

Replaced -emit-obj with -emit-llvm-only in test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp


Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -emit-llvm-only --std=c++17 -fcxx-exceptions -fexceptions %s
+// PR40771: check that this input does not crash or assert
+
+struct Q { Q(); };
+struct R { R(Q); ~R(); };
+struct S { S(Q); ~S(); };
+struct T : R, S {};
+
+Q q;
+T t { R{q}, S{q} };
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1495,6 +1495,13 @@
   // initializers throws an exception.
   SmallVector cleanups;
   llvm::Instruction *cleanupDominator = nullptr;
+  auto addCleanup = [&](const EHScopeStack::stable_iterator ) {
+cleanups.push_back(cleanup);
+if (!cleanupDominator) // create placeholder once needed
+  cleanupDominator = CGF.Builder.CreateAlignedLoad(
+  CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
+  CharUnits::One());
+  };
 
   unsigned curInitIndex = 0;
 
@@ -1519,7 +1526,7 @@
   if (QualType::DestructionKind dtorKind =
   Base.getType().isDestructedType()) {
 CGF.pushDestroy(dtorKind, V, Base.getType());
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
   }
 }
   }
@@ -1596,15 +1603,9 @@
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
   if (CGF.needsEHCleanup(dtorKind)) {
-if (!cleanupDominator)
-  cleanupDominator = CGF.Builder.CreateAlignedLoad(
-  CGF.Int8Ty,
-  llvm::Constant::getNullValue(CGF.Int8PtrTy),
-  CharUnits::One()); // placeholder
-
 CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(),
 CGF.getDestroyer(dtorKind), false);
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
 pushedCleanup = true;
   }
 }
@@ -1620,6 +1621,8 @@
 
   // Deactivate all the partial cleanups in reverse order, which
   // generally means popping them.
+  assert((cleanupDominator || cleanups.empty()) &&
+ "Missing cleanupDominator before deactivating cleanup blocks");
   for (unsigned i = cleanups.size(); i != 0; --i)
 CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator);
 


Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -emit-llvm-only --std=c++17 -fcxx-exceptions -fexceptions %s
+// PR40771: check that this input does not crash or assert
+
+struct Q { Q(); };
+struct R { R(Q); ~R(); };
+struct S { S(Q); ~S(); };
+struct T : R, S {};
+
+Q q;
+T t { R{q}, S{q} };
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1495,6 +1495,13 @@
   // initializers throws an exception.
   SmallVector cleanups;
   llvm::Instruction *cleanupDominator = nullptr;
+  auto addCleanup = [&](const EHScopeStack::stable_iterator ) {
+cleanups.push_back(cleanup);
+if (!cleanupDominator) // create placeholder once needed
+  cleanupDominator = CGF.Builder.CreateAlignedLoad(
+  CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
+  CharUnits::One());
+  };
 
   unsigned curInitIndex = 0;
 
@@ -1519,7 +1526,7 @@
   if (QualType::DestructionKind dtorKind =
   Base.getType().isDestructedType()) {
 CGF.pushDestroy(dtorKind, V, Base.getType());
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
   }
 }
   }
@@ -1596,15 +1603,9 @@
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
   if (CGF.needsEHCleanup(dtorKind)) {
-if (!cleanupDominator)
-  cleanupDominator = CGF.Builder.CreateAlignedLoad(
-  CGF.Int8Ty,
-  llvm::Constant::getNullValue(CGF.Int8PtrTy),
-  CharUnits::One()); // placeholder
-
 CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(),
 CGF.getDestroyer(dtorKind), false);
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:1
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions %s
+// PR40771: check that this input does not crash or assert

This will run the backend and (I think) drop an unwanted output file somewhere. 
You can use `-emit-llvm-only` instead of `-emit-obj` so that we stop after 
creating the IR and don't actually write it anywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread Øystein Dale via Phabricator via cfe-commits 
oydale updated this revision to Diff 209627.
oydale added a comment.

Minimized test case further, removed misleading CHECK from comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp


Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions %s
+// PR40771: check that this input does not crash or assert
+
+struct Q { Q(); };
+struct R { R(Q); ~R(); };
+struct S { S(Q); ~S(); };
+struct T : R, S {};
+
+Q q;
+T t { R{q}, S{q} };
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1495,6 +1495,13 @@
   // initializers throws an exception.
   SmallVector cleanups;
   llvm::Instruction *cleanupDominator = nullptr;
+  auto addCleanup = [&](const EHScopeStack::stable_iterator ) {
+cleanups.push_back(cleanup);
+if (!cleanupDominator) // create placeholder once needed
+  cleanupDominator = CGF.Builder.CreateAlignedLoad(
+  CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
+  CharUnits::One());
+  };
 
   unsigned curInitIndex = 0;
 
@@ -1519,7 +1526,7 @@
   if (QualType::DestructionKind dtorKind =
   Base.getType().isDestructedType()) {
 CGF.pushDestroy(dtorKind, V, Base.getType());
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
   }
 }
   }
@@ -1596,15 +1603,9 @@
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
   if (CGF.needsEHCleanup(dtorKind)) {
-if (!cleanupDominator)
-  cleanupDominator = CGF.Builder.CreateAlignedLoad(
-  CGF.Int8Ty,
-  llvm::Constant::getNullValue(CGF.Int8PtrTy),
-  CharUnits::One()); // placeholder
-
 CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(),
 CGF.getDestroyer(dtorKind), false);
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
 pushedCleanup = true;
   }
 }
@@ -1620,6 +1621,8 @@
 
   // Deactivate all the partial cleanups in reverse order, which
   // generally means popping them.
+  assert((cleanupDominator || cleanups.empty()) &&
+ "Missing cleanupDominator before deactivating cleanup blocks");
   for (unsigned i = cleanups.size(); i != 0; --i)
 CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator);
 


Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions %s
+// PR40771: check that this input does not crash or assert
+
+struct Q { Q(); };
+struct R { R(Q); ~R(); };
+struct S { S(Q); ~S(); };
+struct T : R, S {};
+
+Q q;
+T t { R{q}, S{q} };
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1495,6 +1495,13 @@
   // initializers throws an exception.
   SmallVector cleanups;
   llvm::Instruction *cleanupDominator = nullptr;
+  auto addCleanup = [&](const EHScopeStack::stable_iterator ) {
+cleanups.push_back(cleanup);
+if (!cleanupDominator) // create placeholder once needed
+  cleanupDominator = CGF.Builder.CreateAlignedLoad(
+  CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
+  CharUnits::One());
+  };
 
   unsigned curInitIndex = 0;
 
@@ -1519,7 +1526,7 @@
   if (QualType::DestructionKind dtorKind =
   Base.getType().isDestructedType()) {
 CGF.pushDestroy(dtorKind, V, Base.getType());
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
   }
 }
   }
@@ -1596,15 +1603,9 @@
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
   if (CGF.needsEHCleanup(dtorKind)) {
-if (!cleanupDominator)
-  cleanupDominator = CGF.Builder.CreateAlignedLoad(
-  CGF.Int8Ty,
-  llvm::Constant::getNullValue(CGF.Int8PtrTy),
-  CharUnits::One()); // placeholder
-
 CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(),
 CGF.getDestroyer(dtorKind), false);
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread Øystein Dale via Phabricator via cfe-commits 
oydale marked an inline comment as done.
oydale added inline comments.



Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:2
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s
+// CHECK no crash
+

rjmccall wrote:
> oydale wrote:
> > lebedev.ri wrote:
> > > Either add missing `:` or please write better comment :)
> > I'm not sure what else that comment would contain. I want the test to 
> > verify that clang does not crash given the provided input. 
> We have a testing tool called FileCheck which is used to pattern-match 
> compiler output, and it uses a lot of lines that look like `CHECK: blah`, so 
> the all-caps CHECK makes people think that it's supposed to be a FileCheck 
> line.  Please just write the comment normally, like "PR4771: check that this 
> doesn't crash."
> 
> Was this crash specific to optimization being enabled?
I'll change it to a normal-looking comment.

Yes, for a release build the crash does not happen unless -O1 or greater is 
specified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread John McCall via Phabricator via cfe-commits 
rjmccall added a comment.

Oh, I see you've already answered that.  I think it's fine to just leave this 
testing debug output if generated optimized output doesn't affect it; the bulk 
of our regression testing is with assertions-enabled compilers anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread John McCall via Phabricator via cfe-commits 
rjmccall added inline comments.



Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:2
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s
+// CHECK no crash
+

oydale wrote:
> lebedev.ri wrote:
> > Either add missing `:` or please write better comment :)
> I'm not sure what else that comment would contain. I want the test to verify 
> that clang does not crash given the provided input. 
We have a testing tool called FileCheck which is used to pattern-match compiler 
output, and it uses a lot of lines that look like `CHECK: blah`, so the 
all-caps CHECK makes people think that it's supposed to be a FileCheck line.  
Please just write the comment normally, like "PR4771: check that this doesn't 
crash."

Was this crash specific to optimization being enabled?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread Øystein Dale via Phabricator via cfe-commits 
oydale marked 2 inline comments as done.
oydale added inline comments.



Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:1
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s
+// CHECK no crash

lebedev.ri wrote:
> lebedev.ri wrote:
> > Why do you need `-O1`?
> > 
> Also, you only check $? Don't you want to check the actual IR?
> 
> 

I wanted the test case to cover both a debug build and a release build of 
clang, hence the use of -O1.

For a release build without asserts, the issue is only visible when building 
with -O1 or higher. For a debug build the optimization level doesn't matter 
because it hits an assert (the one mentioned in the commit message) earlier in 
the compile process.

Using clang++ as provided by my distro and on godbolt the following works fine:
```
clang++ --std=c++17 -c crash.cpp
```

The following crashes:
```
clang++ --std=c++17 -c -O1 crash.cpp
```

I do not have sufficient knowledge of what the IR is expected to look like in 
this case to write a test for it. Further, all I have done is to ensure that a 
placeholder instruction is created when it's needed. This placeholder 
instruction is erased at the end of the function where it was created, so the 
generated IR should already be covered by other tests.



Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:2
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s
+// CHECK no crash
+

lebedev.ri wrote:
> Either add missing `:` or please write better comment :)
I'm not sure what else that comment would contain. I want the test to verify 
that clang does not crash given the provided input. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added inline comments.



Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:1
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s
+// CHECK no crash

Why do you need `-O1`?




Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:1
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s
+// CHECK no crash

lebedev.ri wrote:
> Why do you need `-O1`?
> 
Also, you only check $? Don't you want to check the actual IR?





Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:2
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s
+// CHECK no crash
+

Either add missing `:` or please write better comment :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:4-24
+struct outer
+{
+struct inner
+{
+~inner() {}
+};
+

Please use a more minimal testcase, such as:

```
struct P { ~P(); };
struct Q { Q(); ~Q(); };
struct R : P, Q {};
R c = { P(), Q() }; 
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread Øystein Dale via Phabricator via cfe-commits 
oydale updated this revision to Diff 209585.
oydale added a comment.

Reduce test case to a single execution with -O1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp


Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s
+// CHECK no crash
+
+struct outer
+{
+struct inner
+{
+~inner() {}
+};
+
+outer() : member() {}
+outer(const outer&) : member() {}
+
+inner member;
+};
+
+template  struct ctad : Ts... {};
+template  ctad(Ts...) -> ctad;
+
+void crash()
+{
+outer s;
+ctad c { [s] (int) {}, [s] (short) {} };
+}
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1495,6 +1495,13 @@
   // initializers throws an exception.
   SmallVector cleanups;
   llvm::Instruction *cleanupDominator = nullptr;
+  auto addCleanup = [&](const EHScopeStack::stable_iterator ) {
+cleanups.push_back(cleanup);
+if (!cleanupDominator) // create placeholder once needed
+  cleanupDominator = CGF.Builder.CreateAlignedLoad(
+  CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
+  CharUnits::One());
+  };
 
   unsigned curInitIndex = 0;
 
@@ -1519,7 +1526,7 @@
   if (QualType::DestructionKind dtorKind =
   Base.getType().isDestructedType()) {
 CGF.pushDestroy(dtorKind, V, Base.getType());
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
   }
 }
   }
@@ -1596,15 +1603,9 @@
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
   if (CGF.needsEHCleanup(dtorKind)) {
-if (!cleanupDominator)
-  cleanupDominator = CGF.Builder.CreateAlignedLoad(
-  CGF.Int8Ty,
-  llvm::Constant::getNullValue(CGF.Int8PtrTy),
-  CharUnits::One()); // placeholder
-
 CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(),
 CGF.getDestroyer(dtorKind), false);
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
 pushedCleanup = true;
   }
 }
@@ -1620,6 +1621,8 @@
 
   // Deactivate all the partial cleanups in reverse order, which
   // generally means popping them.
+  assert((cleanupDominator || cleanups.empty()) &&
+ "Missing cleanupDominator before deactivating cleanup blocks");
   for (unsigned i = cleanups.size(); i != 0; --i)
 CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator);
 


Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s
+// CHECK no crash
+
+struct outer
+{
+struct inner
+{
+~inner() {}
+};
+
+outer() : member() {}
+outer(const outer&) : member() {}
+
+inner member;
+};
+
+template  struct ctad : Ts... {};
+template  ctad(Ts...) -> ctad;
+
+void crash()
+{
+outer s;
+ctad c { [s] (int) {}, [s] (short) {} };
+}
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1495,6 +1495,13 @@
   // initializers throws an exception.
   SmallVector cleanups;
   llvm::Instruction *cleanupDominator = nullptr;
+  auto addCleanup = [&](const EHScopeStack::stable_iterator ) {
+cleanups.push_back(cleanup);
+if (!cleanupDominator) // create placeholder once needed
+  cleanupDominator = CGF.Builder.CreateAlignedLoad(
+  CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
+  CharUnits::One());
+  };
 
   unsigned curInitIndex = 0;
 
@@ -1519,7 +1526,7 @@
   if (QualType::DestructionKind dtorKind =
   Base.getType().isDestructedType()) {
 CGF.pushDestroy(dtorKind, V, Base.getType());
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
   }
 }
   }
@@ -1596,15 +1603,9 @@
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
   if (CGF.needsEHCleanup(dtorKind)) {
-if (!cleanupDominator)
-  cleanupDominator = CGF.Builder.CreateAlignedLoad(
-  CGF.Int8Ty,
-  llvm::Constant::getNullValue(CGF.Int8PtrTy),
-

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread Øystein Dale via Phabricator via cfe-commits 
oydale updated this revision to Diff 209584.
oydale added a comment.

Added regression test based on code in bug report
Moved logic to a lambda


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp


Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O0 %s
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O0 %s
+// CHECK no crash
+
+struct outer
+{
+struct inner
+{
+~inner() {}
+};
+
+outer() : member() {}
+outer(const outer&) : member() {}
+
+inner member;
+};
+
+template  struct ctad : Ts... {};
+template  ctad(Ts...) -> ctad;
+
+void crash()
+{
+outer s;
+ctad c { [s] (int) {}, [s] (short) {} };
+}
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1495,6 +1495,13 @@
   // initializers throws an exception.
   SmallVector cleanups;
   llvm::Instruction *cleanupDominator = nullptr;
+  auto addCleanup = [&](const EHScopeStack::stable_iterator ) {
+cleanups.push_back(cleanup);
+if (!cleanupDominator) // create placeholder once needed
+  cleanupDominator = CGF.Builder.CreateAlignedLoad(
+  CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
+  CharUnits::One());
+  };
 
   unsigned curInitIndex = 0;
 
@@ -1519,7 +1526,7 @@
   if (QualType::DestructionKind dtorKind =
   Base.getType().isDestructedType()) {
 CGF.pushDestroy(dtorKind, V, Base.getType());
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
   }
 }
   }
@@ -1596,15 +1603,9 @@
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
   if (CGF.needsEHCleanup(dtorKind)) {
-if (!cleanupDominator)
-  cleanupDominator = CGF.Builder.CreateAlignedLoad(
-  CGF.Int8Ty,
-  llvm::Constant::getNullValue(CGF.Int8PtrTy),
-  CharUnits::One()); // placeholder
-
 CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(),
 CGF.getDestroyer(dtorKind), false);
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
 pushedCleanup = true;
   }
 }
@@ -1620,6 +1621,8 @@
 
   // Deactivate all the partial cleanups in reverse order, which
   // generally means popping them.
+  assert((cleanupDominator || cleanups.empty()) &&
+ "Missing cleanupDominator before deactivating cleanup blocks");
   for (unsigned i = cleanups.size(); i != 0; --i)
 CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator);
 


Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O0 %s
+// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O0 %s
+// CHECK no crash
+
+struct outer
+{
+struct inner
+{
+~inner() {}
+};
+
+outer() : member() {}
+outer(const outer&) : member() {}
+
+inner member;
+};
+
+template  struct ctad : Ts... {};
+template  ctad(Ts...) -> ctad;
+
+void crash()
+{
+outer s;
+ctad c { [s] (int) {}, [s] (short) {} };
+}
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1495,6 +1495,13 @@
   // initializers throws an exception.
   SmallVector cleanups;
   llvm::Instruction *cleanupDominator = nullptr;
+  auto addCleanup = [&](const EHScopeStack::stable_iterator ) {
+cleanups.push_back(cleanup);
+if (!cleanupDominator) // create placeholder once needed
+  cleanupDominator = CGF.Builder.CreateAlignedLoad(
+  CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
+  CharUnits::One());
+  };
 
   unsigned curInitIndex = 0;
 
@@ -1519,7 +1526,7 @@
   if (QualType::DestructionKind dtorKind =
   Base.getType().isDestructedType()) {
 CGF.pushDestroy(dtorKind, V, Base.getType());
-cleanups.push_back(CGF.EHStack.stable_begin());
+addCleanup(CGF.EHStack.stable_begin());
   }
 }
   }
@@ -1596,15 +1603,9 @@
   = field->getType().isDestructedType()) {
   assert(LV.isSimple());
   if (CGF.needsEHCleanup(dtorKind)) {
-

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread John McCall via Phabricator via cfe-commits 
rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:1627
+llvm::Constant::getNullValue(CGF.Int8PtrTy),
+CharUnits::One()); // placeholder
+

Please declare a lambda to add a cleanup which implicitly creates this 
dominator and then just call it in the places that currently directly do 
`cleanups.push_back`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

Test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64656



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

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread Øystein Dale via Phabricator via cfe-commits 
oydale created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

A placeholder instruction for use in generation of cleanup code for an
initializer list would not be emitted if the base class contained a
non-trivial destructor and the class contains no fields of its own. This
would be the case when using CTAD to deduce the template arguments for a
struct with an overloaded call operator, e.g.

template  struct ctad : Ts... {};
template  ctad(Ts...)->ctad;

and this class was initialized with a list of lambdas capturing by copy,
e.g.

  ctad c {[s](short){}, [s](long){}};

In a release build the bug would manifest itself as a crash in the SROA
pass, however, in a debug build the following assert in CGCleanup.cpp
would fail:

  assert(dominatingIP && "no existing variable and no dominating IP!");

By ensuring that a placeholder instruction is emitted even if there's no
fields in the class, neither the assert nor the crash is reproducible.

See https://bugs.llvm.org/show_bug.cgi?id=40771


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64656

Files:
  clang/lib/CodeGen/CGExprAgg.cpp


Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1618,6 +1618,14 @@
   GEP->eraseFromParent();
   }
 
+  // Base class has a destructor and there's no fields in the class itself,
+  // create a placeholder here since we did not create one earlier.
+  if (!cleanupDominator && !cleanups.empty())
+cleanupDominator = CGF.Builder.CreateAlignedLoad(
+CGF.Int8Ty,
+llvm::Constant::getNullValue(CGF.Int8PtrTy),
+CharUnits::One()); // placeholder
+
   // Deactivate all the partial cleanups in reverse order, which
   // generally means popping them.
   for (unsigned i = cleanups.size(); i != 0; --i)


Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -1618,6 +1618,14 @@
   GEP->eraseFromParent();
   }
 
+  // Base class has a destructor and there's no fields in the class itself,
+  // create a placeholder here since we did not create one earlier.
+  if (!cleanupDominator && !cleanups.empty())
+cleanupDominator = CGF.Builder.CreateAlignedLoad(
+CGF.Int8Ty,
+llvm::Constant::getNullValue(CGF.Int8PtrTy),
+CharUnits::One()); // placeholder
+
   // Deactivate all the partial cleanups in reverse order, which
   // generally means popping them.
   for (unsigned i = cleanups.size(); i != 0; --i)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits