[Coroutines] Insert lifetime intrinsics even O0 is used

[junparser]

Summary

As issue (invalid access caused by multithread coroutine) described in https://reviews.llvm.org/D75664, it also happens when await_suspend is attached with always_inline attribute. This because alwaysinliner pass does not insert lifetime intrinsics in O0 while in corosplit pass we depend on these intrinsics to prevent the promotion from alloca to frame.

This patch just enable to insert lifetime intrinsics when coroutine function is build with O0

TestPlan: check-clang; check-llvm; cppcoro

[modocache]

Nice, thank you. I like the test case you added, it feels more reduced than the one in D76118 -- even if it does use long type names such as awaitable, they make the test case easy to understand. But I had a suggestion to expand upon the test case that I think would help this patch.

clang/lib/CodeGen/BackendUtil.cpp

1181

Two small nit-picks:

• "However, we need to insert" -- lowecase "w" in "we", add "to"
• "caused by multithreaded coroutines" -- add "ed" to "multithreaded", pluralize "coroutine"

clang/test/CodeGenCoroutines/coro-always-inline.cpp

44

I feel these checks may not be explicit enough, since this would pass as long as lifetime intrinsics appear in any coroutine funclet. Could you expanded upon these to check that they mark the start and end of the correct pointer, instead of only checking that calls to the intrinsic exist?

[modocache]

clang/lib/CodeGen/BackendUtil.cpp

1183

These past few months I haven't had great luck attempting to summon @chandlerc for patch review, but I do feel it would be good to get someone else's opinion on whether lifetime markers ought to be used here. Is there a good reason why these were disabled? @wenlei, maybe you know, or know of someone who could review?

[junparser]

clang/lib/CodeGen/BackendUtil.cpp

1181

Thanks for your careful review!

1183

AFAIK, lifetime markers is needed by stack-coloring optimization in codegen. However, this patch is also disabled in O0, and the different behavior in opt with O0 (lifetime insertion is enabled) also confuse me.

clang/test/CodeGenCoroutines/coro-always-inline.cpp

44

yep, I'll

[junparser]

address the comments.

[junparser]

hi, @leonardchan, It seems that you disabled lifetime insertion at https://reviews.llvm.org/D63153. Is there any other reason to disable it beyond fix testcases? Also, could you please review this? we need lifetime check to prevent potential ‘use-after-free’ issue described at https://reviews.llvm.org/D75664

[leonardchan]

In D76119#1936439, @junparser wrote:

hi, @leonardchan, It seems that you disabled lifetime insertion at https://reviews.llvm.org/D63153. Is there any other reason to disable it beyond fix testcases? Also, could you please review this? we need lifetime check to prevent potential ‘use-after-free’ issue described at https://reviews.llvm.org/D75664

The main reason was just for the test fixes and parity with the legacy PM which didn't seem to include these intrinsics at O0. Beyond that, there was no real technical reason for disabling it.

LGTM in regards to adding them if LangOpts.Coroutines is set, but do make sure everyone else's comments are addressed.

clang/lib/CodeGen/BackendUtil.cpp

1183

My best guess is that opt has different behavior because the always inliner is added with lifetimes inserted by default (https://github.com/llvm/llvm-project/blob/ac1d23ed7de01fb3a18b340536842a419b504d86/llvm/tools/opt/opt.cpp#L404), but I can't seem to find where the pass would be added for the opt O0 new PM pipeline.

[junparser]

In D76119#1937630, @leonardchan wrote:

In D76119#1936439, @junparser wrote:

hi, @leonardchan, It seems that you disabled lifetime insertion at https://reviews.llvm.org/D63153. Is there any other reason to disable it beyond fix testcases? Also, could you please review this? we need lifetime check to prevent potential ‘use-after-free’ issue described at https://reviews.llvm.org/D75664

The main reason was just for the test fixes and parity with the legacy PM which didn't seem to include these intrinsics at O0. Beyond that, there was no real technical reason for disabling it.

LGTM in regards to adding them if LangOpts.Coroutines is set, but do make sure everyone else's comments are addressed.

Thanks for your review!

clang/lib/CodeGen/BackendUtil.cpp

1183

It seems always inliner pass need to be added in parseModulePassPipeline for opt O0 new PM pipeline.

[junparser]

update testcase.

clang/lib/CodeGen/BackendUtil.cpp

MPM.add(createLowerTypeTestsPass(/*ExportSummary=*/nullptr,
                                      /*ImportSummary=*/nullptr,
                                      /*DropTypeTests=*/true));
 
   PassManagerBuilderWrapper PMBuilder(TargetTriple, CodeGenOpts, LangOpts);
 
   // At O0 and O1 we only run the always inliner which is more efficient. At
   // higher optimization levels we run the normal inliner.
   if (CodeGenOpts.OptimizationLevel <= 1) {
-    bool InsertLifetimeIntrinsics = (CodeGenOpts.OptimizationLevel != 0 &&
-                                     !CodeGenOpts.DisableLifetimeMarkers);
+    bool InsertLifetimeIntrinsics = ((CodeGenOpts.OptimizationLevel != 0 &&
+                                      !CodeGenOpts.DisableLifetimeMarkers) ||
+                                     LangOpts.Coroutines);
     PMBuilder.Inliner = createAlwaysInlinerLegacyPass(InsertLifetimeIntrinsics);
   } else {
     // We do not want to inline hot callsites for SamplePGO module-summary build
     // because profile annotation will happen again in ThinLTO backend, and we
     // want the IR of the hot path to match the profile.
     PMBuilder.Inliner = createFunctionInliningPass(
         CodeGenOpts.OptimizationLevel, CodeGenOpts.OptimizeSize,
         (!CodeGenOpts.SampleProfileFile.empty() &&
if (CodeGenOpts.OptimizationLevel == 0) {
       if (Optional<InstrProfOptions> Options =
               getInstrProfOptions(CodeGenOpts, LangOpts))
         MPM.addPass(InstrProfiling(*Options, false));
 
       // Build a minimal pipeline based on the semantics required by Clang,
       // which is just that always inlining occurs. Further, disable generating
       // lifetime intrinsics to avoid enabling further optimizations during
       // code generation.
-      MPM.addPass(AlwaysInlinerPass(/*InsertLifetimeIntrinsics=*/false));
+      // However, we need to insert lifetime intrinsics to avoid invalid access
+      // caused by multithreaded coroutines.
+      MPM.addPass(
+          AlwaysInlinerPass(/*InsertLifetimeIntrinsics=*/LangOpts.Coroutines));
 
       // At -O0, we can still do PGO. Add all the requested passes for
       // instrumentation PGO, if requested.
       if (PGOOpt && (PGOOpt->Action == PGOOptions::IRInstr ||
                      PGOOpt->Action == PGOOptions::IRUse))
         PB.addPGOInstrPassesForO0(
             MPM, CodeGenOpts.DebugPassManager,
             /* RunProfileGen */ (PGOOpt->Action == PGOOptions::IRInstr),

clang/test/CodeGenCoroutines/coro-always-inline.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fcoroutines-ts \
+// RUN:   -fexperimental-new-pass-manager -O0 %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fcoroutines-ts \
+// RUN:   -fexperimental-new-pass-manager -fno-inline -O0 %s -o - | FileCheck %s
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fcoroutines-ts \
+// RUN:   -O0 %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fcoroutines-ts \
+// RUN:   -fno-inline -O0 %s -o - | FileCheck %s
+
+namespace std {
+namespace experimental {
+
+struct handle {};
+
+struct awaitable {
+  bool await_ready() { return true; }
+  // CHECK-NOT: await_suspend
+  inline void __attribute__((__always_inline__)) await_suspend(handle) {}
+  bool await_resume() { return true; }
+};
+
+template <typename T>
+struct coroutine_handle {
+  static handle from_address(void *address) { return {}; }
+};
+
+template <typename T = void>
+struct coroutine_traits {
+  struct promise_type {
+    awaitable initial_suspend() { return {}; }
+    awaitable final_suspend() { return {}; }
+    void return_void() {}
+    T get_return_object() { return T(); }
+    void unhandled_exception() {}
+  };
+};
+} // namespace experimental
+} // namespace std
+
+// CHECK-LABEL: @_Z3foov
+// CHECK-LABEL: entry:
+// CHECK-NEXT: %this.addr.i{{[0-9]*}} = alloca %"struct.std::experimental::awaitable"*, align 8
+// CHECK-NEXT: %this.addr.i{{[0-9]*}} = alloca %"struct.std::experimental::awaitable"*, align 8
+// CHECK: [[CAST0:%[0-9]+]] = bitcast %"struct.std::experimental::awaitable"** %this.addr.i{{[0-9]*}} to i8*
+// CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 8, i8* [[CAST0]])
+// CHECK: [[CAST1:%[0-9]+]] = bitcast %"struct.std::experimental::awaitable"** %this.addr.i{{[0-9]*}} to i8*
+// CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 8, i8* [[CAST1]])
+
+// CHECK: [[CAST2:%[0-9]+]] = bitcast %"struct.std::experimental::awaitable"** %this.addr.i{{[0-9]*}} to i8*
+// CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 8, i8* [[CAST2]])
+// CHECK: [[CAST3:%[0-9]+]] = bitcast %"struct.std::experimental::awaitable"** %this.addr.i{{[0-9]*}} to i8*
+// CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 8, i8* [[CAST3]])
+void foo() { co_return; }