[COFF] don't replace import library if contents are unchanged

[inglorion]

Summary

This detects when an import library is about to be overwritten with a
newly built one with the same contents, and keeps the old library
instead. The use case for this is to avoid needlessly rebuilding
targets that depend on the import library in build systems that rely
on timestamps to determine whether a target requires rebuilding.

This feature was requested in PR35917.

[inglorion]

Example of the speedup. I built chromium's net_unittests in a component build, then changed src/base/rand_utils.cc by removing the DCHECK_LE at the beginning of RandInt, rebuilt, put the DCHECK_LE back in, rebuilt, etc. The following times are representative of my results.

Without -keep-unchanged-implib:

tim ninja -C out/clang net_unittests
ninja: Entering directory `out/clang'
[41/41] LINK net_unittests.exe

peak memory: 408.08MB
real: 0m37.594s
qpc: 37594502us

With -keep-unchanged-implib:

tim ninja -C out/clang net_unittests
ninja: Entering directory `out/clang'
[2/41] LINK(DLL) base.dll base.dll.lib

peak memory: 187.51MB
real: 0m3.672s
qpc: 3660537us

The args.gn I used for this is

is_clang = true
is_component_build = true
clang_base_path = "C:/src/clang"
clang_use_chrome_plugins = false
is_cfi = false
is_debug = false
is_official_build = false
symbol_level = 0
use_lld = true
use_thin_lto = false

[inglorion]

Some alternatives I considered:

1. Instead of doing this for only import libraries, do it for all archives. This has the potential to allow more things to be sped up, but it also means that every archive will be byte-for-byte compared if an older version already exists. Since archives in general are larger than import libraries, and the general expectation is that if the build system rebuilt it, something changed, this doesn't seem worth it.

2. Enabling the new functionality by default. I did not consider this safe to do, because it breaks the assumptions many people and build systems make about how things work. If A depends on B and B depends on C, then, without the change, changing C will cause A and B to be rebuilt and receive new timestamps. With the change, if rebuilding B results in the same bytes that were already in there, B will retain its old timestamp and A will not be rebuilt. This also means that B will now look older than C. This seems like an unexpected enough change in semantics that I would want people to explicitly opt into it.

3. Making changes on a lower level than just in the COFF linker driver. I had thought of something along the lines of adding a writeArchive function that produces a MemoryBuffer or something similar, so that bytes can be compared before making a decision about actually committing something to the filesystem. Perhaps adding a flag to writeArchive to easily enable the check. These things may be worth doing in a follow-up especially if we want to have similar behavior in more places. For now, I felt like not changing common code for a special case in the COFF linker was the better choice.

[pcc]

Enabling the new functionality by default. I did not consider this safe to do, because it breaks the assumptions many people and build systems make about how things work

Doesn't link.exe already do this though?

[zturner]

In D42326#982664, @inglorion wrote:

2. Enabling the new functionality by default. I did not consider this safe to do, because it breaks the assumptions many people and build systems make about how things work. If A depends on B and B depends on C, then, without the change, changing C will cause A and B to be rebuilt and receive new timestamps. With the change, if rebuilding B results in the same bytes that were already in there, B will retain its old timestamp and A will not be rebuilt. This also means that B will now look older than C. This seems like an unexpected enough change in semantics that I would want people to explicitly opt into it.

We should do by default whatever MSVC does by default. Can you do a few simple tests with MSVC and just mimic exactly what they do in every case?

[ruiu]

lld/COFF/Driver.cpp

546 ↗

