Page MenuHomePhabricator

[libc++] Do not cleverly link against libc++abi just because it happens to be there
ClosedPublic

Authored by ldionne on Jun 27 2019, 8:59 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ldionne created this revision.Jun 27 2019, 8:59 AM

This solves the problem that currently, NOT specifying LLVM_ENABLE_PROJECTS=libcxxabi and building libc++ will fail because cxxabi_shared can't be found.

Still an open question is whether we need to retain the in-tree include paths of libc++abi, or whether linking against the cxxabi_shared target is enough. @EricWF, can you remind me why we install libc++abi headers in include/c++/v1 again?

Here's my take on this, which I hadn't had time to follow up on: https://reviews.llvm.org/D62060 Maybe it provides at least some mild entertainment.

Here's my take on this, which I hadn't had time to follow up on: https://reviews.llvm.org/D62060 Maybe it provides at least some mild entertainment.

Your approach is fine too, but I like mine better because it gets rid of LIBCXX_CXX_ABI_INTREE and removes more complexity than yours.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 5 2019, 10:06 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2019, 10:06 AM
vitalybuka reopened this revision.Jul 8 2019, 10:47 AM
vitalybuka added a subscriber: vitalybuka.

Reverted with r365359

Yes. Eric told me the bots were now specifying the ABI library to be libc++abi explicitly, but apparently that's not true (or at least not for some of them).

I'll coordinate with him to check this in again once the build bots have the fix.

Okay, so what happened is that it usually takes about a week for changes to the build bot configuration to actually be reflected on the bots. I should have known that. I'll wait before I recommit this.

Sanitizers use sh scripts which can be checked out and executed on most linux dev workstations.
http://llvm.org/viewvc/llvm-project/zorg/trunk/zorg/buildbot/builders/sanitizers/
Changes to these scripts are effective immediately.

Could you tell me specifically what change to buildbot is requested? I must've missed the original thread.

Could you tell me specifically what change to buildbot is requested? I must've missed the original thread.

The build bots must specify -DLIBCXX_CXX_ABI=libcxxabi at the CMake configure step. Otherwise, they'll end up using the default ABI library on Linux, which uses the xxx_fallback.ipp files from libc++, and that doesn't implement everything we need.

mgorny added a comment.Jul 9 2019, 6:42 AM

The build bots must specify -DLIBCXX_CXX_ABI=libcxxabi at the CMake configure step. Otherwise, they'll end up using the default ABI library on Linux, which uses the xxx_fallback.ipp files from libc++, and that doesn't implement everything we need.

Thanks. I've added that to NetBSD buildbot, so feel free to recommit once others are ready.

smeenai added a subscriber: smeenai.Jul 9 2019, 1:06 PM

I think it's worth advertising the required configuration change (to explicitly say -DLIBCXX_CXX_ABI=libcxxabi) in the documentation and/or the release notes, otherwise the behavior change will be surprising.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Aug 7, 2:35 PM
This revision was automatically updated to reflect the committed changes.

I'll monitor the bots, but if someone gets here because their build is broken by this commit, it's fine to revert it (but please notify me)!

For some reason, this change appears to be very trick to land.