sunfish marked an inline comment as done. sunfish added a comment. In D70500#1757735 <https://reviews.llvm.org/D70500#1757735>, @thakis wrote:
> Please don't add code to the driver that runs programs off PATH. Nothing else > does this. Clang's normal `GetProgramPath` does do this: https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/Driver.cpp#L4719 > If you need to run an external program, look for it next to the compiler, > like we do for gas with -fno-integrated-as, linker, etc. People can use the > existing -B flag to make clang look elsewhere if they need that. Unfortunately, wasm-opt isn't typically installed alongside the compiler. It's also not a typical system tool like an assembler, which is why it didn't seem right to search the -B paths. The first version of my patch used a dedicated environment variable, WASM_OPT, rather than PATH, but I changed it to PATH after review feedback. If people feel using -B paths, and COMPILER_PATH, are appropriate, we can use `GetProgramPath` itself, though note that that still does have a PATH fallback. ================ Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:137 + getFilePaths().push_back(getDriver().SysRoot + "/lib/" + MultiarchTriple + + "/llvm-lto"); + } ---------------- thakis wrote: > sunfish wrote: > > sbc100 wrote: > > > Is there any kind of president here? i.e. do any other platforms have > > > support for this kind thing? Seems like good idea I'd just like to be > > > sure we follow existing practices. > > I looked around, but didn't see anything similar. > One immediate problem of this approach is that if HAVE_VCS_VERSION_INC is not > set, then the test fails, but if it is set, `clang --version` reports a > current git hash, which is either out of date or requires a pretty big > rebuild on every single git commit (i.e. not just after `git pull` but also > after local commits). Neither's great. > > Do you expected that sysroots will ship 3-4 LTO folders, to support ~2 years > of clang releases? Or do you expect that a sysroot will always support a > single clang/llvm revision? If so, maybe the LLVM version is enough? Yes, I think we can switch from the VCS string to the LLVM_VERSION string. The documentation says LLVM guarantees bitcode backwards compatibility between minor versions, so this should be sufficient. I'll post a new patch for that, which should also fix the test when HAVE_VCS_VERSION_INC is false. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70500/new/ https://reviews.llvm.org/D70500 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
