[libc++] [libc++abi] Initialize exception directly in make_exception_ptr if __cxa_init_primary_exception is available in ABI-library

[itrofimow]

This patch implements an ABI-extension function __cxa_init_primary_exception in libcxxabi, which is already present as an extension in both libsupc++ and libcxxrt, and makes use of this function in std::make_exception_ptr: now instead of going through full throw + catch cycle we are able to initialize an exception directly, thus making std::make_exception_ptr around x30 times faster.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[itrofimow]

I had to split typeinfo header to break the exception -> typeinfo -> exception circular dependency, hence a sizeable +/-, other than that the PR is basically a couple dozen LoC.

The build is failing mostly due to missing changes in abi-lists for various architectures, i'm planning to fix that once/if the general idea is approved.

One question left is the way to determine whether the new __cxa_init_primary_exception function is available: right now i'm relying on LIBCXX_BUILDING_LIBCXXABI, but i think it's not enough.
Should i add a new define into libcxxabi, or check for __cxa_init_primary_exception presence at runtime via dlsym?
Personally i'm leaning towards dlsym, since it would not only allow linking with other versions of libcxxabi, but potentially implementing the same optimization when linked against libcxxrt, see libcxxrt/libcxxrt#23.

@philnik777 @EricWF Could you please have a look?

[philnik777]

I haven't looked at the actual changes closely yet, but the amount of extra code seems reasonable for a significant speedup.

[philnik777]

This should be guarded by an availability macro instead.

[itrofimow]

Should it be a macro or a runtime availability check instead?

[itrofimow]

I've added support for libcxxrt as well and made it a runtime check

[itrofimow]

The ifdef became a bit too big to copy-paste it, so i moved it into __config

[itrofimow]

Benchmarks to back the performance claims up:

itrofimow@itrofimow-nix:~/work/llvm-project/ex_ptr_bench$ taskset -c 8-15 ~/work/gbench/tools/compare.py benchmarks ./main_libcxxabi ./patch_libcxxabi 
RUNNING: ./main_libcxxabi --benchmark_out=/tmp/tmpi96tlrpa
2023-10-15T20:57:14+03:00
Running ./main_libcxxabi
Run on (16 X 1280.96 MHz CPU s)
CPU Caches:
  L1 Data 48 KiB (x8)
  L1 Instruction 32 KiB (x8)
  L2 Unified 1280 KiB (x8)
  L3 Unified 24576 KiB (x1)
Load Average: 0.36, 1.30, 1.61
--------------------------------------------------------------------------
Benchmark                                Time             CPU   Iterations
--------------------------------------------------------------------------
bm_make_exception_ptr/threads:1        745 ns          745 ns       950011
bm_make_exception_ptr/threads:2        816 ns         1633 ns       455610
bm_make_exception_ptr/threads:4        863 ns         3208 ns       227044
bm_make_exception_ptr/threads:8       1120 ns         8064 ns        87896
RUNNING: ./patch_libcxxabi --benchmark_out=/tmp/tmpvd2nklsl
2023-10-15T20:57:16+03:00
Running ./patch_libcxxabi
Run on (16 X 1259.78 MHz CPU s)
CPU Caches:
  L1 Data 48 KiB (x8)
  L1 Instruction 32 KiB (x8)
  L2 Unified 1280 KiB (x8)
  L3 Unified 24576 KiB (x1)
Load Average: 0.36, 1.30, 1.61
--------------------------------------------------------------------------
Benchmark                                Time             CPU   Iterations
--------------------------------------------------------------------------
bm_make_exception_ptr/threads:1       25.6 ns         25.6 ns     27321613
bm_make_exception_ptr/threads:2       12.7 ns         25.4 ns     27881074
bm_make_exception_ptr/threads:4       6.84 ns         27.4 ns     27500684
bm_make_exception_ptr/threads:8       3.56 ns         28.5 ns     25787520
Comparing ./main_libcxxabi to ./patch_libcxxabi
Benchmark                                         Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------
bm_make_exception_ptr/threads:1                -0.9656         -0.9656           745            26           745            26
bm_make_exception_ptr/threads:2                -0.9844         -0.9844           816            13          1633            25
bm_make_exception_ptr/threads:4                -0.9921         -0.9915           863             7          3208            27
bm_make_exception_ptr/threads:8                -0.9968         -0.9965          1120             4          8064            28
OVERALL_GEOMEAN                                -0.9892         -0.9887             0             0             0             0

