[lld][WebAssembly] Allow exporting of mutable globals

[sbc100]

Summary

In particular allow explict exporting of __stack_pointer but
exclude this from --export-all to avoid requiring the mutable
globals feature whenenve --export-all is used.

This uncovered a bug in populateTargetFeatures regarding checking
if the mutable-globals feature is allowed.

See: https://github.com/WebAssembly/binaryen/issues/2934

[sbc100]

Typeos

[tlively]

I think it's kind of confusing that export-all doesn't actually export all the symbols. What if we instead made export-all exactly equivalent to individually exporting all the symbols so that it errors out if mutable-globals is not enabled and any of the symbols is a mutable global? Then we could add a --no-export=<symbol> flag that users or drivers could use to filter out the mutable global symbols if they can't enable mutable-globals and need to avoid the error.

lld/wasm/Writer.cpp

456

Oops! Nice catch.

579

[sbc100]

In D88506#2301679, @tlively wrote:

I think it's kind of confusing that export-all doesn't actually export all the symbols. What if we instead made export-all exactly equivalent to individually exporting all the symbols so that it errors out if mutable-globals is not enabled and any of the symbols is a mutable global? Then we could add a --no-export=<symbol> flag that users or drivers could use to filter out the mutable global symbols if they can't enable mutable-globals and need to avoid the error.

The problem with that is that it would be breaking change for pretty much all current users of --export-all. This is not ideal... but I think its reasonable compromise between internal consistency and backward compatibility.

Also its not clear that any users of --export-all really want these internal symbols to be exported. Indeed they have made do up until now without them.

[tlively]

Ah, I didn't realize export-all already didn't export these symbols, but that makes sense. Can we make sure the help documentation mentions that export-all does not include synthetic symbols?

[sbc100]

docs

lld/docs/WebAssembly.rst

 .. option:: --import-table
 
   Import the function table from the environment.
 
 .. option:: --export-all
 
   Export all symbols (normally combined with --no-gc-sections)
 
+  Note that this will not export linker-generated mutable globals unless
+  the resulting binaryen already includes the 'mutable-globals' features
+  since that would otherwise create and invalid binaryen.
+
 .. option:: --export-dynamic
 
   When building an executable, export any non-hidden symbols.  By default only
   the entry point and any symbols marked as exports (either via the command line
   or via the `export-name` source attribute) are exported.
 
 .. option:: --global-base=<value>
 

lld/test/wasm/mutable-global-exports.s

+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
+#
+# Should fail without mutable globals feature enabled.
+# RUN: not wasm-ld --export-all %t.o -o %t.wasm 2>&1 | FileCheck -check-prefix=CHECK-ERR %s
+# RUN: not wasm-ld --export=foo_global %t.o -o %t.wasm 2>&1 | FileCheck -check-prefix=CHECK-ERR %s
+#
+# RUN: wasm-ld --features=mutable-globals --export=foo_global %t.o -o %t.wasm
+# RUN: obj2yaml %t.wasm | FileCheck %s
+
+# Explcitly check that __stack_pointer can be exported
+# RUN: wasm-ld --features=mutable-globals --export=__stack_pointer %t.o -o %t.wasm
+# RUN: obj2yaml %t.wasm | FileCheck -check-prefix=CHECK-SP %s
+
+# RUN: wasm-ld --features=mutable-globals --export-all %t.o -o %t.wasm
+# RUN: obj2yaml %t.wasm | FileCheck -check-prefix=CHECK-ALL %s
+
+
+.globl _start
+.globl foo_global
+
+.globaltype foo_global, i32
+foo_global:
+
+_start:
+  .functype _start () -> ()
+  end_function
+
+# CHECK-ERR: mutable global exported but 'mutable-globals' feature not present in inputs: `foo_global`. Use --no-check-features to suppress
+
+#      CHECK:  - Type:            EXPORT
+# CHECK-NEXT:    Exports:
+# CHECK-NEXT:      - Name:            memory
+# CHECK-NEXT:        Kind:            MEMORY
+# CHECK-NEXT:        Index:           0
+# CHECK-NEXT:      - Name:            _start
+# CHECK-NEXT:        Kind:            FUNCTION
+# CHECK-NEXT:        Index:           0
+# CHECK-NEXT:      - Name:            foo_global
+# CHECK-NEXT:        Kind:            GLOBAL
+# CHECK-NEXT:        Index:           1
+# CHECK-NEXT:  - Type:            CODE
+
+#      CHECK-SP:  - Type:            EXPORT
+# CHECK-SP-NEXT:    Exports:
+# CHECK-SP-NEXT:      - Name:            memory
+# CHECK-SP-NEXT:        Kind:            MEMORY
+# CHECK-SP-NEXT:        Index:           0
+# CHECK-SP-NEXT:      - Name:            __stack_pointer
+# CHECK-SP-NEXT:        Kind:            GLOBAL
+# CHECK-SP-NEXT:        Index:           0
+# CHECK-SP-NEXT:      - Name:            _start
+# CHECK-SP-NEXT:        Kind:            FUNCTION
+# CHECK-SP-NEXT:        Index:           0
+# CHECK-SP-NEXT:  - Type:            CODE
+
+#      CHECK-ALL:  - Type:            EXPORT
+# CHECK-ALL-NEXT:    Exports:
+# CHECK-ALL-NEXT:      - Name:            memory
+# CHECK-ALL-NEXT:        Kind:            MEMORY
+# CHECK-ALL-NEXT:        Index:           0
+# CHECK-ALL-NEXT:      - Name:            __wasm_call_ctors
+# CHECK-ALL-NEXT:        Kind:            FUNCTION
+# CHECK-ALL-NEXT:        Index:           0
+# CHECK-ALL-NEXT:      - Name:            _start
+# CHECK-ALL-NEXT:        Kind:            FUNCTION
+# CHECK-ALL-NEXT:        Index:           1
+# CHECK-ALL-NEXT:      - Name:            foo_global
+# CHECK-ALL-NEXT:        Kind:            GLOBAL
+# CHECK-ALL-NEXT:        Index:           1
+# CHECK-ALL-NEXT:      - Name:            __dso_handle
+# CHECK-ALL-NEXT:        Kind:            GLOBAL
+# CHECK-ALL-NEXT:        Index:           2
+# CHECK-ALL-NEXT:      - Name:            __data_end
+# CHECK-ALL-NEXT:        Kind:            GLOBAL
+# CHECK-ALL-NEXT:        Index:           3
+# CHECK-ALL-NEXT:      - Name:            __global_base
+# CHECK-ALL-NEXT:        Kind:            GLOBAL
+# CHECK-ALL-NEXT:        Index:           4
+# CHECK-ALL-NEXT:      - Name:            __heap_base
+# CHECK-ALL-NEXT:        Kind:            GLOBAL
+# CHECK-ALL-NEXT:        Index:           5
+# CHECK-ALL-NEXT:      - Name:            __memory_base
+# CHECK-ALL-NEXT:        Kind:            GLOBAL
+# CHECK-ALL-NEXT:        Index:           6
+# CHECK-ALL-NEXT:      - Name:            __table_base
+# CHECK-ALL-NEXT:        Kind:            GLOBAL
+# CHECK-ALL-NEXT:        Index:           7
+# CHECK-ALL-NEXT:  - Type:            CODE

