Page MenuHomePhabricator

[libcxxabi] Prevent cmake from removing our explicit system C++ include paths
ClosedPublic

Authored by sbc100 on Nov 7 2019, 2:58 PM.

Details

Summary

We build with -nostdinc++ and add our own header path via
LIBCXXABI_LIBCXX_INCLUDES. However cmake tries to be clever and if
LIBCXXABI_LIBCXX_INCLUDES happens to match the compilers system path
it will remove the -I flag meaning we can't access any C++ headers.

This problem only happens when LIBCXXABI_LIBCXX_INCLUDES and the
compilers default C++ include path are the same thing. This happens on the
wasm waterfall where we build and install libcxx into the compiler default
location before we build libcxxabi.

Ideally cmake would be able see that we are using -nostdinc++ and
disable this behaviour.

See https://gitlab.kitware.com/cmake/cmake/issues/19227

Diff Detail

Event Timeline

sbc100 created this revision.Nov 7 2019, 2:58 PM
Herald added a project: Restricted Project. · View Herald Transcript
sbc100 edited the summary of this revision. (Show Details)Nov 7 2019, 2:58 PM
sbc100 added a reviewer: dschuff.
sbc100 edited the summary of this revision. (Show Details)Nov 7 2019, 3:02 PM
sbc100 added a comment.Nov 7 2019, 3:05 PM

We upgraded cmake and our build started failing. This was the best fix I could come up with.

phosek added a subscriber: phosek.

@beanz @abdulras @smeenai do you know if there's a better way to handle this?

sbc100 added a comment.Nov 7 2019, 3:09 PM

I'm not sure of the exact cmake version where this starting breaking but we saw it when upgrading from 3.7.2 to 3.15.3.

You should report a bug to CMake (you're right that their implicit removal should account for the various -nostdinc-like options), but this seems like an okay workaround in the meantime.

Do you need something like this in libc++ as well?

sbc100 added a comment.Nov 7 2019, 8:38 PM

I don't think so no, its the dependency between libcxxabi and the libcxx that is the issue. I think libcxxabi assumes the libcxx is in non-default place when it builds. Perhaps out case that actually isn't true so another possible fix might be to add LIBCXXABI_USE_SYSTEM_LIBCXX which would avoid the need to -nostdinc (although maybe thats needed for other reasons too).

beanz added a comment.Nov 8 2019, 10:29 AM

Reviewing the CMake change more closely, that is a bug. They shouldn't have changed the default behavior in that way. At the very least that change should have been a policy change.

Even before that change it looks like stripImplicitInclDirs = true.. looks like the behaviour might predate that change?

sbc100 updated this revision to Diff 228537.Nov 8 2019, 3:41 PM
  • Fix typos and add link to cmake bug
sbc100 edited the summary of this revision. (Show Details)Nov 8 2019, 3:41 PM

Great. I added a reference to the upstream bug. This should be good to go no. Sadly we will need to keep this around even after the bug is fixes since the issue exists large version range of existing cmake releases.

beanz added a comment.Nov 8 2019, 4:00 PM

This should only be broken for CMake 3.14 and later. We should include that in the comments around this change.

sbc100 updated this revision to Diff 228542.Nov 8 2019, 4:50 PM
sbc100 edited the summary of this revision. (Show Details)
  • update comment

I don't think so no, its the dependency between libcxxabi and the libcxx that is the issue. I think libcxxabi assumes the libcxx is in non-default place when it builds. Perhaps out case that actually isn't true so another possible fix might be to add LIBCXXABI_USE_SYSTEM_LIBCXX which would avoid the need to -nostdinc (although maybe thats needed for other reasons too).

Hmm, something like that might be cleaner, though the -nostdinc++ might still be useful for e.g. making sure you don't accidentally include the system's cxxabi.h (although it appears that Xcode puts cxxabi.h in the sysroot include directory instead of the C++-specific include directory, so -nostdinc++ isn't gonna do you any good there).

@ldionne, what do you think?

Ping. I'd like to land this ASAP as its currently causing out waterfall fail: https://ci.chromium.org/p/wasm/g/toolchain/console

smeenai accepted this revision.Nov 11 2019, 11:09 AM

Accepting to unblock, though I'd like to see the LIBCXXABI_USE_SYSTEM_LIBCXX solution explored as a follow-up.

This revision is now accepted and ready to land.Nov 11 2019, 11:09 AM
phosek accepted this revision.Nov 11 2019, 5:52 PM

LGTM

This revision was automatically updated to reflect the committed changes.