[Reproduce] Fix thin archive member reproducer paths - #1741
[Reproduce] Fix thin archive member reproducer paths#1741Rachit Mehta (rachitmeht) wants to merge 1 commit into
Conversation
Rachit Mehta (rachitmeht)
changed the title
|
Rachit Mehta (@rachitmeht) Can you please add tests? |
Parth (parth-07)
left a comment
Parth (parth-07)
left a comment
There was a problem hiding this comment.
Rachit Mehta (@rachitmeht) With this fix, will the created reproducer work standalone?
|
|
||
| std::string OutputTarWriter::getHashAndExtension(const Input *Ipt) const { | ||
| uint64_t InputHash = Ipt->getResolvedPathHash(); | ||
| if (const auto *ArchiveMember = llvm::dyn_cast<ArchiveMemberInput>(Ipt)) |
There was a problem hiding this comment.
Won't we only call getHashAndExtension on top-level inputs, and thus, it should never be called for archive members?
cc5b631 to
481ff1c
Compare
| @@ -0,0 +1,27 @@ | |||
| #UNSUPPORTED: windows, reproduce_fail | |||
There was a problem hiding this comment.
Why is the test unsupported for windows? and we likely do not need reproduce_fail either here.
There was a problem hiding this comment.
The windows restriction was there because I used bash -x to replay response.txt, I will change it.
| RUN: %clang %clangopts -c %p/Inputs/thin-main.c -o %t.tmpdir/main.o | ||
| RUN: cd %t.tmpdir | ||
| RUN: %ar cr %aropts --thin libsame.a a/foo.o b/foo.o | ||
| RUN: %link --no-threads main.o libsame.a -o same.out --reproduce repro.tar --dump-mapping-file mapping.ini |
There was a problem hiding this comment.
Can you prefix all output files using %t? It makes debugging easier as then we do not need additional efforts in finding the full path of the outputs.
| if (const auto *ArchiveMember = llvm::dyn_cast<ArchiveMemberInput>(Ipt)) | ||
| // Thin archive members share the parent archive's resolved path. Include | ||
| // the member identity so members with the same basename cannot collide. | ||
| InputHash = llvm::hash_combine(InputHash, ArchiveMember->getMemberName(), |
There was a problem hiding this comment.
Would using the member name alone not be enough to make the hash different here?
There was a problem hiding this comment.
I took reference for this from this lld implementation. ELF/InputFiles.cpp
// ThinLTO assumes that all MemoryBufferRefs given to it have a unique
// name. If two archives define two members with the same name, this
// causes a collision which result in only one of the objects being taken
// into consideration at LTO time (which very likely causes undefined
// symbols later in the link stage). So we append file offset to make
// filename unique.
For a/foo.c and b/foo.c
without offset it will be only "libname.a + foo.c"
There was a problem hiding this comment.
The archive member names should be a/foo.o and b/foo.o instead of just foo.o.
There was a problem hiding this comment.
I read gnu ar docs, -q allows duplicate files to get appended. Maybe that's the reason lld implements with child offset for uniqueness.
Include the thin archive member name and child offset in the generated input identity. This prevents members with identical basenames from colliding in --reproduce archives and preserves both files during replay. Resolves qualcomm#1737
481ff1c to
e7aa304
Compare
2 participants
Include the thin archive member name and child offset in the generated input identity. This prevents members with identical basenames from colliding in --reproduce archives and preserves both files during replay.
Resolves #1737