lld/test/wasm/mutable-globals.s

 # RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
 # RUN: not wasm-ld %t.o -o %t.wasm 2>&1 | FileCheck %s
+# RUN: wasm-ld --features=mutable-globals %t.o -o %t.wasm
 
 .globl _start
 _start:
   .functype _start () -> ()
   i32.const 1
   global.set foo
   end_function
 
 .globaltype foo, i32
 .import_module foo, env
 .import_name foo, foo
 
 # CHECK: error: mutable global imported but 'mutable-globals' feature not present in inputs: `foo`. Use --no-check-features to suppress.

lld/wasm/Writer.cpp

void Writer::populateTargetFeatures() {
 
   if (inferFeatures)
     for (const auto &key : used.keys())
       allowed.insert(std::string(key));
 
   if (!config->checkFeatures)
     return;
 
-  if (!config->relocatable && used.count("mutable-globals") == 0) {
+  if (!config->relocatable && allowed.count("mutable-globals") == 0) {
     for (const Symbol *sym : out.importSec->importedSymbols) {
       if (auto *global = dyn_cast<GlobalSymbol>(sym)) {
         if (global->getGlobalType()->Mutable) {
           error(Twine("mutable global imported but 'mutable-globals' feature "
                       "not present in inputs: `") +
                 toString(*sym) + "`. Use --no-check-features to suppress.");
         }
       }
for (Symbol *sym : symtab->getSymbols()) {
     StringRef name = sym->getName();
     WasmExport export_;
     if (auto *f = dyn_cast<DefinedFunction>(sym)) {
       if (Optional<StringRef> exportName = f->function->getExportName()) {
         name = *exportName;
       }
       export_ = {name, WASM_EXTERNAL_FUNCTION, f->getFunctionIndex()};
     } else if (auto *g = dyn_cast<DefinedGlobal>(sym)) {
-      // TODO(sbc): Remove this check once to mutable global proposal is
-      // implement in all major browsers.
-      // See: https://github.com/WebAssembly/mutable-global
-      if (g->getGlobalType()->Mutable) {
-        // Only __stack_pointer and __tls_base should ever be create as mutable.
-        assert(g == WasmSym::stackPointer || g == WasmSym::tlsBase);
+      if (g->getGlobalType()->Mutable && !g->getFile() && !g->forceExport) {
+        // Avoid exporting mutable globals are linker synthesized (e.g.
+        // __stack_pointer or __tls_base) unless they are explicitly exported
+        // from the command line.
+        // Without this check `--export-all` would cause any program using the
+        // stack pointer to export a mutable global even if none of the input
+        // files were built with the `mutable-globals` feature.
         continue;
       }
       export_ = {name, WASM_EXTERNAL_GLOBAL, g->getGlobalIndex()};
     } else if (auto *e = dyn_cast<DefinedEvent>(sym)) {
       export_ = {name, WASM_EXTERNAL_EVENT, e->getEventIndex()};
     } else {
       auto *d = cast<DefinedData>(sym);
       out.globalSec->dataAddressGlobals.push_back(d);