[lld][WebAssembly] Fix use of undefined funcs under --warn-unresolved-symbols

[sbc100]

When undefined functions exist in the final link we need to create
stub functions (otherwise direct calls to those functions could
not be generated). We were creating those stub when
--unresolved-symbols=ignore-all was passed but overlooked the fact
that --warn-unresolved-symbols essentially has the same effect (i.e.
undefined function can exist in the final link).

Fixes: #53987

[llvmbot]

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-wasm

Author: Sam Clegg (sbc100)

Changes

Fixes: #53987

---

Full diff: https://github.com/llvm/llvm-project/pull/78643.diff

2 Files Affected:

• (modified) lld/test/wasm/unresolved-symbols.s (+11-5)
• (modified) lld/wasm/Relocations.cpp (+12-11)

diff --git a/lld/test/wasm/unresolved-symbols.s b/lld/test/wasm/unresolved-symbols.s
index 5de54d76c6de81..4f132751c0502a 100644
--- a/lld/test/wasm/unresolved-symbols.s
+++ b/lld/test/wasm/unresolved-symbols.s
@@ -18,10 +18,15 @@
 # RUN:   FileCheck -check-prefix=ERR1 %s
 
 ## Ignore all should not produce error and should not produce
-# any imports.  It should create a stub function in the place of the missing
-# function symbol.
+## any imports.  It should create a stub function in the place of the missing
+## function symbol.
 # RUN: wasm-ld %t1.o -o %t2.wasm --unresolved-symbols=ignore-all
 # RUN: obj2yaml %t2.wasm | FileCheck -check-prefix=IGNORE %s
+
+## --warn-unresolved-symbols should behave the same
+# RUN: wasm-ld %t1.o -o %t2.wasm --warn-unresolved-symbols
+# RUN: obj2yaml %t2.wasm | FileCheck -check-prefix=IGNORE %s
+
 # IGNORE-NOT: - Type:            IMPORT
 # IGNORE-NOT: - Type:            ELEM
 #
@@ -53,9 +58,10 @@
 # IGNORE-NEXT:        Name:            get_func_addr
 
 ## --import-undefined should handle unresolved funtions symbols
-# by importing them but still report errors/warning for missing data symbols.
-# `--allow-undefined` should behave like `--import-undefined` +
-# `--unresolve-symbols=ignore`
+## by importing them but still report errors/warning for missing data symbols.
+## `--allow-undefined` should behave like `--import-undefined` +
+## `--unresolve-symbols=ignore`
+# RUN: wasm-ld %t1.o -o %t3.wasm --import-undefined --unresolved-symbols=ignore-all
 # RUN: wasm-ld %t1.o -o %t3.wasm --import-undefined --unresolved-symbols=ignore-all
 # RUN: obj2yaml %t3.wasm | FileCheck -check-prefix=IMPORT %s
 #      IMPORT:  - Type:            IMPORT
diff --git a/lld/wasm/Relocations.cpp b/lld/wasm/Relocations.cpp
index fcffacbc77af01..0444ccd69955e0 100644
--- a/lld/wasm/Relocations.cpp
+++ b/lld/wasm/Relocations.cpp
@@ -54,21 +54,22 @@ static void reportUndefined(Symbol *sym) {
     case UnresolvedPolicy::Ignore:
       LLVM_DEBUG(dbgs() << "ignoring undefined symbol: " + toString(*sym) +
                                "\n");
-      if (!config->importUndefined) {
-        if (auto *f = dyn_cast<UndefinedFunction>(sym)) {
-          if (!f->stubFunction) {
-            f->stubFunction = symtab->createUndefinedStub(*f->getSignature());
-            f->stubFunction->markLive();
-            // Mark the function itself as a stub which prevents it from being
-            // assigned a table entry.
-            f->isStub = true;
-          }
-        }
-      }
       break;
     case UnresolvedPolicy::ImportDynamic:
       break;
     }
+
+    if (auto *f = dyn_cast<UndefinedFunction>(sym)) {
+      if (config->unresolvedSymbols != UnresolvedPolicy::ImportDynamic && !config->importUndefined) {
+        if (!f->stubFunction) {
+          f->stubFunction = symtab->createUndefinedStub(*f->getSignature());
+          f->stubFunction->markLive();
+          // Mark the function itself as a stub which prevents it from being
+          // assigned a table entry.
+          f->isStub = true;
+        }
+      }
+    }
   }
 }
 

[dschuff]

Can you expand the commit message to say what the problem actually was, or what the change does?

[sbc100]

Done

[felipepiovezan]

This windows test started failing: https://lab.llvm.org/buildbot/#/builders/42/builds/13006
Might be related?

[sbc100]

That failure looks like its is in the elf linker, so I think it cannot be related to this change, unless I'm missing something.

[felipepiovezan]

Yeah that's fair, the only reason I brought it up is because it is a crash in LLD, and the other change that got included in that build was a debug info change...