[PATCH] D121992: [Clang] [Driver] Add option to set alternative toolchain path

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D121992#3407220 <https://reviews.llvm.org/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.



================
Comment at: clang/docs/ClangCommandLineReference.rst:520
+
+Specify a toolchain with higher priority than sysroot in search paths.
+
----------------
The file is generated. The doc should be added to `Options.td`


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1870
 
+  if (const Arg *X = Args.getLastArg(
+          clang::driver::options::OPT__overlay_platform_toolchain_EQ))
----------------
qiucf wrote:
> MaskRay wrote:
> > Why was this  rule added?
> 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`.
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.


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:266
+  if (!D.OverlayToolChainPath.empty()) {
+    addPathIfExists(D, ExtraPath + "/lib/" + MultiarchTriple, Paths);
+    addPathIfExists(D, ExtraPath + "/lib/../" + OSLibDir, Paths);
----------------
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.


================
Comment at: clang/test/Driver/overlay-toolchain.cpp:9
+
+// OVERLAY: "-internal-externc-isystem"
+// OVERLAY: "[[TOOLCHAIN:[^"]+]]/powerpc64le-linux-gnu-tree/gcc-11.2.0/include"
----------------
usr/lib is an important detail which should be tested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121992/new/

https://reviews.llvm.org/D121992