[ELF] - Set DF_TEXTREL flag properly.

[grimar]

Summary

Documentation says that DT_TEXTREL has been superseded by the DF_TEXTREL flag.

I think that happened 18 years ago in this commit:
https://sourceware.org/git/?p=glibc.git;a=commit;h=06535ae9487708dad9048552c9c92828d998a897

https://sourceware.org/git/?p=glibc.git;a=blobdiff;f=elf/dynamic-link.h;h=59a6001069f56b075c2bebaa9bff609b8ce92d9d;hp=d322c1231faa19900cf1f5e078d93d4203b8ce7a;hb=06535ae9487708dad9048552c9c92828d998a897;hpb=fc9cfb28c05def1bfc0edc099d8001f83654fd10

In that case it looks adding DT_TEXTREL is a bit outdated and we can use DF_TEXTREL always instead.

[grimar]

• Uploaded correct patch.

[davide]

This may be be fine, but it makes the strong assumption that the world is Linux.
@emaste / @joerg should take a look and make sure the dynamic loader in their *BSDs knows about the flag (at least Ed).

[davide]

Side note: if everything works just fine as is (TM), why do we need to make this change?
It buys us a new shiny flag that is pretty much equivalent to the old one for systems supporting both the DT_ and the DF_ variant, but it potentially breaks backcompatibility with older loaders not supporting the new form.
I'm not entirely sure this risk is worth it.

[grimar]

In D33046#751071, @davide wrote:

Side note: if everything works just fine as is (TM), why do we need to make this change?
It buys us a new shiny flag that is pretty much equivalent to the old one for systems supporting both the DT_ and the DF_ variant, but it potentially breaks backcompatibility with older loaders not supporting the new form.
I'm not entirely sure this risk is worth it.

We implemented -z notext initially because had no way to produce relocations to readonly segment (here LLD differs in default behavior with bfd). That was D30530 and feature was intended to be used for linux kernel.
There was no way to link it at all that time. And DT_TEXTREL was added just as a part of a patch, but in fact kernel does not use the flags or anything, it do all things on its side and just
needs something like this option to complete link. Later, I think I saw in llvm-mails that there are other possible users of feature, not sure what they exactly need.

And my point was that if there is no real users of depricated flag, why not to try to remove it ? That will be consistent with what spec says.
In LLD we usually did not implement things that are not known to be useful, I think when I implemented D30530 I should have use DF_ and not DT_ at first place.

[davide]

In D33046#751085, @grimar wrote:

In D33046#751071, @davide wrote:

Side note: if everything works just fine as is (TM), why do we need to make this change?
It buys us a new shiny flag that is pretty much equivalent to the old one for systems supporting both the DT_ and the DF_ variant, but it potentially breaks backcompatibility with older loaders not supporting the new form.
I'm not entirely sure this risk is worth it.

We implemented -z notext initially because had no way to produce relocations to readonly segment (here LLD differs in default behavior with bfd). That was D30530 and feature was intended to be used for linux kernel.
There was no way to link it at all that time. And DT_TEXTREL was added just as a part of a patch, but in fact kernel does not use the flags or anything, it do all things on its side and just
needs something like this option to complete link. Later, I think I saw in llvm-mails that there are other possible users of feature, not sure what they exactly need.

And my point was that if there is no real users of depricated flag, why not to try to remove it ? That will be consistent with what spec says.
In LLD we usually did not implement things that are not known to be useful, I think when I implemented D30530 I should have use DF_ and not DT_ at first place.

If there's only one user, and that works with DF_ then feel free to switch.
But:

1. Make sure you point out in the review next time so people don't have to guess.
2. Please double check there are no other reasonable uses of this flag before doing the switch.

(2) is less important, as we can always add the flag back).

[ruiu]

If the Linux kernel was the only reason you wanted -z text, and the Linux kernel doesn't actually use that flag, I'd support removing -z text flag entirely.

[davide]

In D33046#751201, @ruiu wrote:

If the Linux kernel was the only reason you wanted -z text, and the Linux kernel doesn't actually use that flag, I'd support removing -z text flag entirely.

Indeed, that would be even better :)

[joerg]

At least NetBSD doesn't support/use DF_TEXTREL and only DT_TEXTREL.

[grimar]

In D33046#751201, @ruiu wrote:

If the Linux kernel was the only reason you wanted -z text, and the Linux kernel doesn't actually use that flag, I'd support removing -z text flag entirely.

What I was trying to say that lunux kernel does not need DT_TEXTREL or DF_TEXTREL flags, because it self relocates and not depend on flags to do that.
But without -z notext it was impossible to link it when it configured with KASLR. Because bfd allows text relocations by default and we - don't. We reported an error,
option allows to fix that.

[grimar]

In D33046#751430, @joerg wrote:

At least NetBSD doesn't support/use DF_TEXTREL and only DT_TEXTREL.

Thanks Joerg ! I am abandoning this patch then.

ELF/SyntheticSections.cpp

if (Config->ZNow) {
     DtFlags1 |= DF_1_NOW;
   }
   if (Config->ZOrigin) {
     DtFlags |= DF_ORIGIN;
     DtFlags1 |= DF_1_ORIGIN;
   }
   if (Config->HasStaticTlsModel)
     DtFlags |= DF_STATIC_TLS;
+  if (!Config->ZText)
+    DtFlags |= DF_TEXTREL;
 
   if (DtFlags)
     add({DT_FLAGS, DtFlags});
   if (DtFlags1)
     add({DT_FLAGS_1, DtFlags1});
 
   this->Link = In<ELFT>::DynStrTab->OutSec->SectionIndex;
   if (In<ELFT>::RelaDyn->OutSec->Size > 0) {
add({Config->EMachine == EM_MIPS ? DT_MIPS_PLTGOT : DT_PLTGOT,
          In<ELFT>::GotPlt});
     add({DT_PLTREL, uint64_t(Config->IsRela ? DT_RELA : DT_REL)});
   }
 
   add({DT_SYMTAB, In<ELFT>::DynSymTab});
   add({DT_SYMENT, sizeof(Elf_Sym)});
   add({DT_STRTAB, In<ELFT>::DynStrTab});
   add({DT_STRSZ, In<ELFT>::DynStrTab->getSize()});
-  if (!Config->ZText)
-    add({DT_TEXTREL, (uint64_t)0});
   if (In<ELFT>::GnuHashTab)
     add({DT_GNU_HASH, In<ELFT>::GnuHashTab});
   if (In<ELFT>::HashTab)
     add({DT_HASH, In<ELFT>::HashTab});
 
   if (Out::PreinitArray) {
     add({DT_PREINIT_ARRAY, Out::PreinitArray});
     add({DT_PREINIT_ARRAYSZ, Out::PreinitArray, Entry::SecSize});

test/ELF/ztext-text-notext.s

 # 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:  ]
 # CHECK: DynamicSection [
-# CHECK: 0x0000000000000016 TEXTREL 0x0
+# CHECK: 0x000000000000001E FLAGS TEXTREL
 
 # STATIC:      Relocations [
 # STATIC-NEXT:    Section {{.*}} .rela.dyn {
 # STATIC-NEXT:      0x201008 R_X86_64_64 bar 0x0
 # STATIC-NEXT:      0x201010 R_X86_64_PC64 zed 0x0
 # STATIC-NEXT:    }
 # STATIC-NEXT:  ]
 # STATIC: DynamicSection [
-# STATIC: 0x0000000000000016 TEXTREL 0x0
+# STATIC: 0x000000000000001E FLAGS TEXTREL
 
 foo:
 .quad foo
 .quad bar
 .quad zed - .