This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Do not emit bugprone-exception-escape warnings from coroutines
ClosedPublic

Authored by denizevrenci on Apr 2 2023, 2:01 PM.

Details

Summary

All exceptions thrown in coroutine bodies are caught and
unhandled_exception member of the coroutine promise type is called.
In accordance with the existing rules of diagnostics related to
exceptions thrown in functions marked noexcept, even if the promise
type's constructor, get_return_object, or unhandled_exception
throws, diagnostics should not be emitted.

Fixes #61905.

Diff Detail

Event Timeline

denizevrenci created this revision.Apr 2 2023, 2:01 PM
denizevrenci requested review of this revision.Apr 2 2023, 2:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2023, 2:01 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Remove line breaks in CHECK-MESSAGES lines

Fix the line number for the warning

I don't have commit access so after the review is complete, please commit this diff in my place.

PiotrZSL added inline comments.Apr 3 2023, 12:37 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
115

add tests with c_yield and co_await

ChuanqiXu added inline comments.Apr 3 2023, 12:53 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
75–79

I don't understand why we shouldn't emit the warning here. Since the function is marked noexcept but it may throw actually in unhandled_exception. I think it is meaningful to warn for this.

PiotrZSL added inline comments.Apr 3 2023, 8:23 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
91

And based on ChuanqiXu comment, same here.

"All exceptions thrown in coroutine bodies are caught and
unhandled_exception member of the coroutine promise type is called."
This isn't expected behaviour, as it may hide exception.
Based on that we should assume, if there is noexcept, then it should not throw, and if its no noexcept, then it could throw. No point to change default behaviour then.

Other option is to add check option to handle this.

denizevrenci added inline comments.Apr 3 2023, 6:01 PM
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
75–79

Right, I now see that this behavior is different between Clang's -Wexceptions and Clang Tidy's bugprone-exception-escape. The former does not warn on this code, the latter does.

int foo() {
  throw 1;
}

int bar() noexcept {
  return foo();
}

We need to treat coroutines differently and check whether task::task, promise::promise, promise::initial_suspend, promise::get_return_object, and promise::unhandled_exception can throw instead of the body of the coroutine.

ChuanqiXu added inline comments.Apr 3 2023, 7:25 PM
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
75–79

I investigated the exception issue in coroutines before: https://reviews.llvm.org/D108277. And it is much more complex than I thought. The short conclusion here is that the coroutine is still may throw even if all the promise's method wouldn't throw. For example:

struct Evil {
    ~Evil() noexcept(false) { throw 32; }
};

task foo() noexcept { // all promise's method of task wouldn't throw
    throw Evil;
}

And in the above example, foo() may throw actually. Although the implicit catch block of foo() will catch Evil, the exception in the destructor of Evil will be thrown again.

So we can't be sure that a coroutine wouldn't throw even if all of its promise's method wouldn't throw.

denizevrenci added inline comments.Apr 4 2023, 1:23 PM
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
75–79

It looks like the function foo can throw until the first suspension point in the coroutine. If promise::initial_suspend is std::suspend_always, then it will not throw. Of course, determining this statically is quite complicated.
But I also think that this is a rather niche example, it looks like clang-tidy already warns with bugprone-exception-escape on the destructor of Evil even when it is marked noexcept(false). I assume this is due to the other
complications brought by throwing from destructors. Would that not be the appropriate place to warn about this anyway?

For example, the code below terminates because the destructor of Evil gets called while there is an active exception.

task foo() { // all promise's method of task wouldn't throw
    Evil e;
    throw 1;
    co_return;
}

" clang-tidy already warns with bugprone-exception-escape on the destructor of Evil even when it is marked noexcept(false)"
https://reviews.llvm.org/D145865

ChuanqiXu added inline comments.Apr 5 2023, 7:07 PM
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
75–79

If we've handled the case, the strategy makes sense to me.

Address comments. Implement a special case for coroutines in ExceptionAnalyzer

Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2023, 4:43 PM
denizevrenci marked 4 inline comments as done.May 15 2023, 4:53 PM

a_shouldNotDiag -> b_shouldNotDiag

Fix commit message

Add tests for co_yield and co_await

@PiotrZSL & @ChuanqiXu may I request a re-review? I addressed all comments.

PiotrZSL accepted this revision.May 29 2023, 12:37 AM

LGTM

  • Missing release notes entry.
  • I'm not sure about change in StmtCXX.h, do we need it ? Coudn't we just use children() + getBody()
  • CHECK-MESSAGES-NOT get me a little bit confused. Maybe we dont need those entrys, just make sure that test fail when non listed warning show up.
This revision is now accepted and ready to land.May 29 2023, 12:37 AM
  • Update release notes
  • Remove checks for no messages

Addressed 1 and 3. As for the new methods in StmtCXX.h, I think they may be used elsewhere too where we need to make a distinction between the visible body of the coroutine and the desugared version.

  • Rebase on trunk
  • Remove unused helpers
denizevrenci marked an inline comment as done.May 29 2023, 10:58 AM

Hi @PiotrZSL if you are happy with the changes, could you commit the diff for me as I don't have commit access to LLVM repo?

ChuanqiXu accepted this revision.May 29 2023, 6:43 PM

LGTM. Thanks.