[PATCH] D144680: [Coroutines] Avoid creating conditional cleanup markers in suspend block

[Wei Wang via Phabricator via cfe-commits]

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