[libcxx-commits] [PATCH] D57761: [libc++] Avoid UB in the no-exceptions mode in a few places

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked an inline comment as done.
ldionne added a comment.

In D57761#1385562 <https://reviews.llvm.org/D57761#1385562>, @jfb wrote:

> I'm wondering if, as a follow-up, the XFAIL tests that validate the behavior for exceptions should instead:
>
> - Not XFAIL
> - Install a SIGABORT handler, and call `exit(0)` or something
> - Instead of `catch`, call `exit(1)` because we expected the abort handler to be called.
>
>   That type of testing will find any missing fixes from this patch (and prevent regressions).


Yes, I agree we should test the no-exceptions behaviour just like we test the behaviour with exceptions enabled. There's a question of whether this should be part of `test/std` or `test/libcxx`, however, since the Standard currently doesn't mention implementations without exceptions (I think).



================
Comment at: libcxx/src/ios.cpp:271
     if (((state | (__rdbuf_ ? goodbit : badbit)) & __exceptions_) != 0)
-        throw failure("ios_base::clear");
-#endif  // _LIBCPP_NO_EXCEPTIONS
+        __throw_failure("ios_base::clear");
 }
----------------
mclow.lists wrote:
> dexonsmith wrote:
> > ldionne wrote:
> > > Actually, looking at this again, it seems like we probably don't want to abort here, since the stream will report the error with the error flags. So I should not change this one -- @mclow.lists does that seem right to you?
> > `::abort` seems more consistent to me.  The user has set the exception bit, so I wouldn't expect them to have control flow to handle errors otherwise.  Moreover, the direct user could be templated/inlined header code which doesn't know that `-fno-exceptions` has been passed for this project/TU.
> > 
> > More generally, I think a strict library policy of turning throws into aborts will lead to behaviour that's predictable for users.
> If we do this, we should be consistent about it.  
> Do we want to say "Compiling with exceptions disabled makes iostreams act as if `state & __exceptions_` is always `false`?
> 
> And then what do we do if the user explicitly sets the `__exceptions_` flag? Abort there?
> Moreover, the direct user could be templated/inlined header code which doesn't know that `-fno-exceptions` has been passed for this project/TU.

We don't have a story for mixing code built with `-fno-exceptions` with code built with `-fexceptions`. For starters, it's an ODR violation to do that because the functions in the headers will have different definitions. One would technically have to enable per-TU ABI insulation with `_LIBCPP_HIDE_FROM_ABI_PER_TU` to make this work, but I would _strongly_ recommend against recommending that as a solution.

> More generally, I think a strict library policy of turning throws into aborts will lead to behaviour that's predictable for users.

I agree.

> Do we want to say "Compiling with exceptions disabled makes iostreams act as if `state & __exceptions_` is always `false`?
> And then what do we do if the user explicitly sets the `__exceptions_` flag? Abort there?

That's certainly not what I want. I think I'm with Duncan when he says that a simple and strict policy of turning throws into aborts is the right thing to do. Cleverness is the enemy here.

In that case, it seems like I should keep this change.



Repository:
  rCXX libc++

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

https://reviews.llvm.org/D57761