[CodeGen] Disable UBSan for coroutine functions

[modocache]

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.

[vsk]

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.

[modocache]

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

[vsk]

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

[modocache]

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]

Lgtm, thanks!

[modocache]

Thank you!

[leonardchan]

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]

In D44672#1637891, @leonardchan 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.

[leonardchan]

In D44672#1637984, @vsk wrote:

In D44672#1637891, @leonardchan 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.

cfe/trunk/lib/CodeGen/CodeGenFunction.cpp

#undef SANITIZER
   // Ignore unrelated casts in STL allocate() since the allocator must cast
   // from void* to T* before object initialization completes. Don't match on the
   // namespace because not all allocators are in std::
   if (D && SanOpts.has(SanitizerKind::CFIUnrelatedCast)) {
     if (matchesStlAllocatorFn(D, getContext()))
       SanOpts.Mask &= ~SanitizerKind::CFIUnrelatedCast;
   }
 
+  // Ignore null checks in coroutine functions since the coroutines passes
+  // are not aware of how to move the extra UBSan instructions across the split
+  // coroutine boundaries.
+  if (D && SanOpts.has(SanitizerKind::Null))
+    if (const auto *FD = dyn_cast<FunctionDecl>(D))
+      if (FD->getBody() &&
+          FD->getBody()->getStmtClass() == Stmt::CoroutineBodyStmtClass)
+        SanOpts.Mask &= ~SanitizerKind::Null;
+
   // Apply xray attributes to the function (as a string, for now)
   if (D) {
     if (const auto *XRayAttr = D->getAttr<XRayInstrumentAttr>()) {
       if (CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
               XRayInstrKind::Function)) {
         if (XRayAttr->alwaysXRayInstrument() && ShouldXRayInstrumentFunction())
           Fn->addFnAttr("function-instrument", "xray-always");
         if (XRayAttr->neverXRayInstrument())

cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp

+// This test merely verifies that emitting the object file does not cause a
+// crash when the LLVM coroutines passes are run.
+// RUN: %clang_cc1 -emit-obj -std=c++2a -fsanitize=null %s -o %t.o
+
+namespace std::experimental {
+template <typename R, typename... T> struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template <class Promise = void> struct coroutine_handle;
+template <> struct coroutine_handle<void> {
+  static coroutine_handle from_address(void *) noexcept;
+  coroutine_handle() = default;
+  template <class PromiseType>
+  coroutine_handle(coroutine_handle<PromiseType>) noexcept;
+};
+template <class Promise> struct coroutine_handle : coroutine_handle<void> {
+  coroutine_handle() = default;
+  static coroutine_handle from_address(void *) noexcept;
+};
+}
+
+struct suspend_always {
+  bool await_ready() noexcept;
+  void await_suspend(std::experimental::coroutine_handle<>) noexcept;
+  void await_resume() noexcept;
+};
+
+struct task {
+  struct promise_type {
+    task get_return_object() { return task(); }
+    suspend_always initial_suspend() { return {}; }
+    suspend_always final_suspend() { return {}; }
+    void return_void() {}
+    void unhandled_exception() {}
+  };
+};
+
+struct awaitable {
+  task await() { (void)co_await *this; }
+  bool await_ready() { return false; }
+  bool await_suspend(std::experimental::coroutine_handle<> awaiter) { return false; }
+  bool await_resume() { return false; }
+};
+
+int main() {
+  awaitable a;
+  a.await();
+}