Page MenuHomePhabricator

[Coroutines] PR34897: Fix incorrect elisions
ClosedPublic

Authored by modocache on Feb 13 2018, 8:47 AM.

Details

Summary

https://bugs.llvm.org/show_bug.cgi?id=34897 demonstrates an incorrect
coroutine frame allocation elision in the coro-elide pass. The elision
is performed on the basis that the SSA variables from all llvm.coro.begin
are directly referenced in subsequent llvm.coro.destroy instructions.

However, this ignores the fact that the function may exit through paths
that do not run these destroy instructions. In the sample program from
PR34897, for example, the llvm.coro.destroy instruction is only
executed in exception handling code. When the coroutine function exits
normally, llvm.coro.destroy is not called. Eliding the allocation in
this case causes a subsequent reference to the coroutine handle from
outside of the function to access freed memory.

To fix the issue, when finding an llvm.coro.destroy for each llvm.coro.begin,
only consider llvm.coro.destroy that are executed along non-exceptional paths.

Test Plan:

  1. Download the sample program from https://bugs.llvm.org/show_bug.cgi?id=34897, compile it with clang++ -fcoroutines-ts -stdlib=libc++ -std=c++1z -O2, and run it. It should print "run1\ncheck1\nrun2\ncheck2" and then exit successfully.
  2. Compile https://godbolt.org/g/mCKfnr and confirm it is still optimized to a single instruction, 'return 1190'.
  3. check-llvm

Diff Detail

Event Timeline

modocache created this revision.Feb 13 2018, 8:47 AM

Friendly ping! Let me know if this seems totally wrong to you, @GorNishanov :)

modocache updated this revision to Diff 137544.Mar 7 2018, 9:03 PM

Just tidying up one line with clang-format. I guess everyone's busy preparing for the Jacksonville standards meeting. Please give this a review when you all have a second -- in addition to the reported bug, https://bugs.llvm.org/show_bug.cgi?id=34897, I've also confirmed that this diff fixes ASAN errors that surfaced in a project at work. Thanks! :)

GorNishanov requested changes to this revision.Mar 30 2018, 11:10 AM

I believe that this check is too aggressive. It prevents http://godbolt.org/g/26viuZ from optimizing as well.
I think a distinction should be made for exceptional paths and happy path.
We need to make sure that we call coro.destroy on all happy paths.

I did not page-in everything I thought about the when_all_test bug, but, I scribbled somewhere this note about it: "Ignore coro.destroy on exceptional paths for the purpose of heap elision".

This revision now requires changes to proceed.Mar 30 2018, 11:10 AM
modocache updated this revision to Diff 145491.May 7 2018, 9:45 AM
modocache edited the summary of this revision. (Show Details)

Sorry it took me a while to get around to this, but I think this new approach fits more along the lines of what you were thinking, @GorNishanov. I also confirmed that it optimizes the program you linked to just as well as before.

This revision is now accepted and ready to land.May 10 2018, 6:46 PM
This revision was automatically updated to reflect the committed changes.

Hooray! Thanks again for the review, @GorNishanov.