[ELF][PPC64] Detect missing R_PPC64_TLSGD/R_PPC64_TLSLD and disable TLS relaxation

[MaskRay]

Summary

Alternative to D91611.

The TLS General Dynamic/Local Dynamic code sequences need to mark
__tls_get_addr with R_PPC64_TLSGD or R_PPC64_TLSLD, e.g.

addis r3, r2, x@got@tlsgd@ha # R_PPC64_GOT_TLSGD16_HA
addi r3, r3, x@got@tlsgd@l   # R_PPC64_GOT_TLSGD16_LO
bl __tls_get_addr(x@tlsgd)   # R_PPC64_TLSGD followed by R_PPC64_REL24
nop

However, there are two deviations form the above:

1. direct call to __tls_get_addr. This is essential to implement ld.so in glibc/musl/FreeBSD.

bl __tls_get_addr
nop

This is only used in a -shared link, and thus not subject to the GD/LD to IE/LE
relaxation issue below.

2. Missing R_PPC64_TLSGD/R_PPC64_TLSGD for compiler generated TLS references

According to Stefan Pintille, "In the early days of the transition from the
ELFv1 ABI that is used for big endian PowerPC Linux distributions to the ELFv2
ABI that is used for little endian PowerPC Linux distributions, there was some
ambiguity in the specification of the relocations for TLS. The GNU linker has
implemented support for correct handling of calls to __tls_get_addr with a
missing relocation. Unfortunately, we didn't notice that the IBM XL compiler
did not handle TLS according to the updated ABI until we tried linking XL
compiled libraries with LLD."

In short, LLD needs to work around the old IBM XL compiler issue.
Otherwise, if the object file is linked in -no-pie or -pie mode,
the result will be incorrect because the 4 instructions are partially
rewritten (the latter 2 are not changed).

Work around the compiler bug by disable General Dynamic/Local Dynamic to
Initial Exec/Local Exec relaxation. Note, we also disable Initial Exec
to Local Exec relaxation for implementation simplicity, though technically it can be kept.

ppc64-tls-missing-gdld.s demonstrates the updated behavior.

[MaskRay]

Improve tests

[MaskRay]

lld/test/ELF/ppc64-options.s

1 ↗

