[Driver] Introduce -stdlib++-isystem

[smeenai]

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.

[danalbert]

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 :)

[smeenai]

In D64089#1567028, @danalbert wrote:

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.

[smeenai]

Ping.

[ldionne]

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.

[smeenai]

In D64089#1576902, @ldionne wrote:

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

[smeenai]

Ping.

[ldionne]

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]

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]

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

[smeenai]

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

[phosek]

LGTM

cfe/trunk/include/clang/Driver/Options.td

ValuesCode<[{
     const char *Values =
     #define LANGSTANDARD(id, name, lang, desc, features) name ","
     #define LANGSTANDARD_ALIAS(id, alias) alias ","
     #include "clang/Basic/LangStandards.def"
     ;
   }]>;
 def stdlib_EQ : Joined<["-", "--"], "stdlib=">, Flags<[CC1Option]>,
   HelpText<"C++ standard library to use">, Values<"libc++,libstdc++,platform">;
+def stdlibxx_isystem : JoinedOrSeparate<["-"], "stdlib++-isystem">,
+  Group<clang_i_Group>,
+  HelpText<"Use directory as the C++ standard library include path">,
+  Flags<[DriverOption]>, MetaVarName<"<directory>">;
 def unwindlib_EQ : Joined<["-", "--"], "unwindlib=">, Flags<[CC1Option]>,
   HelpText<"Unwind library to use">, Values<"libgcc,unwindlib,platform">;
 def sub__library : JoinedOrSeparate<["-"], "sub_library">;
 def sub__umbrella : JoinedOrSeparate<["-"], "sub_umbrella">;
 def system_header_prefix : Joined<["--"], "system-header-prefix=">,
   Group<clang_i_Group>, Flags<[CC1Option]>, MetaVarName<"<prefix>">,
   HelpText<"Treat all #include paths starting with <prefix> as including a "
            "system header.">;

cfe/trunk/include/clang/Driver/ToolChain.h

public:
   virtual UnwindLibType GetUnwindLibType(const llvm::opt::ArgList &Args) const;
 
   /// AddClangCXXStdlibIncludeArgs - Add the clang -cc1 level arguments to set
   /// the include paths to use for the given C++ standard library type.
   virtual void
   AddClangCXXStdlibIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                                llvm::opt::ArgStringList &CC1Args) const;
 
+  /// AddClangCXXStdlibIsystemArgs - Add the clang -cc1 level arguments to set
+  /// the specified include paths for the C++ standard library.
+  void AddClangCXXStdlibIsystemArgs(const llvm::opt::ArgList &DriverArgs,
+                                    llvm::opt::ArgStringList &CC1Args) const;
+
   /// Returns if the C++ standard library should be linked in.
   /// Note that e.g. -lm should still be linked even if this returns false.
   bool ShouldLinkCXXStdlib(const llvm::opt::ArgList &Args) const;
 
   /// AddCXXStdlibLibArgs - Add the system specific linker arguments to use
   /// for the given C++ standard library type.
   virtual void AddCXXStdlibLibArgs(const llvm::opt::ArgList &Args,
                                    llvm::opt::ArgStringList &CmdArgs) const;

cfe/trunk/lib/Driver/ToolChain.cpp

void ToolChain::AddClangCXXStdlibIncludeArgs(const ArgList &DriverArgs,
   //
   // While it is being called, replicate a bit of a hack to propagate the
   // '-stdlib=' flag down to CC1 so that it can in turn customize the C++
   // header search paths with it. Once all systems are overriding this
   // function, the CC1 flag and this line can be removed.
   DriverArgs.AddAllArgs(CC1Args, options::OPT_stdlib_EQ);
 }
 
