[libc++] Bypass calling exception-throwing functions in the dylib with -fno-exceptions

[ldionne]

Summary

basic_string and vector currently have a hard dependency on the compiled
library because they need to call vector_base_common::throw_xxx(),
which are externally instantiated in the compiled library. That makes
sense when exceptions are enabled (because we're trying to localize the
exception-throwing code to the compiled library), but it doesn't really
make sense when exceptions are disabled, and the __throw_xxx functions
are just calling abort() anyways.

This patch simply overrides the __throw_xxx() functions so that they
don't rely on the compiled library when exceptions are disabled.

[• Quuxplusone]

libcxx/include/string

1721

#include <cstdlib>?

[mclow.lists]

What's the goal here? In terms of behavior?

[Mordante]

LGTM, provided the build passes.

libcxx/include/string

1724

Other places that directly throw and exception use a style like

#ifndef _LIBCPP_NO_EXCEPTIONS
        throw bad_variant_access();
#else
        _VSTD::abort();
#endif

Maybe use the same pattern here for consistency?

[ldionne]

In D108389#2955148, @mclow.lists wrote:

What's the goal here? In terms of behavior?

So it's a little bit embarrassing, but I came across a dubious use of libc++ where they were overriding _LIBCPP_EXTERN_TEMPLATE with their custom thing. It should never have worked, but starting in D103964, it really stopped working because we are not checking for #ifdef _LIBCPP_EXTERN_TEMPLATE anymore. Note that this is intended - we don't want users to override _LIBCPP_EXTERN_TEMPLATE, and in fact I'll even move forward with the more assertive D103960 once I've had time to clean up the debug mode. The proper solution here would be for those users to stop overriding _LIBCPP_EXTERN_TEMPLATE and start linking against libc++ (which they currently don't do), however they can't do that yet for complicated reasons.

So what I'm trying to do is basically remove the undefined reference that's breaking them without making the library worse for it. Technically, this change is a slight improvement over the status quo cause it removes the reliance on weak references (__throw_xxxx) when exceptions are disabled, and it solves the concrete problem I have at hand.

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.

libcxx/include/string

1721

Good catch, thanks.

[ldionne]

Address reviewer comments (thanks)!

[jyknight]

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

[ldionne]

In D108389#2963744, @jyknight wrote:

In 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?

[rupprecht]

In D108389#2965032, @ldionne wrote:

In D108389#2963744, @jyknight wrote:

In 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]

[jyknight]

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?

Yes, I agree, that is a convincing argument.

I think I was getting mixed up in my mind with libstdc++'s behavior, which does put nearly all throws (historically, it may have been all) in the library. (Additionally, its errors are more useful, too, e.g. showing the actual out of range index, which makes that all the more valuable to emit in both modes).

[ldionne]

In D108389#2973427, @jyknight wrote:

(Additionally, its errors are more useful, too, e.g. showing the actual out of range index, which makes that all the more valuable to emit in both modes).

100% agree here, our errors suck. I think there's a bug report to improve that somewhere.

In D108389#2973423, @rupprecht wrote:

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

Please let me know if you experience anything serious from this change. I would expect it's NFC for 99.9% of people, except if you were doing something shady. If that's not correct, I might learn something useful so please let me know.

[rupprecht]

In D108389#3015689, @ldionne wrote:

In D108389#2973427, @jyknight wrote:

(Additionally, its errors are more useful, too, e.g. showing the actual out of range index, which makes that all the more valuable to emit in both modes).

100% agree here, our errors suck. I think there's a bug report to improve that somewhere.

In D108389#2973423, @rupprecht wrote:

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

Please let me know if you experience anything serious from this change. I would expect it's NFC for 99.9% of people, except if you were doing something shady. If that's not correct, I might learn something useful so please let me know.

We did a bit of cleanup internally. "shady" is a close enough word for all the instances we had. I think we have now resolved all issues related to this commit.

The most common (but very uninteresting) cleanup we did was death tests asserting an exception being thrown, e.g. call at(something_out_of_range) and assert the subprocess dies with "out_of_range" in the stack trace. (This is for build modes that use libc++ built with -fexceptions, but the rest of the TUs are built with -fno-exceptions).

One interesting cleanup was a test failure like this (pseudocode-ish):

auto lambda = [&]() {
  std::vector<int> foo;
  foo.resize(100);
};
auto memory_used = InvokeAndMeasureMemory(lambda);
EXPECT_EQ(memory_used, 800);

