Skip to content

[lld/coff] Fix assert on /start-lib foo.obj /end-lib during eager loads #120292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

nico
Copy link
Contributor

@nico nico commented Dec 17, 2024

If foo.obj is eagerly loaded (due to a prior undef referencing one if its symbols) and has more than one symbol, we used to assert: SymbolTable::addLazyObject() for the first symbol would set lazy to false and load all symbols from the file, but the outer ObjFile::parseLazy() loop would continue to run and call addLazyObject() for the second symbol, which would assert.

Instead, just stop adding lazy symbols if the file got loaded for real while adding a symbol.

(The ELF port has a similar early exit in ObjFile<ELFT>::parseLazy().)

@llvmbot
Copy link
Member

llvmbot commented Dec 17, 2024

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

@llvm/pr-subscribers-platform-windows

Author: Nico Weber (nico)

Changes

If foo.obj is eagerly loaded (due to a prior undef referencing one if its symbols) and has more than one symbol, we used to assert: SymbolTable::addLazyObject() for the first symbol would set lazy to false and load all symbols from the file, but the outer ObjFile::parseLazy() loop would continue to run and call addLazyObject() for the second symbol, which would assert.

Instead, just stop adding lazy symbols if the file got loaded for real while adding a symbol.

(The ELF port has a similar early exit in ObjFile&lt;ELFT&gt;::parseLazy().)


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

2 Files Affected:

  • (modified) lld/COFF/InputFiles.cpp (+6-1)
  • (modified) lld/test/COFF/start-lib.ll (+24-6)
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 6ce075be798e4a..12e3c8db753b48 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -192,6 +192,8 @@ void ObjFile::parseLazy() {
     if (coffSym.isAbsolute() && ignoredSymbolName(name))
       continue;
     symtab.addLazyObject(this, name);
+    if (!lazy)
+      return;
     i += coffSym.getNumberOfAuxSymbols();
   }
 }