It can be seen that with the patch not only the performance is x30(!) in single-threaded case, but it also scales linearly with the number of threads (CPU / Time == number of threads), when on current trunk it's not the case: i'm using not the most recent gcc's unwinder, so contention on the lock in dl_iterate_phdr takes place.

The effect is the same with libcxxrt, although it seems to be a bit slower overall.

I've addressed review comments and the build is green, so re-requesting the review.

[jyknight]

Overall, seems like a good idea; I haven't reviewed the whole thing, but here's some comments.

[jyknight]

It would be better to use a weak reference to the __cxa_init_primary_exception instead of dlsym.

That way, the loader takes care of the lookup, and if you are building libc++ and libc++abi together (which is the common case), there's no extra overhead.

[itrofimow]

Apparently that doesn't seem to work with Apple.
I'm fine with that, since i'm only interested in linux platform, and for now i ifdefed-out Apple

[jyknight]

Can't use typeid with -fno-rtti, but exceptions can be used with -fno-rtti so the previous impl would work. So this code should all be in a !_LIBCPP_HAS_NO_RTTI ifdef.

[frederick-vs-ja]

Can't use typeid with -fno-rtti, but exceptions can be used with -fno-rtti so the previous impl would work. So this code should all be in a !_LIBCPP_HAS_NO_RTTI ifdef.

Hmm... (at least on Itanium ABI?) when exceptions need to be thrown, type_info objects are generated and passed on throwing even if -fno-rtti is used.

Is there any intrinsic to obtain RTTI for EH when -fno-rtti is used?

[itrofimow]

Hi! I've addressed/answered the inline comments and manually checked that libcxx builds + works with libcxxabi/libsupc++/libcxxrt (should there be a CI-build with such combinations?), so re-requesting the review.

@philnik777 @jyknight @ldionne

[EricWF]

This has got to be an aliasing violation that the compiler can optimize on?

The types have two different sizes.

[itrofimow]

Do they?

llvm-project/libcxx/src/support/runtime/exception_pointer_glibcxx.ipp

Lines 42 to 47 in ea1909f

	exception_ptr::exception_ptr(const exception_ptr& other) noexcept
	: __ptr_(other.__ptr_)
	{
	new (reinterpret_cast<void*>(this)) __exception_ptr::exception_ptr(
	reinterpret_cast<const __exception_ptr::exception_ptr&>(other));
	}

My understanding is that this is a hackery of course, but the hackery that should work:

llvm-project/libcxx/src/support/runtime/exception_pointer_glibcxx.ipp

Lines 13 to 16 in ea1909f

	// we have little choice but to hijack std::__exception_ptr::exception_ptr's
	// (which fortunately has the same layout as our std::exception_ptr) copy
	// constructor, assignment operator and destructor (which are part of its
	// stable ABI), and its rethrow_exception(std::__exception_ptr::exception_ptr)

[EricWF]

Explicit please.

[EricWF]

We should have an unreachable annotation at the bottom of this block.

[itrofimow]

Added it, but could you please explain to me why is it needed?

[EricWF]

How do we know that we don't also have to destroy the constructed exception?

[itrofimow]

We only get here if the exception constructor threw, so there's no object to destroy. Am i missing something?

[EricWF]

This code allocates for _Ep here but then constructs _Ep2. Similarly, it uses the typeinfo for _Ep.

We're sure this is correct?

[ldionne]

Why is this disabled on Apple and on Windows? It seems to me like this should be an availability macro instead so that all platforms can take advantage of it as long as the deployment target is recent enough to support this new addition. I think this is what @philnik777 was referring to in another comment.

[itrofimow]

The "availability macro": is that https://clang.llvm.org/docs/AttributeReference.html#availability?
Anyway, i'm failing to understand how that could eliminate the need to have a runtime check for the __cxa_init_primary_exception presence -- say, if i build against 18.0 libcxxabi and then LD_PRELOAD 17.0 version, which should be a supported case, right?

---

To answer your question directly:
Windows: disabled due to mingw requiring this

llvm-project/libcxxabi/include/__cxxabi_config.h

Line 101 in ea1909f

#define _LIBCXXABI_DTOR_FUNC __thiscall

