This is an archive of the discontinued LLVM Phabricator instance.

Differential D32317

[coff] for /msvclto, pass archive members with prevailing symbols first
ClosedPublic

Authored by inglorion on Apr 20 2017, 3:17 PM.

Details

Reviewers
ruiu
pcc
rnk
Commits
rG4110816253a8: [coff] for /msvclto, pass archive members with prevailing symbols first
rLLD301045: [coff] for /msvclto, pass archive members with prevailing symbols first
rL301045: [coff] for /msvclto, pass archive members with prevailing symbols first
Summary

When using /msvclto, lld and MSVC's linker both do their own symbol resolution. This can cause them to select different archive members, which can result in undefined references. This change avoids that situation by extracting archive members that are selected by lld and passing those to link.exe before any archives, so that MSVC's uses those objects for symbol resolution instead of different archive members.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Apr 20 2017, 3:17 PM
Herald added a subscriber: mehdi_amini. · View Herald TranscriptApr 20 2017, 3:17 PM
pcc added inline comments.Apr 20 2017, 3:52 PM
COFF/Driver.cpp
156 ↗(On Diff #96038)

Instead of handling this here, I think you can handle it entirely within the /msvclto implementation by creating temporary files for all members of Symtab.ObjectFiles that have a ParentName.

inglorion added inline comments.Apr 20 2017, 5:56 PM
COFF/Driver.cpp
156 ↗(On Diff #96038)

I like that idea, but I'm not sure how I would write the object file to disk. Its MemoryBufferRef is protected. I could make it public or create a public accessor. Or I could use the ParentName to try and locate the archive and find the object file in there...but that feels kind of brittle. I'm not sure I like either implementation better than what I've already written and tested. If you have a better suggestion, or prefer one of the alternatives I presented, I'll be happy to implement that, though.

pcc added inline comments.Apr 20 2017, 6:06 PM
COFF/Driver.cpp
156 ↗(On Diff #96038)

I would just make it public. Generally we make things public when there is a need to do so. (For example, the MB in the ELF linker's InputFile is public so that we can pass it to a DWARF parser among other things.)

pcc added inline comments.Apr 20 2017, 6:10 PM
COFF/Driver.cpp
161 ↗(On Diff #96038)

FWIW, it doesn't look like these temporary files are ever deleted.

inglorion updated this revision to Diff 96071.Apr 20 2017, 6:22 PM

@pcc's suggestions (thanks!)

pcc added inline comments.Apr 20 2017, 6:32 PM
COFF/Driver.cpp
529 ↗(On Diff #96071)

Can't you s/EarlyObjects/Temps/ here and remove the code dealing with EarlyObjects?

530 ↗(On Diff #96071)

Don't you need to append a newline here?

test/COFF/msvclto-order.ll
12 ↗(On Diff #96071)

CHECK-NOT: lld-msvclto-order-b?

inglorion updated this revision to Diff 96196.Apr 21 2017, 11:57 AM
inglorion marked 3 inline comments as done.

Good call on the unnecessary vector and the missing newline! Fixed.

Harbormaster completed remote builds in B5757: Diff 96196.Apr 21 2017, 11:57 AM
pcc accepted this revision.Apr 21 2017, 12:24 PM

LGTM

test/COFF/msvclto-order.ll
12 ↗(On Diff #96071)

Please address

This revision is now accepted and ready to land.Apr 21 2017, 12:24 PM
inglorion added inline comments.Apr 21 2017, 1:01 PM
test/COFF/msvclto-order.ll
12 ↗(On Diff #96071)

We actually still pass the library, so that check would fail. We could add some code to not pass libraries, but I'm not sure that's always correct, so I decided to not make that effort and just keep the library path in the list of things to be passed to MSVC's linker, even if we don't expect any objects in that library to be used in the link.

inglorion added inline comments.Apr 21 2017, 1:03 PM
test/COFF/msvclto-order.ll
12 ↗(On Diff #96071)

After in person conversion, I realize you meant to check for the absence of the other object file, not the other library. Will do.

inglorion updated this revision to Diff 96214.Apr 21 2017, 1:05 PM

check that msvclto-order-b.obj is not passed to link.exe

Harbormaster completed remote builds in B5764: Diff 96214.Apr 21 2017, 1:05 PM
pcc added a comment.Apr 21 2017, 1:21 PM

Still LGTM

Closed by commit rL301045: [coff] for /msvclto, pass archive members with prevailing symbols first (authored by inglorion). · Explain WhyApr 21 2017, 2:50 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lld/
trunk/
COFF/
17 lines
3 lines
test/
COFF/
Inputs/
7 lines
10 lines
24 lines

Diff 96240

lld/trunk/COFF/Driver.cpp

Show First 20 Lines • Show All 506 Lines • ▼ Show 20 LinesfilterBitcodeFiles(StringRef Path, std::vector<std::string> &TemporaryFiles) {
return Temp; return Temp;
} }
// Create response file contents and invoke the MSVC linker. // Create response file contents and invoke the MSVC linker.
void LinkerDriver::invokeMSVC(opt::InputArgList &Args) { void LinkerDriver::invokeMSVC(opt::InputArgList &Args) {
std::string Rsp = "/nologo\n"; std::string Rsp = "/nologo\n";
std::vector<std::string> Temps; std::vector<std::string> Temps;
// Write out archive members that we used in symbol resolution and pass these
// to MSVC before any archives, so that MSVC uses the same objects to satisfy
// references.
for (const auto *O : Symtab.ObjectFiles) {
if (O->ParentName.empty())
continue;
SmallString<128> S;
int Fd;
if (auto EC = sys::fs::createTemporaryFile(
"lld-" + sys::path::filename(O->ParentName), ".obj", Fd, S))
fatal(EC, "cannot create a temporary file");
raw_fd_ostream OS(Fd, /*shouldClose*/ true);
OS << O->MB.getBuffer();
Temps.push_back(S.str());
Rsp += quote(S) + "\n";
}
for (auto *Arg : Args) { for (auto *Arg : Args) {
switch (Arg->getOption().getID()) { switch (Arg->getOption().getID()) {
case OPT_linkrepro: case OPT_linkrepro:
case OPT_lldmap: case OPT_lldmap:
case OPT_lldmap_file: case OPT_lldmap_file:
case OPT_lldsavetemps: case OPT_lldsavetemps:
case OPT_msvclto: case OPT_msvclto:
// LLD-specific options are stripped. // LLD-specific options are stripped.
▲ Show 20 Lines • Show All 526 Lines • Show Last 20 Lines

lld/trunk/COFF/InputFiles.h

Show First 20 Lines • Show All 52 Lines • ▼ Show 20 Linespublic:
StringRef getName() { return MB.getBufferIdentifier(); } StringRef getName() { return MB.getBufferIdentifier(); }
// Reads a file (the constructor doesn't do that). // Reads a file (the constructor doesn't do that).
virtual void parse() = 0; virtual void parse() = 0;
// Returns the CPU type this file was compiled to. // Returns the CPU type this file was compiled to.
virtual MachineTypes getMachineType() { return IMAGE_FILE_MACHINE_UNKNOWN; } virtual MachineTypes getMachineType() { return IMAGE_FILE_MACHINE_UNKNOWN; }
MemoryBufferRef MB;
// An archive file name if this file is created from an archive. // An archive file name if this file is created from an archive.
StringRef ParentName; StringRef ParentName;
// Returns .drectve section contents if exist. // Returns .drectve section contents if exist.
StringRef getDirectives() { return StringRef(Directives).trim(); } StringRef getDirectives() { return StringRef(Directives).trim(); }
protected: protected:
InputFile(Kind K, MemoryBufferRef M) : MB(M), FileKind(K) {} InputFile(Kind K, MemoryBufferRef M) : MB(M), FileKind(K) {}
MemoryBufferRef MB;
std::string Directives; std::string Directives;
private: private:
const Kind FileKind; const Kind FileKind;
}; };
// .lib or .a file. // .lib or .a file.
class ArchiveFile : public InputFile { class ArchiveFile : public InputFile {
▲ Show 20 Lines • Show All 125 Lines • Show Last 20 Lines

lld/trunk/test/COFF/Inputs/msvclto-order-a.ll

target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc"
define void @foo() {
ret void
}

lld/trunk/test/COFF/Inputs/msvclto-order-b.ll

target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc"
declare void @doesntexist()
define void @foo() {
call void @doesntexist()
ret void
}

lld/trunk/test/COFF/msvclto-order.ll

; RUN: opt -thinlto-bc %s -o %t.obj
; RUN: llc -filetype=obj %S/Inputs/msvclto-order-a.ll -o %T/msvclto-order-a.obj
; RUN: llvm-ar crs %T/msvclto-order-a.lib %T/msvclto-order-a.obj
; RUN: llc -filetype=obj %S/Inputs/msvclto-order-b.ll -o %T/msvclto-order-b.obj
; RUN: llvm-ar crs %T/msvclto-order-b.lib %T/msvclto-order-b.obj
; RUN: lld-link /verbose /msvclto /out:%t.exe /entry:main %t.obj \
; RUN: %T/msvclto-order-a.lib %T/msvclto-order-b.lib > %t.log || true
; RUN: FileCheck %s < %t.log
; CHECK: : link.exe
; CHECK-NOT: .lib{{$}}
; CHECK: lld-msvclto-order-a{{.*}}.obj
; CHECK-NOT: lld-msvclto-order-b{{.*}}.obj
; CHECK: .lib{{$}}
target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc"
declare void @foo()
define i32 @main() {
call void @foo()
ret i32 0
}