Page MenuHomePhabricator

[Coroutines] Insert lifetime intrinsics even O0 is used
ClosedPublic

Authored by junparser on Mar 13 2020, 12:41 AM.

Details

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

Diff Detail

Event Timeline

junparser created this revision.Mar 13 2020, 12:41 AM
modocache requested changes to this revision.Mar 13 2020, 5:26 AM

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?

This revision now requires changes to proceed.Mar 13 2020, 5:26 AM
modocache added inline comments.Mar 13 2020, 5:58 AM
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 marked 3 inline comments as done.Mar 15 2020, 6:28 PM
junparser added inline comments.
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 updated this revision to Diff 251540.Mar 19 2020, 8:11 PM

address the comments.

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 accepted this revision.Mar 23 2020, 12:48 PM

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 marked an inline comment as done.Mar 23 2020, 6:51 PM

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.

update testcase.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 23 2020, 10:50 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2020, 10:50 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript