[libc++] Define new/delete in libc++abi only by default

[ldionne]

Summary

Previously, we would define new/delete in both libc++ and libc++abi. Not only does this cause code bloat, but also it's technically an ODR violation since we don't know which operator will be selected. Furthermore, since those are weak definitions, we should strive to have as few of them as possible (to improve load times).

My preferred choice would have been to put the operators in libc++ only by default, however that would create a circular dependency between libc++ and libc++abi, which GNU linkers don't handle.

Folks who want to ship new/delete in libc++ instead of libc++abi are free to do so by turning on LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS at CMake configure time.

On Apple platforms, this shouldn't be an ABI break because we re-export the new/delete symbols from libc++abi. This change actually makes libc++ behave closer to the system libc++ shipped on Apple platforms.

On other platforms, this is an ABI break for people linking against libc++ but not libc++abi.

[ldionne]

So, it turns out that even with GCC 9, this will cause errors like this (on my Linux Docker vm):

<...>lib/libc++abi.so: undefined reference to `operator delete(void*)'
<...>lib/libc++abi.so: undefined reference to `operator new[](unsigned long)'
<...>lib/libc++abi.so: undefined reference to `operator delete[](void*)'
<...>lib/libc++abi.so: undefined reference to `operator delete(void*, unsigned long)'

I see three solutions:

1. By default, provide new/delete in libc++abi but not in libc++. I'm not sure I like that.
2. Require that libc++abi be built with -DLIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS=ON on GCC. This means we need to change the CI builders to add that option.
3. "Fix" GCC and wait multiple years for GCC 9 not to be supported anymore before we switch the default.

I don't care whether we do (1) or (2) -- at Apple we specify -DLIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS=ON explicitly when we build libc++abi anyway. However, we need to align libc++abi and libc++ regarding who provides new/delete in the default LLVM configuration.

[ldionne]

@EricWF My preference would be to do (2) for the short term, and (3) in the long term. Whoever builds libc++abi from source using GCC needs to know what they're doing anyway. WDYT?

[ldionne]

Fix linker error with GCC

[ldionne]

It turns out the GCC problem was only due to the linker not resolving the undefined new/delete references in libc++abi to libc++. Adding another -lc++ after -lc++abi fixes the issue.

I think this patch is good to go as it is now.

[ldionne]

Pinging vendors for awareness of the change: @phosek @danalbert @dim

This is going in soon and will require a change in how you build libc++abi if you need new/delete definitions to be provided in libc++abi.so, namely to use -DLIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS=ON when you configure CMake for libc++abi.

The upshot of this change is that by default, we won't get both definitions in libc++ and libc++abi, thus unbreaking the default configuration.

[dim]

In D68269#1691913, @ldionne wrote:

Pinging vendors for awareness of the change: @phosek @danalbert @dim

This is going in soon and will require a change in how you build libc++abi if you need new/delete definitions to be provided in libc++abi.so, namely to use -DLIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS=ON when you configure CMake for libc++abi.

The upshot of this change is that by default, we won't get both definitions in libc++ and libc++abi, thus unbreaking the default configuration.

Hm, we don't use libc++abi in FreeBSD (at least not by default), but libcxxrt, which has its own new/delete definitions. I never noticed a conflict with libc++ though, since when is it providing new/delete by itself?

[emaste]

For reference libcxxrt comes from here: https://github.com/libcxxrt/libcxxrt

[mclow.lists]

If the Android and FreeBSD folks are ok with this, I'm fine with it

[ldionne]

In D68269#1692076, @dim wrote:

Hm, we don't use libc++abi in FreeBSD (at least not by default), but libcxxrt, which has its own new/delete definitions. I never noticed a conflict with libc++ though, since when is it providing new/delete by itself?

libc++ always provides new/delete unless you provide -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=OFF at CMake configuration time. Those definitions might be weak, though, and that could be why the ones from libcxxrt are used instead? It would be worth double-checking. At the very least, I can say that the definitions of new/delete are weak in both libc++ and libc++abi, on Ubuntu (tested on a Docker container).

[EricWF]

libcxx/utils/libcxx/test/config.py

765 ↗

