This is an archive of the discontinued LLVM Phabricator instance.

COFF ICF: Merge only functions. Do not merge read-only data.
ClosedPublic

Authored by ruiu on Feb 24 2017, 4:32 PM.

Details

Summary

This seems to be the behavior of the MSVC linker. Previously, this
incompatibility caused nasty issues in chromium build a few times.

Event Timeline

ruiu created this revision.Feb 24 2017, 4:32 PM
ruiu edited the summary of this revision. (Show Details)Feb 24 2017, 4:37 PM

This causes us to eliminate less duplication, right? Is folding of identical non-functions something we may want to re-enable at a later point? Should we make the behavior controllable by a flag - or perhaps only implement the MSVC-compatible behavior when using msvclto?

pcc edited edge metadata.Feb 27 2017, 12:48 PM

This causes us to eliminate less duplication, right? Is folding of identical non-functions something we may want to re-enable at a later point? Should we make the behavior controllable by a flag - or perhaps only implement the MSVC-compatible behavior when using msvclto?

I think we will want to do something very similar to what Rafael proposed for D30365, i.e. we would want to extend the COFF format with an address-taken flag. I think it can be done in a way that would not interfere with any extensions that Microsoft might make to the format.

As a side note, I think this would also be useful for emitting smaller bitmaps for Control Flow Guard, as we would only need to add address-taken functions to the bitmap.

MSVC won't merge read only data COMDATs? Microsoft's documentation has the following:

When /OPT:REF is enabled either explicitly or by default, a limited form of /OPT:ICF is enabled that only folds identical functions. If you want /OPT:REF but not /OPT:ICF, you must specify either /OPT:REF,NOICF or /OPT:NOICF.

The linker behaves differently when /OPT:REF is specified—and ICF is in effect by default—than it does when /OPT:REF,ICF is specified explicitly. The form of ICF that's enabled with /OPT:REF alone does not fold read-only data—this includes .rdata, .pdata, and .xdata. Therefore, fewer functions are folded when images are produced for x64 because functions in these modules are more dependent on read-only data—for example, .pdata and .xdata. To get full ICF folding behavior, explicitly specify /OPT:ICF.

It sounds like Microsoft's default behavior is /OPT:REF. With that flag, they will fold identical functions. However, they provide access to full folding using /OPT:REF,ICF

Perhaps we should provide the functionality via a similar flag?

pcc added a subscriber: rnk.Feb 27 2017, 1:50 PM

MSVC won't merge read only data COMDATs? Microsoft's documentation has the following:

When /OPT:REF is enabled either explicitly or by default, a limited form of /OPT:ICF is enabled that only folds identical functions. If you want /OPT:REF but not /OPT:ICF, you must specify either /OPT:REF,NOICF or /OPT:NOICF.

The linker behaves differently when /OPT:REF is specified—and ICF is in effect by default—than it does when /OPT:REF,ICF is specified explicitly. The form of ICF that's enabled with /OPT:REF alone does not fold read-only data—this includes .rdata, .pdata, and .xdata. Therefore, fewer functions are folded when images are produced for x64 because functions in these modules are more dependent on read-only data—for example, .pdata and .xdata. To get full ICF folding behavior, explicitly specify /OPT:ICF.

It sounds like Microsoft's default behavior is /OPT:REF. With that flag, they will fold identical functions. However, they provide access to full folding using /OPT:REF,ICF

Perhaps we should provide the functionality via a similar flag?

Maybe, but I'd slightly prefer to encode the information in the object files rather than introducing another flag.

Although it looks like Chromium Windows builds are already using /OPT:REF and /OPT:ICF:
https://cs.chromium.org/chromium/src/build/config/win/BUILD.gn?type=cs&q=%22opt:icf%22&sq=package:chromium&l=102
(but only when building with Clang or fastlink because /PROFILE implies /OPT:NOICF contrary to what the comment says (!))

So now I'm confused as to how we were running into problems before.

In D30363#687794, @pcc wrote:

MSVC won't merge read only data COMDATs? Microsoft's documentation has the following:

When /OPT:REF is enabled either explicitly or by default, a limited form of /OPT:ICF is enabled that only folds identical functions. If you want /OPT:REF but not /OPT:ICF, you must specify either /OPT:REF,NOICF or /OPT:NOICF.

