Rename --icf-data and add a corresponding flag for functions

[• espindola]

Summary

When we have --icf=safe we should be able to define --icf=all as a shorthand for --icf=safe --ignore-function-address-equality.

For now --ignore-function-address-equality is used only to control access to non preemptable symbols in shared libraries.

[ruiu]

There exists -icf=all, -icf=safe and -icf=none to choose ICF levels, and this patch adds other flags (-ignore-{data,function}-address-equality) to control ICF levels. It doesn't seems like the new flags are in line with the existing options. I'd keep the existing notation which is -icf=<something>.

If we use clang as a precedence, one candidate would be -icf=everything (just like -Weverything as opposed to -Wall) which would do more than -icf=all.

[• rafael]

Rebased.

[• espindola]

This became D41644

ELF/Config.h

struct Configuration {
   bool ExportDynamic;
   bool FixCortexA53Errata843419;
   bool GcSections;
   bool GdbIndex;
   bool GnuHash = false;
   bool HasDynamicList = false;
   bool HasDynSymTab;
   bool ICF;
-  bool ICFData;
+  bool IgnoreDataAddressEquality;
+  bool IgnoreFunctionAddressEquality;
   bool MergeArmExidx;
   bool MipsN32Abi = false;
   bool NoGnuUnique;
   bool NoUndefinedVersion;
   bool NoinhibitExec;
   bool Nostdlib;
   bool OFormatBinary;
   bool Omagic;

ELF/Driver.cpp

void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   errorHandler().FatalWarnings =
       Args.hasFlag(OPT_fatal_warnings, OPT_no_fatal_warnings, false);
   Config->FilterList = args::getStrings(Args, OPT_filter);
   Config->Fini = Args.getLastArgValue(OPT_fini, "_fini");
   Config->FixCortexA53Errata843419 = Args.hasArg(OPT_fix_cortex_a53_843419);
   Config->GcSections = Args.hasFlag(OPT_gc_sections, OPT_no_gc_sections, false);
   Config->GdbIndex = Args.hasFlag(OPT_gdb_index, OPT_no_gdb_index, false);
   Config->ICF = Args.hasFlag(OPT_icf_all, OPT_icf_none, false);
-  Config->ICFData = Args.hasArg(OPT_icf_data);
+  Config->IgnoreDataAddressEquality =
+      Args.hasArg(OPT_ignore_data_address_equality);
+  Config->IgnoreFunctionAddressEquality =
+      Args.hasArg(OPT_ignore_function_address_equality);
   Config->Init = Args.getLastArgValue(OPT_init, "_init");
   Config->LTOAAPipeline = Args.getLastArgValue(OPT_lto_aa_pipeline);
   Config->LTONewPmPasses = Args.getLastArgValue(OPT_lto_newpm_passes);
   Config->LTOO = args::getInteger(Args, OPT_lto_O, 2);
   Config->LTOPartitions = args::getInteger(Args, OPT_lto_partitions, 1);
   Config->MapFile = Args.getLastArgValue(OPT_Map);
   Config->NoGnuUnique = Args.hasArg(OPT_no_gnu_unique);
   Config->MergeArmExidx =

ELF/ICF.cpp

 // Returns a hash value for S. Note that the information about
 // relocation targets is not included in the hash value.
 template <class ELFT> static uint32_t getHash(InputSection *S) {
   return hash_combine(S->Flags, S->getSize(), S->NumRelocations, S->Data);
 }
 
 // Returns true if section S is subject of ICF.
 static bool isEligible(InputSection *S) {
-  // Don't merge read only data sections unless --icf-data was passed.
-  if (!(S->Flags & SHF_EXECINSTR) && !Config->ICFData)
+  // Don't merge read only data sections unless
+  // --ignore-data-address-equality was passed.
+  if (!(S->Flags & SHF_EXECINSTR) && !Config->IgnoreDataAddressEquality)
     return false;
 
   // .init and .fini contains instructions that must be executed to
   // initialize and finalize the process. They cannot and should not
   // be merged.
   return S->Live && (S->Flags & SHF_ALLOC) && !(S->Flags & SHF_WRITE) &&
          S->Name != ".init" && S->Name != ".fini";
 }

ELF/Options.td

 
 defm hash_style: Eq<"hash-style">,
   HelpText<"Specify hash style (sysv, gnu or both)">;
 
 def help: F<"help">, HelpText<"Print option help">;
 
 def icf_all: F<"icf=all">, HelpText<"Enable identical code folding">;
 
-def icf_data: F<"icf-data">,
-  HelpText<"Enable ICF to also fold identical read only data">;
-
 def icf_none: F<"icf=none">, HelpText<"Disable identical code folding">;
 
+def ignore_function_address_equality: F<"ignore-function-address-equality">,
+  HelpText<"LLD can break the address equality of functions">;
+
+def ignore_data_address_equality: F<"ignore-data-address-equality">,
+  HelpText<"LLD can break the address equality of data">;
+
 defm image_base : Eq<"image-base">, HelpText<"Set the base address">;
 
 defm init: Eq<"init">, HelpText<"Specify an initializer function">,
   MetaVarName<"<symbol>">;
 
