This is an archive of the discontinued LLVM Phabricator instance.

Ensure placeholder instruction for cleanup is created
AbandonedPublic

Authored by oydale on Jul 12 2019, 12:06 PM.

Details

Summary

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 <class... Ts> struct ctad : Ts... {};
template <class... Ts> ctad(Ts...)->ctad<Ts...>;

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

Event Timeline

oydale created this revision.Jul 12 2019, 12:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 12:06 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
oydale edited the summary of this revision. (Show Details)Jul 12 2019, 12:10 PM
oydale added reviewers: rsmith, rjmccall.
rjmccall added inline comments.Jul 12 2019, 12:33 PM
clang/lib/CodeGen/CGExprAgg.cpp
1628

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.

oydale updated this revision to Diff 209584.Jul 12 2019, 1:53 PM

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

oydale marked an inline comment as done.Jul 12 2019, 1:54 PM
oydale updated this revision to Diff 209585.Jul 12 2019, 1:57 PM

Reduce test case to a single execution with -O1

rsmith added inline comments.Jul 12 2019, 2:10 PM
clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
5–25

Please use a more minimal testcase, such as:

struct P { ~P(); };
struct Q { Q(); ~Q(); };
struct R : P, Q {};
R c = { P(), Q() };
lebedev.ri added inline comments.Jul 12 2019, 2:16 PM
clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
2

Why do you need -O1?

2

Also, you only check $? Don't you want to check the actual IR?

3

Either add missing : or please write better comment :)

oydale marked 2 inline comments as done.Jul 12 2019, 2:46 PM
oydale added inline comments.
clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
2

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.

3

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.

rjmccall added inline comments.Jul 12 2019, 3:01 PM
clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
3

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?

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.

oydale marked an inline comment as done.Jul 12 2019, 3:04 PM
oydale added inline comments.
clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
3

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.

oydale updated this revision to Diff 209627.Jul 12 2019, 3:59 PM

Minimized test case further, removed misleading CHECK from comment

oydale marked 4 inline comments as done.Jul 12 2019, 4:01 PM
This revision is now accepted and ready to land.Jul 12 2019, 4:13 PM
rsmith added inline comments.Jul 12 2019, 5:50 PM
clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
2

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.

oydale updated this revision to Diff 209683.Jul 13 2019, 5:22 AM

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

oydale marked an inline comment as done.Jul 13 2019, 5:23 AM
lebedev.ri requested changes to this revision.Jul 13 2019, 5:31 AM

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

This revision now requires changes to proceed.Jul 13 2019, 5:31 AM
oydale updated this revision to Diff 209690.Jul 13 2019, 7:01 AM

Initial attempt at IR output checks

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.

oydale marked an inline comment as done.Jul 13 2019, 7:06 AM

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?

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

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

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.	<eof> parser at end of file
2.	Per-function optimization
3.	Running pass 'SROA' on function '@__cxx_global_var_init.1'
 #0 0x00007f2309f7b7a7 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/maestro/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:494:22
 #1 0x00007f2309f7b83a PrintStackTraceSignalHandler(void*) /home/maestro/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:555:1
 #2 0x00007f2309f79834 llvm::sys::RunSignalHandlers() /home/maestro/llvm/llvm-project/llvm/lib/Support/Signals.cpp:68:20
 #3 0x00007f2309f7b1fd SignalHandler(int) /home/maestro/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:357:1
 #4 0x00007f230929e4d0 __restore_rt (/usr/lib/libpthread.so.0+0x124d0)
 #5 0x00007f230be28fda llvm::PointerIntPair<llvm::ilist_node_base<true>*, 1u, unsigned int, llvm::PointerLikeTypeTraits<llvm::ilist_node_base<true>*>, llvm::PointerIntPairInfo<llvm::ilist_node_base<true>*, 1u, llvm::PointerLikeTypeTraits<llvm::ilist_node_base<true>*> > >::setPointer(llvm::ilist_node_base<true>*) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:63:32
 #6 0x00007f230be267cb llvm::ilist_node_base<true>::setPrev(llvm::ilist_node_base<true>*) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist_node_base.h:40:75
 #7 0x00007f230be3748b llvm::ilist_base<true>::removeImpl(llvm::ilist_node_base<true>&) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist_base.h:34:5
 #8 0x00007f230be37725 void llvm::ilist_base<true>::remove<llvm::ilist_node_impl<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void> > >(llvm::ilist_node_impl<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void> >&) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist_base.h:80:64
 #9 0x00007f230be37166 llvm::simple_ilist<llvm::Instruction>::remove(llvm::Instruction&) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/simple_ilist.h:183:77
