[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

[Dan Gohman via Phabricator via cfe-commits]

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