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/COFF/Config.h

struct Configuration {
   llvm::COFF::MachineTypes Machine = IMAGE_FILE_MACHINE_UNKNOWN;
   bool Verbose = false;
   WindowsSubsystem Subsystem = llvm::COFF::IMAGE_SUBSYSTEM_UNKNOWN;
   Symbol *Entry = nullptr;
   bool NoEntry = false;
   std::string OutputFile;
   std::string ImportName;
   bool DoGC = true;
-  bool DoICF = true;
+  bool DoICF = false;
   bool Relocatable = true;
   bool Force = false;
   bool Debug = false;
   bool WriteSymtab = true;
   unsigned DebugTypes = static_cast<unsigned>(DebugType::None);
   llvm::SmallString<128> PDBPath;
   std::vector<llvm::StringRef> Argv;
 

lld/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/test/COFF/icf-circular.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 %s < %t.log
 
 # CHECK: Selected foo
 # CHECK:   Removed bar
 
 --- !COFF
 header:

lld/test/COFF/icf-circular2.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 %s < %t.log
 
 # CHECK: Selected foo
 # CHECK:   Removed bar
 
 --- !COFF
 header:

lld/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