Page MenuHomePhabricator

[Driver] Introduce -stdlib++-isystem
ClosedPublic

Authored by smeenai on Jul 2 2019, 11:00 AM.

Details

Summary

There are times when we wish to explicitly control the C++ standard
library search paths used by the driver. For example, when we're
building against the Android NDK, we might want to use the NDK's C++
headers (which have a custom inline namespace) even if we have C++
headers installed next to the driver. We might also be building against
a non-standard directory layout and wanting to specify the C++ standard
library include directories explicitly.

We could accomplish this by passing -nostdinc++ and adding an explicit
-isystem for our custom search directories. However, users of our
toolchain may themselves want to use -nostdinc++ and a custom C++ search
path (libc++'s build does this, for example), and our added -isystem
won't respect the -nostdinc++, leading to multiple C++ header
directories on the search path, which causes build failures.

Add a new driver option -stdlib++-isystem to support this use case.
Passing this option suppresses adding the default C++ library include
paths in the driver, and it also respects -nostdinc++ to allow users to
still override the C++ library paths themselves.

It's a bit unfortunate that we end up with both -stdlib++-isystem and
-cxx-isystem, but their semantics differ significantly. -cxx-isystem is
unaffected by -nostdinc++ and is added to the end of the search path
(which is not appropriate for C++ standard library headers, since they
often #include_next into other system headers), while -stdlib++-isystem
respects -nostdinc++, is added to the beginning of the search path, and
suppresses the default C++ library include paths.

Diff Detail

Repository
rL LLVM

Event Timeline

smeenai created this revision.Jul 2 2019, 11:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2019, 11:00 AM
Herald added a subscriber: srhines. · View Herald Transcript

For example, when we're building against the Android NDK, we might want to use the NDK's C++ headers (which have a custom inline namespace) even if we have C++ headers installed next to the driver.

Since NDK r19 the NDK libc++ headers are already installed alongside the driver, so this doesn't benefit that use case. The others sound useful, just FYI in case the NDK was the main motivating factor here :)

For example, when we're building against the Android NDK, we might want to use the NDK's C++ headers (which have a custom inline namespace) even if we have C++ headers installed next to the driver.

Since NDK r19 the NDK libc++ headers are already installed alongside the driver, so this doesn't benefit that use case. The others sound useful, just FYI in case the NDK was the main motivating factor here :)

We're using our own toolchain (which ends up with the upstream libc++ headers installed alongside the driver) but building against the NDK, so that doesn't work for us, unfortunately.

Background question: I'm familiar with the behavior on Darwin, where we prefer C++ headers in the toolchain (alongside the driver), however do other platforms behave the same? I thought this was something specific to Darwin.

Background question: I'm familiar with the behavior on Darwin, where we prefer C++ headers in the toolchain (alongside the driver), however do other platforms behave the same? I thought this was something specific to Darwin.

The Linux toolchain (which is used for non-GNU Linux targets, e.g. Android) also prefers C++ headers in the toolchain: https://github.com/llvm/llvm-project/blob/a6548d04375b49da989103c403d83475cc2f8ce4/clang/lib/Driver/ToolChains/Linux.cpp#L891. There's a FIXME in the GCC toolchain to adopt that behavior too: https://github.com/llvm/llvm-project/blob/a6548d04375b49da989103c403d83475cc2f8ce4/clang/lib/Driver/ToolChains/Gnu.cpp#L2619

ormris removed a subscriber: ormris.Jul 9 2019, 2:46 PM
ldionne added inline comments.Jul 16 2019, 10:40 AM
clang/lib/Driver/ToolChains/Clang.cpp
1307 ↗(On Diff #207593)

So, before, we would populate -stdlib=XXX here unconditionally for cc1.

After the patch, we do _not_ populate -stdlib=XXX and instead we pass -internal-isystem to cc1 with the contents of -stdlib++-isystem whenever that option is provided, otherwise we fall back to the previous behaviour.

Did I get that right? If so, could we investigate getting rid of AddClangCXXStdlibIncludeArgs altogether? I don't think it is needed anymore since my refactor of the driver for Darwin.

smeenai marked an inline comment as done.Jul 24 2019, 5:17 PM
smeenai added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
1307 ↗(On Diff #207593)

Sorry, I'd missed this. You're right; we'll continue passing -stdlib=XXX in the old case (i.e. when there's no -stdlib++-isystem argument), but not if it's there.

When you say getting rid of AddClangCXXStdlibIncludeArgs, that's the one in the driver (in the toolchains) right? I thought your Darwin refactor involved moving C++ header search logic from the frontend into the driver ... did you mean getting rid of the C++ header search path logic in the frontend?

ldionne accepted this revision.Mon, Jul 29, 12:05 PM

This looks fine to me, but I'd like to have someone else familiar with the Driver/Frontend design sign off too. The implementation is trivial but I'd like support on the fact that this is a good idea (which I think it is).

clang/lib/Driver/ToolChains/Clang.cpp
1307 ↗(On Diff #207593)

Sorry, I should have been more clear. I meant specifically getting rid of this: https://github.com/llvm-project/clang/blob/master/lib/Driver/ToolChain.cpp#L832

In that snippet, we pass -stdlib=XXX down to the frontend. My understanding is that this was only necessary because the frontend on Darwin needed to know which stdlib was in use, in order to setup the header search paths correctly. Now that it's not needed anymore, I think we could get rid of that hack altogether.

However, looking at it further, changing that is way beyond the scope of your PR. I had not realized that other non-Darwin toolchain provided implementations of AddClangCXXStdlibIncludeArgs (which is virtual and meant to be overridden).

This revision is now accepted and ready to land.Mon, Jul 29, 12:05 PM
smeenai added a reviewer: rnk.Mon, Aug 5, 7:38 PM
smeenai added a subscriber: rnk.

Adding @rnk as someone familiar with the driver/frontend :)

phosek accepted this revision.Mon, Aug 5, 10:22 PM

LGTM

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Aug 5, 11:49 PM