[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44672/new/

https://reviews.llvm.org/D44672



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44672/new/

https://reviews.llvm.org/D44672



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44672/new/

https://reviews.llvm.org/D44672



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Thank you!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44672/new/

https://reviews.llvm.org/D44672



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368675: [CodeGen] Disable UBSan for coroutine functions 
(authored by modocache, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D44672?vs=214370&id=214809#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44672/new/

https://reviews.llvm.org/D44672

Files:
  cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
  cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp


Index: cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
===
--- cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
+++ cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
@@ -0,0 +1,49 @@
+// 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  struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template  struct coroutine_handle;
+template <> struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  coroutine_handle() = default;
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+};
+template  struct coroutine_handle : coroutine_handle {
+  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();
+}
Index: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
@@ -732,6 +732,15 @@
   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(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()) {


Index: cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
===
--- cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
+++ cfe/trunk/test/CodeGenCXX/ubsan-coroutines.cpp
@@ -0,0 +1,49 @@
+// 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  struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template  struct coroutine_handle;
+template <> struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  coroutine_handle() = default;
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+};
+template  struct coroutine_handle : coroutine_handle {
+  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();
+}
Index: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
@@ -732,6 +732,15 @@
   SanOpts.Mask 

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Vedant Kumar via Phabricator via cfe-commits]

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

Lgtm, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44672/new/

https://reviews.llvm.org/D44672



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Brian Gesiak via Phabricator via cfe-commits]

modocache updated this revision to Diff 214370.
modocache added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44672/new/

https://reviews.llvm.org/D44672

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGenCXX/ubsan-coroutines.cpp


Index: clang/test/CodeGenCXX/ubsan-coroutines.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ubsan-coroutines.cpp
@@ -0,0 +1,49 @@
+// 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  struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template  struct coroutine_handle;
+template <> struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  coroutine_handle() = default;
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+};
+template  struct coroutine_handle : coroutine_handle {
+  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();
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -732,6 +732,15 @@
   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(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()) {


Index: clang/test/CodeGenCXX/ubsan-coroutines.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ubsan-coroutines.cpp
@@ -0,0 +1,49 @@
+// 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  struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template  struct coroutine_handle;
+template <> struct coroutine_handle {
+  static coroutine_handle from_address(void *) noexcept;
+  coroutine_handle() = default;
+  template 
+  coroutine_handle(coroutine_handle) noexcept;
+};
+template  struct coroutine_handle : coroutine_handle {
+  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() {
+  awaitab

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Vedant Kumar via Phabricator via cfe-commits]

vsk requested changes to this revision.
vsk added a comment.
This revision now requires changes to proceed.
Herald added a project: clang.

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44672/new/

https://reviews.llvm.org/D44672



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D44672



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

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.




Comment at: lib/CodeGen/CodeGenFunction.cpp:1307
+  if (Body && Body->getStmtClass() == Stmt::CoroutineBodyStmtClass)
+SanOpts.clear();
+

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.


Repository:
  rC Clang

https://reviews.llvm.org/D44672



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: GorNishanov, vsk, eric_niebler, lewissbaker.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D44672

Files:
  lib/CodeGen/CodeGenFunction.cpp


Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -1298,6 +1298,14 @@
 
   Stmt *Body = FD->getBody();
 
+  // TODO: As mentioned in the TODO added in https://reviews.llvm.org/rL280678,
+  // coro-split is not capable of moving spills whose users' users are not
+  // dominated by 'llvm.coro.begin'. '-fsanitize=null', for example, generates
+  // such code: null checks that occur before 'llvm.coro.begin'. For now,
+  // disable UBSan checks within coroutine function bodies.
+  if (Body && Body->getStmtClass() == Stmt::CoroutineBodyStmtClass)
+SanOpts.clear();
+
   // Initialize helper which will detect jumps which can cause invalid lifetime
   // markers.
   if (Body && ShouldEmitLifetimeMarkers)


Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -1298,6 +1298,14 @@
 
   Stmt *Body = FD->getBody();
 
+  // TODO: As mentioned in the TODO added in https://reviews.llvm.org/rL280678,
+  // coro-split is not capable of moving spills whose users' users are not
+  // dominated by 'llvm.coro.begin'. '-fsanitize=null', for example, generates
+  // such code: null checks that occur before 'llvm.coro.begin'. For now,
+  // disable UBSan checks within coroutine function bodies.
+  if (Body && Body->getStmtClass() == Stmt::CoroutineBodyStmtClass)
+SanOpts.clear();
+
   // Initialize helper which will detect jumps which can cause invalid lifetime
   // markers.
   if (Body && ShouldEmitLifetimeMarkers)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits