[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D108905#4655907 <https://reviews.llvm.org/D108905#4655907>, @MaskRay wrote:

> In D108905#4655694 <https://reviews.llvm.org/D108905#4655694>, @ChuanqiXu wrote:
>
>> Oh, I am not saying the legacy and old comment. I mean you need to touch ReleaseNotes.rst and UserManual.rst since we add a new flag. Also we need either add a TODO/FIXME saying we need to emit an error in Sema if we find the the dtor of the exception we're throwing may throw or implement the semantics actually.
>
> Thanks for the reminder about `ReleaseNotes.rst` and `UsersManual.rst`!
>
> I think many changes don't update `UsersManual.rst` but this option is probably quite useful and therefore deserves an entry. Added.
> The primary change is to `clang/lib/CodeGen/ItaniumCXXABI.cpp`, which does not report warnings.
> If we want to implement a warning, we should probably do it in clang/lib/Sema/SemaDeclCXX.cpp `Sema::BuildExceptionDeclaration`, which is not touched in this file, so a TODO seems not appropriate...
>
> Is the warning to warn about `noexcept(false)` destructor in an exception-declaration <https://eel.is/c++draft/except.pre>? When?
> If at catch handlers, unfortunately we are cannot determine the exception object for a `catch (...) { ... }` (used by coroutines).
> Technically, even if a destructor is `noexcept(false)`, the destructor may not throw when `__cxa_end_catch` destroys the object.
>
> So we probably should warn about throw expressions, which can be done in `Sema::CheckCXXThrowOperand` and I will investigate it.
> However, this warning appears orthogonal to `-fassume-nothrow-exception-dtor`.
> We can argue that even without `-fassume-nothrow-exception-dtor`, an thrown object with a `noexcept(false)` destructor is probably not a good idea.
> However, I am on the fence whether the warning should be enabled-by-default.

The idea of diagnosing such cases is originally raised by @rjmccall  in https://github.com/llvm/llvm-project/issues/57375#issuecomment-1230590204. And in fact,  John is suggesting an error instead of a warning. To be honest, in our downstream implementation, we didn't implement that semantical check neither.

For the implementation, I think `Sema::CheckCXXThrowOperand` may be a good place and we can left a TODO there.

Technically, we're using a dialect after we enable `-fassume-nothrow-exception-dtor`.  In our dialect, we don't want to see an exception to throw exceptions. This is a rule for our dialect. And generally we have 2 strategies for implementing such language rules :

- Detect it and emit errors after we found users violating the rules.
- Don't detect it and claims it is the fault of the users.

No matter what the strategy we choose, it is not orthogonal to `-fassume-nothrow-exception-dtor` for sure. It is highly related. Then, I think the first strategy is always a better choice. Generally we only choose 2 when we can't diagnose such cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108905