This is an archive of the discontinued LLVM Phabricator instance.

Differential D79880

[LLD][ELF] Use offset in thin archives to disambiguate thinLTO members
ClosedPublic

Authored by hoyFB on May 13 2020, 9:33 AM.

Details

Reviewers
• espindola
MaskRay
ruiu
wenlei
Commits
rG90af55d8a972: [LLD][ELF] Use offset in thin archives to disambiguate thinLTO members
Summary

This is fixing a thinLTO module collision issue for thin archives. The problem is that we always use a zero offset to name members in a thin archive and that causes the following build error:

ld.lld: error: Expected at most one ThinLTO module per bitcode file

which happens to a thin archive that has two members with the same object file name (whose paths will be ignored by thinLTO driver)

The fix here is to use real member offset instead as is done for non-thin archives.

Diff Detail

Unit TestsFailed

TimeTest
1,440 mslld.ELF/lto::Unknown Unit Message ("")

Event Timeline

hoyFB created this revision.May 13 2020, 9:33 AM
Herald added a reviewer: • espindola. · View Herald TranscriptMay 13 2020, 9:33 AM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, dexonsmith, steven_wu and 5 others. · View Herald Transcript
hoyFB edited the summary of this revision. (Show Details)May 13 2020, 9:34 AM
hoyFB added reviewers: ruiu, wenlei.
hoyFB retitled this revision from [LLD][ELF] Use offset in thin-archive to disambiguate thinLTO members to [LLD][ELF] Use offset in thin archives to disambiguate thinLTO members.May 13 2020, 9:45 AM
MaskRay added inline comments.May 13 2020, 10:42 AM
lld/test/ELF/lto/thinlto-thinarchivecollision.ll
2 ↗(On Diff #263755)

%T is currently deprecated.

You can use RUN: rm -fr %t && mkdir %t && cd %t and update the following lines to be relative to %t

23 ↗(On Diff #263755)

Rename to _start to avoid --entry=main

28 ↗(On Diff #263755)

No newline at end of file

hoyFB updated this revision to Diff 263794.May 13 2020, 11:08 AM
hoyFB marked 3 inline comments as done.
hoyFB edited the summary of this revision. (Show Details)

Updating D79880: [LLD][ELF] Use offset in thin archives to disambiguate thinLTO members

lld/test/ELF/lto/thinlto-thinarchivecollision.ll
2 ↗(On Diff #263755)

Good idea, it also simplifies the CHECK lines.

hoyFB marked an inline comment as done.May 13 2020, 11:09 AM
Harbormaster failed remote builds in B56614: Diff 263755!May 13 2020, 11:25 AM
MaskRay accepted this revision.May 13 2020, 11:38 AM

Looks good to me. Will be worth waiting a day or so to see if there are any further comments.

lld/test/ELF/lto/thinlto-thinarchivecollision.ll
1 ↗(On Diff #263794)

thinlto-thin-archive-collision.ll

8 ↗(On Diff #263794)

--save-temps

This revision is now accepted and ready to land.May 13 2020, 11:38 AM
hoyFB updated this revision to Diff 263813.May 13 2020, 12:01 PM
hoyFB marked 2 inline comments as done.

Updating D79880: [LLD][ELF] Use offset in thin archives to disambiguate thinLTO members

Harbormaster failed remote builds in B56631: Diff 263794!May 13 2020, 1:04 PM
Harbormaster failed remote builds in B56642: Diff 263813!May 13 2020, 3:17 PM
hoyFB updated this revision to Diff 263881.May 13 2020, 4:21 PM

Updating D79880: [LLD][ELF] Use offset in thin archives to disambiguate thinLTO members

hoyFB marked an inline comment as done.May 13 2020, 4:23 PM
hoyFB added inline comments.
lld/test/ELF/lto/thinlto.ll
65

Attempting to make it work on Windows platform which looks like the diff time testing is performed on and with the failure:

$ ":" "RUN: at line 65"
$ "ls" "C:\ws\prod\llvm-project\build\tools\lld\test\ELF\lto\Output\thinlto.ll.tmp.dir/t.a\(t.o\" "at\" "*\).0.preopt.bc"
# command stderr:
s: C:\ws\prod\llvm-project\build\tools\lld\test\ELF\lto\Output\thinlto.ll.tmp.dir/t.a\(t.o\: No such file or directory
ls: at\: No such file or directory
l.s: *\).0.preopt.bc: Invalid argument
Harbormaster completed remote builds in B56679: Diff 263881.May 13 2020, 5:29 PM
ruiu accepted this revision.May 13 2020, 8:29 PM

LGTM

lld/test/ELF/lto/thinlto.ll
65

I think that on Windows only double-quotes are recognized as a quotation mark and single-quotes are just regular characters. So you can probably change ' to ". Using * is fine though.

Closed by commit rG90af55d8a972: [LLD][ELF] Use offset in thin archives to disambiguate thinLTO members (authored by hoy). · Explain WhyMay 15 2020, 12:31 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lld/
ELF/
3 lines
test/
ELF/
lto/
27 lines
2 lines

Diff 263813

lld/ELF/InputFiles.cpp

Show First 20 Lines • Show All 1,155 Lines • ▼ Show 20 Lines MemoryBufferRef mb =
CHECK(c.getMemoryBufferRef(), CHECK(c.getMemoryBufferRef(),
toString(this) + toString(this) +
": could not get the buffer for the member defining symbol " + ": could not get the buffer for the member defining symbol " +
toELFString(sym)); toELFString(sym));
if (tar && c.getParent()->isThin()) if (tar && c.getParent()->isThin())
tar->append(relativeToRoot(CHECK(c.getFullName(), this)), mb.getBuffer()); tar->append(relativeToRoot(CHECK(c.getFullName(), this)), mb.getBuffer());
InputFile *file = createObjectFile( InputFile *file = createObjectFile(mb, getName(), c.getChildOffset());
mb, getName(), c.getParent()->isThin() ? 0 : c.getChildOffset());
file->groupId = groupId; file->groupId = groupId;
parseFile(file); parseFile(file);
} }
size_t ArchiveFile::getMemberCount() const { size_t ArchiveFile::getMemberCount() const {
size_t count = 0; size_t count = 0;
Error err = Error::success(); Error err = Error::success();
for (const Archive::Child &c : file->children(err)) { for (const Archive::Child &c : file->children(err)) {
▲ Show 20 Lines • Show All 494 Lines • Show Last 20 Lines

lld/test/ELF/lto/thinlto-thin-archive-collision.ll

  • This file was added.
; REQUIRES: x86
; RUN: rm -fr %t && mkdir %t && cd %t
; RUN: mkdir thinlto-archives thinlto-archives/a thinlto-archives/b
; RUN: opt -thinlto-bc -o thinlto-archives/main.o %s
; RUN: opt -thinlto-bc -o thinlto-archives/a/thin.o %S/Inputs/thin1.ll
; RUN: opt -thinlto-bc -o thinlto-archives/b/thin.o %S/Inputs/thin2.ll
; RUN: llvm-ar qcT thinlto-archives/thin.a thinlto-archives/a/thin.o thinlto-archives/b/thin.o
; RUN: ld.lld thinlto-archives/main.o thinlto-archives/thin.a -o thinlto-archives/main.exe --save-temps
; RUN: FileCheck %s < thinlto-archives/main.exe.resolution.txt
; CHECK: thinlto-archives/main.o
; CHECK: thinlto-archives/thin.a(thin.o at {{[1-9][0-9]+}})
; CHECK-NEXT: -r=thinlto-archives/thin.a(thin.o at {{[1-9][0-9]+}}),foo,pl
; CHECK: thinlto-archives/thin.a(thin.o at {{[1-9][0-9]+}})
; CHECK-NEXT: -r=thinlto-archives/thin.a(thin.o at {{[1-9][0-9]+}}),blah,pl
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-scei-ps4"
declare i32 @blah(i32 %meh)
declare i32 @foo(i32 %goo)
define i32 @_start() {
call i32 @foo(i32 0)
call i32 @blah(i32 0)
ret i32 0
}

lld/test/ELF/lto/thinlto.ll

Show First 20 Lines • Show All 56 Lines • ▼ Show 20 Lines
; RUN: llvm-nm %t32.lto.o | FileCheck %s --check-prefix=NM2 ; RUN: llvm-nm %t32.lto.o | FileCheck %s --check-prefix=NM2
; Check that -save-temps is usable with thin archives ; Check that -save-temps is usable with thin archives
; RUN: rm -fr %t.dir ; RUN: rm -fr %t.dir
; RUN: mkdir -p %t.dir ; RUN: mkdir -p %t.dir
; RUN: cp %t2.o %t.dir/t.o ; RUN: cp %t2.o %t.dir/t.o
; RUN: llvm-ar rcsT %t.dir/t.a %t.dir/t.o ; RUN: llvm-ar rcsT %t.dir/t.a %t.dir/t.o
; RUN: ld.lld -save-temps %t1.o %t.dir/t.a -o %t.null ; RUN: ld.lld -save-temps %t1.o %t.dir/t.a -o %t.null
; RUN: ls '%t.dir/t.a(t.o at 0).0.preopt.bc' ; RUN: ls %t.dir/t.a\(t.o\ at\ *\).0.preopt.bc
hoyFBAuthorUnsubmitted
Done
ReplyInline Actions

Attempting to make it work on Windows platform which looks like the diff time testing is performed on and with the failure:

$ ":" "RUN: at line 65"
$ "ls" "C:\ws\prod\llvm-project\build\tools\lld\test\ELF\lto\Output\thinlto.ll.tmp.dir/t.a\(t.o\" "at\" "*\).0.preopt.bc"
# command stderr:
s: C:\ws\prod\llvm-project\build\tools\lld\test\ELF\lto\Output\thinlto.ll.tmp.dir/t.a\(t.o\: No such file or directory
ls: at\: No such file or directory
l.s: *\).0.preopt.bc: Invalid argument
hoyFB: Attempting to make it work on Windows platform which looks like the diff time testing is…
ruiuUnsubmitted
Not Done
ReplyInline Actions

I think that on Windows only double-quotes are recognized as a quotation mark and single-quotes are just regular characters. So you can probably change ' to ". Using * is fine though.

ruiu: I think that on Windows only double-quotes are recognized as a quotation mark and single-quotes…
; NM1: T f ; NM1: T f
; NM2: T g ; NM2: T g
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu" target triple = "x86_64-unknown-linux-gnu"
declare void @g(...) declare void @g(...)
define void @f() { define void @f() {
entry: entry:
call void (...) @g() call void (...) @g()
ret void ret void
} }