Disable GC and ICF when /debug is present

[rnk]

Summary

ICF and GC impair debugging, so MSVC disables these optimizations when
/debug is passed. They are still on by default when no PDB is produced.

[pcc]

somewhat expensive

Do you have numbers?

hurts debuggability

According to https://msdn.microsoft.com/en-us/library/bxwfs976.aspx link.exe disables ICF by default when linking with /debug. We could do the same.

technically non-conforming.

True, but from a practical perspective, programs that have been linked with link.exe are unlikely to break as a result of the default.

disabling /OPT:REF by default

We might actually consider doing that in /debug builds because it should provide a better debugging experience.

[rnk]

In D39885#921452, @pcc wrote:

somewhat expensive

Do you have numbers?

For chrome's content.dll:
default
real 0m42.833s
real 0m39.829s
real 0m40.380s
real 0m40.008s
real 0m39.462s
/OPT:NOICF
real 0m37.458s
real 0m39.222s
real 0m38.813s
real 0m38.316s
real 0m38.276s
/OPT:NOREF (implies noicf)
real 0m39.924s
real 0m39.251s
real 0m40.284s
real 0m42.717s
real 0m40.270s

hurts debuggability

According to https://msdn.microsoft.com/en-us/library/bxwfs976.aspx link.exe disables ICF by default when linking with /debug. We could do the same.

Yeah, let's do what they do.

technically non-conforming.

True, but from a practical perspective, programs that have been linked with link.exe are unlikely to break as a result of the default.

disabling /OPT:REF by default

We might actually consider doing that in /debug builds because it should provide a better debugging experience.

I suppose users might be confused if they can't find something unreferenced in their binary. I hadn't considered that.

[rnk]

• key it on /debug

[zturner]

Seems like a good idea to me, for consistency.

[mstorsjo]

For the MinGW frontend, we decided to pass -debug:dwarf by default, unless the caller passed -s. Doesn't this change mean that a user that links without -s and then manually strips the binary later would end up with a larger and less optimized binary than if you would have linked it with -s in the first place?

[pcc]

In D39885#922173, @mstorsjo wrote:

For the MinGW frontend, we decided to pass -debug:dwarf by default, unless the caller passed -s. Doesn't this change mean that a user that links without -s and then manually strips the binary later would end up with a larger and less optimized binary than if you would have linked it with -s in the first place?

I think that for mingw we will want to explicitly pass either /opt:noref or /opt:ref depending on whether the user passed --gc-sections. The same applies to /opt:icf and --icf=all.

lld/COFF/Driver.cpp

915 ↗

(On Diff #122483)

I don't think this is right. It will mean that /debug /opt:ref will not enable ICF, which doesn't appear to be consistent with what link.exe does.

[rnk]

lld/COFF/Driver.cpp

915 ↗

(On Diff #122483)

I have a fix, but we have to make this not do ICF: link /debug /opt:noicf,ref

[rnk]

• make opt:ref imply ICF unless it's explicitly disabled

[rnk]

Rewrite one more time with fewer bools. The tricky case is /opt:noicf,ref.

[rnk]

One last attempt at simplification

[pcc]

LGTM, thanks.

lld/COFF/Driver.cpp

900 ↗

(On Diff #122535)

Nit: I don't think you need this local variable, you could use Config->DoGC directly.

[ruiu]

LGTM

Martin, if the new behavior is not desirable for MinGW, please pass flags from the MinGW driver to the COFF driver so that you'll get desired results, as pcc suggested.

[mstorsjo]

In D39885#923026, @ruiu wrote:

Martin, if the new behavior is not desirable for MinGW, please pass flags from the MinGW driver to the COFF driver so that you'll get desired results, as pcc suggested.

Yep, I'll look into it eventually.

lld/trunk/COFF/Driver.cpp

void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   // Handle /include
   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
+  // 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 == "noref") {
-        Config->DoGC = false;
-        Config->DoICF = false;
-        continue;
-      }
-      if (S == "icf" || S.startswith("icf=")) {
-        Config->DoICF = true;
-        continue;
-      }
-      if (S == "noicf") {
-        Config->DoICF = false;
-        continue;
-      }
-      if (S.startswith("lldlto=")) {
+      if (S == "ref") {
+        DoGC = true;
+      } else if (S == "noref") {
+        DoGC = false;
+      } else if (S == "icf" || S.startswith("icf=")) {
+        ICFLevel = 2;
+      } else if (S == "noicf") {
+        ICFLevel = 0;
+      } else if (S.startswith("lldlto=")) {
         StringRef OptLevel = S.substr(7);
         if (OptLevel.getAsInteger(10, Config->LTOOptLevel) ||
             Config->LTOOptLevel > 3)
           error("/opt:lldlto: invalid optimization level: " + OptLevel);
-        continue;
-      }
-      if (S.startswith("lldltojobs=")) {
+      } else if (S.startswith("lldltojobs=")) {
         StringRef Jobs = S.substr(11);
         if (Jobs.getAsInteger(10, Config->LTOJobs) || Config->LTOJobs == 0)
           error("/opt:lldltojobs: invalid job count: " + Jobs);
-        continue;
-      }
-      if (S.startswith("lldltopartitions=")) {
+      } else if (S.startswith("lldltopartitions=")) {
         StringRef N = S.substr(17);
         if (N.getAsInteger(10, Config->LTOPartitions) ||
             Config->LTOPartitions == 0)
           error("/opt:lldltopartitions: invalid partition count: " + N);
-        continue;
-      }
-      if (S != "ref" && S != "lbr" && S != "nolbr")
+      } else if (S != "lbr" && S != "nolbr")
         error("/opt: unknown option: " + S);
     }
   }
 
+  // Limited ICF is enabled if GC is enabled and ICF was never mentioned
+  // explicitly.
+  // FIXME: LLD only implements "limited" ICF, i.e. it only merges identical
+  // code. If the user passes /OPT:ICF explicitly, LLD should merge identical
+  // comdat readonly data.
+  if (ICFLevel == 1 && !DoGC)
+    ICFLevel = 0;
+  Config->DoGC = DoGC;
+  Config->DoICF = ICFLevel > 0;
+
   // Handle /lldsavetemps
   if (Args.hasArg(OPT_lldsavetemps))
     Config->SaveTemps = true;
 
   // Handle /lldltocache
   if (auto *Arg = Args.getLastArg(OPT_lldltocache))
     Config->LTOCache = Arg->getValue();
 

lld/trunk/test/COFF/icf-associative.test

 # RUN: yaml2obj < %s > %t.obj
-# RUN: lld-link /entry:foo /out:%t.exe /subsystem:console /include:bar \
+# RUN: lld-link /opt:icf /entry:foo /out:%t.exe /subsystem:console /include:bar \
 # RUN:   /debug /verbose %t.obj > %t.log 2>&1
 # RUN: FileCheck %s < %t.log
 
 # CHECK: Selected foo
 # CHECK:   Removed bar
 
 --- !COFF
 header:

lld/trunk/test/COFF/icf-simple.test

 # RUN: yaml2obj < %s > %t.obj
-# RUN: lld-link /entry:foo /out:%t.exe /subsystem:console /include:bar \
+# RUN: lld-link /opt:icf /entry:foo /out:%t.exe /subsystem:console /include:bar \
 # RUN:   /verbose %t.obj > %t.log 2>&1
 # RUN: FileCheck -check-prefix=ICF %s < %t.log
 
 # ICF: Selected foo
 # ICF:   Removed bar
 
 # RUN: lld-link /entry:foo /out:%t.exe /subsystem:console /include:bar \
 # RUN:   /verbose /opt:noicf %t.obj > %t.log 2>&1
 # RUN: FileCheck -check-prefix=NOICF %s < %t.log
 # RUN: lld-link /entry:foo /out:%t.exe /subsystem:console /include:bar \
 # RUN:   /verbose /opt:noref,noicf %t.obj > %t.log 2>&1
 # RUN: FileCheck -check-prefix=NOICF %s < %t.log
 
+# ICF is on by default (no /opt: flags).
+# RUN: lld-link /entry:foo /out:%t.exe /subsystem:console \
+# RUN:   /include:bar /verbose %t.obj > %t.log 2>&1
+# RUN: FileCheck -check-prefix=ICF %s < %t.log
+
+# /debug disables ICF.
+# RUN: lld-link /debug /entry:foo /out:%t.exe /subsystem:console \
+# RUN:   /include:bar /verbose %t.obj > %t.log 2>&1
+# RUN: FileCheck -check-prefix=NOICF %s < %t.log
+
+# /opt:noref disables ICF.
+# RUN: lld-link /opt:noref /entry:foo /out:%t.exe /subsystem:console \
+# RUN:   /include:bar /verbose %t.obj > %t.log 2>&1
+# RUN: FileCheck -check-prefix=NOICF %s < %t.log
+
+# /debug /opt:ref enables ICF.
+# RUN: lld-link /debug /opt:ref /entry:foo /out:%t.exe /subsystem:console \
+# RUN:   /include:bar /verbose %t.obj > %t.log 2>&1
+# RUN: FileCheck -check-prefix=ICF %s < %t.log
+
+# /debug /opt:noicf,ref disables ICF.
+# RUN: lld-link /debug /opt:noicf,ref /entry:foo /out:%t.exe /subsystem:console \
+# RUN:   /include:bar /verbose %t.obj > %t.log 2>&1
+# RUN: FileCheck -check-prefix=NOICF %s < %t.log
+
 # NOICF-NOT: Removed foo
 # NOICF-NOT: Removed bar
 
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_AMD64
   Characteristics: []
 sections:

lld/trunk/test/COFF/pdb-global-gc.yaml

 # RUN: yaml2obj %s -o %t.obj
 # RUN: llvm-mc %S/Inputs/pdb-global-gc.s -triple x86_64-windows-msvc -filetype=obj -o %t2.obj
 # RUN: lld-link %t.obj %t2.obj -debug -entry:main \
-# RUN:          -nodefaultlib -debug -out:%t.exe -pdb:%t.pdb -verbose
+# RUN:          -nodefaultlib -opt:ref -out:%t.exe -pdb:%t.pdb -verbose
 # RUN: llvm-pdbutil dump -symbols -globals %t.pdb | FileCheck %s
 
 # This tests the case where an __imp_ chunk is discarded by linker GC. The debug
 # info may refer to the __imp_ symbol still.
 
 # Compile this code with MSVC to regenerate the test case:
 #   extern char __declspec(dllimport) __wc_mb_cur;
 #   int discarded() { return __wc_mb_cur; }

lld/trunk/test/COFF/pdb-import-gc.yaml

 # RUN: yaml2obj %s -o %t.obj
 # RUN: lld-link %t.obj %S/Inputs/pdb-import-gc.lib -debug -entry:main \
-# RUN:          -nodefaultlib -debug -out:%t.exe -pdb:%t.pdb
+# RUN:          -nodefaultlib -opt:ref -out:%t.exe -pdb:%t.pdb
 # RUN: llvm-pdbutil dump -globals -symbols %t.pdb | FileCheck %s
 
 # This tests the case where an __imp_ chunk is discarded by linker GC. The debug
 # info may refer to the __imp_ symbol still.
 
 # Compile this code with MSVC to regenerate the test case:
 #   extern char __declspec(dllimport) __wc_mb_cur;
 #   int discarded() { return __wc_mb_cur; }