[lld] a1ae956 - [WebAssembly] Disallow 'shared-mem' rather than 'atomics'

Thomas Lively via llvm-commits llvm-commits at lists.llvm.org
Fri May 8 13:52:47 PDT 2020


Author: Thomas Lively
Date: 2020-05-08T13:52:39-07:00
New Revision: a1ae9566ea9ce46bf7f2af9ab1253eed05b5b622

URL: https://github.com/llvm/llvm-project/commit/a1ae9566ea9ce46bf7f2af9ab1253eed05b5b622
DIFF: https://github.com/llvm/llvm-project/commit/a1ae9566ea9ce46bf7f2af9ab1253eed05b5b622.diff

LOG: [WebAssembly] Disallow 'shared-mem' rather than 'atomics'

Summary:
The WebAssembly backend automatically lowers atomic operations and TLS
to nonatomic operations and non-TLS data when either are present and
the atomics or bulk-memory features are not present, respectively. The
resulting object is no longer thread-safe, so the linker has to be
told not to allow it to be linked into a module with shared
memory. This was previously done by disallowing the 'atomics' feature,
which prevented any objct with its atomic operations or TLS removed
from being linked with any object containing atomics or TLS, and
therefore preventing it from being linked into a module with shared
memory since shared memory requires atomics.

However, as of https://github.com/WebAssembly/threads/issues/144, the
validation rules are relaxed to allow atomic operations to validate
with unshared memories, which makes it perfectly safe to link an
object with stripped atomics and TLS with another object that still
contains TLS and atomics as long as the resulting module has an
unshared memory. To allow this kind of link, this patch disallows a
pseudo-feature 'shared-mem' rather than 'atomics' to communicate to
the linker that the object is not thread-safe. This means that the
'atomics' feature is available to accurately reflect whether or not an
object has atomics enabled.

As a drive-by tweak, this change also requires that bulk-memory be
enabled in addition to atomics in order to use shared memory. This is
because initializing shared memories requires bulk-memory operations.

Reviewers: aheejin, sbc100

Subscribers: dschuff, jgravelle-google, hiraditya, sunfish, jfb, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D79542

Added: 
    

Modified: 
    lld/test/wasm/shared-memory-no-atomics.yaml
    lld/wasm/Writer.cpp
    llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
    llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
    llvm/test/CodeGen/WebAssembly/target-features-tls.ll

Removed: 
    


################################################################################
diff  --git a/lld/test/wasm/shared-memory-no-atomics.yaml b/lld/test/wasm/shared-memory-no-atomics.yaml
index 1588f2f0029e..7e01a89c4557 100644
--- a/lld/test/wasm/shared-memory-no-atomics.yaml
+++ b/lld/test/wasm/shared-memory-no-atomics.yaml
@@ -49,7 +49,7 @@ Sections:
     Name:            target_features
     Features:
       - Prefix:        DISALLOWED
-        Name:          "atomics"
+        Name:          "shared-mem"
 ...
 
 # NO-SHARED:      - Type:            MEMORY
@@ -57,4 +57,4 @@ Sections:
 # NO-SHARED-NEXT:     - Initial:         0x00000002
 # NO-SHARED-NOT:        Maximum:
 
-# SHARED: 'atomics' feature is disallowed by {{.*}}shared-memory-no-atomics.yaml.tmp1.o, so --shared-memory must not be used{{$}}
+# SHARED: --shared-memory is disallowed by {{.*}}shared-memory-no-atomics.yaml.tmp1.o because it was not compiled with 'atomics' or 'bulk-memory' features.

