Skip to content

Conversation

@aheejin
Copy link
Member

@aheejin aheejin commented Jan 25, 2025

#124042 caused a problem that when invoking clang with multiple files, the static HasRun variables were set when processing the first file so the appropriate feature flags were not added from the second file. This fixes the problem by making those HasRun variables just normal variables within the enclosing function.

 llvm#124042 caused a problem that when invoking `clang` with multiple
files, the static `HasRun` variables were set when processing the first
file so the appropriate feature flags were not added from the second
file. This fixes the problem by making those `HasRun` variables just
normal variables within the enclosing function.
@aheejin aheejin requested a review from dschuff January 25, 2025 00:26
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jan 25, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 25, 2025

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Heejin Ahn (aheejin)

Changes

#124042 caused a problem that when invoking clang with multiple files, the static HasRun variables were set when processing the first file so the appropriate feature flags were not added from the second file. This fixes the problem by making those HasRun variables just normal variables within the enclosing function.


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

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/WebAssembly.cpp (+7-6)
  • (modified) clang/test/Driver/wasm-toolchain.c (+8)
diff --git a/clang/lib/Driver/ToolChains/WebAssembly.cpp b/clang/lib/Driver/ToolChains/WebAssembly.cpp
index eebe3becada657..bd25fd1a8933a9 100644
--- a/clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ b/clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -344,12 +344,15 @@ void WebAssembly::addClangTargetOptions(const ArgList &DriverArgs,
     }
   }
 
+  bool HasBannedIncompatibleOptionsForWasmEHSjLj = false;
+  bool HasEnabledFeaturesForWasmEHSjLj = false;
+
   // 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)
+    if (HasBannedIncompatibleOptionsForWasmEHSjLj)
       return;
+    HasBannedIncompatibleOptionsForWasmEHSjLj = true;
     if (DriverArgs.hasFlag(options::OPT_mno_exception_handing,
                            options::OPT_mexception_handing, false))
       getDriver().Diag(diag::err_drv_argument_not_allowed_with)
@@ -373,14 +376,13 @@ 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)
+    if (HasEnabledFeaturesForWasmEHSjLj)
       return;
+    HasEnabledFeaturesForWasmEHSjLj = true;
     CC1Args.push_back("-target-feature");
     CC1Args.push_back("+exception-handling");
     // The standardized Wasm EH spec requires multivalue and reference-types.
@@ -390,7 +392,6 @@ 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)) {
diff --git a/clang/test/Driver/wasm-toolchain.c b/clang/test/Driver/wasm-toolchain.c
index 2d140520827768..f516a4e457da73 100644
--- a/clang/test/Driver/wasm-toolchain.c
+++ b/clang/test/Driver/wasm-toolchain.c
@@ -224,6 +224,14 @@
 // RUN:   | FileCheck -check-prefix=WASM_LEGACY_EH_NO_EH %s
 // WASM_LEGACY_EH_NO_EH: invalid argument '-wasm-use-legacy-eh' not allowed with '-mno-exception-handling'
 
+// When invoking clang with multiple files in a single command line, target
+// feature flags should be equally added to the multiple clang-cc1 command lines
+// RUN: %clang -### --target=wasm32-unknown-unknown \
+// RUN:    --sysroot=/foo %s %s -mllvm -wasm-enable-sjlj 2>&1 \
+// RUN:  | FileCheck -check-prefix=WASM_SJLJ_MULTI_FILES %s
+// WASM_SJLJ_MULTI_FILES: "-cc1" {{.*}} "-target-feature" "+exception-handling" "-target-feature" "+multivalue" "-target-feature" "+reference-types" "-exception-model=wasm"
+// WASM_SJLJ_MULTI_FILES: "-cc1" {{.*}} "-target-feature" "+exception-handling" "-target-feature" "+multivalue" "-target-feature" "+reference-types" "-exception-model=wasm"
+
 // RUN: %clang -### %s -fsanitize=address --target=wasm32-unknown-emscripten 2>&1 | FileCheck -check-prefix=CHECK-ASAN-EMSCRIPTEN %s
 // CHECK-ASAN-EMSCRIPTEN: "-fsanitize=address"
 // CHECK-ASAN-EMSCRIPTEN: "-fsanitize-address-globals-dead-stripping"

@aheejin aheejin merged commit 4ea44eb into llvm:main Jan 25, 2025
11 checks passed
@aheejin aheejin deleted the only_once_fix branch January 25, 2025 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants