[lld][WebAssembly] Error on import/export of mutable global without `mutable-globals` feature

[sbc100]

Summary

Also add the +mutable-globals features in clang when
when building with -fPIC since the linker will generate mutable
globals imports and exports in that case.

[tlively]

Discussed offline and decided that it would be better to error out in the linker and depend on the frontend to generate the necessary features based on flags like -mmutable-globals or -fPIC.

[sbc100]

switch to verification in wasm-ld

[sbc100]

OK.. updated PTAL

[tlively]

clang/lib/Driver/ToolChains/WebAssembly.cpp

246–252 ↗

(On Diff #291317)

This should also check that the user didn't pass options::OPT_mno_mutable_globals and a test should be added in clang/test/Driver/wasm-toolchain.c.

lld/wasm/Writer.cpp

499

could this if (sym->isExported()) be turned into an assert(sym->isExported())?

[sbc100]

lld/wasm/Writer.cpp

499

I don't think so. What about regular internal globals that are neither imported nor exported? Unless I'm missing something?

[sbc100]

add test

[tlively]

clang/lib/Driver/ToolChains/WebAssembly.cpp

246–252 ↗

(On Diff #291317)

Sorry, I meant that if the user passes -fPIC as well as -mno-mutable-globals, we should give them a diag::err_drv_argument_not_allowed_with error, like we do above for -pthread features and below for -fwasm-exceptions features,

lld/wasm/Writer.cpp

499

Ah, makes sense.

[sbc100]

Feedback

[tlively]

Thanks!

lld/test/wasm/gc-imports.s

 # CHECK-NEXT:     Name:            name
 # CHECK-NEXT:     FunctionNames:
 # CHECK-NEXT:       - Index:           0
 # CHECK-NEXT:         Name:            used_undef_function
 # CHECK-NEXT:       - Index:           1
 # CHECK-NEXT:         Name:            _start
 # CHECK-NEXT:       - Index:           2
 # CHECK-NEXT:         Name:            use_undef_global
+# CHECK-NEXT:   - Type:            CUSTOM
+# CHECK-NEXT:     Name:            target_features
+# CHECK-NEXT:     Features:
+# CHECK-NEXT:       - Prefix:          USED
+# CHECK-NEXT:         Name:            mutable-globals
 # CHECK-NEXT: ...
 
 # RUN: wasm-ld --no-gc-sections --allow-undefined \
 # RUN:     -o %t1.no-gc.wasm %t.o %t_globals.o
 # RUN: obj2yaml %t1.no-gc.wasm | FileCheck %s -check-prefix=NO-GC
 
 # NO-GC:        - Type:            IMPORT
 # NO-GC-NEXT:     Imports:

 # NO-GC-NEXT:       - Index:           2
 # NO-GC-NEXT:         Name:            __wasm_call_ctors
 # NO-GC-NEXT:       - Index:           3
 # NO-GC-NEXT:         Name:            foo
 # NO-GC-NEXT:       - Index:           4
 # NO-GC-NEXT:         Name:            _start
 # NO-GC-NEXT:       - Index:           5
 # NO-GC-NEXT:         Name:            use_undef_global
+# NO-GC-NEXT:   - Type:            CUSTOM
+# NO-GC-NEXT:     Name:            target_features
+# NO-GC-NEXT:     Features:
+# NO-GC-NEXT:       - Prefix:          USED
+# NO-GC-NEXT:         Name:            mutable-globals
 # NO-GC-NEXT: ...

lld/wasm/Writer.cpp

auto isTLS = [](InputSegment *segment) {
       StringRef name = segment->getName();
       return segment->live &&
              (name.startswith(".tdata") || name.startswith(".tbss"));
     };
     tlsUsed = tlsUsed ||
               std::any_of(file->segments.begin(), file->segments.end(), isTLS);
   }
 
+  // If no object files explictly used mutable globals we still might need
+  // to include it due to linker-generated globals such as those use in the
+  // PIC ABI for importing and exported symbol offsets.
+  if (!config->relocatable && used.count("mutable-globals") == 0) {
+    for (Symbol *sym : symtab->getSymbols()) {
+      if (auto *global = dyn_cast<GlobalSymbol>(sym)) {
+        if (global->getGlobalType()->Mutable) {
+          if (!sym->isLive())
+            continue;
+          if (!sym->isUsedInRegularObj)
+            continue;
+          if (sym->isUndefined() && sym->isWeak() && !config->relocatable)
+            continue;
+          if (sym->isUndefined() || sym->isExported()) {
+            used.insert({"mutable-globals", "<internal>"});
+          }
+        }
+      }
+    }
+  }
+
   if (inferFeatures)
     for (const auto &key : used.keys())
       allowed.insert(std::string(key));
 
   if (!config->relocatable && allowed.count("atomics") &&
       !config->sharedMemory) {
     if (inferFeatures)
       error(Twine("'atomics' feature is used by ") + used["atomics"] +
if (config->sharedMemory) {
     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)))