to be specified for exception destructor, and this attribute now has to appear in libc++ at the call site (where weak declaration of the __cxa_init_primary_exception is present and the exception destructor is defined). I didn't want to copy-paste this macro in libc++, but definitely doable, should i implement it?

Apple: back-deployment-macosx tests fail (https://buildkite.com/llvm-project/libcxx-ci/builds/30969#018b50e0-c82b-42e0-bceb-b74d5808ffad, for example) with dyld[47810]: missing symbol called error. I have very limited knowledge about how linking is performed on Apple, but my quick googling tells that it's much different from what ld does, and weak attributes don't work without some compiler flags specified. I have no clue how this should be fixed and it also takes forever for CI to reach this check, so any help would be appreciated.

[itrofimow]

I've added a mingw support

[ldionne]

If you look at how the availability macros in __availability work, basically we use the --target <arch>-<vendor>-<platform><version> to drive this. The idea is that you tell the compiler what's the oldest version of the platform you are targeting, and based on that we make the function available or not. If you then decide to run your program on an older platform, you effectively lied to the compiler and in practice you'll get a load-time error.

[itrofimow]

I had to spend some time bisecting very cryptic error: undefined symbol: _Unwind_Resume when building libc++ with libcxxrt/libsupc++ to this PR #77687, so i've adjusted the docs to explicitly disable flipped default.

Building with libcxxrt also requires DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS to be set, otherwise operator new/operator delete are undefined.

Also, addressing the last outstanding comment about undef-ing the availability macro i realized that _LIBCPP_AVAILABILITY_INIT_PRIMARY_EXCEPTION isn't used anywhere and I'm not really sure to what it should be applied.

[ldionne]

Thanks, LGTM!

[itrofimow]

The tests failing are ARMv7-picolib tests, and they fail with

Unexpectedly Passed Tests (2):
  armv7m-picolibc-libc++.cfg.in :: libcxx/selftest/modules/std-module.sh.cpp
  armv7m-picolibc-libc++.cfg.in :: std/modules/std.pass.cpp

which i don't think is related to this PR changes

[ldionne]

Indeed, those CI failures are unrelated and should be fixed on main. Merging this now.

[itrofimow]

Indeed, those CI failures are unrelated and should be fixed on main. Merging this now.

Thank you for all the time you've spent helping me with this PR.
Please don't hesitate to ping me here or on discord if something goes wrong with these changes.

[nico]

Hello, my (unsupported, non-standard, likely-not-correctly-set-up) bot started failing to build after this: http://45.33.8.238/linux/128697/step_4.txt

Looking through the code a bit, I think libcxx/src/exception.cpp includes support/runtime/exception_pointer_unimplemented.ipp if we're on non-MS and neither of _LIBCPPABI_VERSION, LIBCXXRT, __GLIBCXX__ are defined. Before this change, things linked fine in that config. But since this adds a call to __from_native_exception_pointer() on non-MS unconditionally but doesn't add an impl of that to exception_pointer_unimplemented.ipp, that config now fails to link. Is that an intentional change?

(For my bot, it's probably incorrect that none of these macros are defined! But exception.cpp kind of looks like that case is supposed to work, so I thought I'd raise this here.)

[itrofimow]

Hi @nico! No, that is absolutely not intentional, i've overlooked that, sorry.
Could you please check if your bot builds with this patch applied? https://github.com/llvm/llvm-project/pull/79040/files

[nico]

I checked out 309dcc1^ and verified that I could repro the link error. Then I patched in #79040 and verified that it went away. So that PR helps, thanks!

[DimitryAndric]

FWIW this caused an incompatibility with recent libcxxrt (after libcxxrt/libcxxrt@03c83f5), since its cxxabi.h declares __cxa_allocate_exception, __cxa_free_exception and __cxa_init_primary_exception without throw() specifiers.

I submitted libcxxrt/libcxxrt#25, since I am not sure about the official signature of these functions, if there is any.

[ldionne]

@DimitryAndric Thanks, please keep us in the loop. If we need to make a fix on our side, @itrofimow we'll want to cherry-pick it to LLVM 18.

[dankm]

@ldionne it looks like the consensus is that GCC and LLVM are correct and that we're fixing libcxxrt. The relevant pull request is libcxxrt/libcxxrt#27.

[ldionne]

Thanks for the heads up!