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

[ruiu]

Summary

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

[inglorion]

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]

In D30363#687625, @inglorion wrote:

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.

[majnemer]

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]

In D30363#687719, @majnemer 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.

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.

[majnemer]

In D30363#687794, @pcc wrote:

In D30363#687719, @majnemer 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]

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]

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]

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]

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]

• Add a comment.

[pcc]

LGTM

lld/COFF/ICF.cpp

return hash_combine(C->getPermissions(),
                       hash_value(C->SectionName),
                       C->NumRelocs,
                       C->getAlign(),
                       uint32_t(C->Header->SizeOfRawData),
                       C->Checksum);
 }
 
 // Returns true if section S is subject of ICF.
+// Essentially, we want to merge only functions.
 bool ICF::isEligible(SectionChunk *C) {
   bool Global = C->Sym && C->Sym->isExternal();
+  bool Executable = C->getPermissions() & llvm::COFF::IMAGE_SCN_MEM_EXECUTE;
   bool Writable = C->getPermissions() & llvm::COFF::IMAGE_SCN_MEM_WRITE;
-  return C->isCOMDAT() && C->isLive() && Global && !Writable;
+  return C->isCOMDAT() && C->isLive() && Global && Executable && !Writable;
 }
 
 // Split a range into smaller ranges by recoloring sections
 void ICF::segregate(size_t Begin, size_t End, bool Constant) {
   while (Begin < End) {
     // Divide [Begin, End) into two. Let Mid be the start index of the
     // second group.
     auto Bound = std::stable_partition(

lld/test/COFF/icf-data.test

+# RUN: yaml2obj < %s > %t.obj
+# RUN: lld-link /entry:foo /out:%t.exe /subsystem:console /include:bar \
+# RUN:   /verbose %t.obj > %t.log 2>&1
+# RUN: FileCheck %s < %t.log
+
+# CHECK-NOT: Removed foo
+# CHECK-NOT: Removed bar
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: []
+sections:
+  - Name:            '.text$mn'
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     4883EC28E8000000004883C428C3
+  - Name:            '.text$mn'
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     4883EC28E8000000004883C428C3
+symbols:
+  - Name:            '.text$mn'
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          14
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        1682752513
+      Number:          0
+      Selection:       IMAGE_COMDAT_SELECT_NODUPLICATES
+  - Name:            '.text$mn'
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          14
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        1682752513
+      Number:          0
+      Selection:       IMAGE_COMDAT_SELECT_NODUPLICATES
+  - Name:            foo
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            bar
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...