[lld-link] Add safe icf mode to lld-link, which does safe icf for all sections.

[zequanwu]

Summary

lld-link's /opt:safeicf will do safe icf for all sections.

[MaskRay]

Code looks good. I don't have suggestion whether a new mode should be added.

[rnk]

We had a user request to bring the "safe" and "all" ICF modes into alignment between the ports: https://crbug.com/1087923

I think LLD's /OPT:ICF behavior should match MSVC, which doesn't give you safe ICF, it should give you real ICF. So, I guess the thing to do is add a safeicf mode (/OPT:SAFEICF?), and then update these tests to use it.

LLD's current behavior is a bit emergent: we do safe ICF for non-executable data. That's pretty useful. Very few programs rely on function pointer address significance, but many programs do things like taking the address of a global and stuffing it in a hash table. The question is, should we have a mode that preserves this behavior? Fewer modes is better, but if we don't have a behavior preserving mode, Chrome either takes a binary size hit (safe ICF) or is potentially broken by global data ICF.

[zequanwu]

• Add /OPT:safeicf, which will mark all sections as unique if .addrsig doesn't exist. By default, /OPT:icf will do aggressive icf, matching the behavior of MSVC linker's /OPT:icf.
• Update test cases.

[pcc]

As far as I know the current behavior is that /opt:icf gives you full ICF for executable sections and safe ICF for non-executable sections. This is more than what MSVC does, which is full ICF for executable sections and no ICF for non-executable sections. I think it makes sense to add a mode that gives you full ICF for non-executable sections, but the behavior of /opt:icf should be preserved.

[zequanwu]

In D97436#2591761, @pcc wrote:

As far as I know the current behavior is that /opt:icf gives you full ICF for executable sections and safe ICF for non-executable sections. This is more than what MSVC does, which is full ICF for executable sections and no ICF for non-executable sections. I think it makes sense to add a mode that gives you full ICF for non-executable sections, but the behavior of /opt:icf should be preserved.

Initially, we found that the current behavior of /opt:icf merges executable sections even if they are in address-significant table, which obviously is a bug. This patch is trying to fix it and adding a /opt:safeicf to do safe icf for both executable and non-executable sections. The behavior of /opt:icf is preserved.

[pcc]

In D97436#2592053, @zequanwu wrote:

In D97436#2591761, @pcc wrote:

As far as I know the current behavior is that /opt:icf gives you full ICF for executable sections and safe ICF for non-executable sections. This is more than what MSVC does, which is full ICF for executable sections and no ICF for non-executable sections. I think it makes sense to add a mode that gives you full ICF for non-executable sections, but the behavior of /opt:icf should be preserved.

Initially, we found that the current behavior of /opt:icf merges executable sections even if they are in address-significant table, which obviously is a bug.

No it isn't. This matches the behavior of MSVC, which doesn't have a concept of an address significance table.

This patch is trying to fix it and adding a /opt:safeicf to do safe icf for both executable and non-executable sections. The behavior of /opt:icf is preserved.

If you think that /opt:icf should do safe ICF for executable sections then I don't get what you think the difference between /opt:icf and /opt:safeicf should be.

[zequanwu]

Preserve the bahavior of /opt:icf. Using /opt:safeicf to do safe icf for all sections.

[zequanwu]

Update test case.

[rnk]

Some nits.

I'm a bit worried that our end state for Chrome is going to be slightly confusing. In order to allow users to suppress ICF with attributes, this is what we will have to do:

• Add a new mode to clang -faddrsig that stops putting address taken functions in .llvm_addrsig (-faddrsig-data(-only)? -fno-addrsig-code?)
• Add a new attribute to clang that puts a function in .llvm_adddrsig (not sure if this should be called literally __attribute__((addrsig)) or something general like noicf)
• Switch from /OPT:IFC to /OPT:SAFEICF in Chrome

Someone reading the build files might think that we are using safe ICF, but the compiler flag (-fno-addrsig-code or whatever) will mean we are effectively using regular, aggressive ICF. I guess we'll just have to live with that. It's pretty normal for build files to be confusing. =/

lld/COFF/Config.h

86