+void ToolChain::AddClangCXXStdlibIsystemArgs(
+    const llvm::opt::ArgList &DriverArgs,
+    llvm::opt::ArgStringList &CC1Args) const {
+  DriverArgs.ClaimAllArgs(options::OPT_stdlibxx_isystem);
+  if (!DriverArgs.hasArg(options::OPT_nostdincxx))
+    for (const auto &P :
+         DriverArgs.getAllArgValues(options::OPT_stdlibxx_isystem))
+      addSystemInclude(DriverArgs, CC1Args, P);
+}
+
 bool ToolChain::ShouldLinkCXXStdlib(const llvm::opt::ArgList &Args) const {
   return getDriver().CCCIsCXX() &&
          !Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs,
                       options::OPT_nostdlibxx);
 }
 
 void ToolChain::AddCXXStdlibLibArgs(const ArgList &Args,
                                     ArgStringList &CmdArgs) const {

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

if (A->getOption().matches(options::OPT_include)) {
       }
     } else if (A->getOption().matches(options::OPT_isystem_after)) {
       // Handling of paths which must come late.  These entries are handled by
       // the toolchain itself after the resource dir is inserted in the right
       // search order.
       // Do not claim the argument so that the use of the argument does not
       // silently go unnoticed on toolchains which do not honour the option.
       continue;
+    } else if (A->getOption().matches(options::OPT_stdlibxx_isystem)) {
+      // Translated to -internal-isystem by the driver, no need to pass to cc1.
+      continue;
     }
 
     // Not translated, render as usual.
     A->claim();
     A->render(Args, CmdArgs);
   }
 
   Args.AddAllArgs(CmdArgs,
void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
   // OBJCPLUS_INCLUDE_PATH - system includes enabled when compiling ObjC++.
   addDirectoryList(Args, CmdArgs, "-objcxx-isystem", "OBJCPLUS_INCLUDE_PATH");
 
   // While adding the include arguments, we also attempt to retrieve the
   // arguments of related offloading toolchains or arguments that are specific
   // of an offloading programming model.
 
   // Add C++ include arguments, if needed.
-  if (types::isCXX(Inputs[0].getType()))
-    forAllAssociatedToolChains(C, JA, getToolChain(),
-                               [&Args, &CmdArgs](const ToolChain &TC) {
-                                 TC.AddClangCXXStdlibIncludeArgs(Args, CmdArgs);
-                               });
+  if (types::isCXX(Inputs[0].getType())) {
+    bool HasStdlibxxIsystem = Args.hasArg(options::OPT_stdlibxx_isystem);
+    forAllAssociatedToolChains(
+        C, JA, getToolChain(),
+        [&Args, &CmdArgs, HasStdlibxxIsystem](const ToolChain &TC) {
+          HasStdlibxxIsystem ? TC.AddClangCXXStdlibIsystemArgs(Args, CmdArgs)
+                             : TC.AddClangCXXStdlibIncludeArgs(Args, CmdArgs);
+  }
 
   // Add system include arguments for all targets but IAMCU.
   if (!IsIAMCU)
     forAllAssociatedToolChains(C, JA, getToolChain(),
                                [&Args, &CmdArgs](const ToolChain &TC) {
                                  TC.AddClangSystemIncludeArgs(Args, CmdArgs);
                                });
   else {

cfe/trunk/test/Driver/stdlibxx-isystem.cpp

+// Backslash escaping makes matching against the installation directory fail on
+// Windows. Temporarily disable the test there until we add an option to print
+// the installation directory unescaped.
+// UNSUPPORTED: system-windows
+
+// By default, we should search for libc++ next to the driver.
+// RUN: mkdir -p %t/bin
+// RUN: mkdir -p %t/include/c++/v1
+// RUN: %clang -target aarch64-linux-android -ccc-install-dir %t/bin \
+// RUN:   -stdlib=libc++ -fsyntax-only %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=LIBCXX %s
+// RUN: %clang -target x86_64-apple-darwin -ccc-install-dir %t/bin \
+// RUN:   -stdlib=libc++ -fsyntax-only %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=LIBCXX %s
+// LIBCXX: InstalledDir: [[INSTALLDIR:.+$]]
+// LIBCXX: "-internal-isystem" "[[INSTALLDIR]]/../include/c++/v1"
+
+// Passing -stdlib++-isystem should suppress the default search.
+// RUN: %clang -target aarch64-linux-android -ccc-install-dir %t/bin \
+// RUN:   -stdlib++-isystem /tmp/foo -stdlib++-isystem /tmp/bar -stdlib=libc++ \
+// RUN:   -fsyntax-only %s -### 2>&1 | FileCheck -check-prefix=NODEFAULT %s
+// RUN: %clang -target x86_64-apple-darwin -ccc-install-dir %t/bin \
+// RUN:   -stdlib++-isystem /tmp/foo -stdlib++-isystem /tmp/bar -stdlib=libc++ \
+// RUN:   -fsyntax-only %s -### 2>&1 | FileCheck -check-prefix=NODEFAULT %s
+// NODEFAULT: InstalledDir: [[INSTALLDIR:.+$]]
+// NODEFAULT-NOT: "-internal-isystem" "[[INSTALLDIR]]/../include/c++/v1"
+
+// And we should add it as an -internal-isystem.
+// RUN: %clang -target aarch64-linux-android -ccc-install-dir %t/bin \
+// RUN:   -stdlib++-isystem /tmp/foo -stdlib++-isystem /tmp/bar -stdlib=libc++ \
+// RUN:   -fsyntax-only %s -### 2>&1 | FileCheck -check-prefix=INCPATH %s
+// RUN: %clang -target x86_64-apple-darwin -ccc-install-dir %t/bin \
+// RUN:   -stdlib++-isystem /tmp/foo -stdlib++-isystem /tmp/bar -stdlib=libc++ \
+// RUN:   -fsyntax-only %s -### 2>&1 | FileCheck -check-prefix=INCPATH %s
+// INCPATH: "-internal-isystem" "/tmp/foo" "-internal-isystem" "/tmp/bar"
+
+// We shouldn't pass the -stdlib++-isystem to cc1.
+// RUN: %clang -target aarch64-linux-android -ccc-install-dir %t/bin \
+// RUN:   -stdlib++-isystem /tmp -stdlib=libc++ -fsyntax-only %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=NOCC1 %s
+// RUN: %clang -target x86_64-apple-darwin -ccc-install-dir %t/bin \
+// RUN:   -stdlib++-isystem /tmp -stdlib=libc++ -fsyntax-only %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=NOCC1 %s
+// NOCC1-NOT: "-stdlib++-isystem" "/tmp"
+
+// It should respect -nostdinc++.
+// RUN: %clang -target aarch64-linux-android -ccc-install-dir %t/bin \
+// RUN:   -stdlib++-isystem /tmp/foo -stdlib++-isystem /tmp/bar -nostdinc++ \
+// RUN:   -fsyntax-only %s -### 2>&1 | FileCheck -check-prefix=NOSTDINCXX %s
+// RUN: %clang -target x86_64-apple-darwin -ccc-install-dir %t/bin \
+// RUN:   -stdlib++-isystem /tmp/foo -stdlib++-isystem /tmp/bar -nostdinc++ \
+// RUN:   -fsyntax-only %s -### 2>&1 | FileCheck -check-prefix=NOSTDINCXX %s
+// NOSTDINCXX-NOT: "-internal-isystem" "/tmp/foo" "-internal-isystem" "/tmp/bar"