weiwang added a comment.
In D144680#4149149 <https://reviews.llvm.org/D144680#4149149>, @ChuanqiXu wrote:
> BTW, what is the conclusion for the concern about sanitizers? Would change
> make sanitizers to perform false positive diagnostics?
This change is limited to the `await.suspend` block, which only does codegen
for `awaiter.await_suspend(coroutine_handle<p>::from_promise(p))`. In this
case, I think the only asan check removed by the change is the conditional
marker for cleanup the temporary `coroutine_handler` used as the parameter of
`await_suspend`, so my understanding is that it shouldn't affect asan
diagnostic.
To show the difference it makes to the source from issue #59181
Before:
%ref.tmp25 = alloca %"struct.std::__1::coroutine_handle.6", align 8
...
// asan check inserted here
bool cond_cleanup_marker = false;
if (cond) {
// get awaiter
...
if (!awaiter.await_ready()) {
call void @llvm.lifetime.start.p0(i64 8, ptr %ref.tmp25)
// asan check inserted here
cond_cleanup_marker = true;
// store handler to %ref.tmp25
...
awaiter.await_suspend();
// asan check inserted here
if (cond_cleanup_marker)
call void @llvm.lifetime.end.p0(i64 8, ptr %ref.tmp25)
call i8 @llvm.coro.suspend(...)
...
}
...
} else {
...
}
...
lpad:
...
// asan check inserted here
if (cond_cleanup_marker)
call void @llvm.lifetime.end.p0(i64 8, ptr %ref.tmp25)
The issue is only reproduced in `-O0`, because ``cond_cleanup_marker` is
optimized away in `-O1` or up opt level.
After:
%ref.tmp25 = alloca %"struct.std::__1::coroutine_handle.6", align 8
...
call void @llvm.lifetime.start.p0(i64 8, ptr %ref.tmp25)
if (cond) {
...
if (!awaiter.await_ready()) {
// store handler to %ref.tmp25
...
awaiter.await_suspend();
call void @llvm.lifetime.end.p0(i64 8, ptr %ref.tmp25)
call i8 @llvm.coro.suspend(...)
...
}
...
} else {
...
}
...
lpad:
call void @llvm.lifetime.end.p0(i64 8, ptr %ref.tmp25)
This also matches with the IR without asan.
================
Comment at: clang/lib/CodeGen/CGExpr.cpp:544
// so that it's unconditional. Don't do this with sanitizers which need
// more precise lifetime marks.
ConditionalEvaluation *OldConditional = nullptr;
----------------
ChuanqiXu wrote:
> We should add comment to explain why we add a forward path for coroutines.
Will do
================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:336
std::unique_ptr<CGCoroData> Data;
+ bool InSuspendBlock = false;
CGCoroInfo();
----------------
bruno wrote:
> Should this live inside CGCoroData instead?
`CGCoroData` is forward declared here, so it does not know what's inside.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144680/new/
https://reviews.llvm.org/D144680
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
