This is an archive of the discontinued LLVM Phabricator instance.

Differential D66239

[LLD][COFF] Add index to disambiguate archive members when using -wholearchive
ClosedPublic

Authored by zero9178 on Aug 14 2019, 12:34 PM.

Details

Reviewers
ruiu
pcc
Commits
rG89efb03463ee: [LLD][COFF] Add index to disambiguate archive members when using -wholearchive
rLLD371509: [LLD][COFF] Add index to disambiguate archive members when using -wholearchive
rL371509: [LLD][COFF] Add index to disambiguate archive members when using -wholearchive
Summary

PR42951:
When linking an archive with members that have the same name linking fails when using the -wholearchive option. This patch passes the index of the member in the archive to the offset parameter to disambiguate the member.

Diff Detail

Event Timeline

zero9178 created this revision.Aug 14 2019, 12:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 12:34 PM
Herald added subscribers: llvm-commits, arphaman, dexonsmith and 2 others. · View Herald Transcript
zero9178 edited projects, added lld; removed Restricted Project.Aug 14 2019, 12:38 PM
zero9178 removed subscribers: mehdi_amini, steven_wu, dexonsmith and 2 others.
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 12:38 PM
zero9178 updated this revision to Diff 215217.Aug 14 2019, 1:15 PM

Fixed test to show regression and verify output

zero9178 added a reviewer: pcc.Aug 14 2019, 2:01 PM
ruiu added inline comments.Aug 19 2019, 4:35 AM
COFF/Driver.cpp
191

Just int would probably be better.

test/COFF/thinlto-whole-archives.ll
13–18

This may break Windows buildbot on which path components are separated not by / but by \.

zero9178 updated this revision to Diff 218064.Aug 30 2019, 3:53 AM

Changed index from std::uint64_t to int. Made test accept both kinds of slashes

ruiu accepted this revision.Sep 2 2019, 1:11 AM

LGTM with this change.

COFF/Driver.cpp
191

Another nit: memberIndex is better than memberOffset because offset implies that that variable contains a distance from the beginning of the file and a current member (which does not always increment by one).

This revision is now accepted and ready to land.Sep 2 2019, 1:11 AM
zero9178 updated this revision to Diff 218439.Sep 3 2019, 5:07 AM

Changed memberOffset to memberIndex as suggested by reviwer. Would need someone to commit this for me

zero9178 marked 3 inline comments as done.Sep 3 2019, 5:07 AM
zero9178 added a comment.Sep 10 2019, 1:02 AM

Friendly after a week ping.

Closed by commit rL371509: [LLD][COFF] Add index to disambiguate archive members when using -wholearchive (authored by ruiu). · Explain WhySep 10 2019, 4:49 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
COFF/
3 lines
test/
COFF/
30 lines

Diff 215217

COFF/Driver.cpp

Context not available.
Archive *archive = file.get(); Archive *archive = file.get();
make<std::unique_ptr<Archive>>(std::move(file)); // take ownership make<std::unique_ptr<Archive>>(std::move(file)); // take ownership
std::uint64_t memberOffset = 0;
ruiuUnsubmitted
Done
ReplyInline Actions

Just int would probably be better.

ruiu: Just `int` would probably be better.
ruiuUnsubmitted
Done
ReplyInline Actions

Another nit: memberIndex is better than memberOffset because offset implies that that variable contains a distance from the beginning of the file and a current member (which does not always increment by one).

ruiu: Another nit: memberIndex is better than memberOffset because offset implies that that variable…
for (MemoryBufferRef m : getArchiveMembers(archive)) for (MemoryBufferRef m : getArchiveMembers(archive))
addArchiveBuffer(m, "<whole-archive>", filename, 0); addArchiveBuffer(m, "<whole-archive>", filename, memberOffset++);
return; return;
} }
symtab->addFile(make<ArchiveFile>(mbref)); symtab->addFile(make<ArchiveFile>(mbref));
Context not available.

test/COFF/thinlto-whole-archives.ll

; REQUIRES: x86
; RUN: rm -fr %T/thinlto-whole-archives
; RUN: mkdir %T/thinlto-whole-archives %T/thinlto-whole-archives/a %T/thinlto-whole-archives/b
; RUN: opt -thinlto-bc -o %T/thinlto-whole-archives/main.obj %s
; RUN: opt -thinlto-bc -o %T/thinlto-whole-archives/a/bar.obj %S/Inputs/lto-dep.ll
; RUN: opt -thinlto-bc -o %T/thinlto-whole-archives/b/bar.obj %S/Inputs/bar.ll
; RUN: llvm-ar crs %T/thinlto-whole-archives/a.lib %T/thinlto-whole-archives/a/bar.obj %T/thinlto-whole-archives/b/bar.obj
; RUN: lld-link -out:%T/thinlto-whole-archives/main.exe -entry:main \
; RUN: -wholearchive -lldsavetemps -subsystem:console %T/thinlto-whole-archives/main.obj \
; RUN: %T/thinlto-whole-archives/a.lib
; RUN: FileCheck %s < %T/thinlto-whole-archives/main.exe.resolution.txt
; CHECK: {{/thinlto-whole-archives/main.obj$}}
; CHECK: {{^-r=.*/thinlto-whole-archives/main.obj,main,px$}}
; CHECK: {{/thinlto-whole-archives/a.libbar.obj[0-9]+$}}
; CHECK-NEXT: {{^-r=.*/thinlto-whole-archives/a.libbar.obj[0-9]+,foo,p$}}
; CHECK-NEXT: {{/thinlto-whole-archives/a.libbar.obj[0-9]+$}}
; CHECK-NEXT: {{^-r=.*/thinlto-whole-archives/a.libbar.obj[0-9]+,bar,p$}}
ruiuUnsubmitted
Done
ReplyInline Actions

This may break Windows buildbot on which path components are separated not by / but by \.

ruiu: This may break Windows buildbot on which path components are separated not by / but by \.
target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc"
declare void @bar()
declare void @foo()
define i32 @main() {
call void @foo()
call void @bar()
ret i32 0
}