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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
