[LLD][COFF] Avoid forcing lazy symbols in loadMinGWSymbols during symbol table enumeration

[cjacek]

Forcing lazy symbols at this point may introduce new entries into the symbol table.

[llvmbot]

@llvm/pr-subscribers-platform-windows
@llvm/pr-subscribers-lld-coff

@llvm/pr-subscribers-lld

Author: Jacek Caban (cjacek)

Changes

Forcing lazy symbols at this point may introduce new entries into the symbol table.

---

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

2 Files Affected:

• (modified) lld/COFF/SymbolTable.cpp (+9)
• (added) lld/test/COFF/stdcall-alias.s (+24)

diff --git a/lld/COFF/SymbolTable.cpp b/lld/COFF/SymbolTable.cpp
index d6f771284aa83..979acd2ef5975 100644
--- a/lld/COFF/SymbolTable.cpp
+++ b/lld/COFF/SymbolTable.cpp
@@ -245,6 +245,7 @@ void SymbolTable::reportUndefinedSymbol(const UndefinedDiag &undefDiag) {
 }
 
 void SymbolTable::loadMinGWSymbols() {
+  std::vector<Symbol *> undefs;
   for (auto &i : symMap) {
     Symbol *sym = i.second;
     auto *undef = dyn_cast<Undefined>(sym);
@@ -252,7 +253,15 @@ void SymbolTable::loadMinGWSymbols() {
       continue;
     if (undef->getWeakAlias())
       continue;
+    undefs.push_back(sym);
+  }
 
+  for (auto sym : undefs) {
+    auto *undef = dyn_cast<Undefined>(sym);
+    if (!undef)
+      continue;
+    if (undef->getWeakAlias())
+      continue;
     StringRef name = undef->getName();
 
     if (machine == I386 && ctx.config.stdcallFixup) {
diff --git a/lld/test/COFF/stdcall-alias.s b/lld/test/COFF/stdcall-alias.s
new file mode 100644
index 0000000000000..546aace9f1dfa
--- /dev/null
+++ b/lld/test/COFF/stdcall-alias.s
@@ -0,0 +1,24 @@
+// REQUIRES: x86
+// RUN: split-file %s %t.dir && cd %t.dir
+
+// RUN: llvm-mc -filetype=obj -triple=i686-windows test.s -o test.obj
+// RUN: llvm-mc -filetype=obj -triple=i686-windows lib.s -o lib.obj
+// RUN: lld-link -dll -noentry -out:out.dll test.obj -start-lib lib.obj -end-lib -lldmingw
+
+#--- test.s
+     .section .test,"dr"
+     .rva _func@4
+
+#--- lib.s
+     .globl _func
+_func:
+     ret
+
+     // These symbols don't have lazy entries in the symbol table initially,
+     // but will be added during resolution from _func@4 to _func. Make sure this
+     // scenario is handled properly.
+     .weak_anti_dep _func@5
+     .set _func@5,_func
+
+     .weak_anti_dep _func@3
+     .set _func@3,_func

[cjacek]

This issue can also be reproduced with ARM64EC DLL imports. DLLFile::parse(), even with #141587, does not include all the symbols that ImportFile::parse() does, it omits __auximpcopy_ and __impchk_. While we could consider defining them in DLLFile::parse() as well, that's not how real import libraries behave; they also skip these symbols in the archive map, so they are only defined if the import file is pulled in by other means.

Since the problem is reproducible with the attached test case too, I went ahead and fixed it in SymbolTable::loadMinGWSymbols instead.

[mstorsjo]

LGTM

[mstorsjo]

So the problem we're fixing here is that we avoid mutating symMap while iterating over it?