This is an archive of the discontinued LLVM Phabricator instance.

Make it possible to build a -fno-exceptions libc++abi variant.
ClosedPublic

Authored by rmaprath on May 26 2016, 6:20 AM.

Details

Summary

Support for building a -fno-exceptions libc++ variant was reinstated recently. However, some of the standard library functions like std::terminate() are currently spread across lib++ and libc++abi, making the -fno-exceptions libc++ variant useless when those functions are used (as they pull in all the exceptions machinery that we are trying to get rid of in the final image).

This patch makes it possilbe to also build libc++abi with -fno-exceptions so that functions like std::terminate() can be used in a no-exceptions environment.

Most of the sources are already correctly setup to support a -fno-exceptions build, I just filled in the missing bits.

If there are no objections, I will get this committed over the weekend.

Diff Detail

Repository
rL LLVM

Event Timeline

rmaprath updated this revision to Diff 58597.May 26 2016, 6:20 AM
rmaprath retitled this revision from to Make it possible to build a -fno-exceptions libc++abi variant..
rmaprath updated this object.
rmaprath added reviewers: bcraig, jroelofs, EricWF.
rmaprath added a subscriber: cfe-commits.
bcraig added inline comments.May 26 2016, 7:06 AM
CMakeLists.txt
359 ↗(On Diff #58597)

There's one area / test that I would still like to see when exceptions are off. "magic" / thread safe static initialization has a test (test_guard.pass.cpp). I think it would be pretty easy to #if out the one sub-test that uses exceptions there.

There's also some demangling and backtracing tests, but I'm much less concerned with those. If you go through the trouble to get any tests working with exceptions turned off, you might as well get those too.

src/cxa_aux_runtime.cpp
19 ↗(On Diff #58597)

So you're turning off exceptions, but leaving RTTI? You might want to keep some of the RTTI tests around too if that's the case.

src/cxa_new_delete.cpp
85 ↗(On Diff #58597)

Question: Should "throwing" new call terminate on allocation failure, or is returning null from "throwing" new fine?

I'm leaning towards returning null, as I would rapidly grow tired of std::nothrow everywhere, but it does seem a bit dangerous.

rmaprath updated this revision to Diff 58620.May 26 2016, 8:47 AM
rmaprath marked 2 inline comments as done.May 26 2016, 8:53 AM
rmaprath added inline comments.
src/cxa_aux_runtime.cpp
19–20 ↗(On Diff #58620)

I'm not very familiar with how RTTI works, I was simply getting rid of the throw statements which get in the way of compiling with -fno-exceptions.

So, I presume these __cxa_bad_xxx calls are inserted by the compiler?

The tests dynamic_cast_xxx all seem to pass, which I found a bit weird because I would've expected some of those to fail because of the std::terminate(). Or are they not related to RTTI?

src/cxa_new_delete.cpp
85–86 ↗(On Diff #58620)

I agree. I've update it to be inline with new.cpp from libcxx.

At least the users will have some method of recovery if we return null, calling std::terminate() would be a bit rude.

bcraig added inline comments.May 26 2016, 9:17 AM
src/cxa_aux_runtime.cpp
19–24 ↗(On Diff #58597)

_cxa_bad_cast should be inserted by the compiler. It is called when a dynamic_cast to a reference type fails.
_cxa_bad_typeid should be inserted by the compiler. It is called when typeid is used on a null pointer.

_cxa_throw_bad_array_new_length should probably stay put. The most common reason for that to get hit is if sizeof(obj) * number_of_elts > SIZE_T_MAX.

rmaprath marked an inline comment as done.May 26 2016, 9:36 AM
rmaprath added inline comments.
src/cxa_aux_runtime.cpp
19–25 ↗(On Diff #58620)

Thanks. I will try to get clang to generate these (btw, do you have a reference for those RTTI functions? I couldn't find much on the interwebs.

I suppose leaving these functions (which call std::terminate()) in the -fno-exceptions variant is not a problem?

jroelofs edited edge metadata.May 26 2016, 9:40 AM

This is the canonical reference for the Itanium ABI: https://mentorembedded.github.io/cxx-abi/abi.html

This is the canonical reference for the Itanium ABI: https://mentorembedded.github.io/cxx-abi/abi.html

I was indeed looking at this, but couldn't find those exact function signatures (or something that looks like it). Perhaps it's covered there as a more general concept. I'll have a read.

Thanks!

/ Asiri

bcraig edited edge metadata.May 26 2016, 1:02 PM

LGTM. Probably want a "LGTM" from at least one other person though.

This is the canonical reference for the Itanium ABI: https://mentorembedded.github.io/cxx-abi/abi.html

I was indeed looking at this, but couldn't find those exact function signatures (or something that looks like it). Perhaps it's covered there as a more general concept. I'll have a read.

https://mentorembedded.github.io/cxx-abi/abi-eh.html mentions the functions, but it doesn't really say what they are for. The C++ standard mentions when an exception of std::bad_cast and std::bad_typeid are thrown. Put those two documents together, and you get my earlier statements. Granted, it's just a well informed guess, but a guess I'm pretty confident in.

Leaving those functions in should be harmless, other than the size of the dead code. There are also all the private type_info classes that are dead weight if RTTI is turned off.

While it may make sense to remove those functions, that's probably best left for a different code review.

EricWF edited edge metadata.May 26 2016, 3:01 PM

I want to take a look at this as well. I'll review it tomorrow.

EricWF added a subscriber: mclow.lists.

Adding @mclow.lists to this as well.

https://mentorembedded.github.io/cxx-abi/abi-eh.html mentions the functions, but it doesn't really say what they are for. The C++ standard mentions when an exception of std::bad_cast and std::bad_typeid are thrown. Put those two documents together, and you get my earlier statements. Granted, it's just a well informed guess, but a guess I'm pretty confident in.

Leaving those functions in should be harmless, other than the size of the dead code. There are also all the private type_info classes that are dead weight if RTTI is turned off.

While it may make sense to remove those functions, that's probably best left for a different code review.

Thanks, added that to my arsenal of references :)

I want to take a look at this as well. I'll review it tomorrow.

Thanks! will wait until we have ironed out all the details, no hurry.

/ Asiri

mclow.lists edited edge metadata.May 27 2016, 11:57 AM

If you're really going to do this, you should probably empty out most of the routines in src/cxa_exception.cpp as well.

I have an issue with this change since it allows a libc++abi built without exceptions to be used in a program compiled with them. I assert that this should not be supported in any way.

My personal preference would be to remove as much of the exception API from the library when it's built without exceptions. This should cause link errors if a program contains any code that throws. If this is not done it creates bugs within code like __terminate as mentioned in an inline comment.

I would like to see another revision of this patch that removes as much of the exception API as possible. Looking at the symbols defined in cxxabi.h I would say ever symbol in sections 2.4 and 2.5 should not longer be defined in the library.

src/cxa_handlers.cpp
62 ↗(On Diff #58620)

If any part of your executable was compiled with exceptions on, and it sets a terminate handler that throws an exception this does the wrong thing. I don't really like that although I admit that would certainly be an exceptional situation.

At minimum this requirement needs to be documented explicitly somewhere. Within the CMake option description seems fine for now.

src/cxa_personality.cpp
1204 ↗(On Diff #58620)

This is yucky. We are in exception handling code and we are saying it has been built without exceptions. I understand this "gets libc++abi to compile" without exceptions but it doesn't really make sense.

I have an issue with this change since it allows a libc++abi built without exceptions to be used in a program compiled with them. I assert that this should not be supported in any way.

My personal preference would be to remove as much of the exception API from the library when it's built without exceptions. This should cause link errors if a program contains any code that throws. If this is not done it creates bugs within code like __terminate as mentioned in an inline comment.

I would like to see another revision of this patch that removes as much of the exception API as possible. Looking at the symbols defined in cxxabi.h I would say ever symbol in sections 2.4 and 2.5 should not longer be defined in the library.

That sounds generally OK to me. The toolchain I'm working on (ARM Compiler 6) has an elaborate mechanism (based on build attributes) to select the correct variant of libraries. In other words, if any of your objects are compiled with exceptions, you are guaranteed to no select any of the no-exceptions library variants. This avoids the problem you mentioned altogether.

But I can see that this can be a problem for any other toolchain that does not have such a functionality, and a link-time error would be more suitable.

I'll re-spin a patch soon.

Thanks.

/ Asiri

I have an issue with this change since it allows a libc++abi built without exceptions to be used in a program compiled with them. I assert that this should not be supported in any way.

My personal preference would be to remove as much of the exception API from the library when it's built without exceptions. This should cause link errors if a program contains any code that throws. If this is not done it creates bugs within code like __terminate as mentioned in an inline comment.

I would like to see another revision of this patch that removes as much of the exception API as possible. Looking at the symbols defined in cxxabi.h I would say ever symbol in sections 2.4 and 2.5 should not longer be defined in the library.

That sounds generally OK to me. The toolchain I'm working on (ARM Compiler 6) has an elaborate mechanism (based on build attributes) to select the correct variant of libraries. In other words, if any of your objects are compiled with exceptions, you are guaranteed to no select any of the no-exceptions library variants. This avoids the problem you mentioned altogether.

Note that that behavior will cause different issues in some cases. If you link an executable to two libraries, libfoo.so and libbar.so, where one was compiled with exceptions and the other without you'll get two copies of libc++abi in the program. Now you have two copies of the global terminate handler and two copies of __terminate for the linker to select from.
If it selects the nothrow __terminate and the terminate handler throws an exception your in trouble.

That's not a reason to hold up this patch, but it is a potential problem you should be aware of.

I have an issue with this change since it allows a libc++abi built without exceptions to be used in a program compiled with them. I assert that this should not be supported in any way.

My personal preference would be to remove as much of the exception API from the library when it's built without exceptions. This should cause link errors if a program contains any code that throws. If this is not done it creates bugs within code like __terminate as mentioned in an inline comment.

I would like to see another revision of this patch that removes as much of the exception API as possible. Looking at the symbols defined in cxxabi.h I would say ever symbol in sections 2.4 and 2.5 should not longer be defined in the library.

That sounds generally OK to me. The toolchain I'm working on (ARM Compiler 6) has an elaborate mechanism (based on build attributes) to select the correct variant of libraries. In other words, if any of your objects are compiled with exceptions, you are guaranteed to no select any of the no-exceptions library variants. This avoids the problem you mentioned altogether.

Note that that behavior will cause different issues in some cases. If you link an executable to two libraries, libfoo.so and libbar.so, where one was compiled with exceptions and the other without you'll get two copies of libc++abi in the program. Now you have two copies of the global terminate handler and two copies of __terminate for the linker to select from.
If it selects the nothrow __terminate and the terminate handler throws an exception your in trouble.

In this case, I expect the linker to only select one variant of libc++abi (the one with exceptions). The idea behind build-attributes is to capture user intentions, if the user links his program with libbar.a and libfoo.a user libraries (with and without exceptions), the linker would assume that the user meant to use exceptions rather than not, and hence select the with-exceptions libc++abi variant. The theory behind build-attributes is bit more complicated that that though, I'm not the best person to go on about this.

In any case, I agree that getting the linker to produce an error (when linking with-exceptions objects with a no-exceptions libc++abi) is much more clearer.

I have a small concern about how difficult this would make things for us though - our toolchain setup uses -ffunction-sections and -fdata-sections along with linker's unused-section elimination to get rid of most of the stuff not necessary for the final image, but I can't remember if unused-section elimination happens before or after checking for missing symbols. If it is the latter, then we might be in trouble with this approach. I will have a look into this when I get back to work.

Cheers,

/ Asiri

I have a small concern about how difficult this would make things for us though - our toolchain setup uses -ffunction-sections and -fdata-sections along with linker's unused-section elimination to get rid of most of the stuff not necessary for the final image, but I can't remember if unused-section elimination happens before or after checking for missing symbols. If it is the latter, then we might be in trouble with this approach. I will have a look into this when I get back to work.

Come to think of it, I'm 90% sure it's the latter (I don't see how the former can work - the linker has to load all the objects while resolving all the dependencies, then only it can get rid of the stuff the final image doesn't really need). The linker will give up as soon as it hits a missing symbol referred from a section which is actually unused in the final image (but is within some object which contains sections that are actually needed for the image).

Of course I need to confirm that 10% when I get my hands on the linker, but I have a feeling this is going to be a blocker for us.

rmaprath updated this revision to Diff 58927.May 29 2016, 5:48 PM
rmaprath edited edge metadata.

OK, managed to solve this problem by removing cxa_exception.cpp and cxa_personality.cpp from the no-exceptions libcxxabi build.

In order to do this, a small patch was needed for exception.cpp of libcxx: D20784

@EricWF, @mclow.lists: WDYT?

Thanks.

/ Asiri

rmaprath added inline comments.May 29 2016, 5:51 PM
CMakeLists.txt
111 ↗(On Diff #58927)

My bad, this should be ON by default (leftover from testing).

rmaprath updated this revision to Diff 58929.May 29 2016, 5:53 PM

Fixed a few typos.

rmaprath marked an inline comment as done.May 29 2016, 5:54 PM
rmaprath updated this revision to Diff 59000.May 30 2016, 2:11 PM

Addressing review comments from @EricWF:

  • Rather than explicitly decoupling the no-exceptions libc++ library from the __cxa_* routines (D20784), provide a placeholder implementation of those functions within the no-exceptions libc++abi library. We still have some symbols like __cxa_throw removed from the no-exceptions libc++abi library (those symbols are generated from the compiler, rather than found in the library), so the original requirement of not allowing linking with-exceptions code with no-exceptions libc++abi library is satisfied.

Looking good. I would like to see some tests for the methods we have specificly defined to have behavior in -fno-exceptions mode.

src/cxa_noexception.cpp
27 ↗(On Diff #59000)

I think either all of the functions in this file should have _LIBCXXABI_FUNC_VIS or none of them should. My preference is none since the declarations should have them.

29 ↗(On Diff #59000)

nit: nullptr

45 ↗(On Diff #59000)

I think we should terminate if void* is non-null.

test/backtrace_test.pass.cpp
19 ↗(On Diff #59000)

Does this test have any value in -fno-exceptions mode?

test/libcxxabi/test/config.py
46 ↗(On Diff #59000)

Nit: I would prefer LIBCXXABI_HAS_NO_EXCEPTIONS since the default should be on. The fact that a macro is missing shouldn't disable half the tests.

test/test_vector1.pass.cpp
8 ↗(On Diff #59000)

Don't you want these tests to run? They aren't part of the exception API AFAIK.

test/uncaught_exceptions.pass.cpp
10 ↗(On Diff #59000)

We provided a different implementation so we should test it, not disable the test.

rmaprath updated this revision to Diff 59009.May 30 2016, 5:24 PM

Addressed review comments from @EricWF:

  • Addressed the nits
  • Disabled backtrace_test.pass.cpp for the moment, need to check if using _Unwind_backtrace with no unwinding tables (-fno-exceptions) makes sense
  • Enabled a small part of test_vector1.pass.cpp, most of this test and other vector tests are to do with how exceptions are dealt with
  • Added tests for the new functions introduced to cxa_noexceptions.cpp. These are mostly trivial.
  • Added tests for the __cxa_bad_cast and __cxa_bad_typeid functions. Could not manage to invoke __cxa_throw_bad_array_new_length, which does not seem to work like documented in [1]. Need to take this offline.

[1] http://en.cppreference.com/w/cpp/memory/new/bad_array_new_length

rmaprath marked 6 inline comments as done.May 30 2016, 5:26 PM

For some reason I'm having trouble applying your patch. Could you please upload it with more context? (ie git diff -U999 or similar)

Nevermind. I figured out why it wasn't working. Phab didn't know the libc++ patch was in a different repo and so it wouldn't apply the patch.

EricWF accepted this revision.May 30 2016, 5:54 PM
EricWF edited edge metadata.

Just for reference here is a list of symbols which have been removed:

https://gist.github.com/EricWF/bf00dbc5fccc78b779f8d260727a0710

LGTM. Your free to submit it after addressing the inline comments.

test/cxa_bad_cast.pass.cpp
40 ↗(On Diff #59009)

return 0;

45 ↗(On Diff #59009)

return 1;

test/cxa_bad_typeid.pass.cpp
39 ↗(On Diff #59009)

return 0 here instead. "assert(true)" is pretty much un-observable.

44 ↗(On Diff #59009)

This should return non-zero.

test/noexception1.pass.cpp
26 ↗(On Diff #59009)

Before setting the terminate handling call the function once with null to make sure it returns.

test/noexception2.pass.cpp
26 ↗(On Diff #59009)

Before setting the terminate handling call the function once with null to make sure it returns.

test/noexception3.pass.cpp
27 ↗(On Diff #59009)

Before setting the terminate handling call the function once with null to make sure it returns.

This revision is now accepted and ready to land.May 30 2016, 5:54 PM

Thanks Eric.

I think I noted a few other assert(true) statements elsewhere (with similar application), will update those separately.

Btw, do you use some systematic way to detect ABI breakages? I've only bumped into http://lvc.github.io/abi-compliance-checker/ but didn't go as far as setting it up.

/ Asiri

Thanks Eric.

I think I noted a few other assert(true) statements elsewhere (with similar application), will update those separately.

Btw, do you use some systematic way to detect ABI breakages? I've only bumped into http://lvc.github.io/abi-compliance-checker/ but didn't go as far as setting it up.

Not really, the two primary ways I try to detect ABI breakage are using:

  • the scripts under libcxx/utils/sym_check/. They provide a way to diff the symbols between two dylibs sym_diff.py old-lib.so new-lib.so.
  • Compiling against an old library and swapping in a new library at runtime.

I also used libabigail for a while but it was a bit too noisy with false positives. It may be better now though.

/ Asiri

This revision was automatically updated to reflect the committed changes.