#10 0x00007f230be36aa2 llvm::iplist_impl<llvm::simple_ilist<llvm::Instruction>, llvm::SymbolTableListTraits<llvm::Instruction> >::remove(llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void>, false, false>&) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist.h:253:12
#11 0x00007f230be35bf1 llvm::iplist_impl<llvm::simple_ilist<llvm::Instruction>, llvm::SymbolTableListTraits<llvm::Instruction> >::erase(llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void>, false, false>) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist.h:266:5
#12 0x00007f230bf7bb5d llvm::Instruction::eraseFromParent() /home/maestro/llvm/llvm-project/llvm/lib/IR/Instruction.cpp:69:1
#13 0x00007f230a611a47 llvm::SROA::deleteDeadInstructions(llvm::SmallPtrSetImpl<llvm::AllocaInst*>&) /home/maestro/llvm/llvm-project/llvm/lib/Transforms/Scalar/SROA.cpp:4520:13
#14 0x00007f230a611df1 llvm::SROA::runImpl(llvm::Function&, llvm::DominatorTree&, llvm::AssumptionCache&) /home/maestro/llvm/llvm-project/llvm/lib/Transforms/Scalar/SROA.cpp:4564:15
#15 0x00007f230a62979e llvm::sroa::SROALegacyPass::runOnFunction(llvm::Function&) /home/maestro/llvm/llvm-project/llvm/lib/Transforms/Scalar/SROA.cpp:4620:31
#16 0x00007f230bfc820a llvm::FPPassManager::runOnFunction(llvm::Function&) /home/maestro/llvm/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1648:20
#17 0x00007f230bfc7e2c llvm::legacy::FunctionPassManagerImpl::run(llvm::Function&) /home/maestro/llvm/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1585:13
#18 0x00007f230bfc7a14 llvm::legacy::FunctionPassManager::run(llvm::Function&) /home/maestro/llvm/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1511:1
#19 0x00007f2307ff5d67 (anonymous namespace)::EmitAssemblyHelper::EmitAssembly(clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) /home/maestro/llvm/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:885:25
#20 0x00007f2307ffa457 clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) /home/maestro/llvm/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1516:27
#21 0x00007f230845c506 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) /home/maestro/llvm/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:303:24
#22 0x00007f2302b1f7c3 clang::ParseAST(clang::Sema&, bool, bool) /home/maestro/llvm/llvm-project/clang/lib/Parse/ParseAST.cpp:178:14
#23 0x00007f2307749f4f clang::ASTFrontendAction::ExecuteAction() /home/maestro/llvm/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1041:11
#24 0x00007f2308458e65 clang::CodeGenAction::ExecuteAction() /home/maestro/llvm/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:1060:1
#25 0x00007f23077498b0 clang::FrontendAction::Execute() /home/maestro/llvm/llvm-project/clang/lib/Frontend/FrontendAction.cpp:938:38
#26 0x00007f23076d1d84 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /home/maestro/llvm/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:944:42
#27 0x00007f230736f70f clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /home/maestro/llvm/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:291:38
#28 0x00005583d3c15863 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /home/maestro/llvm/llvm-project/clang/tools/driver/cc1_main.cpp:249:40
#29 0x00005583d3c0af31 ExecuteCC1Tool(llvm::ArrayRef<char const*>, llvm::StringRef) /home/maestro/llvm/llvm-project/clang/tools/driver/driver.cpp:309:64
#30 0x00005583d3c0b60f main /home/maestro/llvm/llvm-project/clang/tools/driver/driver.cpp:381:26
#31 0x00007f2306860ce3 __libc_start_main (/usr/lib/libc.so.6+0x23ce3)
#32 0x00005583d3c096ee _start (/home/maestro/llvm/llvm-project/build/bin/clang-9+0x426ee)
[1]    23467 segmentation fault (core dumped)  ~/llvm/llvm-project/build/bin/clang-9 -cc1 -triple x86_64-pc-linux-gnu

With my patch (and the assert not commented out), the test case passes, and there is no segfault in the SROA pass either using the command from the latter code block. With my patch applied, I'm also able to compile and execute the original test input I had with CTAD and lambdas using copy capture and get the expected output.

I wholeheartedly agree that there's two distinct issues at play here, one being the incorrect IR output from clang and the other being the crash in the SROA pass. My patch merely fixes the IR output from clang, making it less likely to trigger the crash in the SROA pass. The crash in the SROA pass should also be fixed, I personally would prefer to keep that separate from this commit.

lebedev.ri resigned from this revision.Jul 13 2019, 8:34 AM

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.

clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
15–19

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
This revision is now accepted and ready to land.Jul 13 2019, 8:34 AM
oydale updated this revision to Diff 209701.Jul 13 2019, 9:41 AM

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

oydale marked an inline comment as done.Jul 13 2019, 9:42 AM
oydale accepted this revision.Jul 13 2019, 10:17 AM

Feel free to commit this patch on my behalf.

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

compnerd closed this revision.Jul 25 2019, 10:59 AM
compnerd added a subscriber: compnerd.

SVN r367042

jfb reopened this revision.Jul 25 2019, 1:50 PM
jfb added a subscriber: jfb.

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]])
               ^
<stdin>:37:2: note: scanning from here
 %8 = call %struct.R* @_ZN1RC1E1Q(%struct.R* %1)
 ^
<stdin>:37:2: note: with "TMP_R" equal to "%1"
 %8 = call %struct.R* @_ZN1RC1E1Q(%struct.R* %1)
 ^
<stdin>:37:17: note: possible intended match here
 %8 = call %struct.R* @_ZN1RC1E1Q(%struct.R* %1)
                ^
This revision is now accepted and ready to land.Jul 25 2019, 1:50 PM
jfb requested changes to this revision.Jul 25 2019, 1:50 PM
This revision now requires changes to proceed.Jul 25 2019, 1:50 PM
oydale abandoned this revision.Jul 27 2019, 12:42 PM

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