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

[Duncan P. N. Exon Smith via Phabricator via libcxx-commits]

dexonsmith added inline comments.


================
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");
 }
----------------
ldionne wrote:
> 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.
> 

>> 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.

FWIW, I wasn't necessarily suggesting mixing code between `-fexceptions` and `-fno-exceptions`.  Consider:

- A header-only library A has code that uses iostream somehow.
- A project B builds with `-fno-exceptions` and uses library A.
- A project C builds with `-fexceptions` and uses library A.
- Project B and project C are completely unrelated, except that they share a dependency.

I don't think library A should need to detect whether exceptions are enabled to do the right thing for both project B and project C.  The simple abort-on-throw rule will simplify life for library A.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D57761