The linker behaves differently when /OPT:REF is specified—and ICF is in effect by default—than it does when /OPT:REF,ICF is specified explicitly. The form of ICF that's enabled with /OPT:REF alone does not fold read-only data—this includes .rdata, .pdata, and .xdata. Therefore, fewer functions are folded when images are produced for x64 because functions in these modules are more dependent on read-only data—for example, .pdata and .xdata. To get full ICF folding behavior, explicitly specify /OPT:ICF.

It sounds like Microsoft's default behavior is /OPT:REF. With that flag, they will fold identical functions. However, they provide access to full folding using /OPT:REF,ICF

Perhaps we should provide the functionality via a similar flag?

Maybe, but I'd slightly prefer to encode the information in the object files rather than introducing another flag.

It'd be nice if we can swap out clang with MSVC and still get the /OPT:ICF semantics (when desired). Making this feature available _only_ via the object file format would forbid that.

ruiu added a comment.Feb 27 2017, 2:12 PM

Maybe, but I'd slightly prefer to encode the information in the object files rather than introducing another flag.

Although it looks like Chromium Windows builds are already using /OPT:REF and /OPT:ICF:
https://cs.chromium.org/chromium/src/build/config/win/BUILD.gn?type=cs&q=%22opt:icf%22&sq=package:chromium&l=102
(but only when building with Clang or fastlink because /PROFILE implies /OPT:NOICF contrary to what the comment says (!))

So now I'm confused as to how we were running into problems before.

Me too. If the MSVC linker behaves as described by the text that David pasted, the LLD'd behavior is not new to Chrome, so it shouldn't have caused an issue.

pcc added a comment.Feb 27 2017, 2:15 PM
In D30363#687836, @ruiu wrote:

Maybe, but I'd slightly prefer to encode the information in the object files rather than introducing another flag.

Although it looks like Chromium Windows builds are already using /OPT:REF and /OPT:ICF:
https://cs.chromium.org/chromium/src/build/config/win/BUILD.gn?type=cs&q=%22opt:icf%22&sq=package:chromium&l=102
(but only when building with Clang or fastlink because /PROFILE implies /OPT:NOICF contrary to what the comment says (!))

So now I'm confused as to how we were running into problems before.

Me too. If the MSVC linker behaves as described by the text that David pasted, the LLD'd behavior is not new to Chrome, so it shouldn't have caused an issue.

Reid, I think you mentioned that you saw this problem only with LLD, do you have any more details?

rnk added a comment.Mar 1 2017, 10:26 AM
In D30363#687837, @pcc wrote:

Reid, I think you mentioned that you saw this problem only with LLD, do you have any more details?

The one instance that I remember was http://crbug.com/635943. I think there was one other issue much earlier on that didn't require a restructuring.

pcc added a comment.Mar 1 2017, 3:40 PM
In D30363#689809, @rnk wrote:
In D30363#687837, @pcc wrote:

Reid, I think you mentioned that you saw this problem only with LLD, do you have any more details?

The one instance that I remember was http://crbug.com/635943. I think there was one other issue much earlier on that didn't require a restructuring.

Standalone reproducer:

C:\src\icftest>type icftest.cc
#include <stdio.h>

extern const wchar_t foo[];
extern const wchar_t bar[];

const wchar_t foo[] = L"";
const wchar_t bar[] = L"";

int main() {
  printf("%p %p\n", &foo, &bar);
}

C:\src\icftest>cl /Gw /O2 /c icftest.cc
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

icftest.cc

C:\src\icftest>link icftest.obj /opt:icf
Microsoft (R) Incremental Linker Version 14.00.24215.1
Copyright (C) Microsoft Corporation.  All rights reserved.


C:\src\icftest>icftest
000000013F2C12C0 000000013F2C12C4

C:\src\icftest>..\llvm-project\ra\bin\lld-link icftest.obj /opt:icf

C:\src\icftest>icftest
000000013FC02088 000000013FC02088

So it does appear that MSVC's behaviour is not as documented at least some of the time. In that case I think I'd be fine with this change with an explanatory comment.

ruiu updated this revision to Diff 97015.Apr 27 2017, 4:11 PM
  • Add a comment.
pcc accepted this revision.Apr 27 2017, 4:14 PM

LGTM

This revision is now accepted and ready to land.Apr 27 2017, 4:14 PM
This revision was automatically updated to reflect the committed changes.