diff  --git a/lld/wasm/Writer.cpp b/lld/wasm/Writer.cpp
index e80fbc763c49..a1b5a463a530 100644
--- a/lld/wasm/Writer.cpp
+++ b/lld/wasm/Writer.cpp
@@ -441,21 +441,24 @@ void Writer::populateTargetFeatures() {
   if (!config->checkFeatures)
     return;
 
-  if (disallowed.count("atomics") && config->sharedMemory)
-    error("'atomics' feature is disallowed by " + disallowed["atomics"] +
-          ", so --shared-memory must not be used");
-
-  if (!allowed.count("atomics") && config->sharedMemory)
-    error("'atomics' feature must be used in order to use shared "
-          "memory");
-
-  if (!allowed.count("bulk-memory") && config->sharedMemory)
-    error("'bulk-memory' feature must be used in order to use shared "
-          "memory");
+  if (config->sharedMemory) {
+    if (disallowed.count("shared-mem"))
+      error("--shared-memory is disallowed by " + disallowed["shared-mem"] +
+            " because it was not compiled with 'atomics' or 'bulk-memory' "
+            "features.");
+
+    for (auto feature : {"atomics", "bulk-memory"})
+      if (!allowed.count(feature))
+        error(StringRef("'") + feature +
+              "' feature must be used in order to use shared memory");
+  }
 
-  if (!allowed.count("bulk-memory") && tlsUsed)
-    error("'bulk-memory' feature must be used in order to use thread-local "
-          "storage");
+  if (tlsUsed) {
+    for (auto feature : {"atomics", "bulk-memory"})
+      if (!allowed.count(feature))
+        error(StringRef("'") + feature +
+              "' feature must be used in order to use thread-local storage");
+  }
 
   // Validate that used features are allowed in output
   if (!inferFeatures) {

diff  --git a/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
index c701e715152a..96fa13d30729 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
@@ -230,20 +230,20 @@ void WebAssemblyAsmPrinter::EmitProducerInfo(Module &M) {
 void WebAssemblyAsmPrinter::EmitTargetFeatures(Module &M) {
   struct FeatureEntry {
     uint8_t Prefix;
-    StringRef Name;
+    std::string Name;
   };
 
   // Read target features and linkage policies from module metadata
   SmallVector<FeatureEntry, 4> EmittedFeatures;
-  for (const SubtargetFeatureKV &KV : WebAssemblyFeatureKV) {
-    std::string MDKey = (StringRef("wasm-feature-") + KV.Key).str();
+  auto EmitFeature = [&](std::string Feature) {
+    std::string MDKey = (StringRef("wasm-feature-") + Feature).str();
     Metadata *Policy = M.getModuleFlag(MDKey);
     if (Policy == nullptr)
-      continue;
+      return;
 
     FeatureEntry Entry;
     Entry.Prefix = 0;
-    Entry.Name = KV.Key;
+    Entry.Name = Feature;
 
     if (auto *MD = cast<ConstantAsMetadata>(Policy))
       if (auto *I = cast<ConstantInt>(MD->getValue()))
@@ -253,10 +253,16 @@ void WebAssemblyAsmPrinter::EmitTargetFeatures(Module &M) {
     if (Entry.Prefix != wasm::WASM_FEATURE_PREFIX_USED &&
         Entry.Prefix != wasm::WASM_FEATURE_PREFIX_REQUIRED &&
         Entry.Prefix != wasm::WASM_FEATURE_PREFIX_DISALLOWED)
-      continue;
+      return;
 
     EmittedFeatures.push_back(Entry);
+  };
+
+  for (const SubtargetFeatureKV &KV : WebAssemblyFeatureKV) {
+    EmitFeature(KV.Key);
   }
+  // This pseudo-feature tells the linker whether shared memory would be safe
+  EmitFeature("shared-mem");
 
   if (EmittedFeatures.size() == 0)
     return;

diff  --git a/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
index 35b76c6768cb..8a92abd72881 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
@@ -273,21 +273,22 @@ class CoalesceFeaturesAndStripAtomics final : public ModulePass {
 
   void recordFeatures(Module &M, const FeatureBitset &Features, bool Stripped) {
     for (const SubtargetFeatureKV &KV : WebAssemblyFeatureKV) {
-      std::string MDKey = (StringRef("wasm-feature-") + KV.Key).str();
-      if (KV.Value == WebAssembly::FeatureAtomics && Stripped) {
-        // "atomics" is special: code compiled without atomics may have had its
-        // atomics lowered to nonatomic operations. In that case, atomics is
-        // disallowed to prevent unsafe linking with atomics-enabled objects.
-        assert(!Features[WebAssembly::FeatureAtomics] ||
-               !Features[WebAssembly::FeatureBulkMemory]);
-        M.addModuleFlag(Module::ModFlagBehavior::Error, MDKey,
-                        wasm::WASM_FEATURE_PREFIX_DISALLOWED);
-      } else if (Features[KV.Value]) {
-        // Otherwise features are marked Used or not mentioned
+      if (Features[KV.Value]) {
+        // Mark features as used
+        std::string MDKey = (StringRef("wasm-feature-") + KV.Key).str();
         M.addModuleFlag(Module::ModFlagBehavior::Error, MDKey,
                         wasm::WASM_FEATURE_PREFIX_USED);
       }
     }
+    // Code compiled without atomics or bulk-memory may have had its atomics or
+    // thread-local data lowered to nonatomic operations or non-thread-local
+    // data. In that case, we mark the pseudo-feature "shared-mem" as disallowed
+    // to tell the linker that it would be unsafe to allow this code ot be used
+    // in a module with shared memory.
+    if (Stripped) {
+      M.addModuleFlag(Module::ModFlagBehavior::Error, "wasm-feature-shared-mem",
+                      wasm::WASM_FEATURE_PREFIX_DISALLOWED);
+    }
   }
 };
 char CoalesceFeaturesAndStripAtomics::ID = 0;

diff  --git a/llvm/test/CodeGen/WebAssembly/target-features-tls.ll b/llvm/test/CodeGen/WebAssembly/target-features-tls.ll
index c25b9e59b1b2..e146bdae5f54 100644
--- a/llvm/test/CodeGen/WebAssembly/target-features-tls.ll
+++ b/llvm/test/CodeGen/WebAssembly/target-features-tls.ll
@@ -13,8 +13,8 @@ target triple = "wasm32-unknown-unknown"
 ; NO-BULK-MEM-LABEL: .custom_section.target_features,"",@
 ; NO-BULK-MEM-NEXT: .int8 1
 ; NO-BULK-MEM-NEXT: .int8 45
-; NO-BULK-MEM-NEXT: .int8 7
-; NO-BULK-MEM-NEXT: .ascii "atomics"
+; NO-BULK-MEM-NEXT: .int8 10
+; NO-BULK-MEM-NEXT: .ascii "shared-mem"
 ; NO-BULK-MEM-NEXT: .bss.foo,"",@
 
 ; +bulk-memory


        


More information about the llvm-commits mailing list