[lld][WebAssembly] Fail if bitcode objects are pulled in after LTO

[sbc100]

Summary

This can happen if lto::LTO::getRuntimeLibcallSymbols doesn't return
an complete/accurate list of libcalls. In this case new bitcode
object can be linked in after LTO.

For example the WebAssembly backend currently calls:
setLibcallName(RTLIB::FPROUND_F32_F16, "__truncsfhf2");

But __truncsfhf2 is not part of getRuntimeLibcallSymbols so if
this symbol is generated during LTO the link will currently fail.

Without this change the linker crashes because the bitcode symbol
makes it all the way to the output phase.

See: https://bugs.llvm.org/show_bug.cgi?id=44353

[sbc100]

• spelling

[sbc100]

I think this problem could also effect other backend.. working on test case to verify if this is true.

[dschuff]

lld/wasm/InputFiles.cpp

546

This string is confusing. I think it's not really necessary; emscripten only had this problem because compiler-rt was incorrectly compiled as bitcode.

[sbc100]

Actually it looks like we can handle this more gracefully, like ELF does:
https://reviews.llvm.org/D50017
https://reviews.llvm.org/D50475

[ruiu]

lld/wasm/InputFiles.cpp

544–546

For a single error call error only once.

[sbc100]

I'm going abandon this change in favor of copying the code that already exists in COFF and ELF for handling this.

[sbc100]

• rebase

[sbc100]

• feedback

[sbc100]

@ruiu, I tested the ELF linking I an believe it has the same problem. With ELF it looks like the effect is that bitcode symbols all end up with address 0 in the final binary. I think its better to fail at link time.. so we should probably port this change, or something like it to ELF.

Sadly its very hard to test this, since it requires knowledge of what the codegen is going to do, and finding a symbol that is not covered in the static libcall list returned by getRuntimeLibcallSymbols

[sbc100]

• revert

[sbc100]

@ruiu I'm curious what you think about this problem, and this approach to solving it.

[merge_guards_bot]

Unit tests: pass. 61985 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[sbc100]

ping ..

[sbc100]

Adding @pcc, perhaps @ruiu is OOO?

This change is pretty uncontroversial, and prevents crash bugs that can occur when LTO introduces new symbols that themselves are stored in bitcode. I'm keen to land it to get feedback on if/when this ever happens in the wild.

[sbc100]

@pcc or @ruiu could I get your approval?

[dschuff]

FWIW this LGTM, especially if this is the same approach used in the other backends.

lld/test/wasm/lto/Inputs/builtins.ll

+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+define half @__truncsfhf2(float) {
+  ret half 0.0
+}

lld/test/wasm/lto/builtins.ll

+; References to builtin symbols can be created for the first time during LTO.
+; If those builtins are implemented themselved in lazy LTO objects there are
+; otherwise not referenced this can result in bitfiles being added to the link
+; *after* LTO, which will not work.
+; Veriftr that we detect this and report a meaningful error.
+
+; RUN: llvm-as %s -o %t.o
+; RUN: llvm-as %p/Inputs/builtins.ll -o %t.truncsfhf2.o
+; RUN: rm -f %t.a
+; RUN: llvm-ar rcs %t.a %t.truncsfhf2.o
+; RUN: not wasm-ld --export-all %t.o %t.a -o %t.wasm 2>&1 | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+@g_float = global float 0.0
+@g_half = global half 0.0
+
+define void @_start() {
+  %val1 = load float, float* @g_float
+  %v0 = fptrunc float %val1 to half
+  store half %v0, half* @g_half
+  ret void
+}
+
+; CHECK: wasm-ld: error: {{.*}}truncsfhf2.o: attempt to add bitcode file after LTO.

lld/wasm/InputFiles.h

public:
   explicit BitcodeFile(MemoryBufferRef m, StringRef archiveName)
       : InputFile(BitcodeKind, m) {
     this->archiveName = archiveName;
   }
   static bool classof(const InputFile *f) { return f->kind() == BitcodeKind; }
 
   void parse();
   std::unique_ptr<llvm::lto::InputFile> obj;
+
+  // Set to true once LTO is complete in order prevent further bitcode objects
+  // being added.
+  static bool doneLTO;
 };
 
 // Will report a fatal() error if the input buffer is not a valid bitcode
 // or wasm object file.
 InputFile *createObjectFile(MemoryBufferRef mb, StringRef archiveName = "");
 
 // Opens a given file.
 llvm::Optional<MemoryBufferRef> readFile(StringRef path);
 
 } // namespace wasm
 
 std::string toString(const wasm::InputFile *file);
 
 } // namespace lld
 
 #endif

lld/wasm/InputFiles.cpp

if (objSym.isUndefined() || excludedByComdat) {
     return symtab->addUndefinedData(name, flags, &f);
   }
 
   if (objSym.isExecutable())
     return symtab->addDefinedFunction(name, flags, &f, nullptr);
   return symtab->addDefinedData(name, flags, &f, nullptr, 0, 0);
 }
 
+bool BitcodeFile::doneLTO = false;
+
 void BitcodeFile::parse() {
+  if (doneLTO) {
+    error(toString(mb.getBufferIdentifier()) +
+          ": attempt to add bitcode file after LTO.");
+    error("This is most often caused by builtin functions into LTO objects.");
+    return;
+  }
+
   obj = check(lto::InputFile::create(MemoryBufferRef(
       mb.getBuffer(), saver.save(archiveName + mb.getBufferIdentifier()))));
   Triple t(obj->getTargetTriple());
   if (t.getArch() != Triple::wasm32) {
     error(toString(mb.getBufferIdentifier()) + ": machine type must be wasm32");
     return;
   }
   std::vector<bool> keptComdats;

lld/wasm/SymbolTable.cpp

 // This function is where all the optimizations of link-time
 // optimization happens. When LTO is in use, some input files are
 // not in native object file format but in the LLVM bitcode format.
 // This function compiles bitcode files into a few big native files
 // using LLVM functions and replaces bitcode symbols with the results.
 // Because all bitcode files that the program consists of are passed
 // to the compiler at once, it can do whole-program optimization.
 void SymbolTable::addCombinedLTOObject() {
+  // Prevent further LTO objects being included
+  BitcodeFile::doneLTO = true;
+
   if (bitcodeFiles.empty())
     return;
 
   // Compile bitcode files and replace bitcode symbols.
   lto.reset(new BitcodeCompiler);
   for (BitcodeFile *f : bitcodeFiles)
     lto->add(*f);