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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
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
| lld/COFF/Driver.cpp | ||
|---|---|---|
| 697–698 ↗ | (On Diff #132069) | 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. |
| 1036–1046 ↗ | (On Diff #132069) | 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 ↗ | (On Diff #132069) | This description is misleading as we don't actually do incremental linking. |
| lld/COFF/Driver.cpp | ||
|---|---|---|
| 1036–1046 ↗ | (On Diff #132069) | 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 ↗ | (On Diff #132069) | 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? |
| lld/COFF/Driver.cpp | ||
|---|---|---|
| 1036–1046 ↗ | (On Diff #132069) | 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 ↗ | (On Diff #132069) | Either help message is fine to me, but we shouldn't change the option name. |