This is an archive of the discontinued LLVM Phabricator instance.

COFF: Handle references from LTO object to lazy symbols correctly.
ClosedPublic

Authored by pcc on Jun 8 2015, 6:00 PM.

Details

Summary

The code generator may create references to runtime library symbols such as
__chkstk which were not visible via LTOModule. Handle these cases by loading
the object file from the library, but abort if we end up having loaded any
bitcode objects.

Because loading the object file may have introduced new undefined references,
call reportRemainingUndefines again to detect and report them.

Diff Detail

Event Timeline

pcc updated this revision to Diff 27354.Jun 8 2015, 6:00 PM
pcc retitled this revision from to COFF: Handle references from LTO object to lazy symbols correctly..
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added a reviewer: ruiu.
pcc added a subscriber: Unknown Object (MLST).
ruiu edited edge metadata.Jun 8 2015, 6:27 PM

__chkstk is a special symbol. Does the code generator introduce arbitrary
symbols? If not (if it could introduce only a limited set of symbols), we
may want to resolve them unconditionally as if they were specified with
/include.
2015/06/08 午後6:00 "Peter Collingbourne" <peter@pcc.me.uk>:

Hi ruiu,

The code generator may create references to runtime library symbols such as
__chkstk which were not visible via LTOModule. Handle these cases by
loading
the object file from the library, but abort if we end up having loaded any
bitcode objects.

Because loading the object file may have introduced new undefined
references,
call reportRemainingUndefines again to detect and report them.

http://reviews.llvm.org/D10332

Files:

COFF/SymbolTable.cpp
test/COFF/Inputs/lto-chkstk-chkstk.s
test/COFF/Inputs/lto-chkstk-foo.s
test/COFF/lto-chkstk.ll

Index: COFF/SymbolTable.cpp

  • COFF/SymbolTable.cpp

+++ COFF/SymbolTable.cpp
@@ -307,8 +307,27 @@

    return make_error_code(LLDError::BrokenFile);
  }
}

+
+ We may see new references to runtime library symbols such as
__chkstk
+
here. These symbols must be wholly defined in non-bitcode files.
+ if (auto *B = dyn_cast<Lazy>(Sym->Body)) {
+ unsigned NumBitcodeFiles = BitcodeFiles.size();
+ auto EC = addMemberFile(B);
+ if (EC)
+ return EC;
+ if (BitcodeFiles.size() != NumBitcodeFiles) {
+ llvm::errs()
+ << "LTO: late loaded symbol created new bitcode reference: "
<< Name
+ << "\n";
+ return make_error_code(LLDError::BrokenFile);
+ }
+ }

}

+ // New runtime library symbol references may have created undefined
references.
+ if (reportRemainingUndefines())
+ return make_error_code(LLDError::BrokenFile);
+

return std::error_code();

}

Index: test/COFF/Inputs/lto-chkstk-chkstk.s

  • /dev/null

+++ test/COFF/Inputs/lto-chkstk-chkstk.s
@@ -0,0 +1,3 @@
+.globl chkstk
+
chkstk:
+ret

Index: test/COFF/Inputs/lto-chkstk-foo.s

  • /dev/null

+++ test/COFF/Inputs/lto-chkstk-foo.s
@@ -0,0 +1,3 @@
+.globl foo
+foo:
+ret

Index: test/COFF/lto-chkstk.ll

  • /dev/null

+++ test/COFF/lto-chkstk.ll
@@ -0,0 +1,17 @@
+; RUN: llvm-as -o %t.obj %s
+; RUN: llvm-mc -triple=x86_64-pc-windows-msvc -filetype=obj -o
%T/lto-chkstk-foo.obj %S/Inputs/lto-chkstk-foo.s
+; RUN: llvm-mc -triple=x86_64-pc-windows-msvc -filetype=obj -o
%T/lto-chkstk-chkstk.obj %S/Inputs/lto-chkstk-chkstk.s
+; RUN: llvm-ar cru %t.lib %T/lto-chkstk-chkstk.obj
+; RUN: lld -flavor link2 /out:%t.exe /entry:main /subsystem:console
%t.obj %T/lto-chkstk-foo.obj %t.lib
+
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+define void @main() {
+entry:
+ %array4096 = alloca [4096 x i8]
+ call void @foo([4096 x i8]* %array4096)
+ ret void
+}
+
+declare void @foo([4096 x i8]*)

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/
ruiu accepted this revision.Jun 8 2015, 8:24 PM
ruiu edited edge metadata.

LGTM

Thank you for the explanation. My concern was that before this patch, adding a combined compiled bitcode file is really a final operation in the linker, but after this patch, a new call of addMember() can read arbitrary number of new files. That would increase complexity. That being said, this change seems reasonable. It's probably better than maintaining a list of symbols to be linked unconditionally for LTO.

COFF/SymbolTable.cpp
314

size_t

317
if (auto EC = ...)
  return EC
329

Can we call reportRemainingUndefines after Symtab.addCombinedLTOObject in the Driver to eliminate this call from here?

This revision is now accepted and ready to land.Jun 8 2015, 8:24 PM
pcc added inline comments.Jun 8 2015, 8:34 PM
COFF/SymbolTable.cpp
329

My reasoning was that because LTO is generally an expensive operation, we'll want to report undefined symbols as soon as possible, rather than wasting time building the LTO object file.

This revision was automatically updated to reflect the committed changes.