[WebAssembly] "atomics" feature requires shared memory

[tlively]

Summary

Makes it a linker error if the "atomics" feature is used but the user
does not opt in to shared memory or if "atomics" is disallowed but the
user does opt in to shared memory. Also check that an appropriate max
memory size is supplied if shared memory is used.

[sunfish]

In the future, atomics will likely be enabled by default, however it will continue to be desirable to be able to select non-shared memory, since non-shared memory doesn't require a maximum size, and since it can make interoperating with JS and other external code simpler. Is it possible to structure this patch in a way that supports this?

[sbc100]

In D59281#1429109, @sunfish wrote:

In the future, atomics will likely be enabled by default, however it will continue to be desirable to be able to select non-shared memory, since non-shared memory doesn't require a maximum size, and since it can make interoperating with JS and other external code simpler. Is it possible to structure this patch in a way that supports this?

When building for non-shared memory, shouldn't input code check for "ifdef _REENTRANT" and not generate any atomic instructions?

The wasm spec says that atomics are simply not allowed when non-shared memory is used. Do we need to hide this fact from the user code?

[sunfish]

In D59281#1429332, @sbc100 wrote:

In D59281#1429109, @sunfish wrote:

In the future, atomics will likely be enabled by default, however it will continue to be desirable to be able to select non-shared memory, since non-shared memory doesn't require a maximum size, and since it can make interoperating with JS and other external code simpler. Is it possible to structure this patch in a way that supports this?

When building for non-shared memory, shouldn't input code check for "ifdef _REENTRANT" and not generate any atomic instructions?

That, or they could use -mthread-model single and lower atomics into non-atomics.

It sounds like the answer is, users that want non-shared memory should use -mno-atomics. I guess that works, it's just surprising if we think of -matomics as a pure target feature flag.

[tlively]

In D59281#1429940, @sunfish wrote:

It sounds like the answer is, users that want non-shared memory should use -mno-atomics. I guess that works, it's just surprising if we think of -matomics as a pure target feature flag.

Yes, that is a little surprising but it also makes the flags simpler. If -matomics was purely for the target and we also used the thread model flags, -matomics would become entirely redundant; there would be no reason to use -matomics without -mthread-model posix and vice versa. So as a simplification, we just have -matomics carry both meanings. The corollary of that is that we will never include -matomics by default in any CPU configuration. Thankfully, this is all entirely transparent to the user, who still uses -pthread if they want threads and don't use -pthread if they don't want threads and everything just works.

Here's a deeper explanation of why treating -matomics this way is fine as far as link-time feature validation is concerned. Consider object files in the following situations for a target that supports atomics:

A) application is single-threaded and atomic operations are lowered to non-atomic operations.
B) application is single-threaded but atomic operations are preserved
C) application is multithreaded so atomic operations are preserved

Here are our goals:

1. A should link with B. This is safe because the applications is single-threaded and the target supports atomics.
2. A should not link with C. It is not safe to link a module with atomics stripped into a multithreaded application.
3. B and C are the same object. Otherwise users would have to build their libraries twice when the only difference is the threadedness of the final application.

These goals form a contradiction, so we can only choose two of them. 2 enforces an important safety property and dropping 3 would be a larger ergonomic loss than dropping 1, so we drop goal 1. The three possible situations above are then represented by two types of object files:

• An object where atomic operations are stripped (A)
• An object where atomic operations are preserved (B and C)

These two types of objects cannot in general be safely linked together, so the first is no different from an object that does not support atomics at all. QED.

What that argument does not capture is the case where atomic operations are neither stripped or preserved because there were none in the source to begin with. In this case it would be ideal to produce an object that can be linked into both mutithreaded and single-threaded applications. This is totally possible as far as the backend and linker are concerned*, but it is an open UI problem. The only way I see to achieve this is to detect the use of atomic operations in the backend and magically change the atomics feature linkage policy depending on their presence. And that seems perhaps too magical. WDYT?

(*) This would be achieved by making objects containing atomic ops use "+atomics" in their target feature section, making objects with any stripped atomics use "-atomics", and making objects with no atomics to begin with simply not mention "atomics".

[jfb]

