[COFF] make /incremental control overwriting unchanged import libraries

[inglorion]

Summary

r323164 made lld-link not overwrite import libraries when their
contents haven't changed. MSVC's link.exe does this only when
performing incremental linking. This change makes lld-link's import
library overwriting similarly dependent on whether or not incremental
linking is being performed. This is controlled by the /incremental or
/incremental:no options. In addition, /opt:icf, /opt:ref, and /order
turn off /incremental and issue a warning if /incremental was
specified on the command line.

[inglorion]

Here is the observed behavior of link.exe:

C> link -nologo -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -incremental -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -incremental -dll -out:test.dll test.obj

C> link -nologo -debug -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -debug -dll -out:test.dll test.obj

C> link -nologo -debug -incremental:no -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -debug -incremental:no -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -opt:icf -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -opt:icf -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -incremental -opt:icf -dll -out:test.dll test.obj
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF' specification
   Creating library test.lib and object test.exp

C> link -nologo -incremental -opt:icf -dll -out:test.dll test.obj
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF' specification
   Creating library test.lib and object test.exp

C> link -nologo -debug -opt:icf -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -debug -opt:icf -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -incremental -opt:icf -dll -out:test.dll test.obj
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF' specification
   Creating library test.lib and object test.exp

C> link -nologo -incremental -opt:ref -dll -out:test.dll test.obj
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:REF' specification
   Creating library test.lib and object test.exp

C> link -nologo -incremental -opt:ref -dll -out:test.dll test.obj
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:REF' specification
   Creating library test.lib and object test.exp

C> link -nologo -debug -opt:ref -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -debug -opt:ref -dll -out:test.dll test.obj
   Creating library test.lib and object test.exp

C> link -nologo -incremental -opt:icf -dll -out:test.dll test.obj
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF' specification
   Creating library test.lib and object test.exp

C> link -nologo -opt:icf -incremental -dll -out:test.dll test.obj
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF' specification
   Creating library test.lib and object test.exp

C> link -nologo -ignore:4075 -opt:icf -incremental -dll -out:test.dll test.obj
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF' specification
   Creating library test.lib and object test.exp

C> link -nologo -wx -ignore:4075 -opt:icf -incremental -dll -out:test.dll test.obj
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/OPT:ICF' specification
LINK : error LNK1218: warning treated as error; no output file generated

[ruiu]

lld/COFF/Driver.cpp

697–698

Usually we define get<OptionName> (e.g. getIncremental) which returns a boolean value, so you can write

Config->Incremental = getIncremental(Args);

in LinkerDriver::link.

Then after parsing all options, we do check option incompatibility like this

if (Config->Incremental && Config->DoICF) {
  warn("ignoring /INCREMENTAL due to /icf specification");
  return;
}

if (Config->Incremental && Config->DoGC) {
  warn("ignoring /INCREMENTAL due to /ref specification");
  Config->Incremental = false;
}

if (Config->Incremental && !Config->Order.empty()) {
  warn("ignoring /INCREMENTAL due to /order specification");
  Config->Incremental = false;
}

This is slightly lengthy than the code that uses a lambda, but this is perhaps easier to read.

1014–1015

I don't think we actually have to mimic this behavior. If you have a real incremental linking you have to do this because otherwise it is very hard to do incremental linking, but that's an artifact of the implementation. Can we just keep the original code?

lld/COFF/Options.td

107–108

This description is misleading as we don't actually do incremental linking.

[inglorion]

lld/COFF/Driver.cpp

1014–1015

My idea was to make /incremental be on and off according to the same rules MSVC follows, so that we get warnings in the same circumstances link.exe emits them. In particular, if you don't default to /opt:noref when /incremental is specified, then specifying /incremental but not /opt:noref will result in a warning, or an error if /wx is in effect.

A secondary reason is that making the interaction between /debug, /incremental, /opt:icf, and /opt:ref work the same way as it does on link.exe means we won't have to change the defaults if we implement real incremental linking - assuming that we actually want the defaults to mimic MSVC's.

If you don't think we need the specific behavior I implemented I'll be happy to simplify the code - I just wanted to make sure I pointed out why I did it this way.

lld/COFF/Options.td

107–108

True. Would you be happier with something like "Keep original import library if contents haven't changed" and "Replace import library file even if contents are unchanged"? Should I give the option a different name?

