[LLD] [COFF] Fix wholearchive with thin archives

[mstorsjo]

Summary

The Archive object created when loading an archive specified with wholearchive got cleaned up immediately, when the owning std::unique_ptr went out of scope, even if persisted StringRefs pointed to memory that belonged to the archive, which no longer was mapped in memory.

This hasn't been an issue with regular (as opposed to thin) archives, as references to the member objects has kept the mapping for the whole archive file alive - but with thin archives, all such references point to other files.

Add the std::unique_ptr to the arena allocator, to retain it as long as necessary.

This fixes (the last issue raised in) PR42388.

@hans I think this should be a safe backport once it lands.

[ruiu]

LGTM

[orgads]

Seems to fix the issue. Thanks!

[mstorsjo]

In D65565#1610082, @orgads wrote:

Seems to fix the issue. Thanks!

FYI, in case you're considering backporting this to LLD 8.0; I had to tweak the change a little bit to make it not break non-thin archives with wholearchive.

[hans]

In D65565#1610802, @mstorsjo wrote:

In D65565#1610082, @orgads wrote:

Seems to fix the issue. Thanks!

FYI, in case you're considering backporting this to LLD 8.0; I had to tweak the change a little bit to make it not break non-thin archives with wholearchive.

The commit doesn't merge cleanly to the release_90 branch.

If you think it would be useful to merge this, could you prepare a diff that applies cleanly to the branch? Doing it against the git monorepo might be easiest.

[orgads]

The commit doesn't merge cleanly to the release_90 branch.

If you think it would be useful to merge this, could you prepare a diff that applies cleanly to the branch? Doing it against the git monorepo might be easiest.

The change in Driver.cpp does apply cleanly. The test file just doesn't exist in 9.0. Do you need the test?

[mstorsjo]

In D65565#1612269, @orgads wrote:

The commit doesn't merge cleanly to the release_90 branch.

If you think it would be useful to merge this, could you prepare a diff that applies cleanly to the branch? Doing it against the git monorepo might be easiest.

The change in Driver.cpp does apply cleanly. The test file just doesn't exist in 9.0. Do you need the test?

A backport to the 9.x branch on the monorepo, including a test, is available at https://martin.st/temp/0001-COFF-Fix-wholearchive-with-thin-archives.patch. I picked the tests from the earlier commit on master, minus the tests for the new stuff in the commit that actually added them, but with the tests for wholearchive+thin instead.

[hans]

In D65565#1612908, @mstorsjo wrote:

In D65565#1612269, @orgads wrote:

The commit doesn't merge cleanly to the release_90 branch.

If you think it would be useful to merge this, could you prepare a diff that applies cleanly to the branch? Doing it against the git monorepo might be easiest.

The change in Driver.cpp does apply cleanly. The test file just doesn't exist in 9.0. Do you need the test?

A backport to the 9.x branch on the monorepo, including a test, is available at https://martin.st/temp/0001-COFF-Fix-wholearchive-with-thin-archives.patch. I picked the tests from the earlier commit on master, minus the tests for the new stuff in the commit that actually added them, but with the tests for wholearchive+thin instead.

Many thanks! Committed that to the branch in r367806.

[thakis]

This is fairly different than D18669 which fixed the same thing for the ELF port. Any opinion on which approach is better?

lld/trunk/COFF/Driver.cpp

void LinkerDriver::addBuffer(std::unique_ptr<MemoryBuffer> mb,
   switch (identify_magic(mbref.getBuffer())) {
   case file_magic::windows_resource:
     resources.push_back(mbref);
     break;
   case file_magic::archive:
     if (wholeArchive) {
       std::unique_ptr<Archive> file =
           CHECK(Archive::create(mbref), filename + ": failed to parse archive");
+      Archive *archive = file.get();
+      make<std::unique_ptr<Archive>>(std::move(file)); // take ownership
 
-      for (MemoryBufferRef m : getArchiveMembers(file.get()))
+      for (MemoryBufferRef m : getArchiveMembers(archive))
         addArchiveBuffer(m, "<whole-archive>", filename, 0);
       return;
     }
     symtab->addFile(make<ArchiveFile>(mbref));
     break;
   case file_magic::bitcode:
     symtab->addFile(make<BitcodeFile>(mbref, "", 0));
     break;

lld/trunk/test/COFF/thin-archive.s

 # REQUIRES: x86
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-windows-msvc -o %t.main.obj %s
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-windows-msvc -o %t.lib.obj \
 # RUN:     %S/Inputs/mangled-symbol.s
 # RUN: lld-link /lib /out:%t.lib %t.lib.obj
 # RUN: lld-link /lib /llvmlibthin /out:%t_thin.lib %t.lib.obj
 
 # RUN: lld-link /entry:main %t.main.obj %t.lib /out:%t.exe 2>&1 | \
 # RUN:     FileCheck --allow-empty %s
 # RUN: lld-link /entry:main %t.main.obj %t_thin.lib /out:%t.exe 2>&1 | \
 # RUN:     FileCheck --allow-empty %s
+# RUN: lld-link /entry:main %t.main.obj /wholearchive:%t_thin.lib /out:%t.exe 2>&1 | \
+# RUN:     FileCheck --allow-empty %s
 
 # RUN: rm %t.lib.obj
 # RUN: lld-link /entry:main %t.main.obj %t.lib /out:%t.exe 2>&1 | \
 # RUN:     FileCheck --allow-empty %s
 # RUN: not lld-link /entry:main %t.main.obj %t_thin.lib /out:%t.exe 2>&1 | \
 # RUN:     FileCheck --check-prefix=NOOBJ %s
 # RUN: not lld-link /entry:main %t.main.obj %t_thin.lib /out:%t.exe \
 # RUN:     /demangle:no 2>&1 | FileCheck --check-prefix=NOOBJNODEMANGLE %s