[libcxx-commits] [PATCH] D108389: [libc++] Bypass calling exception-throwing functions in the dylib with -fno-exceptions

[Jordan Rupprecht via Phabricator via libcxx-commits]

rupprecht added a comment.

In D108389#2965032 <https://reviews.llvm.org/D108389#2965032>, @ldionne wrote:

> In D108389#2963744 <https://reviews.llvm.org/D108389#2963744>, @jyknight wrote:
>
>> In D108389#2955525 <https://reviews.llvm.org/D108389#2955525>, @ldionne wrote:
>>
>>> To answer your question more directly, the goal here in terms of behavior is to ensure that a TU using `std::vector::at` (and friends) under `-fno-exceptions` doesn't rely on a weak-def symbol located in the dylib -- which it technically doesn't need cause all we do is `abort()` anyway.
>>
>> This does not seem like a desirable change.
>>
>> Previously, if you compiled libc++ itself with exceptions (as is generally the case), then a call to `std::vector::at` from a TU with -fno-exceptions _does_ still throw an exception. Of course, this exception cannot validly unwind through that TU, so it will eventually call std::terminate -- but not before printing its error message (e.g. "terminating with uncaught exception of type std::out_of_range: vector").
>>
>> Switching to call abort() directly is a degradation in functionality, for no real gain, AFAICT.
>
> The only two classes that have this behavior are `std::string` and `std::vector`. All other classes define their exception-throwing functions in the headers, so they end up calling `abort()` directly. That looks accidental to me, and not really something we'd want to consider a contract with our users. WDYT?

As a user, I'd probably be confused if I build my code with `-fno-exceptions` and end up getting an exception because of how the toolchain's libc++ was built. IMHO, this patch is therefore a step in the right direction. Having a discussion around that & getting it written down could be useful -- maybe not as strict as a "contract", but as a general "libc++ will try not to ..." set of guidelines?

[We experienced some churn from this commit, but so far just seems like brittle tests]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108389