(On Diff #222629)

We want the link line we generate to match exactly what Clang spits out. Otherwise we're potentially hiding bugs.

In this case, Clang will fail to link programs with the installed version of libc++.

[danalbert]

If the Android and FreeBSD folks are ok with this, I'm fine with it

Fine for Android for at least the platform and the NDK (we don't actually build with CMake in those cases). +srhines just in case there's another case I don't know about.

[ldionne]

So, the problem with this patch as it stands is that it introduces a link-time dependency from libc++abi back to libc++, because libc++abi needs the definition of new and delete (which would be in libc++ only with this patch). This is what I'm working around in the tests by re-adding -lc++ after -lc++abi on the compiler command line. This isn't a problem on Apple platforms IIUC since the order of command-line -l<xxx> arguments doesn't matter, but it does for linkers on most other platforms.

The alternative would be to only provide new/delete inside libc++abi, not in libc++ (by default). So, vendors (@phosek @srhines @danalbert @dim @emaste), are you OK with the default becoming that libc++abi provides new/delete, and libc++ DOES NOT (by default). If you want to keep shipping new/delete as part of libc++, you'll need to specify -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=ON at CMake configure time.

Side note: I don't care which way we go, because at Apple we specify both LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=OFF and LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS=ON explicitly anyway. What I care about is that by default, we provide the symbols in exactly one place, because that will allow me to do interesting simplifications without breaking the default configuration in upstream LLVM.

[thakis]

In D68269#1695143, @ldionne wrote:

So, the problem with this patch as it stands is that it introduces a link-time dependency from libc++abi back to libc++, because libc++abi needs the definition of new and delete (which would be in libc++ only with this patch). This is what I'm working around in the tests by re-adding -lc++ after -lc++abi on the compiler command line. This isn't a problem on Apple platforms IIUC since the order of command-line -l<xxx> arguments doesn't matter, but it does for linkers on most other platforms.

We (Chromium) currently statically link libc++ on all platforms, but use the platform default abi library. If I read the thread here right, this means we currently get operator new from libc++ but going forward we'd get it from the platform abi libraries -- but at the moment none (except darwin) have it yet. So that'd likely be an issue for us.

[ldionne]

In D68269#1695159, @thakis wrote:

In D68269#1695143, @ldionne wrote:

So, the problem with this patch as it stands is that it introduces a link-time dependency from libc++abi back to libc++, because libc++abi needs the definition of new and delete (which would be in libc++ only with this patch). This is what I'm working around in the tests by re-adding -lc++ after -lc++abi on the compiler command line. This isn't a problem on Apple platforms IIUC since the order of command-line -l<xxx> arguments doesn't matter, but it does for linkers on most other platforms.

We (Chromium) currently statically link libc++ on all platforms, but use the platform default abi library. If I read the thread here right, this means we currently get operator new from libc++ but going forward we'd get it from the platform abi libraries -- but at the moment none (except darwin) have it yet. So that'd likely be an issue for us.

This means that you folks need to keep shipping new/delete in libc++.a, which entails that you'd configure it with -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=ON at CMake-configure time. If you don't use CMake to build libc++, none of this matters to you.

[srhines]

In D68269#1693496, @danalbert wrote:

If the Android and FreeBSD folks are ok with this, I'm fine with it

Fine for Android for at least the platform and the NDK (we don't actually build with CMake in those cases). +srhines just in case there's another case I don't know about.

I don't think there are any other problems for us. When we build libc++ as part of the toolchain build, it is primarily to be used for the Linux host, because external/libcxx handles our on-device cases. I looked at our build rules briefly to make sure that everything should be fine, and agree with Dan.

[danalbert]

The alternative would be to only provide new/delete inside libc++abi, not in libc++ (by default). So, vendors (@phosek @srhines @danalbert @dim @emaste), are you OK with the default becoming that libc++abi provides new/delete, and libc++ DOES NOT (by default). If you want to keep shipping new/delete as part of libc++, you'll need to specify -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=ON at CMake configure time.

Also fine for Android. libc++abi and libc++ aren't separate (shared) libraries on Android, so it wouldn't make a difference for us.

[ldionne]

Rebase on top of master.

[phosek]

This is fine with Fuchsia, thanks for checking!

[ldionne]

Move new/delete to libc++abi instead of libc++.

This is the suggestion that had been informally approved in the review but
not implemented yet.

[ldionne]

I just updated the review to match the behavior we had agreed on informally. Because this constitutes a tricky change, I'm going to re-iterate what the change does and what the potential implications of it are, and ask that vendors acknowledge the change again.

Currently, in the vanilla LLVM build, both libc++ and libc++abi define and export the various new/delete operators. This is both wrong (ODR violation) and also bad from a code size perspective. It's also just plain confusing. This change moves the operators to libc++abi only, and removes them from libc++. This means:

• On Linux: libc++.so will NOT be exporting new/delete operators. You'll have to link against libc++abi to get those (which should already be the case in most cases). This is an ABI break if you're shipping libc++.so and it links against another ABI library that doesn't provide new/delete.
• On Apple: libc++.dylib re-exports the new/delete operators from libc++abi.dylib, so there should be no functional change here.

For vendors who want to keep the new/delete operators in libc++.dylib, they can define -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=ON (and -DLIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS=OFF) to get that behavior.

Ping @thakis @srhines @emaste @dim @danalbert @phosek @mgorny @christof @EricWF . Sorry for pinging such a large number of people, but I'd rather have a large audience for a change of this nature.

[ldionne]

Ping. Please take a look. I'll ship this in a week if nobody cares.

[dim]

In D68269#2322151, @ldionne wrote:

Ping. Please take a look. I'll ship this in a week if nobody cares.

On FreeBSD we replaced (quite a while ago now) libsupc++ with libcxxrt. Currently this also provides new and delete operators, but not all that are provided by libc++ (as of ~11.0.0 rc5):

libcxxrt:

% nm -D /lib/libcxxrt.so.1|c++filt|grep -Ew 'new|delete' 
0000000000017d10 W operator delete[](void*)
0000000000017d30 W operator delete[](void*, unsigned long)
0000000000017cc0 W operator delete(void*)
0000000000017d20 W operator delete(void*, unsigned long)
0000000000017cd0 W operator new[](unsigned long)
0000000000017c20 W operator new(unsigned long)
0000000000017c90 W operator new(unsigned long, std::nothrow_t const&)

libc++:

00000000000b5c80 W operator delete[](void*)
00000000000b5c90 W operator delete[](void*, std::nothrow_t const&)
00000000000b5e10 W operator delete[](void*, std::align_val_t)
00000000000b5e20 W operator delete[](void*, std::align_val_t, std::nothrow_t const&)
00000000000b5ca0 W operator delete[](void*, unsigned long)
00000000000b5e30 W operator delete[](void*, unsigned long, std::align_val_t)
00000000000b5c50 W operator delete(void*)
00000000000b5c60 W operator delete(void*, std::nothrow_t const&)
00000000000b5de0 W operator delete(void*, std::align_val_t)
00000000000b5df0 W operator delete(void*, std::align_val_t, std::nothrow_t const&)
00000000000b5c70 W operator delete(void*, unsigned long)
00000000000b5e00 W operator delete(void*, unsigned long, std::align_val_t)
00000000000b5c10 W operator new[](unsigned long)
00000000000b5c20 W operator new[](unsigned long, std::nothrow_t const&)
00000000000b5da0 W operator new[](unsigned long, std::align_val_t)
00000000000b5db0 W operator new[](unsigned long, std::align_val_t, std::nothrow_t const&)
00000000000b5b70 W operator new(unsigned long)
00000000000b5be0 W operator new(unsigned long, std::nothrow_t const&)
00000000000b5cb0 W operator new(unsigned long, std::align_val_t)
00000000000b5d70 W operator new(unsigned long, std::align_val_t, std::nothrow_t const&)

The libcxxrt implementations basically call malloc and free, and it has a comment:

* These definitions are intended to be used for testing and are weak symbols
 * to allow them to be replaced by definitions from a STL implementation.
 * These versions simply wrap malloc() and free(), they do not provide a
 * C++-specific allocator.

but it seems that libc++ doesn't really have a substantially different implementation. (Not sure how you would generally implement a C++ specific allocator, but there will probably be cases where you can do things more efficiently if you know that you're not just allocating flat blobs.)

That said, both libcxxrt and libc++ mark their implementations as weak, so it's good guess which one wins... I'm actually not completely sure, except for the variants that libcxxrt doesn't implement. :)

In any case, we'd at least have to add the variants with align_val_t and with the const nothrow_t to libcxxrt, to make it backwards compatible. Another option would be to switch to libc++abi, but that is quite a lot more work.

[ldionne]

In D68269#2322291, @dim wrote:

In D68269#2322151, @ldionne wrote:

Ping. Please take a look. I'll ship this in a week if nobody cares.

On FreeBSD we replaced (quite a while ago now) libsupc++ with libcxxrt. Currently this also provides new and delete operators, but not all that are provided by libc++ (as of ~11.0.0 rc5):

[...]

That said, both libcxxrt and libc++ mark their implementations as weak, so it's good guess which one wins... I'm actually not completely sure, except for the variants that libcxxrt doesn't implement. :)

In any case, we'd at least have to add the variants with align_val_t and with the const nothrow_t to libcxxrt, to make it backwards compatible. Another option would be to switch to libc++abi, but that is quite a lot more work.

The other option, which I would argue is the least amount of work, is for you to configure CMake with -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=ON. The libc++ produced with that option should match what you have today.

[dim]

In D68269#2322295, @ldionne wrote:

In D68269#2322291, @dim wrote:

In D68269#2322151, @ldionne wrote:

Ping. Please take a look. I'll ship this in a week if nobody cares.

On FreeBSD we replaced (quite a while ago now) libsupc++ with libcxxrt. Currently this also provides new and delete operators, but not all that are provided by libc++ (as of ~11.0.0 rc5):

[...]

That said, both libcxxrt and libc++ mark their implementations as weak, so it's good guess which one wins... I'm actually not completely sure, except for the variants that libcxxrt doesn't implement. :)

In any case, we'd at least have to add the variants with align_val_t and with the const nothrow_t to libcxxrt, to make it backwards compatible. Another option would be to switch to libc++abi, but that is quite a lot more work.

The other option, which I would argue is the least amount of work, is for you to configure CMake with -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=ON. The libc++ produced with that option should match what you have today.

Ah of course, silly me! I was under the impression that you were going to throw out all the operators, but you added a configure-time option instead. That would indeed be just fine for FreeBSD, at least for the time being. (Still it's probably better to update libcxxrt or switch to libc++abi at some point.)

[phosek]

LGTM for Fuchsia.

[danalbert]

LGTM for Android.

[ldionne]

Thank you folks! I'm going to ship this on Friday Oct 16th if nobody objects before then.

[rsmith]

Quite a few buildbots have been broken since this landed. For example: http://lab.llvm.org:8011/#/builders/60/builds/161

Do the bot configurations need to be updated? If so, is someone working on that?

[mstorsjo]

Just FWIW, this broke my setup marginally, but it's trivial for me to fix it. (I just hadn't noticed this thread before.) As both libcxxabi and libcxx used to enable it by default, I had resorted to disabling the one in libcxxabi, but keeping the one in libcxx enabled implicitly - as that was the default (I hadn't actually noticed that there were cmake options for controlling it in both, I only knew about the one in libcxxabi). But it's trivial to fix it by explicitly setting the desired state for both of them so that it behaves the same regardless if building the latest release or the latest master version. And I can also switch to enable it in libcxxabi and disable the one in libcxx, to match the new default config.

[ldionne]

In D68269#2340961, @rsmith wrote:

Quite a few buildbots have been broken since this landed. For example: http://lab.llvm.org:8011/#/builders/60/builds/161

Do the bot configurations need to be updated? If so, is someone working on that?

I'm looking at this.

[ldionne]

@rsmith The only two bots I'm seeing fail because of this issue seem to be:

http://lab.llvm.org:8011/#/builders/119/builds/187
http://lab.llvm.org:8011/#/builders/60/builds/147

Are you seeing others?

libcxx/docs/ReleaseNotes.rst

 What's New in Libc++ 10.0.0?
 ============================
 
 New Features
 ------------
 
 API Changes
 -----------
-- ...
+- By default, libc++abi will not include the definition for new and delete,
+  since those are provided in libc++. Vendors wishing to provide new and
+  delete in libc++abi can build the library with ``-DLIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS=ON``
+  to get back the old behavior.
+

libcxxabi/CMakeLists.txt

 option(LIBCXXABI_HAS_PTHREAD_API "Ignore auto-detection and force use of pthread API" OFF)
 option(LIBCXXABI_HAS_EXTERNAL_THREAD_API
   "Build libc++abi with an externalized threading API.
   This option may only be set to ON when LIBCXXABI_ENABLE_THREADS=ON." OFF)
 option(LIBCXXABI_BUILD_EXTERNAL_THREAD_LIBRARY
   "Build libc++abi with an externalized threading library.
    This option may only be set to ON when LIBCXXABI_ENABLE_THREADS=ON" OFF)
 
-# FIXME: This option should default to off. Unfortunatly GCC 4.9 fails to link
-# programs to due undefined references to new/delete in libc++abi. Once this
-# has been fixed or worked around the default value should be changed.
 option(LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS
     "Build libc++abi with definitions for operator new/delete. Normally libc++
-    provides these definitions" ON)
+    provides these definitions" OFF)
 option(LIBCXXABI_BUILD_32_BITS "Build 32 bit libc++abi." ${LLVM_BUILD_32_BITS})
 option(LIBCXXABI_INCLUDE_TESTS "Generate build targets for the libc++abi unit tests." ${LLVM_INCLUDE_TESTS})
 set(LIBCXXABI_LIBDIR_SUFFIX "${LLVM_LIBDIR_SUFFIX}" CACHE STRING
     "Define suffix of library directory name (32/64)")
 option(LIBCXXABI_INSTALL_LIBRARY "Install the libc++abi library." ON)
 set(LIBCXXABI_TARGET_TRIPLE "" CACHE STRING "Target triple for cross compiling.")
 set(LIBCXXABI_GCC_TOOLCHAIN "" CACHE PATH "GCC toolchain for cross compiling.")
 set(LIBCXXABI_SYSROOT "" CACHE PATH "Sysroot for cross compiling.")