(On Diff #310618)

I'll move it to target-specific-options.s

[stefanp]

Thank you for your help!

It make sense what you have written but I would still prefer to explicitly check for calls to __tls_get_addr and look at the relocations on the call.

How would you like to proceed from here? Do you want me to take the patch over and make changes?

lld/ELF/Relocations.cpp

1566

I'm not sure what the code after the switch is for.

1568

The issue is that we don't actually check for calls to __tls_get_addr.
We just check that if there exists some R_PPC64_GOT_TLS... we also have at least one R_PPC64_TLSLD/R_PPC64_TLSGD. My concern with this is that we can have a place where the relocations are specified correctly and another place where they are not and this is not going to be detected.

[MaskRay]

Delete the option

[MaskRay]

In D92959#2447003, @stefanp wrote:

Thank you for your help!

It make sense what you have written but I would still prefer to explicitly check for calls to __tls_get_addr and look at the relocations on the call.

How would you like to proceed from here? Do you want me to take the patch over and make changes?

This patch is essentially a rewrite.
If this patch looks fine, I'd like if we take this route instead....

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=727fc41e077139570ea8b8ddfd6c546b2a55627c introduced the R_PPC64_TLSGD/R_PPC64_TLSLD behavior in 2009 so assumably the problem existed before 2009. It made feel pretty uneasy that you are needing the pre-2009 workaround....

I am not sure checking "__tls_get_addr" is useful. R_PPC64_TLSGD/R_PPC64_TLSLD is only used by "__tls_get_addr". GD/LD GOT relocations are not used otherwise, are they?
So a simple check like this patch should work. We don't necessarily bring all the complexity from GNU ld.

lld/ELF/Relocations.cpp

1566

Thanks for the catch. It is unneeded.

[MaskRay]

Delete unused variable

[MaskRay]

Improve test

[stefanp]

In D92959#2447405, @MaskRay wrote:

In D92959#2447003, @stefanp wrote:

Thank you for your help!

It make sense what you have written but I would still prefer to explicitly check for calls to __tls_get_addr and look at the relocations on the call.

How would you like to proceed from here? Do you want me to take the patch over and make changes?

This patch is essentially a rewrite.
If this patch looks fine, I'd like if we take this route instead....

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=727fc41e077139570ea8b8ddfd6c546b2a55627c introduced the R_PPC64_TLSGD/R_PPC64_TLSLD behavior in 2009 so assumably the problem existed before 2009. It made feel pretty uneasy that you are needing the pre-2009 workaround....

I am not sure checking "__tls_get_addr" is useful. R_PPC64_TLSGD/R_PPC64_TLSLD is only used by "__tls_get_addr". GD/LD GOT relocations are not used otherwise, are they?
So a simple check like this patch should work. We don't necessarily bring all the complexity from GNU ld.

I don't want to implement a complete fix of this. I am completely happy with just detecting the issue and disabling the relaxation if we find it.
However, if we have a scenario like this one:

Right:
  addis 3, 2, x@got@tlsgd@ha
  addi 3, 3, x@got@tlsgd@l
  bl __tls_get_addr(x@tlsgd)
  nop
  blr

Wrong:
  addis 3, 2, x@got@tlsgd@ha
  addi 3, 3, x@got@tlsgd@l
  bl __tls_get_addr
  nop
  blr

the function Wrong won't be caught by the code that you posted. We basically have two functions, one was compiled correctly and the other was not.

In the code that you have what if you just look for R_PPC64_REL24 and check those relocations for __tls_get_addr? I know that this adds more overhead but it helps with the backwards compatibility and I feel that is important. Also, if it adds too much overhead we can keep the option and that way we only do this extra check if the user asks for it.

[stefanp]

In D92959#2449603, @stefanp wrote:

In D92959#2447405, @MaskRay wrote:

In D92959#2447003, @stefanp wrote:

Thank you for your help!

It make sense what you have written but I would still prefer to explicitly check for calls to __tls_get_addr and look at the relocations on the call.

How would you like to proceed from here? Do you want me to take the patch over and make changes?

This patch is essentially a rewrite.
If this patch looks fine, I'd like if we take this route instead....

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=727fc41e077139570ea8b8ddfd6c546b2a55627c introduced the R_PPC64_TLSGD/R_PPC64_TLSLD behavior in 2009 so assumably the problem existed before 2009. It made feel pretty uneasy that you are needing the pre-2009 workaround....

I am not sure checking "__tls_get_addr" is useful. R_PPC64_TLSGD/R_PPC64_TLSLD is only used by "__tls_get_addr". GD/LD GOT relocations are not used otherwise, are they?
So a simple check like this patch should work. We don't necessarily bring all the complexity from GNU ld.

I don't want to implement a complete fix of this. I am completely happy with just detecting the issue and disabling the relaxation if we find it.
However, if we have a scenario like this one:

Right:
  addis 3, 2, x@got@tlsgd@ha
  addi 3, 3, x@got@tlsgd@l
  bl __tls_get_addr(x@tlsgd)
  nop
  blr

Wrong:
  addis 3, 2, x@got@tlsgd@ha
  addi 3, 3, x@got@tlsgd@l
  bl __tls_get_addr
  nop
  blr

the function Wrong won't be caught by the code that you posted. We basically have two functions, one was compiled correctly and the other was not.

In the code that you have what if you just look for R_PPC64_REL24 and check those relocations for __tls_get_addr? I know that this adds more overhead but it helps with the backwards compatibility and I feel that is important. Also, if it adds too much overhead we can keep the option and that way we only do this extra check if the user asks for it.

On second thought. Don't worry about this. :)
The chance that the same object file will have a combination of good and bad code gen is so low that it is probably not worth the trouble to try to find it.

I think this patch is on the right track!
Thank you for your help MaskRay!

[MaskRay]

Missing R_PPC64_TLSGD/R_PPC64_TLSGD for one function and correct for another in the same translation unit - does this only happen with relocatable links? I'd hope we simply consider this too niche to fix and don't add more logic on this...

[stefanp]

In D92959#2449669, @MaskRay wrote:

Missing R_PPC64_TLSGD/R_PPC64_TLSGD for one function and correct for another in the same translation unit - does this only happen with relocatable links? I'd hope we simply consider this too niche to fix and don't add more logic on this...

I agree this is too niche. We don't need to add anything for it.

[stefanp]

Tested this on some actual legacy compiled code and it does what we want.
Only one minor question. Otherwise, LGTM.

Thank you MaskRay!

lld/test/ELF/ppc64-tls-missing-gdld.s

3

I see that the BE test was removed. Given that this test is getting bigger I think this is fine.

85

Is this to get rid of the warning for _start ?

[MaskRay]

lld/test/ELF/ppc64-tls-missing-gdld.s

85

Yes. Otherwise the test needs to write -e 0 or specify another entry point.

[grimar]

Few comments/suggestions.

lld/ELF/Relocations.cpp

1541

You probably don't need to execute the body when sec.file->ppc64DisableTLSRelax is already true?
My concern is that scanning over all relocations twice can be slow.

1554

I'd probably exit the loop early when we have a "marker".
I guess it might look nicer if you move this code to a helper function like checkPPC64Relaxations.

if (config->emachine == EM_PPC64)
  checkPPC64Relaxations(...)

1570

lld/test/ELF/ppc64-tls-missing-gdld.s

19

It reads like the relaxation was disabled for a particular place.
I.e the message contains the file name and the section name (what is good), but I think the message could mention that the relaxation was disabled for a whole object.

[MaskRay]

comments

[MaskRay]

Improve diagnostic

lld/test/ELF/ppc64-tls-missing-gdld.s

19

I'll drop the section name. The diagnostic can mention R_PPC64_GOT_TLS* then the user should know readelf -r should be used to invesigate the issue.

[grimar]

LGTM

lld/ELF/Config.h

struct Configuration {
   bool sysvHash = false;
   bool target1Rel;
   bool trace;
   bool thinLTOEmitImportsFiles;
   bool thinLTOIndexOnly;
   bool timeTraceEnabled;
   bool tocOptimize;
   bool pcRelOptimize;
+  bool fixPPC64TLSReloc;
   bool undefinedVersion;
   bool unique;
   bool useAndroidRelrTags = false;
   bool warnBackrefs;
   std::vector<llvm::GlobPattern> warnBackrefsExclude;
   bool warnCommon;
   bool warnIfuncTextrel;
   bool warnMissingEntry;

lld/ELF/InputFiles.h

public:
   // their .toc sections sorted closer to the .got section than files that do
   // not contain any small code model relocations. Thats because the toc-pointer
   // is defined to point at .got + 0x8000 and the instructions used with small
   // code model relocations support immediates in the range [-0x8000, 0x7FFC],
   // making the addressable range relative to the toc pointer
   // [.got, .got + 0xFFFC].
   bool ppc64SmallCodeModelTocRelocs = false;
 
+  // True if the file has TLSGD/TLSLD GOT relocations without R_PPC64_TLSGD or
+  // R_PPC64_TLSLD. Disable TLS relaxation to avoid bad code generation.
+  bool ppc64DisableTLSRelax = false;
+
   // groupId is used for --warn-backrefs which is an optional error
   // checking feature. All files within the same --{start,end}-group or
   // --{start,end}-lib get the same group ID. Otherwise, each file gets a new
   // group ID. For more info, see checkDependency() in SymbolTable.cpp.
   uint32_t groupId;
   static bool isInGroup;
   static uint32_t nextGroupId;
 

lld/ELF/Relocations.cpp

if (in.got->addDynTlsEntry(sym)) {
       mainPart->relaDyn->addReloc(
           {target->tlsDescRel, in.got, off, !sym.isPreemptible, &sym, 0});
     }
     if (expr != R_TLSDESC_CALL)
       c.relocations.push_back({expr, type, offset, addend, &sym});
     return 1;
   }
 
+  // ARM, Hexagon and RISC-V do not support GD/LD to IE/LE relaxation.  For
+  // PPC64, if the file has missing R_PPC64_TLSGD/R_PPC64_TLSLD, disable
+  // relaxation as well.
   bool toExecRelax = !config->shared && config->emachine != EM_ARM &&
                      config->emachine != EM_HEXAGON &&
-                     config->emachine != EM_RISCV;
+                     config->emachine != EM_RISCV &&
+                     !c.file->ppc64DisableTLSRelax;
 
   // If we are producing an executable and the symbol is non-preemptable, it
   // must be defined and the code sequence can be relaxed to use Local-Exec.
   //
   // ARM and RISC-V do not support any relaxations for TLS relocations, however,
   // we can omit the DTPMOD dynamic relocations and resolve them at link time
   // because them are always 1. This may be necessary for static linking as
   // DTPMOD may not be expected at load time.

 
 template <class ELFT, class RelTy>
 static void scanRelocs(InputSectionBase &sec, ArrayRef<RelTy> rels) {
   OffsetGetter getOffset(sec);
 
   // Not all relocations end up in Sec.Relocations, but a lot do.
   sec.relocations.reserve(rels.size());
 
+  if (config->emachine == EM_PPC64) {
+    // R_PPC64_TLSGD/R_PPC64_TLSLD is required to mark `bl __tls_get_addr` for
+    // General Dynamic/Local Dynamic code sequences. If a GD/LD GOT relocation
+    // is found but no R_PPC64_TLSGD/R_PPC64_TLSLD is seen, we assume that the
+    // instructions are generated by very old IBM XL compilers. Work around the
+    // issue by disabling GD/LD to IE/LE relaxation.
+    bool hasMarker = false, hasGDLD = false;
+    for (const RelTy &rel : rels) {
+      RelType type = rel.getType(false);
+      switch (type) {
+      case R_PPC64_TLSGD:
+      case R_PPC64_TLSLD:
+        hasMarker = true;
+        break;
+      case R_PPC64_GOT_TLSGD16:
+      case R_PPC64_GOT_TLSGD16_HA:
+      case R_PPC64_GOT_TLSGD16_HI:
+      case R_PPC64_GOT_TLSGD16_LO:
+      case R_PPC64_GOT_TLSLD16:
+      case R_PPC64_GOT_TLSLD16_HA:
+      case R_PPC64_GOT_TLSLD16_HI:
+      case R_PPC64_GOT_TLSLD16_LO:
+        hasGDLD = true;
+        break;
+      }
+    }
+    if (hasGDLD && !hasMarker) {
+      sec.file->ppc64DisableTLSRelax = true;
+      warn(toString(&sec) + ": disable TLS relaxation due to missing "
+                            "R_PPC64_TLSGD/R_PPC64_TLSLD");
+    }
+  }
+
   for (auto i = rels.begin(), end = rels.end(); i != end;)
     scanReloc<ELFT>(sec, getOffset, i, rels.begin(), end);
 
   // Sort relocations by offset for more efficient searching for
   // R_RISCV_PCREL_HI20 and R_PPC64_ADDR64.
   if (config->emachine == EM_RISCV ||
       (config->emachine == EM_PPC64 && sec.name == ".toc"))
     llvm::stable_sort(sec.relocations,

lld/test/ELF/ppc64-tls-missing-gdld.s

 # REQUIRES: ppc
-# RUN: llvm-mc --triple=powerpc64le %s --filetype=obj -o %t1.o
-# RUN: llvm-mc --triple=powerpc64 %s --filetype=obj -o %t2.o
-# RUN: ld.lld --shared --fatal-warnings %t1.o -o /dev/null
-# RUN: ld.lld --shared --fatal-warnings %t2.o -o /dev/null
-
-## User code can call __tls_get_addr by specifying the tls_index parameter.
-## We need to allow R_PPC64_REL24/R_PPC64_REL24_NOTOC referencing __tls_get_addr
-## without a pairing R_PPC64_TLSGD/R_PPC64_TLSLD.
+# RUN: split-file %s %t
+# RUN: llvm-mc --triple=ppc64le %t/a.s --filetype=obj -o %t/a.o
+# RUN: llvm-mc --triple=ppc64le %t/b.s --filetype=obj -o %t/b.o
+# RUN: llvm-mc --triple=ppc64le %t/tga.s --filetype=obj -o %t/tga.o
+
+## Warn missing R_PPC64_TLSGD/R_PPC64_TLSLD.
+# RUN: ld.lld --shared %t/a.o %t/b.o -o %t.so 2>&1 | FileCheck %s --check-prefix=WARN
+# RUN: llvm-objdump -d --no-leading-addr %t.so | FileCheck %s --check-prefix=DIS
+
+# RUN: ld.lld %t/a.o %t/tga.o -o %t2 2>&1 | FileCheck %s --check-prefix=WARN
+# RUN: llvm-readelf -x .got %t2 | FileCheck %s --check-prefix=HEX
+# RUN: llvm-objdump -d --no-leading-addr %t2 | FileCheck %s --check-prefix=DIS
+
+# WARN: warning: {{.*}}.o:(.text): disable TLS relaxation due to missing R_PPC64_TLSGD/R_PPC64_TLSLD
+
+## .got+0: x is local - relaxed to LE - its DTPMOD/DTPREL slots are link-time constants.
+## DTPMOD is 1. DTPREL is st_value-0x8000 = -0x8000.
+## .got+16: DTPMOD/DTPREL for _TLS_MODULE_BASE_ is 1 and 0, respectively.
+## .got+32: TPOFFSET for x = st_value-0x7000
+# HEX:      section '.got':
+# HEX-NEXT: [[#%x,IGNORE:]] 01000000 00000000 0080ffff ffffffff
+# HEX-NEXT: [[#%x,IGNORE:]] 01000000 00000000 00000000 00000000
+# HEX-NEXT: [[#%x,IGNORE:]] 0090ffff ffffffff
+
+## .TOC.-32768 = (.got+0x8000)-32768 = .got
+# DIS-LABEL: <GeneralDynamic>:
+# DIS-NEXT:    addis 3, 2, 0
+# DIS-NEXT:    addi 3, 3, -32768
+# DIS-NEXT:    bl [[#%x,TGA:]]
+# DIS-LABEL: <GeneralDynamic_NOTOC>:
+# DIS-NEXT:    addis 3, 2, 0
+# DIS-NEXT:    addi 3, 3, -32768
+# DIS-NEXT:    bl [[#TGA]]
+
+## LocalDynamic references _TLS_MODULE_BASE_.
+## .TOC.-32752 = (.got+0x8000)-32752 = .got+16
+# DIS-LABEL: <LocalDynamic>:
+# DIS-NEXT:    addis 3, 2, 0
+# DIS-NEXT:    addi 3, 3, -32752
+# DIS-NEXT:    bl [[#TGA]]
+# DIS-LABEL: <LocalDynamic_NOTOC>:
+# DIS-NEXT:    addis 3, 2, 0
+# DIS-NEXT:    addi 3, 3, -32752
+# DIS-NEXT:    bl [[#TGA]]
+
+## Technically we don't have to disable IE to LE relaxation,
+## but disabling it for implementation simplicity does not hurt.
+# DIS-LABEL: <InitialExec>:
+# DIS-NEXT:    addis 3, 2, 0
+# DIS-NEXT:    ld 3, -32736(3)
+# DIS-NEXT:    add 3, 3, 13
 
+#--- a.s
 GeneralDynamic:
   addis 3, 2, x@got@tlsgd@ha
   addi 3, 3, x@got@tlsgd@l
   bl __tls_get_addr
-  blr
+  nop
 
 GeneralDynamic_NOTOC:
   addis 3, 2, x@got@tlsgd@ha
   addi 3, 3, x@got@tlsgd@l
   bl __tls_get_addr@notoc
-  blr
+  nop
 
 LocalDynamic:
   addis 3, 2, x@got@tlsld@ha
   addi 3, 3, x@got@tlsld@l
   bl __tls_get_addr
-  blr
+  nop
 
 LocalDynamic_NOTOC:
   addis 3, 2, x@got@tlsld@ha
   addi 3, 3, x@got@tlsld@l
   bl __tls_get_addr@notoc
-  blr
+  nop
+
+InitialExec:
+  addis 3, 2, x@got@tprel@ha
+  ld 3, x@got@tprel@l(3)
+  add 3, 3, x@tls
+
+.globl _start
+_start:
+
+.section .tbss,"awT",@nobits
+.globl x
+x:
+  .quad 0
 
+#--- b.s
+## User code can call __tls_get_addr by specifying the tls_index parameter.
+## We need to allow R_PPC64_REL24/R_PPC64_REL24_NOTOC referencing __tls_get_addr
+## without a pairing R_PPC64_TLSGD/R_PPC64_TLSLD.
 CallOnly:
   bl __tls_get_addr
+  nop
+  blr
+
+#--- tga.s
+.globl __tls_get_addr
+__tls_get_addr:
   blr