 defm library: Eq<"library">, HelpText<"Root name of library to use">,
   MetaVarName<"<libName>">;
 

ELF/Relocations.cpp

 
 static void errorOrWarn(const Twine &Msg) {
   if (!Config->NoinhibitExec)
     error(Msg);
   else
     warn(Msg);
 }
 
+// Return true if we can define a symbol in the executable that
+// contains the value/function of a symbol defined in a shared
+// library.
+static bool canDefineSymbolInExecutable(Symbol &Sym) {
+  // If the symbol has default visibility the symbol defined in the
+  // executable will preempt it.
+  if (Sym.getVisibility() == STV_DEFAULT)
+    return true;
+
+  // If we are allowed to break address equality of functions, defining
+  // a plt entry will allow the program to call the function in the
+  // .so, but the .so and the executable will no agree on the address
+  // of the function.
+  if (Sym.isFunc() && Config->IgnoreFunctionAddressEquality)
+    return true;
+
+  // Similar, but for objects.
+  if (Sym.isObject() && Config->IgnoreDataAddressEquality)
+    return true;
+
+  return false;
+}
+
 template <class ELFT>
 static RelExpr adjustExpr(Symbol &Sym, RelExpr Expr, RelType Type,
                           InputSectionBase &S, uint64_t RelOff) {
   // We can create any dynamic relocation if a section is simply writable.
   if (S.Flags & SHF_WRITE)
     return Expr;
 
   // Or, if we are allowed to create dynamic relocations against
if (Config->Shared || (Config->Pic && !isRelExpr(Expr))) {
     error(
         "can't create dynamic relocation " + toString(Type) + " against " +
         (Sym.getName().empty() ? "local symbol" : "symbol: " + toString(Sym)) +
         " in readonly segment; recompile object files with -fPIC" +
         getLocation<ELFT>(S, Sym, RelOff));
     return Expr;
   }
 
-  if (Sym.getVisibility() != STV_DEFAULT) {
+  if (!canDefineSymbolInExecutable(Sym)) {
     error("cannot preempt symbol: " + toString(Sym) +
           getLocation<ELFT>(S, Sym, RelOff));
     return Expr;
   }
 
   if (Sym.isObject()) {
     // Produce a copy relocation.
     auto *B = dyn_cast<SharedSymbol>(&Sym);

test/ELF/Inputs/protected-data-access.s

+        .section .rodata,"a"
+        .global foo
+        .protected foo
+        .type foo, @object
+        .size foo, 8
+foo:
+        .quad 42

test/ELF/Inputs/protected-function-access.s

+        .global foo
+        .protected foo
+        .type foo, @function
+foo:
+        ret

test/ELF/icf9.s

 # REQUIRES: x86
 
 ### Make sure that we do not merge data.
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
 # RUN: ld.lld %t -o %t2 --icf=all --verbose 2>&1 | FileCheck %s
 # RUN: llvm-readelf -S -W %t2 | FileCheck --check-prefix=SEC %s
 
 # SEC:  .rodata      PROGBITS  0000000000200120 000120 000002 00 A 0 0 1
 
 # CHECK-NOT: selected .rodata.d1
 # CHECK-NOT: selected .rodata.d2
 
 # We do merge rodata if passed --icf-data
-# RUN: ld.lld %t -o %t2 --icf=all --verbose --icf-data 2>&1 | FileCheck --check-prefix=DATA %s
+# RUN: ld.lld %t -o %t2 --icf=all --verbose --ignore-data-address-equality 2>&1 | FileCheck --check-prefix=DATA %s
 # RUN: llvm-readelf -S -W %t2 | FileCheck --check-prefix=DATA-SEC %s
 
 # DATA: selected .rodata.d1
 # DATA: removed .rodata.d2
 
 # DATA-SEC:  .rodata      PROGBITS  0000000000200120 000120 000001 00 A 0 0 1
 
 .globl _start, d1, d2

test/ELF/protected-data-access.s

+# REQUIRES: x86
+# RUN: llvm-mc -triple x86_64-pc-linux -filetype=obj %p/Inputs/protected-data-access.s -o %t2.o
+# RUN: ld.lld %t2.o -o %t2.so -shared
+# RUN: llvm-mc -triple x86_64-pc-linux -filetype=obj %s -o %t.o
+
+# RUN: not ld.lld %t.o %t2.so -o %t 2>&1 | FileCheck --check-prefix=ERR %s
+# ERR: error: cannot preempt symbol: foo
+
+# RUN: ld.lld --ignore-data-address-equality %t.o %t2.so -o %t
+# RUN: llvm-readobj --dyn-symbols --relocations %t | FileCheck %s
+
+# Check that we have a capy relocation.
+
+# CHECK: R_X86_64_COPY foo 0x0
+
+# CHECK:      Name: foo
+# CHECK-NEXT: Value:
+# CHECK-NEXT: Size: 8
+# CHECK-NEXT: Binding: Global
+# CHECK-NEXT: Type: Object
+# CHECK-NEXT: Other:
+# CHECK-NEXT: Section: .bss.rel.ro
+
+.global _start
+_start:
+  .quad foo
+

test/ELF/protected-function-access.s

+# REQUIRES: x86
+# RUN: llvm-mc -triple x86_64-pc-linux -filetype=obj %p/Inputs/protected-function-access.s -o %t2.o
+# RUN: ld.lld %t2.o -o %t2.so -shared
+# RUN: llvm-mc -triple x86_64-pc-linux -filetype=obj %s -o %t.o
+
+# RUN: not ld.lld %t.o %t2.so -o %t 2>&1 | FileCheck --check-prefix=ERR %s
+# ERR: error: cannot preempt symbol: foo
+
+# RUN: ld.lld --ignore-function-address-equality %t.o %t2.so -o %t
+# RUN: llvm-readobj --dyn-symbols --relocations %t | FileCheck %s
+
+# Check that we have a relocation and an undefined symbol with a non zero address
+
+# CHECK: R_X86_64_JUMP_SLOT foo 0x0
+
+# CHECK:      Name: foo
+# CHECK-NEXT: Value: 0x201020
+# CHECK-NEXT: Size:
+# CHECK-NEXT: Binding: Global
+# CHECK-NEXT: Type: Function
+# CHECK-NEXT: Other:
+# CHECK-NEXT: Section: Undefined
+
+.global _start
+_start:
+  .quad foo
+