[Clang] [Driver] Add option to set alternative toolchain path

[qiucf]

Summary

In some cases, we need to set alternative toolchain path other than the default with system (headers, libraries, dynamic linker prefix, ld path, etc.), e.g., to pick up newer components, but keep sysroot at the same time (to pick up extra packages).

This patch introduces a new option --overlay-platform-toolchain to set up such alternative toolchain path.

[hubert.reinterpretcast]

clang/include/clang/Driver/Driver.h

152

I don't understand the use of "in" in the comment. Perhaps "used" was meant?

clang/lib/Driver/ToolChains/Linux.cpp

270–271

The /lib and /usr/lib cases should occur after the multiarch and OSLibDir cases taken from the sysroot.

clang/test/Driver/gcc-toolchain.cpp

42 ↗

(On Diff #416454)

Should specify a sysroot and test relative ordering between resource paths taken from the "overlay platform toochain" and the sysroot.

[qiucf]

• Move test to another file
• Add documentation on option

[hubert.reinterpretcast]

LGTM; thank you.

[MaskRay]

Why is --overlay-platform-toolchain added instead of using -isystem and -L?

In some cases, we need to set alternative toolchain path other than the default with system (headers, libraries, dynamic linker prefix, ld path, etc.), e.g., to pick up newer components, but keep sysroot at the same time (to pick up extra packages).

The functionality overlaps with -B. Unsure why introduce a new mechanism.

clang/include/clang/Driver/Options.td

4184

Separate-form driver options are not conventional. New driver options should just avoid them.

clang/lib/Driver/ToolChains/Gnu.cpp

1870

Why was this rule added?

[qiucf]

Hi,

Why is --overlay-platform-toolchain added instead of using -isystem and -L?

The functionality overlaps with -B. Unsure why introduce a new mechanism.

We may want to use an extra toolchain like the Advance Toolchain (https://github.com/advancetoolchain/advance-toolchain) which includes Glibc/GCC/GDB/LD/etc. but is not a complete OS distribution. So we should not simply change sysroot here.

Using -isystem and -L is okay in principle, but (1) it breaks expected include order (-isystem just inserts it in the top); (2) we have to manually insert many -isystem paths; (3) we want to reuse the logic in clang driver code. -B changes search path of crt runtime files, but include/library/dynamic linker paths are the same.

What this option does is to insert the extra toolchain in all search paths but with higher priority than system default.

clang/include/clang/Driver/Options.td

4184

Thanks for the reminder!

clang/lib/Driver/ToolChains/Gnu.cpp

1870

Linker and other paths relies on location of specified GCC toolchain. And the toolchain specified by overlay-platform-toolchain is expected to have GCC installation included. But for sure, it has lower priority than gcc-toolchain.

[MaskRay]

In D121992#3407220, @qiucf wrote:

Hi,

Why is --overlay-platform-toolchain added instead of using -isystem and -L?

The functionality overlaps with -B. Unsure why introduce a new mechanism.

We may want to use an extra toolchain like the Advance Toolchain (https://github.com/advancetoolchain/advance-toolchain) which includes Glibc/GCC/GDB/LD/etc. but is not a complete OS distribution. So we should not simply change sysroot here.

OK. Then this information is missing from the commit message and makes the new option IMO less justified.

Using -isystem and -L is okay in principle, but (1) it breaks expected include order (-isystem just inserts it in the top); (2) we have to manually insert many -isystem paths; (3) we want to reuse the logic in clang driver code. -B changes search path of crt runtime files, but include/library/dynamic linker paths are the same.

What this option does is to insert the extra toolchain in all search paths but with higher priority than system default.

Some other groups may have similar needs. I feel that the addition of --overlay-platform-toolchain might better involve more clang driver folks.
(you can run git shortlog clang/lib/Driver to get some idea who usually care about this area)

I've left some other comments. At this point I am going to say it is probably cleaner reverting the patch and having more discussions.

I'm still not so convinced we need a new option. There are a bunch which perform similar tasks (--sysroot, -B, --gcc-toolchain). We really need to think about what the missing gap is harder.

Perhaps it's ll be helpful if you put to the description the intended search paths printed by clang ... -v '-###' |& sed -E 's/ "?-[iIL]/\n&/g' and we can revisit whether a new option is needed.

clang/docs/ClangCommandLineReference.rst

520

The file is generated. The doc should be added to Options.td

clang/lib/Driver/ToolChains/Gnu.cpp

1870

This makes --overlay-platform-toolchain conceptually a superset of --gcc-toolchain but their behaviors are not exactly the same. Anyway, the interaction with --gcc-toolchain is important but untested.

clang/lib/Driver/ToolChains/Linux.cpp

266

Only a sysroot-like directory have both /lib and /usr. If you have just an incomplete toolchain directory, it likely just needs /lib, not /lib plus /usr/lib.

clang/test/Driver/overlay-toolchain.cpp

9

usr/lib is an important detail which should be tested.

[MaskRay]

To add more why I think the current semantics may need more discussion before we quickly commit to such a driver option:

• interaction with --dyld-prefix: --sysroot does not affect the fallback --dyld-prefix. It seems even less appropriate for --overlay-platform-toolchain to affect the fallback --dyld-prefix.

If you intend to overlay ld.so, you'll necessarily overlay libc, then --sysroot seems just unneeded at all.

• interaction with --gcc-toolchain: I am a bit unclear we still want the tricky GCC installation detection after --overlay-platform-toolchain is specified. Do you propose that both will add include and library search paths?
• -B: in gcc, when -B prefix specifies a directory, GCC adds $prefix/include to the include search directory. Clang does not do this right now. I think adding it may be an alternative approach to introducing the new option.

The currently picked rules may be suitable for https://github.com/advancetoolchain/advance-toolchain, but could be arbitrary for many other use cases.
Have you considered putting some options into a configuration file and using --config?

I understand that you probably have some short-term goal to make somethings done, but as I mentioned, there might be some process issue here.
This significant new feature very quickly landed without other driver folks possibly had a chance to chime in, when AIUI neither you nor your reviewer is a regular contributor to clang driver.
(FWIW I decided to subscribe all clang/lib/Driver patches since I care about this area.)
I very rarely do this but I think it is probably cleaner to revert this patch and discuss it more carefully. I am happy to help you achieve your goal. It's possible that we may still need a driver option.
(I'll take a trip for one week shortly, but I realized that I should make this comment before lack of objection makes the patch set in stone)

[qiucf]

In D121992#3418443, @MaskRay wrote:

To add more why I think the current semantics may need more discussion before we quickly commit to such a driver option:

• interaction with --dyld-prefix: --sysroot does not affect the fallback --dyld-prefix. It seems even less appropriate for --overlay-platform-toolchain to affect the fallback --dyld-prefix.

If you intend to overlay ld.so, you'll necessarily overlay libc, then --sysroot seems just unneeded at all.

• interaction with --gcc-toolchain: I am a bit unclear we still want the tricky GCC installation detection after --overlay-platform-toolchain is specified. Do you propose that both will add include and library search paths?
• -B: in gcc, when -B prefix specifies a directory, GCC adds $prefix/include to the include search directory. Clang does not do this right now. I think adding it may be an alternative approach to introducing the new option.

The currently picked rules may be suitable for https://github.com/advancetoolchain/advance-toolchain, but could be arbitrary for many other use cases.
Have you considered putting some options into a configuration file and using --config?

I understand that you probably have some short-term goal to make somethings done, but as I mentioned, there might be some process issue here.
This significant new feature very quickly landed without other driver folks possibly had a chance to chime in.
(FWIW I decided to subscribe all clang/lib/Driver patches since I care about this area.)
I very rarely do this but I think it is probably cleaner to revert this patch and discuss it more carefully. I am happy to help you achieve your goal. It's possible that we may still need a driver option.

Thanks for further information. I agree that we should not change to break the consistent behavior in such short period of time. It's reasonable for --gcc-toolchain to not add include into path (since driver only expects it to have GCC). I also saw GCC's different behavior on -B. We may need to fix it, but that's beyond this patch's scope.

It's okay to me to compose 'orthogonal' options into config file when available. Anyway, I'd like to revert this commit since there're comments not addressed.

[hubert.reinterpretcast]

In D121992#3418443, @MaskRay wrote:

If you intend to overlay ld.so, you'll necessarily overlay libc, then --sysroot seems just unneeded at all.

Why? The header and library search paths are not restricted to artifacts from libc. I had consulted some Advance Toolchain developers and they indicted that it was important for paths from the --sysroot to be incorporated (as GCC from the Advance Toolchain does).

The library search order has the issue of needing the multiarch/"OS lib dir" paths from both the overlay toolchain and the sysroot before attempting fallback plain-"lib" paths. Aside from a new option, is there a way to achieve that?

I agree, however, that the new option doesn't have to incorporate effects on what the --dyld-prefix default is.

clang/docs/ClangCommandLineReference.rst

 .. option:: --offload-arch=<arg>, --cuda-gpu-arch=<arg>, --no-offload-arch=<arg>
 
 CUDA offloading device architecture (e.g. sm\_35), or HIP offloading target ID in the form of a device architecture followed by target ID features delimited by a colon. Each target ID feature is a pre-defined string followed by a plus or minus sign (e.g. gfx908:xnack+:sramecc-).  May be specified more than once.
 
 .. option:: --offload=<arg1>,<arg2>...
 
 Specify comma-separated list of offloading target triples (CUDA and HIP only)
 
+.. option:: --overlay-platform-toolchain=<arg>
+
+Specify a toolchain with higher priority than sysroot in search paths.
+
 .. option:: -p, --profile
 
 .. option:: -pagezero\_size<arg>
 
 .. option:: -pg
 
 Enable mcount instrumentation
 

clang/include/clang/Driver/Driver.h

public:
 
   /// A prefix directory used to emulate a limited subset of GCC's '-Bprefix'
   /// functionality.
   /// FIXME: This type of customization should be removed in favor of the
   /// universal driver when it is ready.
   typedef SmallVector<std::string, 4> prefix_list;
   prefix_list PrefixDirs;
 
+  /// Alternative toolchain path used prior to sysroot.
+  std::string OverlayToolChainPath;
+
   /// sysroot, if present
   std::string SysRoot;
 
   /// Dynamic loader prefix, if present
   std::string DyldPrefix;
 
   /// Driver title to use with help.
   std::string DriverTitle;

clang/include/clang/Driver/Options.td

 def _no_undefined : Flag<["--"], "no-undefined">, Flags<[LinkerInput]>;
 def _no_warnings : Flag<["--"], "no-warnings">, Alias<w>;
 def _optimize_EQ : Joined<["--"], "optimize=">, Alias<O>;
 def _optimize : Flag<["--"], "optimize">, Alias<O>;
 def _output_class_directory_EQ : Joined<["--"], "output-class-directory=">, Alias<foutput_class_dir_EQ>;
 def _output_class_directory : Separate<["--"], "output-class-directory">, Alias<foutput_class_dir_EQ>;
 def _output_EQ : Joined<["--"], "output=">, Alias<o>;
 def _output : Separate<["--"], "output">, Alias<o>;
+def _overlay_platform_toolchain_EQ : Joined<["--"], "overlay-platform-toolchain=">;
+def _overlay_platform_toolchain : Separate<["--"], "overlay-platform-toolchain">, Alias<_overlay_platform_toolchain_EQ>;
 def _param : Separate<["--"], "param">, Group<CompileOnly_Group>;
 def _param_EQ : Joined<["--"], "param=">, Alias<_param>;
 def _precompile : Flag<["--"], "precompile">, Flags<[NoXarchOption]>,
   Group<Action_Group>, HelpText<"Only precompile the input">;
 def _prefix_EQ : Joined<["--"], "prefix=">, Alias<B>;
 def _prefix : Separate<["--"], "prefix">, Alias<B>;
 def _preprocess : Flag<["--"], "preprocess">, Alias<E>;
 def _print_diagnostic_categories : Flag<["--"], "print-diagnostic-categories">;

clang/lib/Driver/Driver.cpp

if (Optional<std::string> CompilerPathValue =
     StringRef CompilerPath = *CompilerPathValue;
     while (!CompilerPath.empty()) {
       std::pair<StringRef, StringRef> Split =
           CompilerPath.split(llvm::sys::EnvPathSeparator);
       PrefixDirs.push_back(std::string(Split.first));
       CompilerPath = Split.second;
     }
   }
+  if (const Arg *A =
+          Args.getLastArg(options::OPT__overlay_platform_toolchain_EQ)) {
+    OverlayToolChainPath = A->getValue();
+    DyldPrefix = A->getValue();
+  }
   if (const Arg *A = Args.getLastArg(options::OPT__sysroot_EQ))
     SysRoot = A->getValue();
   if (const Arg *A = Args.getLastArg(options::OPT__dyld_prefix_EQ))
     DyldPrefix = A->getValue();
 
   if (const Arg *A = Args.getLastArg(options::OPT_resource_dir))
     ResourceDir = A->getValue();
 

clang/lib/Driver/ToolChains/Gnu.cpp

 }
 
 static llvm::StringRef getGCCToolchainDir(const ArgList &Args,
                                           llvm::StringRef SysRoot) {
   const Arg *A = Args.getLastArg(clang::driver::options::OPT_gcc_toolchain);
   if (A)
     return A->getValue();
 
+  if (const Arg *X = Args.getLastArg(
+          clang::driver::options::OPT__overlay_platform_toolchain_EQ))
+    return X->getValue();
+
   // If we have a SysRoot, ignore GCC_INSTALL_PREFIX.
   // GCC_INSTALL_PREFIX specifies the gcc installation for the default
   // sysroot and is likely not valid with a different sysroot.
   if (!SysRoot.empty())
     return "";
 
   return GCC_INSTALL_PREFIX;
 }

clang/lib/Driver/ToolChains/Linux.cpp

#endif
   // the GCC driver itself uses, as this is part of the GCC-compatible driver.
   // This was determined by running GCC in a fake filesystem, creating all
   // possible permutations of these directories, and seeing which ones it added
   // to the link paths.
   path_list &Paths = getFilePaths();
 
   const std::string OSLibDir = std::string(getOSLibDir(Triple, Args));
   const std::string MultiarchTriple = getMultiarchTriple(D, Triple, SysRoot);
+  const std::string &ExtraPath = D.OverlayToolChainPath;
+
+  if (!D.OverlayToolChainPath.empty()) {
+    addPathIfExists(D, ExtraPath + "/lib/" + MultiarchTriple, Paths);
+    addPathIfExists(D, ExtraPath + "/lib/../" + OSLibDir, Paths);
+    addPathIfExists(D, ExtraPath + "/usr/lib/" + MultiarchTriple, Paths);
+    addPathIfExists(D, ExtraPath + "/usr/lib/../" + OSLibDir, Paths);
+  }
 
   // mips32: Debian multilib, we use /libo32, while in other case, /lib is
   // used. We need add both libo32 and /lib.
   if (Arch == llvm::Triple::mips || Arch == llvm::Triple::mipsel) {
     Generic_GCC::AddMultilibPaths(D, SysRoot, "libo32", MultiarchTriple, Paths);
     addPathIfExists(D, SysRoot + "/libo32", Paths);
     addPathIfExists(D, SysRoot + "/usr/libo32", Paths);
   }
   Generic_GCC::AddMultilibPaths(D, SysRoot, OSLibDir, MultiarchTriple, Paths);
#endif
   if (StringRef(D.Dir).startswith(SysRoot)) {
     // Even if OSLibDir != "lib", this is needed for Clang in the build
     // directory (not installed) to find libc++.
     addPathIfExists(D, D.Dir + "/../lib", Paths);
     if (OSLibDir != "lib")
       addPathIfExists(D, D.Dir + "/../" + OSLibDir, Paths);
   }
 
+  if (!D.OverlayToolChainPath.empty()) {
+    addPathIfExists(D, ExtraPath + "/lib", Paths);
+    addPathIfExists(D, ExtraPath + "/usr/lib", Paths);
+  }
+
   addPathIfExists(D, SysRoot + "/lib", Paths);
   addPathIfExists(D, SysRoot + "/usr/lib", Paths);
 }
 
 ToolChain::RuntimeLibType Linux::GetDefaultRuntimeLibType() const {
   if (getTriple().isAndroid())
     return ToolChain::RLT_CompilerRT;
   return Generic_ELF::GetDefaultRuntimeLibType();
void Linux::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
   llvm::sys::path::append(ResourceDirInclude, "include");
   if (!DriverArgs.hasArg(options::OPT_nobuiltininc) &&
       (!getTriple().isMusl() || DriverArgs.hasArg(options::OPT_nostdlibinc)))
     addSystemInclude(DriverArgs, CC1Args, ResourceDirInclude);
 
   if (DriverArgs.hasArg(options::OPT_nostdlibinc))
     return;
 
+  if (!D.OverlayToolChainPath.empty())
+    addExternCSystemInclude(DriverArgs, CC1Args,
+                            D.OverlayToolChainPath + "/include");
+
   // LOCAL_INCLUDE_DIR
   addSystemInclude(DriverArgs, CC1Args, SysRoot + "/usr/local/include");
   // TOOL_INCLUDE_DIR
   AddMultilibIncludeArgs(DriverArgs, CC1Args);
 
   // Check for configure-time C include directories.
   StringRef CIncludeDirs(C_INCLUDE_DIRS);
   if (CIncludeDirs != "") {

clang/test/Driver/overlay-toolchain.cpp

+// RUN: %clangxx %s -### --target=powerpc64le-linux-gnu \
+// RUN:   --overlay-platform-toolchain=%S/Inputs/powerpc64le-linux-gnu-tree/gcc-11.2.0 \
+// RUN:   2>&1 | FileCheck %s --check-prefix=OVERLAY
+// RUN: %clangxx %s -### --target=powerpc64le-linux-gnu \
+// RUN:   --sysroot=%S/Inputs/powerpc64le-linux-gnu-tree/gcc-8.3.0 \
+// RUN:   --overlay-platform-toolchain=%S/Inputs/powerpc64le-linux-gnu-tree/gcc-11.2.0 \
+// RUN:   2>&1 | FileCheck %s --check-prefixes=OVERLAY,ROOT
+
+// OVERLAY: "-internal-externc-isystem"
+// OVERLAY: "[[TOOLCHAIN:[^"]+]]/powerpc64le-linux-gnu-tree/gcc-11.2.0/include"
+// ROOT: "-internal-externc-isystem"
+// ROOT: "[[TOOLCHAIN]]/powerpc64le-linux-gnu-tree/gcc-8.3.0/include"
+// OVERLAY: "-dynamic-linker"
+// OVERLAY: "[[TOOLCHAIN]]/powerpc64le-linux-gnu-tree/gcc-11.2.0/lib64/ld64.so.2"
+// OVERLAY: "-L[[TOOLCHAIN]]/powerpc64le-linux-gnu-tree/gcc-11.2.0/lib/../lib64"
+// ROOT: "-L[[TOOLCHAIN]]/powerpc64le-linux-gnu-tree/gcc-8.3.0/lib/../lib64"
+// OVERLAY: "-L[[TOOLCHAIN]]/powerpc64le-linux-gnu-tree/gcc-11.2.0/lib"
+// ROOT: "-L[[TOOLCHAIN]]/powerpc64le-linux-gnu-tree/gcc-8.3.0/lib"