This isn't precisely MSVC's behavior. LLD will do ICF for code like MSVC, but it will do safe ICF for data using .llvm_addrsig, which MSVC doesn't look at. I'd make the comment more explicit, like: "Aggressive ICF for code, but safe ICF for data, similar to MSVC's behavior"

It seems to me that MSVC will not merge any readonly data sections:

extern "C" int printf(const char *, ...);
extern const int g1;
extern const int g2;
const int g1 = 42;
const int g2 = 42;
int main() {
  printf("g1: %d, g2: %d\n", g1, g2);
  printf("&g1: %p, &g2: %p\n", &g1, &g2);
}

g1 and g2 have distinct addresses:

$ cl -Z7 -Gy -Gw -O1 -c t.cpp -Facl.asm && link t.obj  -OPT:REF,ICF -debug && ./t.exe
Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29336 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

t.cpp
Microsoft (R) Incremental Linker Version 14.28.29336.0
Copyright (C) Microsoft Corporation.  All rights reserved.

g1: 42, g2: 42
&g1: 00007FF77EDD22E0, &g2: 00007FF77EDD22E4

lld/COFF/ICF.cpp

86–89

nit: I feel like this would read better as:

// Under regular (not safe) ICF, all code sections are eligible.
if (icfLevel == ICFLevel::All && ...EXECUTE)
  return true;

I think that's functionally equivalent: under safe ICF we'd fall through to the main return.

[zequanwu]

address comment.

[rnk]

Thanks, I have some more driver code simplification suggestions.

lld/COFF/Driver.cpp

1554–1557

I see, I had forgotten about this limited ICF behavior.

1606–1607

I think this FIXME is wrong, LLD will merge readonly data in both safe and regular ICF modes. In both modes, it will refuse to merge readonly data unless the object file has .llvm_addrsig and the data is not address taken.

Let's make the following changes:

• get rid of the "limited" ICF level 1, it is equivalent to ICFLevel::All
• Use Optional<ICFLevel> as the type for icfLevel
• if (!icfLevel && config->doGC) icfLevel = ICFLevel::All; -- that should make /opt:ref enable ICF
• Delete this FIXME comment and the discussion of limited ICF.

[zequanwu]

lld/COFF/Driver.cpp

1606–1607

This will change the default behavior. Do you meant that? Or I misunderstand.
In current behavior, if doGC is false and /opt:icf is not given (icfLevel ==1), it will set icfLevel to 0, which disable icf:

if (icfLevel == 1 && !doGC)
  icfLevel = 0;

By your changes, if doGC is false and /opt:icf is not given, icfLevel will be ICFLevel::ALL which enables icf.

[rnk]

lld/COFF/Driver.cpp

1606–1607

Yes, sorry, my intent is to use Optional to handle the case where neither /OPT:REF nor /OPT:ICF appear. In that case, icfLevel will be None, and doICF should become ICFLevel::None. Maybe a better way of writing that is:

if (!icfLevel)
  icfLevel = config->doGC ? ICFLevel::All : ICFLevel::None;

[zequanwu]

Address comment.

[rnk]

lgtm

[thakis]

I'm super confused by this change. lld-link used to do aggressive icf for code and safe icf for data. The consensus on https://bugs.chromium.org/p/chromium/issues/detail?id=1087923 was that we want aggressive icf for both code and data, like lld on other platforms. But this change adds an option to make ICF less aggressive, not more aggressive, from what I understand (correct?)

[zequanwu]

In D97436#2601752, @thakis wrote:

I'm super confused by this change. lld-link used to do aggressive icf for code and safe icf for data. The consensus on https://bugs.chromium.org/p/chromium/issues/detail?id=1087923 was that we want aggressive icf for both code and data, like lld on other platforms. But this change adds an option to make ICF less aggressive, not more aggressive, from what I understand (correct?)

The motivation for this is from: https://bugs.chromium.org/p/chromium/issues/detail?id=1169276, which is for better debugging experience.

lld/COFF/Config.h

 };
 
 enum class GuardCFLevel {
   Off,
   NoLongJmp, // Emit gfids but no longjmp tables
   Full,      // Enable all protections.
 };
 
+enum class ICFLevel {
+  None,
+  Safe, // Safe ICF for all sections.
+  All,  // Aggressive ICF for code, but safe ICF for data, similar to MSVC's
+        // behavior.
+};
+
 // Global configuration.
 struct Configuration {
   enum ManifestKind { SideBySide, Embed, No };
   bool is64() { return machine == AMD64 || machine == ARM64; }
 
   llvm::COFF::MachineTypes machine = IMAGE_FILE_MACHINE_UNKNOWN;
   size_t wordsize;
   bool verbose = false;
   WindowsSubsystem subsystem = llvm::COFF::IMAGE_SUBSYSTEM_UNKNOWN;
   Symbol *entry = nullptr;
   bool noEntry = false;
   std::string outputFile;
   std::string importName;
   bool demangle = true;
   bool doGC = true;
-  bool doICF = true;
+  ICFLevel doICF = ICFLevel::None;
   bool tailMerge;
   bool relocatable = true;
   bool forceMultiple = false;
   bool forceMultipleRes = false;
   bool forceUnresolved = false;
   bool debug = false;
   bool debugDwarf = false;
   bool debugGHashes = false;

lld/COFF/Driver.cpp

void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   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.
   bool doGC = debug == DebugKind::None || args.hasArg(OPT_profile);
-  unsigned icfLevel =
-      args.hasArg(OPT_profile) ? 0 : 1; // 0: off, 1: limited, 2: on
+  Optional<ICFLevel> icfLevel = None;
+  if (args.hasArg(OPT_profile))
+    icfLevel = ICFLevel::None;
   unsigned tailMerge = 1;
   bool ltoNewPM = LLVM_ENABLE_NEW_PASS_MANAGER;
   bool ltoDebugPM = false;
   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 == "ref") {
         doGC = true;
       } else if (s == "noref") {
         doGC = false;
       } else if (s == "icf" || s.startswith("icf=")) {
-        icfLevel = 2;
+        icfLevel = ICFLevel::All;
+      } else if (s == "safeicf") {
+        icfLevel = ICFLevel::Safe;
       } else if (s == "noicf") {
-        icfLevel = 0;
+        icfLevel = ICFLevel::None;
       } else if (s == "lldtailmerge") {
         tailMerge = 2;
       } else if (s == "nolldtailmerge") {
         tailMerge = 0;
       } else if (s == "ltonewpassmanager") {
         ltoNewPM = true;
       } else if (s == "noltonewpassmanager") {
         ltoNewPM = false;
for (StringRef s : vec) {
         StringRef n = s.substr(17);
         if (n.getAsInteger(10, config->ltoPartitions) ||
             config->ltoPartitions == 0)
           error("/opt:lldltopartitions: invalid partition count: " + n);
       } 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;
+  if (!icfLevel)
+    icfLevel = doGC ? ICFLevel::All : ICFLevel::None;
   config->doGC = doGC;
-  config->doICF = icfLevel > 0;
-  config->tailMerge = (tailMerge == 1 && config->doICF) || tailMerge == 2;
+  config->doICF = icfLevel.getValue();
+  config->tailMerge =
+      (tailMerge == 1 && config->doICF != ICFLevel::None) || tailMerge == 2;
   config->ltoNewPassManager = ltoNewPM;
   config->ltoDebugPassManager = ltoDebugPM;
 
   // Handle /lldsavetemps
   if (args.hasArg(OPT_lldsavetemps))
     config->saveTemps = true;
 
   // Handle /kill-at
config->thinLTOObjectSuffixReplace =
       getOldNewOptions(args, OPT_thinlto_object_suffix_replace);
   config->ltoObjPath = args.getLastArgValue(OPT_lto_obj_path);
   // Handle miscellaneous boolean flags.
   config->allowBind = args.hasFlag(OPT_allowbind, OPT_allowbind_no, true);
   config->allowIsolation =
       args.hasFlag(OPT_allowisolation, OPT_allowisolation_no, true);
   config->incremental =
       args.hasFlag(OPT_incremental, OPT_incremental_no,
-                   !config->doGC && !config->doICF && !args.hasArg(OPT_order) &&
-                       !args.hasArg(OPT_profile));
+                   !config->doGC && config->doICF == ICFLevel::None &&
+                       !args.hasArg(OPT_order) && !args.hasArg(OPT_profile));
   config->integrityCheck =
       args.hasFlag(OPT_integritycheck, OPT_integritycheck_no, false);
   config->cetCompat = args.hasFlag(OPT_cetcompat, OPT_cetcompat_no, false);
   config->nxCompat = args.hasFlag(OPT_nxcompat, OPT_nxcompat_no, true);
   for (auto *arg : args.filtered(OPT_swaprun))
     parseSwaprun(arg->getValue());
   config->terminalServerAware =
       !config->dll && args.hasFlag(OPT_tsaware, OPT_tsaware_no, true);
void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   }
 
   if (config->incremental && config->doGC) {
     warn("ignoring '/incremental' because REF is enabled; use '/opt:noref' to "
          "disable");
     config->incremental = false;
   }
 
-  if (config->incremental && config->doICF) {
+  if (config->incremental && config->doICF != ICFLevel::None) {
     warn("ignoring '/incremental' because ICF is enabled; use '/opt:noicf' to "
          "disable");
     config->incremental = false;
   }
 
   if (errorCount())
     return;
 
void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   // Identify unreferenced COMDAT sections.
   if (config->doGC)
     markLive(symtab->getChunks());
 
   // Needs to happen after the last call to addFile().
   convertResources();
 
   // Identify identical COMDAT sections to merge them.
-  if (config->doICF) {
+  if (config->doICF != ICFLevel::None) {
     findKeepUniqueSections();
-    doICF(symtab->getChunks());
+    doICF(symtab->getChunks(), config->doICF);
   }
 
   // Write the result.
   writeResult();
 
   // Stop early so we can print the results.
   rootTimer.stop();
   if (config->showTiming)
     Timer::root().print();
 }
 
 } // namespace coff
 } // namespace lld

lld/COFF/ICF.h

 //===- ICF.h --------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_COFF_ICF_H
 #define LLD_COFF_ICF_H
 
+#include "Config.h"
 #include "lld/Common/LLVM.h"
 #include "llvm/ADT/ArrayRef.h"
 
 namespace lld {
 namespace coff {
 
 class Chunk;
 
-void doICF(ArrayRef<Chunk *> chunks);
+void doICF(ArrayRef<Chunk *> chunks, ICFLevel);
 
 } // namespace coff
 } // namespace lld
 
 #endif

lld/COFF/ICF.cpp

 
 namespace lld {
 namespace coff {
 
 static Timer icfTimer("ICF", Timer::root());
 
 class ICF {
 public:
+  ICF(ICFLevel icfLevel) : icfLevel(icfLevel){};
   void run(ArrayRef<Chunk *> v);
 
 private:
   void segregate(size_t begin, size_t end, bool constant);
 
   bool assocEquals(const SectionChunk *a, const SectionChunk *b);
 
   bool equalsConstant(const SectionChunk *a, const SectionChunk *b);
   bool equalsVariable(const SectionChunk *a, const SectionChunk *b);
 
   bool isEligible(SectionChunk *c);
 
   size_t findBoundary(size_t begin, size_t end);
 
   void forEachClassRange(size_t begin, size_t end,
                          std::function<void(size_t, size_t)> fn);
 
   void forEachClass(std::function<void(size_t, size_t)> fn);
 
   std::vector<SectionChunk *> chunks;
   int cnt = 0;
   std::atomic<bool> repeat = {false};
+  ICFLevel icfLevel = ICFLevel::All;
 };
 
 // Returns true if section S is subject of ICF.
 //
 // Microsoft's documentation
 // (https://msdn.microsoft.com/en-us/library/bxwfs976.aspx; visited April
 // 2017) says that /opt:icf folds both functions and read-only data.
 // Despite that, the MSVC linker folds only functions. We found
 // a few instances of programs that are not safe for data merging.
 // Therefore, we merge only functions just like the MSVC tool. However, we also
 // merge read-only sections in a couple of cases where the address of the
 // section is insignificant to the user program and the behaviour matches that
 // of the Visual C++ linker.
 bool ICF::isEligible(SectionChunk *c) {
   // Non-comdat chunks, dead chunks, and writable chunks are not eligible.
   bool writable = c->getOutputCharacteristics() & llvm::COFF::IMAGE_SCN_MEM_WRITE;
   if (!c->isCOMDAT() || !c->live || writable)
     return false;
 
-  // Code sections are eligible.
-  if (c->getOutputCharacteristics() & llvm::COFF::IMAGE_SCN_MEM_EXECUTE)
+  // Under regular (not safe) ICF, all code sections are eligible.
+  if ((icfLevel == ICFLevel::All) &&
+      c->getOutputCharacteristics() & llvm::COFF::IMAGE_SCN_MEM_EXECUTE)
     return true;
 
   // .pdata and .xdata unwind info sections are eligible.
   StringRef outSecName = c->getSectionName().split('$').first;
   if (outSecName == ".pdata" || outSecName == ".xdata")
     return true;
 
   // So are vtables.
   if (c->sym && c->sym->getName().startswith("??_7"))
forEachClass([&](size_t begin, size_t end) {
     for (size_t i = begin + 1; i < end; ++i) {
       log("  Removed " + chunks[i]->getDebugName());
       chunks[begin]->replace(chunks[i]);
     }
   });
 }
 
 // Entry point to ICF.
-void doICF(ArrayRef<Chunk *> chunks) { ICF().run(chunks); }
+void doICF(ArrayRef<Chunk *> chunks, ICFLevel icfLevel) {
+  ICF(icfLevel).run(chunks);
+}
 
 } // namespace coff
 } // namespace lld

lld/test/COFF/icf-safe.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-win32 %s -o %t1.obj
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-win32 %S/Inputs/icf-safe.s -o %t2.obj
 # RUN: lld-link /dll /noentry /out:%t.dll /verbose /opt:noref,icf %t1.obj %t2.obj 2>&1 | FileCheck %s
 # RUN: lld-link /dll /noentry /out:%t.dll /verbose /opt:noref,icf /export:g3 /export:g4 %t1.obj %t2.obj 2>&1 | FileCheck --check-prefix=EXPORT %s
+# RUN: lld-link /dll /noentry /out:%t.dll /verbose /opt:noref,safeicf %t1.obj %t2.obj 2>&1 | FileCheck %s --check-prefix=SAFEICF
 
 # CHECK-NOT: Selected
 # CHECK: Selected g3
 # CHECK-NEXT:   Removed g4
+# CHECK: Selected f1
+# CHECK-NEXT:   Removed f2
+# CHECK-NEXT:   Removed f3
+# CHECK-NEXT:   Removed f4
 # CHECK-NOT: Removed
 # CHECK-NOT: Selected
 
-# EXPORT-NOT: Selected
+# EXPORT-NOT: Selected g3
+# EXPORT-NOT: Selected g4
+
+# SAFEICF-NOT: Selected
+# SAFEICF: Selected g3
+# SAFEICF-NEXT:   Removed g4
+# SAFEICF: Selected f3
+# SAFEICF-NEXT:   Removed f4
+# SAFEICF-NOT: Removed
+# SAFEICF-NOT: Selected
 
 .section .rdata,"dr",one_only,g1
 .globl g1
 g1:
 .byte 1
 
 .section .rdata,"dr",one_only,g2
 .globl g2
 g2:
 .byte 1
 
 .section .rdata,"dr",one_only,g3
 .globl g3
 g3:
 .byte 2
 
 .section .rdata,"dr",one_only,g4
 .globl g4
 g4:
 .byte 2
 
+.section .text,"xr",one_only,f1
+.globl f1
+f1:
+ nop
+
+.section .text,"xr",one_only,f2
+.globl f2
+f2:
+ nop
+
+.section .text,"xr",one_only,f3
+.globl f3
+f3:
+ nop
+
+.section .text,"xr",one_only,f4
+.globl f4
+f4:
+ nop
+
 .addrsig
 .addrsig_sym g1
 .addrsig_sym g2
+.addrsig_sym f1
+.addrsig_sym f2