[WebAssembly] When loading libraries look for companion `.imports` file

[sbc100]

Summary

This allows libraries to supply a list of symbols which are
allowed to be undefined at link time (i.e. result in imports).

This method replaces the existing mechanism (-allow-undefined-file)
used by the clang driver to allow undefined symbols in libc.

For more on the motivation for this see:
https://github.com/WebAssembly/tool-conventions/issues/35

In the long run we hope to remove this features and instead
include this information in the object format itself.

[sbc100]

• Support all library usage

[ruiu]

wasm/Driver.cpp

183–194

This looks suspicious --

1. It loads an entire .import file for an .a file, even if we won't end up linking any object file from that archive file. That doesn't make much sense to me. Shouldn't this be per-file?

2. IMO a side-by-side magic text file is in general not a good idea. We don't have such notion in Unix linker, and we get used to copy .a and .o files to other directories expecting that they would just work fine. This seems error-prone.

[sbc100]

wasm/Driver.cpp

183–194

I agree its not ideal. But it is incrementally better than that current solution which requires the user (normally the clang driver) to pass extra linker args. The long term solution probably involved marking the symbold with attribute(something) in the source and marking those symbols in the object format. So I'd like to land this incremental solution now. We can leave the bug open to create a more robust solution.

This change will also allow us to remove the -allow-undefined-file option, which will be nice cleanup.

[sbc100]

• fix test

[ruiu]

wasm/Driver.cpp

164

nit: "read" is more common than "process" for reading a file in lld.

183–194

If that's the case, I'm fine with this, but could you describe it as the comment for this code? It's somewhat surprising feature, so it needs some explanation to keep it readable.

193

I'd return here instead of } else {.

[sbc100]

• feedback

[ruiu]

How about adding a comment about the .import file?

[sbc100]

• add comment

[ruiu]

LGTM

wasm/Driver.cpp

165

typo: are can

167

that list -> that lists
syscall function -> syscall functions

[ncw]

I've been having further thoughts about this libc.imports business.

What I'm doing in my "minscripten" minimal Wasm toolchain turns the problem the other way up: it's not the business of a library file like libc.a to declare what symbols it "imports". Instead, at the point when you link, the linker should have all the JavaScript files and so on all available - so as a pre-link step, Emscripten should be generating the --allow-undefined-symbols file based on the JavaScript symbols. Emscripten definitely knows what JS symbols are available at the point when it invokes LLD via Clang, so it should be telling LLD which symbols are allowed to be imported.

[sbc100]

In D41922#990507, @ncw wrote:

I've been having further thoughts about this libc.imports business.

What I'm doing in my "minscripten" minimal Wasm toolchain turns the problem the other way up: it's not the business of a library file like libc.a to declare what symbols it "imports". Instead, at the point when you link, the linker should have all the JavaScript files and so on all available - so as a pre-link step, Emscripten should be generating the --allow-undefined-symbols file based on the JavaScript symbols. Emscripten definitely knows what JS symbols are available at the point when it invokes LLD via Clang, so it should be telling LLD which symbols are allowed to be imported.

I'm not sure about that. Isn't it libc that knows exactly what symbols it expects to come from the embeder? Sure with emscripten we could imagine a complex compiler driver that is aware of all the JS functions and construct a long list of possible imports to be passed to linker, but I don't think that a very nice approach and requires complexity in the driver that it would nice to avoid. I like Dan's approach to being able to mark such function in the source language: https://reviews.llvm.org/D42520.

In any case it seems like we can support both methods: import defined by the libraries you link as well as explicitly allowed imports via the linker command line.

On this topic I was considering removing --allow-undefined-file since we are no longer using it.. and perhaps renaming --allow-undefined to --import (to match the existing --export). Are you using these flags currently in your toolchain?

[ncw]

In D41922#990851, @sbc100 wrote:

