[WebAssembly] Enable Wasm EH features only once

[aheejin]

#122466 had an unexpected side effect that, EnableFeaturesForWasmEHSjLj and BanIncompatibleOptionsForWasmEHSjLj can be called multiple times now, every time a Wasm EH flag (-fwasm-exceptions, -wasm-enable-eh, -wasm-enable-sjlj, ..) was checked and handled. This resulted in unnecessarily adding the same feature-enabling arguments multiple times to the command line, for example, -target-feature +exception-handling could be added as many as three times, which didn't cause any errors but unnecessary. Also we ran BanIncompatibleOptionsForWasmEHSjLj more than once, which was harmless but unnecessary.

This guards these functions with a static variable so that we only run them once.

This does not add any new tests because honestly I don't see any value of having a test that checks -target-feature +exception-handling is only added once...

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Heejin Ahn (aheejin)

Changes

#122466 had an unexpected side effect that, EnableFeaturesForWasmEHSjLj and BanIncompatibleOptionsForWasmEHSjLj can be called multiple times now, every time a Wasm EH flag (-fwasm-exceptions, -wasm-enable-eh, -wasm-enable-sjlj, ..) was checked and handled. This resulted in unnecessarily adding the same feature-enabling arguments multiple times to the command line, for example, -target-feature +exception-handling could be added as many as three times, which didn't cause any errors but unnecessary. Also we ran BanIncompatibleOptionsForWasmEHSjLj more than once, which was harmless but unnecessary.

This guards these functions with a static variable so that we only run them once.

This does not add any new tests because honestly I don't see any value of having a test that checks -target-feature +exception-handling is only added once...

---

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

1 Files Affected:

• (modified) clang/lib/Driver/ToolChains/WebAssembly.cpp (+8)

diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index 10f9a4f338f8f1..eebe3becada657 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -347,6 +347,9 @@ void WebAssembly::addClangTargetOptions(const ArgList &DriverArgs,
   // Bans incompatible options for Wasm EH / SjLj. We don't allow using
   // different modes for EH and SjLj.
   auto BanIncompatibleOptionsForWasmEHSjLj = [&](StringRef CurOption) {
+    static bool HasRun = false;
+    if (HasRun)
+      return;
     if (DriverArgs.hasFlag(options::OPT_mno_exception_handing,
                            options::OPT_mexception_handing, false))
       getDriver().Diag(diag::err_drv_argument_not_allowed_with)
@@ -370,10 +373,14 @@ void WebAssembly::addClangTargetOptions(const ArgList &DriverArgs,
               << CurOption << Option;
       }
     }
+    HasRun = true;
   };
 
   // Enable necessary features for Wasm EH / SjLj in the backend.
   auto EnableFeaturesForWasmEHSjLj = [&]() {
+    static bool HasRun = false;
+    if (HasRun)
+      return;
     CC1Args.push_back("-target-feature");
     CC1Args.push_back("+exception-handling");
     // The standardized Wasm EH spec requires multivalue and reference-types.
@@ -383,6 +390,7 @@ void WebAssembly::addClangTargetOptions(const ArgList &DriverArgs,
     CC1Args.push_back("+reference-types");
     // Backend needs '-exception-model=wasm' to use Wasm EH instructions
     CC1Args.push_back("-exception-model=wasm");
+    HasRun = true;
   };
 
   if (DriverArgs.getLastArg(options::OPT_fwasm_exceptions)) {

[llvm-ci]

LLVM Buildbot has detected a new failure on builder clang-aarch64-quick running on linaro-clang-aarch64-quick while building clang at step 5 "ninja check 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/11149

Here is the relevant piece of the build log for the reference

Step 5 (ninja check 1) failure: stage 1 checked (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests/130/174' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/unittests/Support/./SupportTests-LLVM-Unit-3334394-130-174.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=174 GTEST_SHARD_INDEX=130 /home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/unittests/Support/./SupportTests
--

Script:
--
/home/tcwg-buildbot/worker/clang-aarch64-quick/stage1/unittests/Support/./SupportTests --gtest_filter=ProgramEnvTest.TestLockFile
--
Note: Google Test filter = ProgramEnvTest.TestLockFile
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from ProgramEnvTest
../llvm/llvm/unittests/Support/ProgramTest.cpp:558: Failure
Value of: Error.empty()
  Actual: false
Expected: true


../llvm/llvm/unittests/Support/ProgramTest.cpp:558
Value of: Error.empty()
  Actual: false
Expected: true



********************

[aheejin]

@llvm-ci The failure seems irrelevant.