Implement __attribute__((import_module("foo")))

[sunfish]

Summary

This creates an attribute for specifying an explicit import module name for a function declaration. It also renames the default module import name from "env" to "__linker_resolve", which should be less likely to collide with other things, though I'm open to other suggestions.

The expectation is that it's the linker's job to resolve symbols that have the default module name, so that they're either linked to a definition in the same resulting module, or to a definition in a separate module with two-level namespacing. Modules with non-default names should be left alone.

It only applies to functions for now; we can generalize it for global variables later if needed.

[ncw]

Renaming the default module to __linker_resolve seems fine.

However, D42495 currently in progress involves changing the way symbols are read from a Wasm object file. There's now going to be an explicit symbol table, so the linker won't resolve all imports, but only imports which correspond to symbols. I guess we could have WasmObjectWriter skip any imports that aren't in the __linker_resolve module, and not create symbols for them; and the linker could merge together imports from the object files that come from other modules...

Is there any current use-case for allowing the module to be customised?

[pepyakin]

include/llvm/MC/MCSymbolWasm.h

34 ↗

(On Diff #131388)

I guess there should be "__linker_resolve"

[aaron.ballman]

You're missing a lot of test cases for the semantic checks of the attribute in Clang. There should be a Sema test that ensures things like the attribute subject, its argument count and types, etc.

tools/clang/include/clang/Basic/Attr.td

1367 ↗

(On Diff #131388)

No new, undocumented attributes, please.

tools/clang/lib/CodeGen/TargetInfo.cpp

738 ↗

(On Diff #131388)

const auto * -- Also, can D really be null?

739 ↗

(On Diff #131388)

const auto *

tools/clang/lib/Sema/SemaDeclAttr.cpp

5437 ↗

(On Diff #131388)

This should be specified by the Subjects list in Attr.td.

5445 ↗

(On Diff #131388)

Is this function really an alias?

[sunfish]

In D42520#987422, @ncw wrote:

However, D42495 currently in progress involves changing the way symbols are read from a Wasm object file. There's now going to be an explicit symbol table, so the linker won't resolve all imports, but only imports which correspond to symbols. I guess we could have WasmObjectWriter skip any imports that aren't in the __linker_resolve module, and not create symbols for them; and the linker could merge together imports from the object files that come from other modules...

I'm behind on the new symbol table structure. What you propose here makes sense. In fact, in that case we can probably eliminate the "__linker_resolve" special name, and just have a bool in MCSymbolWasm which indicates whether a symbol should be omitted from the symbol table.

Is there any current use-case for allowing the module to be customised?

Linking with ES modules. A tool could generate a C/C++ header file which declares functions provided by the ES module, which could use this attribute to set up the module name.

And actually, thinking about both of these together, I think we should take this attribute a step further and include the fieldname, as in: __attribute__((import("modulename", "fieldname"))) or so, because you may want to use a different fieldname than the C++ declaration name. I'll rework the patch along these lines.

[ncw]

In D42520#988026, @sunfish wrote:

Linking with ES modules. A tool could generate a C/C++ header file which declares functions provided by the ES module, which could use this attribute to set up the module name.

And actually, thinking about both of these together, I think we should take this attribute a step further and include the fieldname, as in: __attribute__((import("modulename", "fieldname"))) or so, because you may want to use a different fieldname than the C++ declaration name. I'll rework the patch along these lines.

That would make sense then. I don't suppose that there's any crossover with C++ modules is there?

[sunfish]

In D42520#988036, @ncw wrote:

I don't suppose that there's any crossover with C++ modules is there?

Some tools may want to have a convention where ES modules are mapped to C++ modules, but that's not the only way things could be set up. I'm thinking of this attribute as a mechanism on top of which a variety of policies could be built.

[sbc100]

In general, this seems like a good idea.

include/llvm/MC/MCSymbolWasm.h

39 ↗

(On Diff #131388)

To keep this change focused, perhaps this rename can be done separately.

This would have several advantages:

1. Separate the addition of this new attribute from the bike-shedding around this default name
2. Allow with change to land without break lld (or requiring any lld change at all)
3. Possible avoid churns since we are looking to landing the symbol table change soon anyway which might eliminate this
4. Keep this change small

If we are going to bikeshed a new name here, I would go for something a little shorter, but I like the __ prefix. Perhaps just __extern?

[sunfish]

• Go back to using "env" for now, so that this is independent of the symbol-table changes and avoids bike-shedding discussions for now.
• Split the clang changes out into a separate patch, which I'll post separately.

I believe this addresses all the comments so far on the LLVM side; I'll address the clang comments on the clang patch.

[sunfish]

@sbc100 I've updated the patch to leave the "env" namespace in place. Do you think this is ready to land now?

[sbc100]

Great!

Might want to update the commit message before submitting.

llvm/trunk/include/llvm/MC/MCSymbolWasm.h

public:
 
   bool isHidden() const { return IsHidden; }
   void setHidden(bool isHidden) { IsHidden = isHidden; }
 
   bool isComdat() const { return IsComdat; }
   void setComdat(bool isComdat) { IsComdat = isComdat; }
 
   const StringRef getModuleName() const { return ModuleName; }
+  void setModuleName(StringRef Name) { ModuleName = Name; }
 
   const SmallVector<wasm::ValType, 1> &getReturns() const {
     assert(ReturnsSet);
     return Returns;
   }
 
   void setReturns(SmallVectorImpl<wasm::ValType> &&Rets) {
     ReturnsSet = true;

llvm/trunk/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h

 #include "llvm/BinaryFormat/Wasm.h"
 #include "llvm/CodeGen/MachineValueType.h"
 #include "llvm/MC/MCStreamer.h"
 
 namespace llvm {
 
 class MCELFStreamer;
 class MCWasmStreamer;
+class MCSymbolWasm;
 
 /// WebAssembly-specific streamer interface, to implement support
 /// WebAssembly-specific assembly directives.
 class WebAssemblyTargetStreamer : public MCTargetStreamer {
 public:
   explicit WebAssemblyTargetStreamer(MCStreamer &S);
 
   /// .param
   virtual void emitParam(MCSymbol *Symbol, ArrayRef<MVT> Types) = 0;
   /// .result
   virtual void emitResult(MCSymbol *Symbol, ArrayRef<MVT> Types) = 0;
   /// .local
   virtual void emitLocal(ArrayRef<MVT> Types) = 0;
   /// .endfunc
   virtual void emitEndFunc() = 0;
   /// .functype
   virtual void emitIndirectFunctionType(MCSymbol *Symbol,
                                         SmallVectorImpl<MVT> &Params,
                                         SmallVectorImpl<MVT> &Results) = 0;
   /// .indidx
   virtual void emitIndIdx(const MCExpr *Value) = 0;
   /// .import_global
   virtual void emitGlobalImport(StringRef name) = 0;
+  /// .import_module
+  virtual void emitImportModule(MCSymbolWasm *Sym, StringRef ModuleName) = 0;
 
 protected:
   void emitValueType(wasm::ValType Type);
 };
 
 /// This part is for ascii assembly output
 class WebAssemblyTargetAsmStreamer final : public WebAssemblyTargetStreamer {
   formatted_raw_ostream &OS;
 
 public:
   WebAssemblyTargetAsmStreamer(MCStreamer &S, formatted_raw_ostream &OS);
 
   void emitParam(MCSymbol *Symbol, ArrayRef<MVT> Types) override;
   void emitResult(MCSymbol *Symbol, ArrayRef<MVT> Types) override;
   void emitLocal(ArrayRef<MVT> Types) override;
   void emitEndFunc() override;
   void emitIndirectFunctionType(MCSymbol *Symbol,
                                 SmallVectorImpl<MVT> &Params,
                                 SmallVectorImpl<MVT> &Results) override;
   void emitIndIdx(const MCExpr *Value) override;
   void emitGlobalImport(StringRef name) override;
+  void emitImportModule(MCSymbolWasm *Sym, StringRef ModuleName) override;
 };
 
 /// This part is for ELF object output
 class WebAssemblyTargetELFStreamer final : public WebAssemblyTargetStreamer {
 public:
   explicit WebAssemblyTargetELFStreamer(MCStreamer &S);
 
   void emitParam(MCSymbol *Symbol, ArrayRef<MVT> Types) override;
   void emitResult(MCSymbol *Symbol, ArrayRef<MVT> Types) override;
   void emitLocal(ArrayRef<MVT> Types) override;
   void emitEndFunc() override;
   void emitIndirectFunctionType(MCSymbol *Symbol,
                                 SmallVectorImpl<MVT> &Params,
                                 SmallVectorImpl<MVT> &Results) override;
   void emitIndIdx(const MCExpr *Value) override;
   void emitGlobalImport(StringRef name) override;
+  void emitImportModule(MCSymbolWasm *Sym, StringRef ModuleName) override;
 };
 
 /// This part is for Wasm object output
 class WebAssemblyTargetWasmStreamer final : public WebAssemblyTargetStreamer {
 public:
   explicit WebAssemblyTargetWasmStreamer(MCStreamer &S);
 
   void emitParam(MCSymbol *Symbol, ArrayRef<MVT> Types) override;
   void emitResult(MCSymbol *Symbol, ArrayRef<MVT> Types) override;
   void emitLocal(ArrayRef<MVT> Types) override;
   void emitEndFunc() override;
   void emitIndirectFunctionType(MCSymbol *Symbol,
                                 SmallVectorImpl<MVT> &Params,
                                 SmallVectorImpl<MVT> &Results) override;
   void emitIndIdx(const MCExpr *Value) override;
   void emitGlobalImport(StringRef name) override;
+  void emitImportModule(MCSymbolWasm *Sym, StringRef ModuleName) override;
 };
 
 } // end namespace llvm
 
 #endif

llvm/trunk/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp

for (auto Ty : Params)
     OS << ", " << WebAssembly::TypeToString(Ty);
   OS << '\n';
 }
 
 void WebAssemblyTargetAsmStreamer::emitGlobalImport(StringRef name) {
   OS << "\t.import_global\t" << name << '\n';
 }
 
+void WebAssemblyTargetAsmStreamer::emitImportModule(MCSymbolWasm *Sym,
+                                                    StringRef ModuleName) {
+  OS << "\t.import_module\t" << Sym->getName() << ", " << ModuleName << '\n';
+}
+
 void WebAssemblyTargetAsmStreamer::emitIndIdx(const MCExpr *Value) {
   OS << "\t.indidx  \t" << *Value << '\n';
 }
 
 void WebAssemblyTargetELFStreamer::emitParam(MCSymbol *Symbol,
                                              ArrayRef<MVT> Types) {
   // Nothing to emit; params are declared as part of the function signature.
 }
void WebAssemblyTargetELFStreamer::emitIndirectFunctionType(
     MCSymbol *Symbol, SmallVectorImpl<MVT> &Params, SmallVectorImpl<MVT> &Results) {
   // Nothing to emit here. TODO: Re-design how linking works and re-evaluate
   // whether it's necessary for .o files to declare indirect function types.
 }
 
 void WebAssemblyTargetELFStreamer::emitGlobalImport(StringRef name) {
 }
 
+void WebAssemblyTargetELFStreamer::emitImportModule(MCSymbolWasm *Sym,
+                                                    StringRef ModuleName) {
+  llvm_unreachable(".import_module encoding not yet implemented");
+}
+
 void WebAssemblyTargetWasmStreamer::emitParam(MCSymbol *Symbol,
                                               ArrayRef<MVT> Types) {
   SmallVector<wasm::ValType, 4> Params;
   for (MVT Ty : Types)
     Params.push_back(WebAssembly::toValType(Ty));
 
   cast<MCSymbolWasm>(Symbol)->setParams(std::move(Params));
 }
void WebAssemblyTargetWasmStreamer::emitIndirectFunctionType(
   WasmSym->setParams(std::move(ValParams));
   WasmSym->setReturns(std::move(ValResults));
   WasmSym->setIsFunction(true);
 }
 
 void WebAssemblyTargetWasmStreamer::emitGlobalImport(StringRef name) {
   llvm_unreachable(".global_import is not needed for direct wasm output");
 }
+
+void WebAssemblyTargetWasmStreamer::emitImportModule(MCSymbolWasm *Sym,
+                                                     StringRef ModuleName) {
+  Sym->setModuleName(ModuleName);
+}

llvm/trunk/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

 
 void WebAssemblyAsmPrinter::EmitEndOfAsmFile(Module &M) {
   for (const auto &F : M) {
     // Emit function type info for all undefined functions
     if (F.isDeclarationForLinker() && !F.isIntrinsic()) {
       SmallVector<MVT, 4> Results;
       SmallVector<MVT, 4> Params;
       ComputeSignatureVTs(F, TM, Params, Results);
-      getTargetStreamer()->emitIndirectFunctionType(getSymbol(&F), Params,
-                                                    Results);
+      MCSymbolWasm *Sym = cast<MCSymbolWasm>(getSymbol(&F));
+      getTargetStreamer()->emitIndirectFunctionType(Sym, Params, Results);
+
+      if (F.hasFnAttribute("wasm-import-module")) {
+        StringRef Name = F.getFnAttribute("wasm-import-module")
+                             .getValueAsString();
+        getTargetStreamer()->emitImportModule(Sym, Name);
+      }
     }
   }
   for (const auto &G : M.globals()) {
     if (!G.hasInitializer() && G.hasExternalLinkage()) {
       if (G.getValueType()->isSized()) {
         uint16_t Size = M.getDataLayout().getTypeAllocSize(G.getValueType());
         if (TM.getTargetTriple().isOSBinFormatELF())
           getTargetStreamer()->emitGlobalImport(G.getGlobalIdentifier());

llvm/trunk/test/CodeGen/WebAssembly/import-module.ll

+; RUN: llc < %s -asm-verbose=false | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown-wasm"
+
+define void @test() {
+  call void @foo()
+  call void @plain()
+  ret void
+}
+
+declare void @foo() #0
+declare void @plain()
+
+attributes #0 = { "wasm-import-module"="bar" }
+
+; CHECK-NOT: .import_module plain
+;     CHECK: .import_module foo, bar
+; CHECK-NOT: .import_module plain

llvm/trunk/test/MC/WebAssembly/external-func-address.ll

 ; RUN: llc -filetype=obj %s -o - | obj2yaml | FileCheck %s
 
 target triple = "wasm32-unknown-unknown-wasm"
 
 ; Verify that addresses of external functions generate correctly typed
 ; imports and relocations or type R_TABLE_INDEX_I32.
 
+declare void @f0(i32) #0
+@ptr_to_f0 = hidden global void (i32)* @f0, align 4
+
+attributes #0 = { "wasm-import-module"="somewhere" }
+
 declare void @f1(i32) #1
 @ptr_to_f1 = hidden global void (i32)* @f1, align 4
 
 ; CHECK:      --- !WASM
 ; CHECK-NEXT: FileHeader:      
 ; CHECK-NEXT:   Version:         0x00000001
 ; CHECK-NEXT: Sections:        
 ; CHECK-NEXT:   - Type:            TYPE
 ; CHECK-NEXT:     Signatures:      
 ; CHECK-NEXT:       - Index:           0
 ; CHECK-NEXT:         ReturnType:      NORESULT
 ; CHECK-NEXT:         ParamTypes:      
 ; CHECK-NEXT:           - I32
 ; CHECK:        - Type:            IMPORT
 ; CHECK-NEXT:     Imports:
 ; CHECK:            - Module:          env
 ; CHECK-NEXT:         Field:           __linear_memory
 ; CHECK:            - Module:          env
 ; CHECK-NEXT:         Field:           __indirect_function_table
+; CHECK:            - Module:          somewhere
+; CHECK-NEXT:         Field:           f0
 ; CHECK:            - Module:          env
 ; CHECK-NEXT:         Field:           f1
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         SigIndex:        0
 ; CHECK:        - Type:            ELEM
 ; CHECK-NEXT:     Segments:
 ; CHECK-NEXT:       - Offset:
 ; CHECK-NEXT:           Opcode:          I32_CONST
 ; CHECK-NEXT:           Value:           1
-; CHECK-NEXT:         Functions:       [ 0 ]
+; CHECK-NEXT:         Functions:       [ 0, 1 ]
 ; CHECK:        - Type:            DATA
 ; CHECK-NEXT:     Relocations:
 ; CHECK-NEXT:       - Type:            R_WEBASSEMBLY_TABLE_INDEX_I32
 ; CHECK-NEXT:         Index:           0
 ; CHECK-NEXT:         Offset:          0x00000006