D) Application uses (non-existent in WebAssembly) signal handling support and atomics just enforce program order.
E) Application is single-threaded but does cooperative interleaving of functions (but doesn't call it threads).

[tlively]

In D59281#1430026, @jfb wrote:

D) Application uses (non-existent in WebAssembly) signal handling support and atomics just enforce program order.
E) Application is single-threaded but does cooperative interleaving of functions (but doesn't call it threads).

Thanks, JF! I definitely want to consider all cases. The cases you mention use atomics independently from threads, so I think they are further evidence that when the user uses -matomics we should never be stripping away their atomic operations and that it is dangerous to allow linking together objects with atomics both stripped and not stripped.

[sbc100]

lld/wasm/Writer.cpp

915 ↗

(On Diff #190366)

I think we decided against this part right? i.e. atomics should require but not imply shared memory? So this should probably be an error instead.

[tlively]

lld/wasm/Writer.cpp

915 ↗

(On Diff #190366)

Yes, sorry. I updated the name and description but I haven't updated the code yet. This CL is going to be blocked on an upcoming CL that detects whether any atomics were stripped. Landing that first will avoid unnecessary turn in some tests.

[tlively]

• Make using --shared-memory when atomics are disallowed or omitting it when atomics are used an error.

[tlively]

• rebase

[tlively]

@sbc100 and @aheejin, this is unblocked and ready to land whenever you have a chance to review it.

[sbc100]

lld/test/wasm/shared-memory.yaml

80 ↗

(On Diff #192750)

This block looks exactly the same as the one above. Perhaps you only need one?

lld/wasm/Writer.cpp

915 ↗

(On Diff #190366)

Use single quotes to avoid the escape char? Here and below.

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

+# RUN: yaml2obj %s -o %t1.o
+
+# RUN: wasm-ld --no-entry %t1.o -o - | obj2yaml | FileCheck %s --check-prefix NO-SHARED
+
+# RUN: not wasm-ld --no-entry --shared-memory --max-memory=131072 %t1.o -o - 2>&1 | FileCheck %s --check-prefix SHARED
+
+--- !WASM
+FileHeader:
+  Version:         0x00000001
+Sections:
+  - Type:            IMPORT
+    Imports:
+      - Module:          env
+        Field:           __linear_memory
+        Kind:            MEMORY
+        Memory:
+          Initial:         0x00000001
+      - Module:          env
+        Field:           __indirect_function_table
+        Kind:            TABLE
+        Table:
+          ElemType:        FUNCREF
+          Limits:
+            Initial:         0x00000000
+  - Type:            DATA
+    Segments:
+      - SectionOffset:   6
+        InitFlags:       0
+        Offset:
+          Opcode:          I32_CONST
+          Value:           0
+        Content:         68656C6C6F0A00
+  - Type:            CUSTOM
+    Name:            linking
+    Version:         2
+    SymbolTable:
+      - Index:           0
+        Kind:            DATA
+        Name:            hello_str
+        Flags:           [  ]
+        Segment:         0
+        Size:            7
+    SegmentInfo:
+      - Index:           0
+        Name:            .rodata.hello_str
+        Alignment:       0
+        Flags:           [  ]
+  - Type:            CUSTOM
+    Name:            target_features
+    Features:
+      - Prefix:        DISALLOWED
+        Name:          "atomics"
+...
+
+# NO-SHARED:      - Type:            MEMORY
+# NO-SHARED-NEXT:   Memories:
+# NO-SHARED-NEXT:     - Initial:         0x00000002
+# NO-SHARED-NOT:        Maximum:
+
+# SHARED: 'atomics' feature is disallowed, so --shared-memory must not be used{{$}}

lld/trunk/test/wasm/shared-memory.yaml

+# RUN: yaml2obj %s -o %t1.o
+
+# RUN: not wasm-ld --no-entry --shared-memory %t1.o -o - 2>&1 | FileCheck %s --check-prefix SHARED-NO-MAX
+
+# RUN: not wasm-ld --no-entry --shared-memory --max-memory=100000 %t1.o -o - 2>&1 | FileCheck %s --check-prefix SHARED-UNALIGNED
+
+# RUN: wasm-ld --no-entry --shared-memory --max-memory=131072 %t1.o -o - | obj2yaml | FileCheck %s --check-prefix SHARED
+
+# RUN: not wasm-ld --no-entry --features=atomics %t1.o -o - 2>&1 | FileCheck %s --check-prefix ATOMICS-NO-SHARED
+
+# RUN: not wasm-ld --no-entry --features=atomics --shared-memory %t1.o -o - 2>&1 | FileCheck %s --check-prefix ATOMICS-NO-MAX
+
+# RUN: not wasm-ld --no-entry --features=atomics --shared-memory --max-memory=100000 %t1.o -o - 2>&1 | FileCheck %s --check-prefix ATOMICS-UNALIGNED
+
+# RUN: wasm-ld --no-entry --features=atomics --shared-memory --max-memory=131072 %t1.o -o - | obj2yaml | FileCheck %s --check-prefix SHARED
+
+
+--- !WASM
+FileHeader:
+  Version:         0x00000001
+Sections:
+  - Type:            IMPORT
+    Imports:
+      - Module:          env
+        Field:           __linear_memory
+        Kind:            MEMORY
+        Memory:
+          Initial:         0x00000001
+      - Module:          env
+        Field:           __indirect_function_table
+        Kind:            TABLE
+        Table:
+          ElemType:        FUNCREF
+          Limits:
+            Initial:         0x00000000
+  - Type:            DATA
+    Segments:
+      - SectionOffset:   6
+        InitFlags:       0
+        Offset:
+          Opcode:          I32_CONST
+          Value:           0
+        Content:         68656C6C6F0A00
+  - Type:            CUSTOM
+    Name:            linking
+    Version:         2
+    SymbolTable:
+      - Index:           0
+        Kind:            DATA
+        Name:            hello_str
+        Flags:           [  ]
+        Segment:         0
+        Size:            7
+    SegmentInfo:
+      - Index:           0
+        Name:            .rodata.hello_str
+        Alignment:       0
+        Flags:           [  ]
+...
+
+# SHARED-NO-MAX: maximum memory too small, 66560 bytes needed{{$}}
+
+# SHARED-UNALIGNED: maximum memory must be 65536-byte aligned{{$}}
+
+# SHARED:        - Type:            MEMORY
+# SHARED-NEXT:     Memories:
+# SHARED-NEXT:       - Flags:           [ HAS_MAX, IS_SHARED ]
+# SHARED-NEXT:         Initial:         0x00000002
+# SHARED-NEXT:         Maximum:         0x00000002
+
+# ATOMICS-NO-SHARED: 'atomics' feature is used, so --shared-memory must be used{{$}}
+
+# ATOMICS-NO-MAX: maximum memory too small, 66560 bytes needed{{$}}
+
+# ATOMICS-UNALIGNED: maximum memory must be 65536-byte aligned{{$}}

lld/trunk/wasm/Writer.cpp

void Writer::createImportSection() {
 
   if (Config->ImportMemory) {
     WasmImport Import;
     Import.Module = DefaultModule;
     Import.Field = "memory";
     Import.Kind = WASM_EXTERNAL_MEMORY;
     Import.Memory.Flags = 0;
     Import.Memory.Initial = NumMemoryPages;
-    if (MaxMemoryPages != 0) {
+    if (MaxMemoryPages != 0 || Config->SharedMemory) {
       Import.Memory.Flags |= WASM_LIMITS_FLAG_HAS_MAX;
       Import.Memory.Maximum = MaxMemoryPages;
     }
     if (Config->SharedMemory)
       Import.Memory.Flags |= WASM_LIMITS_FLAG_IS_SHARED;
     writeImport(OS, Import);
   }
 

 
 void Writer::createMemorySection() {
   if (Config->ImportMemory)
     return;
 
   SyntheticSection *Section = createSyntheticSection(WASM_SEC_MEMORY);
   raw_ostream &OS = Section->getStream();
 
-  bool HasMax = MaxMemoryPages != 0;
+  bool HasMax = MaxMemoryPages != 0 || Config->SharedMemory;
   writeUleb128(OS, 1, "memory count");
   unsigned Flags = 0;
   if (HasMax)
     Flags |= WASM_LIMITS_FLAG_HAS_MAX;
   if (Config->SharedMemory)
     Flags |= WASM_LIMITS_FLAG_IS_SHARED;
   writeUleb128(OS, Flags, "memory limits flags");
   writeUleb128(OS, NumMemoryPages, "initial pages");
if (MemoryPtr > Config->InitialMemory)
       error("initial memory too small, " + Twine(MemoryPtr) + " bytes needed");
     else
       MemoryPtr = Config->InitialMemory;
   }
   MemSize = MemoryPtr;
   NumMemoryPages = alignTo(MemoryPtr, WasmPageSize) / WasmPageSize;
   log("mem: total pages = " + Twine(NumMemoryPages));
 
-  if (Config->MaxMemory != 0) {
+  // Check max if explicitly supplied or required by shared memory
+  if (Config->MaxMemory != 0 || Config->SharedMemory) {
     if (Config->MaxMemory != alignTo(Config->MaxMemory, WasmPageSize))
       error("maximum memory must be " + Twine(WasmPageSize) + "-byte aligned");
     if (MemoryPtr > Config->MaxMemory)
       error("maximum memory too small, " + Twine(MemoryPtr) + " bytes needed");
     MaxMemoryPages = Config->MaxMemory / WasmPageSize;
     log("mem: max pages   = " + Twine(MaxMemoryPages));
   }
 }
for (auto &Feature : File->getWasmObj()->getTargetFeatures()) {
               std::to_string(Feature.Prefix));
       }
     }
   }
 
   if (InferFeatures)
     TargetFeatures.insert(Used.begin(), Used.end());
 
+  if (TargetFeatures.count("atomics") && !Config->SharedMemory)
+    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, so --shared-memory must not be used");
+
   // Validate that used features are allowed in output
   if (!InferFeatures) {
     for (auto &Feature : Used) {
       if (!TargetFeatures.count(Feature))
         error(Twine("Target feature '") + Feature + "' is not allowed.");
     }
   }