[ruiu]

lld/COFF/Driver.cpp

1014–1015

Sure. I understand that this is for compatibility, but still I wouldn't do this at the moment because in general we don't implement until that is required and I'd like to put priority on simplicity.

lld/COFF/Options.td

107–108

Either help message is fine to me, but we shouldn't change the option name.

[inglorion]

simpler change based on @ruiu's comments

[inglorion]

clang-format

[ruiu]

LGTM

lld/COFF/Config.h

struct Configuration {
   bool NxCompat = true;
   bool AllowIsolation = true;
   bool TerminalServerAware = true;
   bool LargeAddressAware = false;
   bool HighEntropyVA = false;
   bool AppContainer = false;
   bool MinGW = false;
   bool WarnLocallyDefinedImported = true;
+  bool Incremental = true;
 };
 
 extern Configuration *Config;
 
 } // namespace coff
 } // namespace lld
 
 #endif

lld/COFF/Driver.cpp

static void createImportLibrary(bool AsLib) {
 
   auto HandleError = [](Error &&E) {
     handleAllErrors(std::move(E),
                     [](ErrorInfoBase &EIB) { error(EIB.message()); });
   };
   std::string LibName = getImportName(AsLib);
   std::string Path = getImplibPath();
 
+  if (!Config->Incremental) {
+    HandleError(writeImportLibrary(LibName, Path, Exports, Config->Machine,
+                                   false, Config->MinGW));
+    return;
+  }
+
   // 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;
   }