This started failing because memory_used is now 0 after this commit. IIUC, having more declarations available in the header gave the optimizer more information that led to completely getting rid of the std::vector usage, so it's an empty lambda.

We had another one that's way too complicated to explain here, but it was another case of "more information = better optimization = test failure because a test asserts fragile things". In general I wouldn't expect this to make a very performance impact, but it's on the positive side of zero.

[ldionne]

In D108389#3015964, @rupprecht wrote:

One interesting cleanup was a test failure like this (pseudocode-ish):

auto lambda = [&]() {
  std::vector<int> foo;
  foo.resize(100);
};
auto memory_used = InvokeAndMeasureMemory(lambda);
EXPECT_EQ(memory_used, 800);

This started failing because memory_used is now 0 after this commit. IIUC, having more declarations available in the header gave the optimizer more information that led to completely getting rid of the std::vector usage, so it's an empty lambda.

We had another one that's way too complicated to explain here, but it was another case of "more information = better optimization = test failure because a test asserts fragile things". In general I wouldn't expect this to make a very performance impact, but it's on the positive side of zero.

Thanks a lot for the feedback, this is quite interesting indeed. I would expect that's because resize() doesn't make an external call to a function defined in the library anymore, and now the optimizer can reason better about things. It's not clear to me it would make a difference in the general case, but oh well.

libcxx/include/string

 
 #include <__config>
 #include <__debug>
 #include <__functional_base>
 #include <__iterator/wrap_iter.h>
 #include <algorithm>
 #include <compare>
 #include <cstdio>  // EOF
+#include <cstdlib>
 #include <cstring>
 #include <cwchar>
 #include <initializer_list>
 #include <iosfwd>
 #include <iterator>
 #include <memory>
 #include <stdexcept>
 #include <string_view>
#endif
 
     template<class _Tp>
     _LIBCPP_INLINE_VISIBILITY
     bool __addr_in_range(_Tp&& __t) const {
         const volatile void *__p = _VSTD::addressof(__t);
         return data() <= __p && __p <= data() + size();
     }
 
+    _LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI
+    void __throw_length_error() const {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+        __basic_string_common<true>::__throw_length_error();
+#else
+        _VSTD::abort();
+#endif
+    }
+
+    _LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI
+    void __throw_out_of_range() const {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+        __basic_string_common<true>::__throw_out_of_range();
+#else
+        _VSTD::abort();
+#endif
+    }
+
     friend basic_string operator+<>(const basic_string&, const basic_string&);
     friend basic_string operator+<>(const value_type*, const basic_string&);
     friend basic_string operator+<>(value_type, const basic_string&);
     friend basic_string operator+<>(const basic_string&, const value_type*);
     friend basic_string operator+<>(const basic_string&, value_type);
 };
 
 // These declarations must appear before any functions are implicitly used

libcxx/include/vector

 #include <__debug>
 #include <__functional_base>
 #include <__iterator/wrap_iter.h>
 #include <__split_buffer>
 #include <__utility/forward.h>
 #include <algorithm>
 #include <climits>
 #include <compare>
+#include <cstdlib>
 #include <cstring>
 #include <initializer_list>
 #include <iosfwd> // for forward declaration of vector
 #include <limits>
 #include <memory>
 #include <stdexcept>
 #include <type_traits>
 #include <version>
#endif
 
     _LIBCPP_INLINE_VISIBILITY
     void __move_assign_alloc(__vector_base& __c)
         _NOEXCEPT_(
             !__alloc_traits::propagate_on_container_move_assignment::value ||
             is_nothrow_move_assignable<allocator_type>::value)
         {__move_assign_alloc(__c, integral_constant<bool,
                       __alloc_traits::propagate_on_container_move_assignment::value>());}
+
+    _LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI
+    void __throw_length_error() const {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+        __vector_base_common<true>::__throw_length_error();
+#else
+        _VSTD::abort();
+#endif
+    }
+
+    _LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI
+    void __throw_out_of_range() const {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+        __vector_base_common<true>::__throw_out_of_range();
+#else
+        _VSTD::abort();
+#endif
+    }
+
 private:
     _LIBCPP_INLINE_VISIBILITY
     void __copy_assign_alloc(const __vector_base& __c, true_type)
         {
             if (__alloc() != __c.__alloc())
             {
                 clear();
                 __alloc_traits::deallocate(__alloc(), __begin_, capacity());