Set DF_TEXTREL only if text relocations are really needed.

[MaskRay]

Summary

bfd/gold emit DF_TEXTREL/DT_TEXTREL only when text relocations are really needed.

[MaskRay]

Update test

[ruiu]

This is perhaps harmless, but I'm leaning towards not doing this unless there's a reason to do this. I understand why GNU linkers want to do this -- it is because they allow text relocations by default. On the other hand, lld sets DF_TEXTREL only when you explicitly make lld allow text relocations. So, simply setting the flag when the flag is given makes sense to me. Can I ask your motivation of making this change?

[MaskRay]

The motivation is that IFUNC (e.g. R_X86_64_IRELATIVE) is incompatible with DF_TEXTREL. Setting DF_TEXTREL unconditionally may be sometimes harmful.

bfd will error when both IFUNC and DF_TEXTREL are used in non-writable segments.

// bfd/elfxx-x86.c:1291
  if ((info->flags & DF_TEXTREL) != 0)
    {
      if (htab->readonly_dynrelocs_against_ifunc)
        {
          info->callbacks->einfo
            (_("%P%X: read-only segment has dynamic IFUNC relocations;"
              " recompile with -fPIC\n"));
          bfd_set_error (bfd_error_bad_value);
          return FALSE;
        }

      if (!add_dynamic_entry (DT_TEXTREL, 0))
        return FALSE;
    }

gold does not error. But glibc ld.so will crash when IFUNC relocations are resolved in runtime

// elf/get-dynamic-info.h  DF_TEXTREL implies DT_TEXTREL
if (l->l_flags & DF_TEXTREL)
    info[DT_TEXTREL] = info[DT_FLAGS];