(On Diff #130725)

Please write the actual type instead of auto.

549–550 ↗

(On Diff #130725)

if (auto EC = ...)

551 ↗

(On Diff #130725)

Error messages should start with a lowercase letter.

556 ↗

(On Diff #130725)

Ditto

558 ↗

(On Diff #130725)

Do you have to call reset?

559–560 ↗

(On Diff #130725)

if (auto EC = ...

[inglorion]

Doesn't link.exe already do this though?

Looks like it, yeah. I'll default this to enabled, then.

[pcc]

In D42326#982708, @inglorion wrote:

Doesn't link.exe already do this though?

Looks like it, yeah. I'll default this to enabled, then.

I'd just remove the option altogether unless/until we discover a need for it.

[ruiu]

I'd just remove the option altogether unless/until we discover a need for it.

No need to provide a way to disable it, which should simplify the code.

[inglorion]

lld/COFF/Driver.cpp

558 ↗

(On Diff #130725)

I think we need it, yes. This is to close the file that we just opened, so that rename can replace it.

See also similar code at the end of writeArchive() in llvm/lib/Object/ArchiveWriter.cpp, and the comment there.

[inglorion]

Made this the new behavior and removed the options to enable/disable it.

Made the code changes suggested by @ruiu.

[ruiu]

lld/COFF/Driver.cpp

544–545 ↗

(On Diff #130737)

Omit llvm::.

555 ↗

(On Diff #130737)

I'd do early return here.

562 ↗

(On Diff #130737)

Also here.

565–567 ↗

(On Diff #130737)

Isn't HandleError(writeImportLibrary(...)) better?

565–567 ↗

(On Diff #130737)

Since "then" part is much longer than this "else" part, I'd flip the condition and early return. I.e.

if (!OldBuf) {
  HandleError(writeImportLibrary(...));
  return;
}

// Replace the existing file only when the new file is different.

[inglorion]

simplifications suggested by @ruiu

[ruiu]

lld/COFF/Driver.cpp

540–541 ↗

(On Diff #130741)

Please write the real types instead of auto.

563 ↗

(On Diff #130741)

Ditto

[rnk]

I thought we'd put this under the existing /incremental flag, rather than doing it all the time, since that's probably what MSVC does.

[inglorion]

@rnk, I ran a few tests with MSVC's linker and found the dependents of the import library weren't rebuilt even with /incremental:no.

[inglorion]

replaced more autos with actual types

[ruiu]

LGTM

[inglorion]

Thanks for the quick review, everyone!

[inglorion]

rebase

lld/trunk/COFF/Driver.cpp

for (Export &E1 : Config->Exports) {
     E2.Ordinal = E1.Ordinal;
     E2.Noname = E1.Noname;
     E2.Data = E1.Data;
     E2.Private = E1.Private;
     E2.Constant = E1.Constant;
     Exports.push_back(E2);
   }
 
-  auto E = writeImportLibrary(getImportName(AsLib), getImplibPath(), Exports,
-                              Config->Machine, false, Config->MinGW);
+  auto HandleError = [](Error &&E) {
     handleAllErrors(std::move(E),
-                  [&](ErrorInfoBase &EIB) { error(EIB.message()); });
+                    [](ErrorInfoBase &EIB) { error(EIB.message()); });
+  };
+  std::string LibName = getImportName(AsLib);
+  std::string Path = getImplibPath();
+
+  // If the import library already exists, replace it only if the contents
+  // have changed.
+  ErrorOr<std::unique_ptr<MemoryBuffer>> OldBuf = MemoryBuffer::getFile(Path);
+  if (!OldBuf) {
+    HandleError(writeImportLibrary(LibName, Path, Exports, Config->Machine,
+                                   false, Config->MinGW));
+    return;
+  }
+
+  SmallString<128> TmpName;
+  if (std::error_code EC =
+          sys::fs::createUniqueFile(Path + ".tmp-%%%%%%%%.lib", TmpName))
+    fatal("cannot create temporary file for import library " + Path + ": " +
+          EC.message());
+
+  if (Error E = writeImportLibrary(LibName, TmpName, Exports, Config->Machine,
+                                   false, Config->MinGW)) {
+    HandleError(std::move(E));
+    return;
+  }
+
+  std::unique_ptr<MemoryBuffer> NewBuf = check(MemoryBuffer::getFile(TmpName));
+  if ((*OldBuf)->getBuffer() != NewBuf->getBuffer()) {
+    OldBuf->reset();
+    HandleError(errorCodeToError(sys::fs::rename(TmpName, Path)));
+  }
 }
 
 static void parseModuleDefs(StringRef Path) {
   std::unique_ptr<MemoryBuffer> MB = CHECK(
       MemoryBuffer::getFile(Path, -1, false, true), "could not open " + Path);
   COFFModuleDefinition M = check(parseCOFFModuleDefinition(
       MB->getMemBufferRef(), Config->Machine, Config->MinGW));
 
if (identify_magic(Member.getBuffer()) != file_magic::bitcode)
       New.emplace_back(Member);
 
   if (New.empty())
     return None;
 
   log("Creating a temporary archive for " + Path + " to remove bitcode files");
 
   SmallString<128> S;
-  if (auto EC = sys::fs::createTemporaryFile("lld-" + sys::path::stem(Path),
-                                             ".lib", S))
+  if (std::error_code EC = sys::fs::createTemporaryFile(
+          "lld-" + sys::path::stem(Path), ".lib", S))
     fatal("cannot create a temporary file: " + EC.message());
   std::string Temp = S.str();
   TemporaryFiles.push_back(Temp);
 
   Error E =
       llvm::writeArchive(Temp, New, /*WriteSymtab=*/true, Archive::Kind::K_GNU,
                          /*Deterministics=*/true,
                          /*Thin=*/false);

lld/trunk/test/COFF/unchanged-importlib.test

+# RUN: yaml2obj < %p/Inputs/export.yaml > %t.obj
+# RUN: lld-link -out:%t.dll -dll %t.obj
+# RUN: touch -t 198002011200.00 %t.lib
+# RUN: lld-link -out:%t.dll -dll %t.obj
+# RUN: ls -l %t.lib | FileCheck --check-prefix=CHECK %s
+
+# CHECK: Feb 1 1980