[PATCH] D42716: [COFF] make /incremental control overwriting unchanged import libraries

Bob Haarman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 31 11:42:41 PST 2018


inglorion added inline comments.


================
Comment at: lld/COFF/Driver.cpp:1036-1046
+  // There is a complex interaction with /incremental. Incremental linking is
+  // incompatible with /opt:icf, and /opt:ref. If you specify both /incremental
+  // and one of those options, you get a warning and incremental linking is
+  // turned off. If you don't explicitly specify /incremental, it is enabled by
+  // default, but turned off without a warning if one of the incompatible
+  // options is specified. Conversely, if you specify /incremental, the defaults
+  // for /opt:icf and /opt:ref are changed to be compatible with it.
----------------
ruiu wrote:
> 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?
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.


================
Comment at: lld/COFF/Options.td:107-108
+defm incremental : B<"incremental",
+                     "Link incrementally",
+                     "Perform a full link">;
 defm largeaddressaware : B<"largeaddressaware",
----------------
ruiu wrote:
> This description is misleading as we don't actually do incremental linking.
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? 


https://reviews.llvm.org/D42716





More information about the llvm-commits mailing list