Page MenuHomePhabricator

[Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter
ClosedPublic

Authored by lxfind on Oct 8 2020, 1:10 PM.

Details

Summary

In https://reviews.llvm.org/D87470 I added the change to tighten the lifetime of the expression awaiter.await_suspend().address.
Howver it was incorrect. ExprWithCleanups will call the dtor and end the lifetime for all the temps created in the current full expr.
When this is called on a normal await call, we don't want to do that.
We only want to do this for the call on the final_awaiter, to avoid writing into the frame after the frame is destroyed.
This change fixes it, by checking IsImplicit.

Diff Detail

Event Timeline

lxfind created this revision.Oct 8 2020, 1:10 PM
lxfind requested review of this revision.Oct 8 2020, 1:10 PM

why we should not do this with normal await call?

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

I'm not familiar with ASAN instrumentation. Do you have any testcases to explain this?

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

I'm not familiar with ASAN instrumentation. Do you have any testcases to explain this?

Unfortunately I don't. But this is not related to ASAN. Basically, this is causing destructing of objects that should still be alive. I suspect that it's because ExprWithCleanups always clean up temps that belongs to the full expression, not just the sub-expression in it.

wenlei accepted this revision.Oct 12 2020, 11:28 AM

LGTM. I think we can get this in first to address the fall out from https://reviews.llvm.org/D87470, while investigating ASAN failure.

clang/lib/Sema/SemaCoroutine.cpp
401–411

Can you add comment explaining why we don't cleanup for all await, probably a TODO?

This revision is now accepted and ready to land.Oct 12 2020, 11:28 AM
lxfind updated this revision to Diff 297656.Oct 12 2020, 11:59 AM

Add more comments and TODO

This revision was landed with ongoing or failed builds.Oct 12 2020, 12:00 PM
This revision was automatically updated to reflect the committed changes.
lxfind added a comment.EditedOct 12 2020, 12:45 PM

There seems to be build failures in the buildbot, but I don't understand why it's happening.. (unable to repro locally and the patterns seem correct)
http://lab.llvm.org:8011/#/builders/12/builds/92/steps/7/logs/FAIL__Clang__coro-semmetric-transfer_cpp

Test failures are being fixed in https://reviews.llvm.org/D89269.

why we should not do this with normal await call?

To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment.
But first of all, without adding any cleanup expression here, I saw ASAN failures due to heap-use-after-free, because sometimes the frame have already been destroyed after the await_suspend call, and yet we are still writing into the frame due to unnecessarily cross-suspend lifetime. However, if I apply the cleanup to all await_suepend calls, it also causes ASAN failures as it's cleaning up data that's still alive.
So this patch is more of a temporary walkaround to stop bleeding without causing any trouble.
I plan to get back to this latter after I am done with the spilling/alloca issues.

I'm not familiar with ASAN instrumentation. Do you have any testcases to explain this?

Unfortunately I don't. But this is not related to ASAN. Basically, this is causing destructing of objects that should still be alive. I suspect that it's because ExprWithCleanups always clean up temps that belongs to the full expression, not just the sub-expression in it.

should we also need to add ExprWithCleanups when expand co_await to await_ready & await_suspend and await_resume expression which may fix this issue?