This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ldionne on Aug 19 2021, 10:12 AM.

Details

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.

Diff Detail

Event Timeline

ldionne requested review of this revision.Aug 19 2021, 10:12 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2021, 10:12 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/include/string
1721

#include <cstdlib>?

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

Mordante accepted this revision as: Mordante.Aug 19 2021, 11:20 AM
Mordante added a subscriber: 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 marked an inline comment as done.Aug 19 2021, 12:07 PM

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 updated this revision to Diff 367586.Aug 19 2021, 12:09 PM
ldionne marked 2 inline comments as done.

Address reviewer comments (thanks)!

ldionne accepted this revision.Aug 20 2021, 5:38 AM
This revision is now accepted and ready to land.Aug 20 2021, 5:38 AM

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.

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?

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]

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

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

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

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

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

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.