@@ -1291,8 +1293,11 @@ void BitcodeFile::parse() {
 
 void BitcodeFile::parseLazy() {
   for (const lto::InputFile::Symbol &sym : obj->symbols())
-    if (!sym.isUndefined())
+    if (!sym.isUndefined()) {
       symtab.addLazyObject(this, sym.getName());
+      if (!lazy)
+        return;
+    }
 }
 
 MachineTypes BitcodeFile::getMachineType() const {
diff --git a/lld/test/COFF/start-lib.ll b/lld/test/COFF/start-lib.ll
index f4f49ed4e4f6e6..c151e3cdcec555 100644
--- a/lld/test/COFF/start-lib.ll
+++ b/lld/test/COFF/start-lib.ll
@@ -6,24 +6,26 @@
 ; RUN: llc -filetype=obj %t.dir/main.ll -o %t.obj
 ; RUN: llc -filetype=obj %t.dir/start-lib1.ll -o %t1.obj
 ; RUN: llc -filetype=obj %t.dir/start-lib2.ll -o %t2.obj
+; RUN: llc -filetype=obj %t.dir/eager.ll -o %t-eager.obj
 ; RUN: opt -thinlto-bc %t.dir/main.ll -o %t.bc
 ; RUN: opt -thinlto-bc %t.dir/start-lib1.ll -o %t1.bc
 ; RUN: opt -thinlto-bc %t.dir/start-lib2.ll -o %t2.bc
+; RUN: opt -thinlto-bc %t.dir/eager.ll -o %t-eager.bc
 ;
 ; RUN: lld-link -out:%t1.exe -entry:main -opt:noref -lldmap:%t1.map \
-; RUN:     %t.obj %t1.obj %t2.obj
+; RUN:     %t.obj %t1.obj %t2.obj %t-eager.obj
 ; RUN: FileCheck --check-prefix=TEST1 %s < %t1.map
 ; RUN: lld-link -out:%t1.exe -entry:main -opt:noref -lldmap:%t1.thinlto.map \
-; RUN:     %t.bc %t1.bc %t2.bc
+; RUN:     %t.bc %t1.bc %t2.bc %t-eager.bc
 ; RUN: FileCheck --check-prefix=TEST1 %s < %t1.thinlto.map
 ; TEST1: foo
 ; TEST1: bar
 ;
 ; RUN: lld-link -out:%t2.exe -entry:main -opt:noref -lldmap:%t2.map \
-; RUN:     %t.obj -start-lib %t1.obj -end-lib %t2.obj
+; RUN:     %t.obj -start-lib %t1.obj %t-eager.obj -end-lib %t2.obj
 ; RUN: FileCheck --check-prefix=TEST2 %s < %t2.map
 ; RUN: lld-link -out:%t2.exe -entry:main -opt:noref -lldmap:%t2.thinlto.map \
-; RUN:     %t.bc -start-lib %t1.bc -end-lib %t2.bc
+; RUN:     %t.bc -start-lib %t1.bc %t-eager.bc -end-lib %t2.bc
 ; RUN: FileCheck --check-prefix=TEST2 %s < %t2.thinlto.map
 ; TEST2:     Address Size Align Out In Symbol
 ; TEST2-NOT:                           {{ }}foo{{$}}
@@ -31,10 +33,10 @@
 ; TEST2-NOT:                           {{ }}foo{{$}}
 ;
 ; RUN: lld-link -out:%t3.exe -entry:main -opt:noref -lldmap:%t3.map \
-; RUN:     %t.obj -start-lib %t1.obj %t2.obj
+; RUN:     %t.obj -start-lib %t1.obj %t2.obj %t-eager.obj
 ; RUN: FileCheck --check-prefix=TEST3 %s < %t3.map
 ; RUN: lld-link -out:%t3.exe -entry:main -opt:noref -lldmap:%t3.thinlto.map \
-; RUN:     %t.bc -start-lib %t1.bc %t2.bc
+; RUN:     %t.bc -start-lib %t1.bc %t2.bc %t-eager.bc
 ; RUN: FileCheck --check-prefix=TEST3 %s < %t3.thinlto.map
 ; TEST3:     Address Size Align Out In Symbol
 ; TEST3-NOT: {{ }}foo{{$}}
@@ -46,7 +48,10 @@
 target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-windows-msvc"
 
+declare void @eager()
+
 define void @main() {
+  %1 = call i32 () @eager()
   ret void
 }
 
@@ -79,3 +84,16 @@ define i32 @bar() {
 
 !llvm.linker.options = !{!0}
 !0 = !{!"/INCLUDE:bar"}
+
+#--- eager.ll
+
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+define void @eager() {
+  ret void
+}
+
+define i32 @ogre() {
+  ret i32 1
+}

If foo.obj is eagerly loaded (due to a prior undef referencing one if
its symbols) and has more than one symbol, we used to assert:
SymbolTable::addLazyObject() for the first symbol would set
`lazy` to false and load all symbols from the file, but the
outer ObjFile::parseLazy() loop would continue to run and call
addLazyObject() for the second symbol, which would assert.

Instead, just stop adding lazy symbols if the file got loaded for
real while adding a symbol.

(The ELF port has a similar early exit in `ObjFile<ELFT>::parseLazy()`.)
@nico nico force-pushed the lld-coff-assert-fix branch from 38daba3 to 31860b0 Compare December 17, 2024 19:52
@nico nico requested a review from mstorsjo December 17, 2024 19:52
nico added a commit to nico/llvm-project that referenced this pull request Dec 17, 2024
Contains tests for the scenarios fixed in lld/COFF in llvm#120292.
They pass without code changes, but I didn't see existing tests
for this.
@nico nico merged commit 2b6713d into llvm:main Dec 19, 2024
8 checks passed
@nico nico deleted the lld-coff-assert-fix branch December 19, 2024 16:22
nico added a commit that referenced this pull request Dec 19, 2024
Contains tests for the scenarios fixed in lld/COFF in #120292. They pass
without code changes, but I didn't see existing tests for this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants