[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 inline comments.


================
Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:96
+  if (Arg *A = Args.getLastArg(options::OPT_O_Group)) {
+    if (const char *WasmOptPath = getenv("WASM_OPT")) {
+      StringRef OOpt = "s";
----------------
dschuff wrote:
> sunfish wrote:
> > dschuff wrote:
> > > What would you think about adding a way to pass arguments through to wasm-opt on the command line, like we have for the linker, assembler, etc? Something like `-Wo,-O2` (or `-Ww` or whatever; analogous to `-Wl` and `-Wa`). That seems nicer than an env var, although it doesn't solve the problem of controlling whether to run the optimizer in the first place.
> > My guess here is that we don't need clang to have an option for passing additional flags -- if people want to do extra special things with wasm-opt on clang's output they can just run wasm-opt directly themselves. Does that sound reasonable?
> Maybe. But I still don't like the use of an env var for this kind of behavior-effecting (i.e. non-debugging) use case.  It's hard enough to get reproducible and hermetic build behavior as it is, I definitely wouldn't want to worry about the environment affecting the output in addition to all the random flags, included files, etc.
If we did -Wo,-O2 or so, we'd still need to be able to find the wasm-opt program to be able to run it. We could just search for it in PATH, but that's also a little dicey from a hermetic build perspective.

I borrowed "WASM_OPT" from [cargo-wasi](https://github.com/bytecodealliance/cargo-wasi). I'm also not a fan of environment variables in general, but this way does have the advantage that people can set it once, and not have to modify their Makefiles to add new flags. Users can think of it as just being part of -O2 and friends.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70500