Page MenuHomePhabricator

[libc++abi] Do not define new/delete by default
Needs RevisionPublic

Authored by ldionne on Oct 1 2019, 6:59 AM.

Details

Reviewers
EricWF
Summary

As the removed comment says, definitions of new/delete are normally provided in libc++, so it doesn't make sense to provide them in libc++abi unless requested explicitly.

The only thing preventing us from switching the default to OFF was thought to be GCC 4.9, but in reality it's that the linker used by GCC on Linux doesn't resolve references to new/delete from libc++abi to the definitions in libc++. Re-linking libc++ after libc++abi in the tests for libc++abi does the trick.

Furthermore, this only changes the default behavior of libc++abi: vendors wishing to ship new/delete in libc++abi can do so by asking for it explicitly when building libc++abi

Event Timeline

ldionne created this revision.Oct 1 2019, 6:59 AM
ldionne added a comment.EditedOct 1 2019, 7:07 AM

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.

@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 updated this revision to Diff 222629.Oct 1 2019, 8:34 AM

Fix linker error with GCC

ldionne edited the summary of this revision. (Show Details)Oct 1 2019, 8:34 AM

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.

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 added a subscriber: emaste.Oct 2 2019, 2:34 PM

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 added a comment.Oct 2 2019, 6:21 PM

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

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

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 requested changes to this revision.Oct 3 2019, 10:59 AM
EricWF added inline comments.
libcxx/utils/libcxx/test/config.py
765

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

This revision now requires changes to proceed.Oct 3 2019, 10:59 AM

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.

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.

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.

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.

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.

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.