Page MenuHomePhabricator

[CodeGen] Disable UBSan for coroutine functions
ClosedPublic

Authored by modocache on Mar 19 2018, 9:05 PM.

Details

Summary

As explained in http://lists.llvm.org/pipermail/llvm-dev/2018-March/121924.html,
the LLVM coroutines transforms are not yet able to move the
instructions for UBSan null checking past coroutine suspend boundaries.
For now, disable all UBSan checks when generating code for coroutines
functions.

I also considered an approach where only '-fsanitize=null' would be disabled,
However in practice this led to other LLVM errors when writing object files:
"Cannot represent a difference across sections". For now, disable all
UBSan checks until coroutine transforms are updated to handle them.

Test Plan:

  1. check-clang
  2. Compile the program in https://gist.github.com/modocache/54a036c3bf9c06882fe85122e105d153 using the '-fsanitize=null' option and confirm it does not crash during LLVM IR generation.

Event Timeline

modocache created this revision.Mar 19 2018, 9:05 PM
vsk added a comment.Mar 20 2018, 11:26 AM

The "Cannot represent a difference across sections" error is almost certainly related to -fsanitize=function. Time permitting, could you file a separate PR for that and attach the IR? It'd be great to know whether the issue reproduces without coroutines involved.

Also, could you check in a simple test (something along the lines of what you posted to llvm-dev)? You can just pass '-emit-llvm -fcoroutines -fsanitize=null' and check that the string '__ubsan' doesn't appear.

lib/CodeGen/CodeGenFunction.cpp
1307 ↗(On Diff #139072)

The suppression should live right next to the logic which handles no_sanitize attributes, in CGF::StartFunction. In general you can end up in CGF::StartFunction without the immediate caller being CGF::GenerateCode.

Oh, I'm sorry I let this languish! I'll address your comments later this week, @vsk. Thanks so much for the review!

vsk requested changes to this revision.Jul 10 2019, 2:31 PM

(Marking this to reflect my comment from 3/20/18 to clear my review queue)

This revision now requires changes to proceed.Jul 10 2019, 2:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 2:31 PM
modocache updated this revision to Diff 214370.Aug 9 2019, 5:51 AM

Thanks for the review, @vsk! Sorry it took me so long to update this diff.

In the mailing list discussion, http://lists.llvm.org/pipermail/llvm-dev/2018-March/121925.html, you mentioned that I should use an allow-list of known good sanitizer options, rather than a deny-list of known bad ones (-fsanitize=null, in this case). However, at this time I'm only aware of problems with -fsanitize=null, so I don't know what else to exclude. In other words, an allow-list of known good sanitizers would, at this point, be every other sanitizer pass. So I figured for now, I'll just deny -fsanitize=null. Does that work for you?

vsk accepted this revision.Aug 12 2019, 1:02 PM

Lgtm, thanks!

This revision is now accepted and ready to land.Aug 12 2019, 1:02 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 5:03 AM

It seems that this test leads to an UNREACHABLE under the new pass manager (can check this by adding -fexperimental-new-pass-manager to the test. I think this is because the passes for lowering the llvm.coro intrinsics are not yet ported to the new PM. Would it be fine to add -fno-experimental-new-pass-manager to the test to indicate it should be run under the legacy PM only? Thanks.

vsk added a comment.Aug 20 2019, 12:35 PM

It seems that this test leads to an UNREACHABLE under the new pass manager (can check this by adding -fexperimental-new-pass-manager to the test. I think this is because the passes for lowering the llvm.coro intrinsics are not yet ported to the new PM. Would it be fine to add -fno-experimental-new-pass-manager to the test to indicate it should be run under the legacy PM only? Thanks.

Sounds reasonable to me. It would be great to include a link to some master PR for the new PM porting effort with the test change.

In D44672#1637984, @vsk wrote:

It seems that this test leads to an UNREACHABLE under the new pass manager (can check this by adding -fexperimental-new-pass-manager to the test. I think this is because the passes for lowering the llvm.coro intrinsics are not yet ported to the new PM. Would it be fine to add -fno-experimental-new-pass-manager to the test to indicate it should be run under the legacy PM only? Thanks.

Sounds reasonable to me. It would be great to include a link to some master PR for the new PM porting effort with the test change.

Thanks. I added you as a reviewer to D66493 and linked in https://bugs.llvm.org/show_bug.cgi?id=42867 which I filed a while back and has the same root problem.