This is an archive of the discontinued LLVM Phabricator instance.

Disable GC and ICF when /debug is present
ClosedPublic

Authored by rnk on Nov 9 2017, 6:23 PM.

Details

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.

Event Timeline

rnk created this revision.Nov 9 2017, 6:23 PM
pcc added a subscriber: pcc.Nov 9 2017, 7:35 PM

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 added a comment.Nov 10 2017, 10:59 AM
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 retitled this revision from Don't enable ICF by default to Disable GC and ICF when /debug is present.Nov 10 2017, 11:04 AM
rnk edited the summary of this revision. (Show Details)
rnk added a reviewer: pcc.
rnk removed a subscriber: pcc.
rnk updated this revision to Diff 122483.Nov 10 2017, 11:05 AM
  • key it on /debug

Seems like a good idea to me, for consistency.

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 edited edge metadata.Nov 10 2017, 12:30 PM

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
907

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 added inline comments.Nov 10 2017, 1:20 PM
lld/COFF/Driver.cpp
907

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

rnk updated this revision to Diff 122515.Nov 10 2017, 1:20 PM
  • make opt:ref imply ICF unless it's explicitly disabled
rnk updated this revision to Diff 122530.Nov 10 2017, 2:17 PM

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

rnk updated this revision to Diff 122535.Nov 10 2017, 2:35 PM

One last attempt at simplification

pcc accepted this revision.Nov 10 2017, 2:50 PM

LGTM, thanks.

lld/COFF/Driver.cpp
900

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

This revision is now accepted and ready to land.Nov 10 2017, 2:50 PM
ruiu accepted this revision.Nov 13 2017, 1:30 AM

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.

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.

This revision was automatically updated to reflect the committed changes.