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

[• espindola]

Summary

This is a replacement to https://reviews.llvm.org/D41207. I could not figure out how to update it with my new phab account.

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.

[smeenai]

Rafael, is the @rafael account stale now, or are you keeping both? (In other words, which one should I be adding to reviews?)

[smeenai]

I don't know of a way to delete it, but a Phabricator admin might be able to help you mark it as disabled (similar to the even older @rafael.espindola account).

[grimar]

ELF/Relocations.cpp

575

Not a big deal but may be just

return (Sym.isFunc() && Config->IgnoreFunctionAddressEquality) ||
       (Sym.isObject() && Config->IgnoreDataAddressEquality);

test/ELF/protected-data-access.s

13

capy -> copy

[• espindola]

Address review comments.

[ruiu]

LGTM

ELF/Options.td

149

LLD -> lld

[• espindola]

322152

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. Similar logic for objects.
+  return ((Sym.isFunc() && Config->IgnoreFunctionAddressEquality) ||
+          (Sym.isObject() && Config->IgnoreDataAddressEquality));
+}
+
 // Returns PLT relocation expression.
 //
 // This handles a non PIC program call to function in a shared library. In
 // an ideal world, we could just report an error saying the relocation can
 // overflow at runtime. In the real world with glibc, crt1.o has a
 // R_X86_64_PC32 pointing to libc.so.
 //
 // The general idea on how to handle such cases is to create a PLT entry and
 // use that as the function value.
 //
 // For the static linking part, we just return a plt expr and everything
 // else will use the the PLT entry as the address.
if (!CanWrite && (Config->Shared || (Config->Pic && !isRelExpr(Expr)))) {
     return Expr;
   }
 
   // Copy relocations are only possible if we are creating an executable and the
   // symbol is shared.
   if (!Sym.isShared() || Config->Shared)
     return Expr;
 
-  if (Sym.getVisibility() != STV_DEFAULT) {
+  if (!canDefineSymbolInExecutable(Sym)) {
     error("cannot preempt symbol: " + toString(Sym) +
           getLocation(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 copy 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
+