// elf/dl-reloc.c
  if (__builtin_expect (l->l_info[DT_TEXTREL] != NULL, 0))
  {
    /////////// the program text segment is remapped as read+write
    ///// PROT_EXEC should also be set if ifunc is used
    if (__mprotect (newp->start, newp->len, PROT_READ|PROT_WRITE) < 0)
  
    ////// IFUNC (R_X86_64_IRELATIVE) is resolved in this call
    /// Code in the program segment is executed while the map is non-executable
    ELF_DYNAMIC_RELOCATE (l, lazy, consider_profiling, skip_ifunc);

[ruiu]

I don't know if setting DF_TEXTREL flag only when text relocation is actually needed is a right solution to solve the issue you described. In lld, when you pass -z textrel, we should assume that the program really needs a text relocations, because otherwise you wouldn't pass that flag to the linker. If it is not compatible with IFUNC, we should emit a warning when we find a IFUNC symbol, no?

[joerg]

"-z textrel" can also be used in build instructions if some platforms will need it, even if not all of them do.

I consider the ifunc in combination with relro example as primary case for those issues a design bug in at least GNU ld. It's not even about text relocations really.

[MaskRay]

I feel lld's -z notext -z text is better than ld.bfd/gold's on-demand DT_TEXTREL tag. FWIW, glibc 2.19 (the next release) has addressed the incompatibility between DT_TEXTREL and IFUNC https://sourceware.org/bugzilla/show_bug.cgi?id=20480#c19

ELF/Config.h

struct Configuration {
   // Each ABI defines its relocation type. IsRela is true if target
   // uses RELA. As far as we know, all 64-bit ABIs are using RELA. A
   // few 32-bit ABIs are using RELA too.
   bool IsRela;
 
   // True if we are creating position-independent code.
   bool Pic;
 
+  // True if non-writable segments contain dynamic relocs so that we need to
+  // emit DF_TEXTREL.
+  bool NeedTextRel = false;
+
   // 4 for ELF32, 8 for ELF64.
   int Wordsize;
 };
 
 // The only instance of Configuration struct.
 extern Configuration *Config;
 
 static inline void errorOrWarn(const Twine &Msg) {

ELF/Relocations.cpp

static void processRelocAux(InputSectionBase &Sec, RelExpr Expr, RelType Type,
                             uint64_t Offset, Symbol &Sym, const RelTy &Rel,
                             int64_t Addend) {
   if (isStaticLinkTimeConstant(Expr, Type, Sym, Sec, Offset)) {
     Sec.Relocations.push_back({Expr, Type, Offset, Addend, &Sym});
     return;
   }
   bool CanWrite = (Sec.Flags & SHF_WRITE) || !Config->ZText;
   if (CanWrite) {
+    if (!(Sec.Flags & SHF_WRITE))
+      Config->NeedTextRel = true;
+
     // R_GOT refers to a position in the got, even if the symbol is preemptible.
     bool IsPreemptibleValue = Sym.IsPreemptible && Expr != R_GOT;
 
     if (!IsPreemptibleValue) {
       InX::RelaDyn->addReloc(Target->RelativeRel, &Sec, Offset, &Sym, Addend,
                              Expr, Type);
       return;
     } else if (RelType Rel = Target->getDynRel(Type)) {

ELF/SyntheticSections.cpp

template <class ELFT> void DynamicSection<ELFT>::finalizeContents() {
   if (Config->ZNow) {
     DtFlags |= DF_BIND_NOW;
     DtFlags1 |= DF_1_NOW;
   }
   if (Config->ZOrigin) {
     DtFlags |= DF_ORIGIN;
     DtFlags1 |= DF_1_ORIGIN;
   }
-  if (!Config->ZText)
+  if (!Config->ZText && Config->NeedTextRel)
     DtFlags |= DF_TEXTREL;
 
   if (DtFlags)
     addInt(DT_FLAGS, DtFlags);
   if (DtFlags1)
     addInt(DT_FLAGS_1, DtFlags1);
 
   // DT_DEBUG is a pointer to debug informaion used by debuggers at runtime. We
if (InX::RelaPlt->getParent()->Live) {
     }
     addInt(DT_PLTREL, Config->IsRela ? DT_RELA : DT_REL);
   }
 
   addInSec(DT_SYMTAB, InX::DynSymTab);
   addInt(DT_SYMENT, sizeof(Elf_Sym));
   addInSec(DT_STRTAB, InX::DynStrTab);
   addInt(DT_STRSZ, InX::DynStrTab->getSize());
-  if (!Config->ZText)
+  if (DtFlags & DF_TEXTREL)
     addInt(DT_TEXTREL, 0);
   if (InX::GnuHashTab)
     addInSec(DT_GNU_HASH, InX::GnuHashTab);
   if (InX::HashTab)
     addInSec(DT_HASH, InX::HashTab);
 
   if (Out::PreinitArray) {
     addOutSec(DT_PREINIT_ARRAY, Out::PreinitArray);

test/ELF/ztext.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/ztext.s -o %t2.o
-# RUN: ld.lld %t2.o -o %t2.so -shared
+# RUN: ld.lld -z notext %t2.o -o %t2.so -shared
+
+# RUN: llvm-readobj -dynamic-table %t2.so | FileCheck --check-prefix=UNNECESSARY %s
 
 # RUN: ld.lld -z notext %t.o %t2.so -o %t -shared
 # RUN: llvm-readobj  -dynamic-table -r %t | FileCheck %s
 # RUN: ld.lld -z notext %t.o %t2.so -o %t2 -pie
 # RUN: llvm-readobj  -dynamic-table -r %t2 | FileCheck %s
 # RUN: ld.lld -z notext %t.o %t2.so -o %t3
 # RUN: llvm-readobj  -dynamic-table -r %t3 | FileCheck --check-prefix=STATIC %s
 
 # RUN: not ld.lld %t.o %t2.so -o %t -shared 2>&1 | FileCheck --check-prefix=ERR %s
 # RUN: not ld.lld -z text %t.o %t2.so -o %t -shared 2>&1 | FileCheck --check-prefix=ERR %s
 # ERR: error: can't create dynamic relocation
 
 # If the preference is to have text relocations, don't create plt of copy relocations.
 
+# UNNECESSARY-NOT: TEXTREL
+
 # CHECK:      Relocations [
 # CHECK-NEXT:   Section {{.*}} .rela.dyn {
 # CHECK-NEXT:     0x1000 R_X86_64_RELATIVE - 0x1000
 # CHECK-NEXT:     0x1008 R_X86_64_64 bar 0x0
 # CHECK-NEXT:     0x1010 R_X86_64_PC64 zed 0x0
 # CHECK-NEXT:   }
 # CHECK-NEXT: ]