Add wasm-opt warning

[mh4ck-Thales]

Add a warning when wasm-opt is requested with a flag (#95208) but is not found in the path.

I'm using #77148 as reference on how to add a new warning but please tell me if you can help in implementing this. Also I'm not sure in which warning group include this, at first glance none seem relevant for this specific problem.

Regarding #98373 another warning might be relevant if people target WASIp2+ and try to use wasm-opt explicitly. This can be grouped in one warning "wasm-opt not available in this context" or separated in 2 for clearer error messages. WDYT?

CC @sunfishcode @sbc100 @alexcrichton

[alexcrichton]

If wasm-opt is explicitly enabled via a flag, then I think it would make sense for this to be an error rather than a warning. Otherwise though if no default behavior was indicated then I would personally fear the churn of this change as most developers I know of on wasm32-wasip1 don't have wasm-opt in their path meaning that this would create many un-suppressable warnings by default for these projects.

Personally I think that wasm-opt should be disabled by default since it's an optional tool to be coupled with a toolchain and it means that the same compiler invocation can produce drastically different results on the same version of LLVM depending on the execution environment. I realize though that disabling it by default is probably controversial, so short of that I don't know how best to thread this needle and communicate the lack of wasm-opt to end users.

[mh4ck-Thales]

wasm-opt is enabled by default for WASIp1, but can be disabled with the --no-wasm-opt flag. Indeed if it is explicitly enabled then an error instead of a warning would make more sense.

I'm not sure what is the problem with

most developers I know of on wasm32-wasip1 don't have wasm-opt in their path meaning that this would create many un-suppressable warnings by default for these projects.

The solution to this is just to add --no-wasm-opt to the compilation command and no warning will be displayed.

I agree that wasm-opt should have been opt-in when it have been introduced into LLVM, it has already been debated in #55781 and the consensus is that we cannot go back on the default behavior as this would be a breaking change. A warning when wasm-opt is supposed to be used but is not present seems a reasonable solution to me in this situation.

In the longer term, it would be nice to see if we can package and distribute wasm-opt alongside LLVM. Maybe it cannot be done in LLVM directly, but it could be a nice addition to wasi-sdk.

[alexcrichton]

Personally I don't find it to be a great user experience when a tool warns by default in default configration. This means that any user using wasi-sdk, for example, would by default with no other action taken receive warnings. That's annoying and misleading to end users because wasm-opt isn't required for a successful compilation. "Just add another flag" does indeed solve the problem but it's annoying to have to configure that and IMO it should not be required to pass extra flags to suppress warnings in default configurations.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 73ef397fcba35b7b4239c00bf3e0b4e689ca0add a798b1fd7a7a6c7b782584021e5c32e52c3abffc --extensions cpp -- clang/lib/Driver/ToolChains/WebAssembly.cpp

View the diff from clang-format here.

diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index e9820ce1d3..1fc74d28ab 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -21,7 +21,6 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
 
-
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang::driver::toolchains;

[mh4ck-Thales]

I implemented a proposition on how to proceed for correctly informing users of wasm-opt problems:

• a warning for when no flags are passed (the default being wasm-opt enabled for wasip1) and wasm-opt is not found
• an error for when --wasm-opt is explicitly passed but wasm-opt is not found (as @alexcrichton suggested)
• another error for when --wasm-opt is explicitly passed but wasm-opt is not supported (wasip2 and components)

We can discuss and change this, especially for the warning "by default" that is controversial. I think that a warning is the way to go if we want to leave wasm-opt enabled as a default, otherwise people with different tooling end up with vastly differing results without knowing why, but the point of @alexcrichton on the annoyance of having a warning is relevant. Another solution could be to just print a message without using warnings? Is there a mechanism in LLVM for printing info notes?

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Quentin Michaud (mh4ck-Thales)

Changes

Add a warning when wasm-opt is requested with a flag (#95208) but is not found in the path.

I'm using #77148 as reference on how to add a new warning but please tell me if you can help in implementing this. Also I'm not sure in which warning group include this, at first glance none seem relevant for this specific problem.

Regarding #98373 another warning might be relevant if people target WASIp2+ and try to use wasm-opt explicitly. This can be grouped in one warning "wasm-opt not available in this context" or separated in 2 for clearer error messages. WDYT?

CC @sunfishcode @sbc100 @alexcrichton

---

Full diff: https://github.com/llvm/llvm-project/pull/100321.diff

4 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticDriverKinds.td (+8)
• (modified) clang/include/clang/Basic/DiagnosticGroups.td (+2)
• (modified) clang/include/clang/Driver/Options.td (+1)
• (modified) clang/lib/Driver/ToolChains/WebAssembly.cpp (+15)

diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index ba90742fbdaabc..2a862edf8788c8 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -826,4 +826,12 @@ def err_drv_triple_version_invalid : Error<
 
 def warn_missing_include_dirs : Warning<
   "no such include directory: '%0'">, InGroup<MissingIncludeDirs>, DefaultIgnore;
+
+def warn_wasm_opt_not_found : Warning<
+  "wasm-opt was not found, some optimizations were not applied">,
+  InGroup<WebAssemblyOptimization>;
+def err_wasm_opt_not_found_with_flag : Error<
+  "wasm-opt was explicitly requested, but was not found">;
+def err_wasm_opt_requested_but_not_supported : Error<
+  "wasm-opt was explicitly requested, but is not supported with '%0'">;
 }
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 28d315f63e5c47..fab11f47492db3 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1520,6 +1520,8 @@ in addition with the pragmas or -fmax-tokens flag to get any warnings.
 
 def WebAssemblyExceptionSpec : DiagGroup<"wasm-exception-spec">;
 
+def WebAssemblyOptimization : DiagGroup<"wasm-opt">;
+
 def RTTI : DiagGroup<"rtti">;
 
 def OpenCLCoreFeaturesDiagGroup : DiagGroup<"pedantic-core-features">;
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 83cf753e824845..ba6e7ab9e11d06 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8923,3 +8923,4 @@ def wasm_opt : Flag<["--"], "wasm-opt">,
   Group<m_Group>,
   HelpText<"Enable the wasm-opt optimizer (default)">,
   MarshallingInfoNegativeFlag<LangOpts<"NoWasmOpt">>;
+def Wwarn_wasm_opt_not_found : Flag<["-"], "Wwarn_wasm_opt_not_found">;
diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index 9aacda5fd5702f..e9820ce1d35995 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -9,6 +9,7 @@
 #include "WebAssembly.h"
 #include "CommonArgs.h"
 #include "Gnu.h"
+#include "clang/Basic/DiagnosticDriver.h"
 #include "clang/Basic/Version.h"
 #include "clang/Config/config.h"
 #include "clang/Driver/Compilation.h"
@@ -20,6 +21,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
 
+
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang::driver::toolchains;
@@ -172,6 +174,11 @@ void wasm::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   bool RunWasmOpt = Args.hasFlag(options::OPT_wasm_opt,
                                  options::OPT_no_wasm_opt, WasmOptDefault);
 
+  if (TargetBuildsComponents(ToolChain.getTriple()) &&
+      Args.hasFlag(options::OPT_wasm_opt, options::OPT_no_wasm_opt, false)) {
+    ToolChain.getDriver().Diag(diag::err_wasm_opt_requested_but_not_supported)
+        << ToolChain.getTriple().str();
+  }
   // If wasm-opt is enabled and optimizations are happening look for the
   // `wasm-opt` program. If it's not found auto-disable it.
   std::string WasmOptPath;
@@ -180,6 +187,14 @@ void wasm::Linker::ConstructJob(Compilation &C, const JobAction &JA,
     if (WasmOptPath == "wasm-opt") {
       WasmOptPath = {};
     }
+    if (WasmOptPath.empty()) {
+      if (Args.hasFlag(options::OPT_wasm_opt, options::OPT_no_wasm_opt,
+                       false)) {
+        ToolChain.getDriver().Diag(diag::err_wasm_opt_not_found_with_flag);
+      } else {
+        ToolChain.getDriver().Diag(diag::warn_wasm_opt_not_found);
+      }
+    }
   }
 
   if (!WasmOptPath.empty()) {

[alexcrichton]

I can see how the current state of the world is resulting in this PR, but at least personally I continue to feel that a warning should not be emitted if no flags are passed and wasm-opt is not found. Part of it I've mentioned above which I can summarize as: a warning would mean that the default installation of wasi-sdk would generate warnings for projects by default. Personally I don't think this is a great situation to be in.

The other part of it is that personally I often specifically don't want wasm-opt to run. I keep it out of my PATH as a result and it runs the risk of making debugging harder by adding a tool in the toolchain to chase down when something goes wrong.