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

[tlively]

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.

[sbc100]

lld/test/wasm/shared-memory-no-atomics.yaml

60

This sentence sounds a little redundant to me? The part about shared memory not being allowed. Maybe its just me ..

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

265

Why not add this to WebAssemblyFeatureKV?

[aheejin]

This was previously done by disallowing the 'atomics' feature, but that could lead to an awkward situation in which an object was marked as disallowing atomics when it in fact used atomics itself. Once atomic operations are allowed in modules with unshared memory, this would result in modules containing atomics but not having the 'atomics' feature in their target-features sections.

Is this possible now? When we disallow atomics now, haven't we already stripped all atomic instructions by then, or we didn't have atomics in the first place? How can a module contain atomics and disallow it at the same time?

[tlively]

In D79542#2024946, @aheejin wrote:

This was previously done by disallowing the 'atomics' feature, but that could lead to an awkward situation in which an object was marked as disallowing atomics when it in fact used atomics itself. Once atomic operations are allowed in modules with unshared memory, this would result in modules containing atomics but not having the 'atomics' feature in their target-features sections.

Is this possible now? When we disallow atomics now, haven't we already stripped all atomic instructions by then, or we didn't have atomics in the first place? How can a module contain atomics and disallow it at the same time?

Consider compiling a module that contains TLS and has 'atomics' enabled but not 'bulk-memory'. The module's TLS and atomic operations will be lowered away because TLS requires bulk-memory. Note that intrinsics like @llvm.wasm.notify and friends are not currently lowered away, which is how you can end up with a module that has its atomics stripped but still contains atomics. But we could fix that by lowering away the notify and wait intrinsics, and then you'd be right that this is impossible.

I suppose the simpler and more important reason for this change is that it is perfectly safe to link an object with atomics with an object with stripped atomics as long as the resulting module does not use a shared memory, but this is disallowed when the stripped object has '-atomics'. I'll update the message to explain that instead.

lld/test/wasm/shared-memory-no-atomics.yaml

60

Sure, I'll remove everything after the comma and change the beginning to use the literal flag name.

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

265

WebAssemblyFeatureKV is generated from the TableGen definitions of the WebAssembly subtarget features. I didn't want to go so far as to add a real feature for this, though, since users should not be manually enabling or disabling it.

[tlively]

• Update error message

lld/test/wasm/shared-memory-no-atomics.yaml

SegmentInfo:
       - Index:           0
         Name:            .rodata.hello_str
         Alignment:       0
         Flags:           [  ]
   - Type:            CUSTOM
     Name:            target_features
     Features:
       - Prefix:        DISALLOWED
-        Name:          "atomics"
+        Name:          "shared-mem"
 ...
 
 # NO-SHARED:      - Type:            MEMORY
 # NO-SHARED-NEXT:   Memories:
 # 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, so --shared-memory must not be used.

lld/wasm/Writer.cpp

if (inferFeatures)
             ", so --shared-memory must be used");
     else
       error("'atomics' feature is used, so --shared-memory must be used");
   }
 
   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 (!allowed.count("bulk-memory") && tlsUsed)
-    error("'bulk-memory' feature must be used in order to use thread-local "
-          "storage");
+  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, so --shared-memory must not be used.");
+
+    for (auto feature : {"atomics", "bulk-memory"})
+      if (!allowed.count(feature))
+        error(StringRef("'") + feature +
+              "' feature must be used in order to use shared memory");
+  }
+
+  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) {
     for (auto &feature : used.keys()) {
       if (!allowed.count(std::string(feature)))
         error(Twine("Target feature '") + feature + "' used by " +
               used[feature] + " is not allowed.");
     }

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

if (FieldCount != 0) {
     }
     OutStreamer->PopSection();
   }
 }
 
 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()))
         Entry.Prefix = I->getZExtValue();
 
     // Silently ignore invalid metadata
     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;
 
   // Emit features and linkage policies into the "target_features" section
   MCSectionWasm *FeaturesSection = OutContext.getWasmSection(
       ".custom_section.target_features", SectionKind::getMetadata());
   OutStreamer->PushSection();

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

for (auto &GV : M.globals()) {
         GV.setThreadLocalMode(GlobalValue::ThreadLocalMode::NotThreadLocal);
       }
     }
     return Stripped;
   }
 
   void recordFeatures(Module &M, const FeatureBitset &Features, bool Stripped) {
     for (const SubtargetFeatureKV &KV : WebAssemblyFeatureKV) {
+      if (Features[KV.Value]) {
+        // Mark features as used
         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
         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;
 
 /// WebAssembly Code Generator Pass Configuration Options.
 class WebAssemblyPassConfig final : public TargetPassConfig {
 public:
   WebAssemblyPassConfig(WebAssemblyTargetMachine &TM, PassManagerBase &PM)

llvm/test/CodeGen/WebAssembly/target-features-tls.ll

 ; RUN: llc < %s -mattr=-bulk-memory | FileCheck %s --check-prefixes NO-BULK-MEM
 ; RUN: llc < %s -mattr=+bulk-memory | FileCheck %s --check-prefixes BULK-MEM
 
 ; Test that the target features section contains -atomics or +atomics
 ; for modules that have thread local storage in their source.
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown"
 
 @foo = internal thread_local global i32 0
 
 ; -bulk-memory
 ; 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
 ; BULK-MEM-LABEL: .custom_section.target_features,"",@
 ; BULK-MEM-NEXT: .int8 1
 ; BULK-MEM-NEXT: .int8 43
 ; BULK-MEM-NEXT: .int8 11
 ; BULK-MEM-NEXT: .ascii "bulk-memory"
 ; BULK-MEM-NEXT: .tbss.foo,"",@