[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

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

478

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

[sbc100]

lld/wasm/Writer.cpp

478

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

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

478

Ah, makes sense.

[sbc100]

Feedback

[tlively]

Thanks!

clang/lib/Driver/ToolChains/WebAssembly.cpp

if (DriverArgs.hasFlag(options::OPT_pthread, options::OPT_no_pthread,
     CC1Args.push_back("-target-feature");
     CC1Args.push_back("+bulk-memory");
     CC1Args.push_back("-target-feature");
     CC1Args.push_back("+mutable-globals");
     CC1Args.push_back("-target-feature");
     CC1Args.push_back("+sign-ext");
   }
 
+  if (!DriverArgs.hasFlag(options::OPT_mmutable_globals,
+                          options::OPT_mno_mutable_globals, false)) {
+    // -fPIC implies +mutable-globals because the PIC ABI used by the linker
+    // depends on importing and exporting mutable globals.
+    llvm::Reloc::Model RelocationModel;
+    unsigned PICLevel;
+    bool IsPIE;
+    std::tie(RelocationModel, PICLevel, IsPIE) =
+        ParsePICArgs(*this, DriverArgs);
+    if (RelocationModel == llvm::Reloc::PIC_) {
+      if (DriverArgs.hasFlag(options::OPT_mno_mutable_globals,
+                             options::OPT_mmutable_globals, false)) {
+        getDriver().Diag(diag::err_drv_argument_not_allowed_with)
+            << "-fPIC"
+            << "-mno-mutable-globals";
+      }
+      CC1Args.push_back("-target-feature");
+      CC1Args.push_back("+mutable-globals");
+    }
+  }
+
   if (DriverArgs.getLastArg(options::OPT_fwasm_exceptions)) {
     // '-fwasm-exceptions' is not compatible with '-mno-exception-handling'
     if (DriverArgs.hasFlag(options::OPT_mno_exception_handing,
                            options::OPT_mexception_handing, false))
       getDriver().Diag(diag::err_drv_argument_not_allowed_with)
           << "-fwasm-exceptions"
           << "-mno-exception-handling";
     // '-fwasm-exceptions' is not compatible with '-mno-reference-types'

clang/test/Driver/wasm-toolchain.c

 // RUN:   | FileCheck -check-prefix=CHECK-COMMAND %s
 // CHECK-COMMAND: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
 // CHECK-COMMAND: wasm-ld{{.*}}" "crt1.o" "[[temp]]" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
 
 // RUN: %clang %s -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=%s/no-sysroot-there -mexec-model=reactor 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-REACTOR %s
 // CHECK-REACTOR: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
 // CHECK-REACTOR: wasm-ld{{.*}}" "crt1-reactor.o" "--entry" "_initialize" "[[temp]]" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+
+// -fPIC implies +mutable-globals
+
+// RUN: %clang %s -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=%s/no-sysroot-there -fPIC 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-PIC %s
+// CHECK-PIC: clang{{.*}}" "-cc1" {{.*}} "-target-feature" "+mutable-globals"
+
+// '-mno-mutable-globals' is not allowed with '-fPIC'
+// RUN: %clang %s -### -no-canonical-prefixes -target wasm32-unknown-unknown --sysroot=%s/no-sysroot-there -fPIC -mno-mutable-globals %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=PIC_NO_MUTABLE_GLOBALS %s
+// PIC_NO_MUTABLE_GLOBALS: error: invalid argument '-fPIC' not allowed with '-mno-mutable-globals'

lld/test/wasm/Inputs/undefined-globals.s

 .globl use_undef_global
 .globl unused_undef_global
 .globl used_undef_global
 
 use_undef_global:
   .functype use_undef_global () -> (i64)
   global.get used_undef_global
   end_function
 
-.globaltype unused_undef_global, i64
-.globaltype used_undef_global, i64
+.globaltype unused_undef_global, i64, immutable
+.globaltype used_undef_global, i64, immutable

lld/test/wasm/emit-relocs-fpic.s

-# RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj -o %t.o < %s
+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown %s -o %t.o
 # RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown %p/Inputs/ret32.s -o %t.ret32.o
-# RUN: wasm-ld -pie --export-all --no-gc-sections --no-entry --emit-relocs -o %t.wasm %t.o %t.ret32.o
+# RUN: wasm-ld -pie --export-all --no-check-features --no-gc-sections --no-entry --emit-relocs -o %t.wasm %t.o %t.ret32.o
 # RUN: obj2yaml %t.wasm | FileCheck %s
 
 load_hidden_data:
     .functype   load_hidden_data () -> (i32)
     i32.const   .L.hidden_data@MBREL
     end_function
 
 .section .rodata.hidden_data,"",@

lld/test/wasm/gc-imports.s

 # CHECK-NEXT:       - Module:          env
 # CHECK-NEXT:         Field:           used_undef_function
 # CHECK-NEXT:         Kind:            FUNCTION
 # CHECK-NEXT:         SigIndex:        0
 # CHECK-NEXT:       - Module:          env
 # CHECK-NEXT:         Field:           used_undef_global
 # CHECK-NEXT:         Kind:            GLOBAL
 # CHECK-NEXT:         GlobalType:      I64
-# CHECK-NEXT:         GlobalMutable:   true
+# CHECK-NEXT:         GlobalMutable:   false
 # CHECK-NEXT:   - Type:
 # CHECK:        - Type:            CUSTOM
 # 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

 # NO-GC-NEXT:       - Module:          env
 # NO-GC-NEXT:         Field:           used_undef_function
 # NO-GC-NEXT:         Kind:            FUNCTION
 # NO-GC-NEXT:         SigIndex:        1
 # NO-GC-NEXT:       - Module:          env
 # NO-GC-NEXT:         Field:           unused_undef_global
 # NO-GC-NEXT:         Kind:            GLOBAL
 # NO-GC-NEXT:         GlobalType:      I64
-# NO-GC-NEXT:         GlobalMutable:   true
+# NO-GC-NEXT:         GlobalMutable:   false
 # NO-GC-NEXT:       - Module:          env
 # NO-GC-NEXT:         Field:           used_undef_global
 # NO-GC-NEXT:         Kind:            GLOBAL
 # NO-GC-NEXT:         GlobalType:      I64
-# NO-GC-NEXT:         GlobalMutable:   true
+# NO-GC-NEXT:         GlobalMutable:   false
 # NO-GC-NEXT:   - Type:
 # NO-GC:        - Type:            CUSTOM
 # NO-GC-NEXT:     Name:            name
 # NO-GC-NEXT:     FunctionNames:
 # NO-GC-NEXT:       - Index:           0
 # NO-GC-NEXT:         Name:            unused_undef_function
 # NO-GC-NEXT:       - Index:           1
 # NO-GC-NEXT:         Name:            used_undef_function

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
+
+.globl _start
+_start:
+  .functype _start () -> ()
+  i32.const 1
+  global.set foo
+  end_function
+
+.globaltype foo, i32
+
+# CHECK: error: mutable global imported but 'mutable-globals' feature not present in inputs: `foo`. Use --no-check-features to suppress.

lld/test/wasm/pie.ll

-; RUN: llc -relocation-model=pic -filetype=obj %s -o %t.o
+; RUN: llc -relocation-model=pic -mattr=+mutable-globals -filetype=obj %s -o %t.o
 ; RUN: wasm-ld --no-gc-sections --allow-undefined -pie -o %t.wasm %t.o
 ; RUN: obj2yaml %t.wasm | FileCheck %s
 
 target triple = "wasm32-unknown-emscripten"
 
 @data = global i32 2, align 4
 @data_external = external global i32
 @indirect_func = local_unnamed_addr global i32 ()* @foo, align 4

lld/test/wasm/shared.ll

-; RUN: llc -relocation-model=pic -filetype=obj %s -o %t.o
+; RUN: llc -relocation-model=pic -mattr=+mutable-globals -filetype=obj %s -o %t.o
 ; RUN: wasm-ld -shared -o %t.wasm %t.o
 ; RUN: obj2yaml %t.wasm | FileCheck %s
 
 target triple = "wasm32-unknown-emscripten"
 
 @data = hidden global i32 2, align 4
 @data_external = external global i32
 @indirect_func = local_unnamed_addr global i32 ()* @foo, align 4

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 (!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())
+            error(Twine("mutable global imported but 'mutable-globals' feature "
+                        "not present in inputs: `") +
+                  toString(*sym) + "`. Use --no-check-features to suppress.");
+          else if (sym->isExported())
+            error(Twine("mutable global exported but 'mutable-globals' feature "
+                        "not present in inputs: `") +
+                  toString(*sym) + "`. Use --no-check-features to suppress.");
+        }
+      }
+    }
+  }
+
   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))