Error E =
                          /*Deterministics=*/true,
                          /*Thin=*/false);
   handleAllErrors(std::move(E), [&](const ErrorInfoBase &EI) {
     error("failed to create a new archive " + S.str() + ": " + EI.message());
   });
   return Temp;
 }
 
 // Create response file contents and invoke the MSVC linker.
 void LinkerDriver::invokeMSVC(opt::InputArgList &Args) {
   std::string Rsp = "/nologo\n";
   std::vector<std::string> Temps;
 
   // Write out archive members that we used in symbol resolution and pass these
   // to MSVC before any archives, so that MSVC uses the same objects to satisfy
   // references.
   for (ObjFile *Obj : ObjFile::Instances) {
     if (Obj->ParentName.empty())
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
 
   // Handle /force or /force:unresolved
   if (Args.hasArg(OPT_force, OPT_force_unresolved))
     Config->Force = true;
 
   // Handle /debug
   if (Args.hasArg(OPT_debug, OPT_debug_dwarf, OPT_debug_ghash)) {
     Config->Debug = true;
+    Config->Incremental = true;
     if (auto *Arg = Args.getLastArg(OPT_debugtype))
       Config->DebugTypes = parseDebugType(Arg->getValue());
     else
       Config->DebugTypes = getDefaultDebugType(Args);
   }
 
   // Handle /pdb
   bool ShouldCreatePDB = Args.hasArg(OPT_debug, OPT_debug_ghash);
for (auto *Arg : Args.filtered(OPT_incl))
     addUndefined(Arg->getValue());
 
   // Handle /implib
   if (auto *Arg = Args.getLastArg(OPT_implib))
     Config->Implib = Arg->getValue();
 
   // Handle /opt.
   bool DoGC = !Args.hasArg(OPT_debug);
   unsigned ICFLevel = 1; // 0: off, 1: limited, 2: on
   for (auto *Arg : Args.filtered(OPT_opt)) {
     std::string Str = StringRef(Arg->getValue()).lower();
     SmallVector<StringRef, 1> Vec;
     StringRef(Str).split(Vec, ',');
     for (StringRef S : Vec) {
       if (S == "ref") {
         DoGC = true;
       } else if (S == "noref") {
         DoGC = false;
if (!Config->ManifestInput.empty() &&
       Config->Manifest != Configuration::Embed) {
     fatal("/MANIFESTINPUT: requires /MANIFEST:EMBED");
   }
 
   // Handle miscellaneous boolean flags.
   Config->AllowBind = Args.hasFlag(OPT_allowbind, OPT_allowbind_no, true);
   Config->AllowIsolation =
       Args.hasFlag(OPT_allowisolation, OPT_allowisolation_no, true);
+  Config->Incremental = Args.hasFlag(OPT_incremental, OPT_incremental_no,
+                                     !Config->DoGC && !Config->DoICF &&
+                                     !Args.hasArg(OPT_order));
   Config->NxCompat = Args.hasFlag(OPT_nxcompat, OPT_nxcompat_no, true);
   Config->TerminalServerAware = Args.hasFlag(OPT_tsaware, OPT_tsaware_no, true);
   Config->DebugDwarf = Args.hasArg(OPT_debug_dwarf);
   Config->DebugGHashes = Args.hasArg(OPT_debug_ghash);
 
   Config->MapFile = getMapFile(Args);
 
+  if (Config->Incremental && Config->DoGC) {
+    warn("ignoring '/INCREMENTAL' because REF is enabled; use '/OPT:NOREF' to disable");
+    Config->Incremental = false;
+  }
+
+  if (Config->Incremental && Config->DoICF) {
+    warn("ignoring '/INCREMENTAL' because ICF is enabled; use '/OPT:NOICF' to disable");
+    Config->Incremental = false;
+  }
+
+  if (Config->Incremental && Args.hasArg(OPT_order)) {
+    warn("ignoring '/INCREMENTAL' due to '/ORDER' specification");
+    Config->Incremental = false;
+  }
+
   if (errorCount())
     return;
 
   bool WholeArchiveFlag = Args.hasArg(OPT_wholearchive_flag);
   // Create a list of input files. Files can be given as arguments
   // for /defaultlib option.
   std::vector<MemoryBufferRef> MBs;
   for (auto *Arg : Args.filtered(OPT_INPUT, OPT_wholearchive_file)) {

lld/COFF/Options.td

defm appcontainer : B<"appcontainer",
                       "Image can run outside an app container (default)">;
 defm dynamicbase : B<"dynamicbase", "Enable ASLR (default unless /fixed)",
                      "Disable ASLR (default when /fixed)">;
 defm fixed : B<"fixed", "Disable base relocations",
                "Enable base relocations (default)">;
 defm highentropyva : B<"highentropyva",
                        "Enable 64-bit ASLR (default on 64-bit)",
                        "Disable 64-bit ASLR">;
+defm incremental : B<"incremental",
+                     "Keep original import library if contents are unchanged",
+                     "Replace import library file even if contents are unchanged">;
 defm largeaddressaware : B<"largeaddressaware",
                            "Enable large addresses (default on 64-bit)",
                            "Disable large addresses (default on 32-bit)">;
 defm nxcompat : B<"nxcompat", "Enable data execution prevention (default)",
                   "Disable data execution provention">;
 defm safeseh : B<"safeseh",
                  "Produce an image with Safe Exception Handler (only for x86)",
                  "Don't produce an image with Safe Exception Handler">;

 
 multiclass QB<string name> {
   def "" : F<name>;
   def _no : F<name#":no">;
 }
 
 def functionpadmin : F<"functionpadmin">;
 def ignoreidl : F<"ignoreidl">;
-def incremental : F<"incremental">;
-def no_incremental : F<"incremental:no">;
 def nologo : F<"nologo">;
 def throwingnew : F<"throwingnew">;
 def editandcontinue : F<"editandcontinue">;
 def fastfail : F<"fastfail">;
 
 def delay : QF<"delay">;
 def errorreport : QF<"errorreport">;
 def idlout : QF<"idlout">;
 def maxilksize : QF<"maxilksize">;
 def natvis : QF<"natvis">;
 def pdbaltpath : QF<"pdbaltpath">;
 def tlbid : QF<"tlbid">;
 def tlbout : QF<"tlbout">;
 def verbose_all : QF<"verbose">;
 def guardsym : QF<"guardsym">;

lld/test/COFF/incremental.test

+# RUN: yaml2obj < %p/Inputs/export.yaml > %t.obj
+# RUN: lld-link -out:%t.dll -dll %t.obj 2>&1 \
+# RUN:   | FileCheck -allow-empty -check-prefix=NOWARN %s
+# RUN: touch -t 198002011200.00 %t.lib
+# RUN: lld-link -out:%t.dll -dll %t.obj
+# RUN: ls -l %t.lib | FileCheck -check-prefix=NOKEEP %s
+
+# RUN: lld-link -out:%t.dll -dll -incremental %t.obj 2>&1 \
+# RUN:   | FileCheck -allow-empty -check-prefix=WARN-REF %s
+# RUN: touch -t 198002011200.00 %t.lib
+# RUN: lld-link -out:%t.dll -dll -incremental %t.obj
+# RUN: ls -l %t.lib | FileCheck -check-prefix=NOKEEP %s
+
+# RUN: lld-link -out:%t.dll -dll -debug %t.obj 2>&1 \
+# RUN:   | FileCheck -allow-empty -check-prefix=NOWARN %s
+# RUN: touch -t 198002011200.00 %t.lib
+# RUN: lld-link -out:%t.dll -dll -debug %t.obj
+# RUN: ls -l %t.lib | FileCheck -check-prefix=KEEP %s
+
+# RUN: lld-link -out:%t.dll -dll -debug -incremental:no %t.obj 2>&1 \
+# RUN:   | FileCheck -allow-empty -check-prefix=NOWARN %s
+# RUN: touch -t 198002011200.00 %t.lib
+# RUN: lld-link -out:%t.dll -dll -debug -incremental:no %t.obj
+# RUN: ls -l %t.lib | FileCheck -check-prefix=NOKEEP %s
+
+# RUN: lld-link -out:%t.dll -dll -opt:icf %t.obj 2>&1 \
+# RUN:   | FileCheck -allow-empty -check-prefix=NOWARN %s
+# RUN: touch -t 198002011200.00 %t.lib
+# RUN: lld-link -out:%t.dll -dll -opt:icf %t.obj
+# RUN: ls -l %t.lib | FileCheck -check-prefix=NOKEEP %s
+
+# RUN: lld-link -out:%t.dll -dll -incremental -opt:noref,icf %t.obj 2>&1 \
+# RUN:   | FileCheck -check-prefix=WARN-ICF %s
+# RUN: touch -t 198002011200.00 %t.lib
+# RUN: lld-link -out:%t.dll -dll -incremental -opt:noref,icf %t.obj
+# RUN: ls -l %t.lib | FileCheck -check-prefix=NOKEEP %s
+
+# RUN: lld-link -out:%t.dll -dll -debug -opt:icf %t.obj 2>&1 \
+# RUN:   | FileCheck -allow-empty -check-prefix=NOWARN %s
+# RUN: touch -t 198002011200.00 %t.lib
+# RUN: lld-link -out:%t.dll -dll -debug -opt:icf %t.obj
+# RUN: ls -l %t.lib | FileCheck -check-prefix=NOKEEP %s
+
+# RUN: lld-link -out:%t.dll -dll -opt:ref %t.obj 2>&1 \
+# RUN:   | FileCheck -allow-empty -check-prefix=NOWARN %s
+# RUN: touch -t 198002011200.00 %t.lib
+# RUN: lld-link -out:%t.dll -dll -opt:ref %t.obj
+# RUN: ls -l %t.lib | FileCheck -check-prefix=NOKEEP %s
+
+# RUN: lld-link -out:%t.dll -dll -incremental -opt:ref %t.obj 2>&1 \
+# RUN:   | FileCheck -check-prefix=WARN-REF %s
+# RUN: touch -t 198002011200.00 %t.lib
+# RUN: lld-link -out:%t.dll -dll -incremental -opt:ref %t.obj
+# RUN: ls -l %t.lib | FileCheck -check-prefix=NOKEEP %s
+
+# RUN: lld-link -out:%t.dll -dll -debug -opt:ref %t.obj 2>&1 \
+# RUN:   | FileCheck -allow-empty -check-prefix=NOWARN %s
+# RUN: touch -t 198002011200.00 %t.lib
+# RUN: lld-link -out:%t.dll -dll -debug -opt:ref %t.obj
+# RUN: ls -l %t.lib | FileCheck -check-prefix=NOKEEP %s
+
+# NOWARN-NOT: ignoring '/INCREMENTAL'
+# WARN-ICF: ignoring '/INCREMENTAL' because ICF is enabled; use '/OPT:NOICF' to disable
+# WARN-REF: ignoring '/INCREMENTAL' because REF is enabled; use '/OPT:NOREF' to disable
+# KEEP: {{Feb 1 1980|1980-02-01}}
+# NOKEEP-NOT: {{Feb 1 1980|1980-02-01}}

lld/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|1980-02-01}}