I'm not sure about that. Isn't it libc that knows exactly what symbols it expects to come from the embeder? Sure with emscripten we could imagine a complex compiler driver that is aware of all the JS functions and construct a long list of possible imports to be passed to linker, but I don't think that a very nice approach and requires complexity in the driver that it would nice to avoid. I like Dan's approach to being able to mark such function in the source language: https://reviews.llvm.org/D42520.

Not from my point of view! That would be an "inversion of control" in the Dependency Injection jargon. We often write software that has an external dependency on a higher layer (for example Musl does this when it invokes the "kernel"/embedder for syscalls, or a library that requires the developer to define/resolve some symbols that the library doesn't provide). The rule is: if you implement it, you get to say where it comes from. So anything that Musl doesn't provide, Musl shouldn't care where it comes from: whoever provides the syscalls has business of getting them through to Musl. Since Musl doesn't provide the syscalls, it feels wrong for me have libc.imports file shipped along with Musl, since that imports file is effectively a file whose whole purpose is to declare the implementation language (JS vs C) for certain routines. Only the person who provides those routines should care which language they're written in (ie which "side" of the JS-Wasm interface provides them).

You mentioned a "complex" linker driver that is "aware of all the JS functions". But what I'm envisaging is fairly simple: Emscripten (and my "minscripten" toolchain) currently construct a JS file that loads the Wasm module and builds some JS shims around it. Those tools are fully aware of which linkable symbols are available on the JS side - it's not a hardcoded list, it's in fact part of the working set of the Emscripten toolchain; they've got a variable already in their Python code that knows about the those symbols, because it's an unavoidable part of building the JS module. There's no practical reason why LLD couldn't be told an exact list of which functions are available on the JS side.

And indeed, the nice thing about that is it's fully automatic - who wants to build a .imports file manually? If you write some functions in C, and some functions in JS, you want Emscripten to link them together for you, rather than having to manually write a second set of declarations mirroring your JS files for the benefit of LLD. Anything that can be derived automatically from the developer's code, should be. Hence the build toolchain ought to be passing the list of JS-defined symbols down to LLD, since those are precisely the symbols that it's OK for LLD to be importing.

In any case it seems like we can support both methods: import defined by the libraries you link as well as explicitly allowed imports via the linker command line.

On this topic I was considering removing --allow-undefined-file since we are no longer using it.. and perhaps renaming --allow-undefined to --import (to match the existing --export). Are you using these flags currently in your toolchain?

I am using --allow-undefined-file, and I think that it is useful. I'm not using --allow-undefined or --export, but I suspect they must be useful to someone.

[sbc100]

In D41922#991056, @ncw wrote:

In D41922#990851, @sbc100 wrote:

I'm not sure about that. Isn't it libc that knows exactly what symbols it expects to come from the embeder? Sure with emscripten we could imagine a complex compiler driver that is aware of all the JS functions and construct a long list of possible imports to be passed to linker, but I don't think that a very nice approach and requires complexity in the driver that it would nice to avoid. I like Dan's approach to being able to mark such function in the source language: https://reviews.llvm.org/D42520.

Not from my point of view! That would be an "inversion of control" in the Dependency Injection jargon. We often write software that has an external dependency on a higher layer (for example Musl does this when it invokes the "kernel"/embedder for syscalls, or a library that requires the developer to define/resolve some symbols that the library doesn't provide). The rule is: if you implement it, you get to say where it comes from. So anything that Musl doesn't provide, Musl shouldn't care where it comes from: whoever provides the syscalls has business of getting them through to Musl. Since Musl doesn't provide the syscalls, it feels wrong for me have libc.imports file shipped along with Musl, since that imports file is effectively a file whose whole purpose is to declare the implementation language (JS vs C) for certain routines. Only the person who provides those routines should care which language they're written in (ie which "side" of the JS-Wasm interface provides them).

You mentioned a "complex" linker driver that is "aware of all the JS functions". But what I'm envisaging is fairly simple: Emscripten (and my "minscripten" toolchain) currently construct a JS file that loads the Wasm module and builds some JS shims around it. Those tools are fully aware of which linkable symbols are available on the JS side - it's not a hardcoded list, it's in fact part of the working set of the Emscripten toolchain; they've got a variable already in their Python code that knows about the those symbols, because it's an unavoidable part of building the JS module. There's no practical reason why LLD couldn't be told an exact list of which functions are available on the JS side.

And indeed, the nice thing about that is it's fully automatic - who wants to build a .imports file manually? If you write some functions in C, and some functions in JS, you want Emscripten to link them together for you, rather than having to manually write a second set of declarations mirroring your JS files for the benefit of LLD. Anything that can be derived automatically from the developer's code, should be. Hence the build toolchain ought to be passing the list of JS-defined symbols down to LLD, since those are precisely the symbols that it's OK for LLD to be importing.

In any case it seems like we can support both methods: import defined by the libraries you link as well as explicitly allowed imports via the linker command line.

On this topic I was considering removing --allow-undefined-file since we are no longer using it.. and perhaps renaming --allow-undefined to --import (to match the existing --export). Are you using these flags currently in your toolchain?

I am using --allow-undefined-file, and I think that it is useful. I'm not using --allow-undefined or --export, but I suspect they must be useful to someone.

I agree that providing the imports file is a hack and needs to go. That is way we want to allow for source level attributes. Once that lands we can remove support for the imports file. If you prefer the idea of the linker driver controlling this stuff we don't need to remove the support for --allow-undefined/-file. I don't think there is a reason these two approaches can't both exist (Unless you are opposed to Dan added the new source attribute?). I think the main thing is that we agree this imports hack should be removed.

[sbc100]

From my POV, I've very keen on the vanilla out-of-the-box clang "just working", and not have to rely on a higher level or external toolchain drivers. I would love just to be able to do "wasm-clang hello.c -o hello.wasm" and have it produce something usable.

[ncw]

In D41922#991090, @sbc100 wrote:

From my POV, I've very keen on the vanilla out-of-the-box clang "just working", and not have to rely on a higher level or external toolchain drivers. I would love just to be able to do "wasm-clang hello.c -o hello.wasm" and have it produce something usable.

Me too! I don't think that the Wasm output should need any post-processing at all. The only tool I want really is to act as a JavaScript "linker" to build the JavaScript module, that calls the Wasm module, and exposes a higher-level API on top of the C-level provided by the Wasm exports. For the moment, that does need an extra tool to build.

test/wasm/archive.ll

 ; RUN: llc -filetype=obj -mtriple=wasm32-unknown-unknown-wasm %s -o %t.o
 ; RUN: llc -filetype=obj -mtriple=wasm32-unknown-unknown-wasm %S/Inputs/archive1.ll -o %t.a1.o
 ; RUN: llc -filetype=obj -mtriple=wasm32-unknown-unknown-wasm %S/Inputs/archive2.ll -o %t.a2.o
 ; RUN: llc -filetype=obj -mtriple=wasm32-unknown-unknown-wasm %S/Inputs/hello.ll -o %t.a3.o
 ; RUN: llvm-ar rcs %t.a %t.a1.o %t.a2.o %t.a3.o
+; RUN: rm -f %t.imports
+; RUN: not lld -flavor wasm %t.a %t.o -o %t.wasm 2>&1 | FileCheck -check-prefix=CHECK-UNDEFINED %s
+
+; CHECK-UNDEFINED: undefined symbol: missing_func
+
+; RUN: echo 'missing_func' > %t.imports
 ; RUN: lld -flavor wasm %t.a %t.o -o %t.wasm
+
 ; RUN: llvm-nm -a %t.wasm | FileCheck %s
 
 declare i32 @foo() local_unnamed_addr #1
+declare i32 @missing_func() local_unnamed_addr #1
 
 define i32 @_start() local_unnamed_addr #0 {
 entry:
-  %call = tail call i32 @foo() #2
-  ret i32 %call
+  %call1 = call i32 @foo() #2
+  %call2 = call i32 @missing_func() #2
+  ret i32 %call2
 }
 
 ; Verify that multually dependant object files in an archive is handled
 ; correctly.
 
-; CHECK:      00000002 T _start
-; CHECK-NEXT: 00000002 T _start
-; CHECK-NEXT: 00000000 T bar
-; CHECK-NEXT: 00000000 T bar
-; CHECK-NEXT: 00000001 T foo
-; CHECK-NEXT: 00000001 T foo
+; CHECK:      00000003 T _start
+; CHECK-NEXT: 00000003 T _start
+; CHECK-NEXT: 00000001 T bar
+; CHECK-NEXT: 00000001 T bar
+; CHECK-NEXT: 00000002 T foo
+; CHECK-NEXT: 00000002 T foo
 
 ; Verify that symbols from unused objects don't appear in the symbol table
 ; CHECK-NOT: hello
 
 ; Specifying the same archive twice is allowed.
 ; RUN: lld -flavor wasm %t.a %t.a %t.o -o %t.wasm

wasm/Driver.cpp

opt::InputArgList WasmOptTable::parse(ArrayRef<const char *> Argv) {
   opt::InputArgList Args = this->ParseArgs(Vec, MissingIndex, MissingCount);
 
   handleColorDiagnostics(Args);
   for (auto *Arg : Args.filtered(OPT_UNKNOWN))
     error("unknown argument: " + Arg->getSpelling());
   return Args;
 }
 
+// Currently we allow a ".imports" to live alongside a library. This can
+// be used to specify a list of symbols which can be undefined at link
+// time (imported from the environment.  For example libc.a include an
+// import file that lists the syscall functions it relies on at runtime.
+// In the long run this information would be better stored as a symbol
+// attribute/flag in the object file itself.
+// See: https://github.com/WebAssembly/tool-conventions/issues/35
+static void readImportFile(StringRef Filename) {
+  if (Optional<MemoryBufferRef> Buf = readFile(Filename))
+    for (StringRef Sym : args::getLines(*Buf))
+      Config->AllowUndefinedSymbols.insert(Sym);
+}
+
 void LinkerDriver::addFile(StringRef Path) {
   Optional<MemoryBufferRef> Buffer = readFile(Path);
   if (!Buffer.hasValue())
     return;
   MemoryBufferRef MBRef = *Buffer;
 
-  if (identify_magic(MBRef.getBuffer()) == file_magic::archive)
+  if (identify_magic(MBRef.getBuffer()) == file_magic::archive) {
+    SmallString<128> ImportFile = Path;
+    path::replace_extension(ImportFile, ".imports");
+    if (fs::exists(ImportFile))
+      readImportFile(ImportFile.str());
+
     Files.push_back(make<ArchiveFile>(MBRef));
-  else
-    Files.push_back(make<ObjFile>(MBRef));
+    return;
+  }
+
 }
 
 // Add a given library by searching it from input search paths.
 void LinkerDriver::addLibrary(StringRef Name) {
   for (StringRef Dir : Config->SearchPaths) {
     if (Optional<std::string> S = findFile(Dir, "lib" + Name + ".a")) {
       addFile(*S);
       return;
     }
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
 
   Config->InitialMemory = args::getInteger(Args, OPT_initial_memory, 0);
   Config->GlobalBase = args::getInteger(Args, OPT_global_base, 1024);
   Config->MaxMemory = args::getInteger(Args, OPT_max_memory, 0);
   Config->ZStackSize =
       args::getZOptionValue(Args, OPT_z, "stack-size", WasmPageSize);
 
   if (auto *Arg = Args.getLastArg(OPT_allow_undefined_file))
-    if (Optional<MemoryBufferRef> Buf = readFile(Arg->getValue()))
-      for (StringRef Sym : args::getLines(*Buf))
-        Config->AllowUndefinedSymbols.insert(Sym);
+    readImportFile(Arg->getValue());
 
   if (Config->OutputFile.empty())
     error("no output file specified");
 
   if (!Args.hasArg(OPT_INPUT))
     error("no input files");
 
   if (Config->Relocatable && !Config->Entry.empty())