[PATCH] D86893: [PowerPC] Add support for R_PPC64_GOT_TPREL_PCREL34 used in TLS Initial Exec

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added inline comments.


================
Comment at: lld/ELF/Arch/PPC64.cpp:880
+    else
+      error("R_PPC64_TLS must be either 4 byte aligned or one byte offset "
+            "from 4 byte aligned");
----------------
MaskRay wrote:
> How is this conveyed in the ABI document (I don't have a copy)?
> 
> Is it: r_offset has to be 0 or 1 modulo 4?
> 
> Note that bufLoc is buffer address plus r_offset. bufLoc can be aligned while r_offset isn't.
>From the ABI:
Section `3.7.3.3. Initial Exec TLS Model`
Here is a paragraph from that section:
```
Note that both the x at tls and x at tls@pcrel assembly forms are annotated with R_PPC64_TLS
relocations. To distinguish between the two, the second of these has a field value displaced by one
byte from the beginning of the instruction.
```
So the idea is that `R_PPC64_TLS` is either aligned with the beginning of the instruction or it is off by 1 byte. On PowerPC all of the instructions are either 4 or 8 bytes so all instructions are at least 4 byte aligned. Therefore, if this relocation is 4 byte aligned it will match the beginning of an instruction.


================
Comment at: lld/ELF/Arch/PPC64.cpp:868
+      // instruction it references.
+      uint32_t primaryOp = getPrimaryOpCode(read32(loc - 1));
+      if (primaryOp != 31)
----------------
NeHuang wrote:
> I am not sure if the relocation we have here will be either 4 byte aligned or offset by one byte.  Seems all the code here is based on the relocation is offset by one byte.  Do we need to add an `error` if the offset is not offset by 1 byte?
You are correct. I should add an error here for the cases where the offset is 2 or 3 bytes offset.


================
Comment at: lld/ELF/Arch/PPC64.cpp:1270
   case R_PPC64_GOT_PCREL34:
+  case R_PPC64_GOT_TPREL_PCREL34:
   case R_PPC64_TPREL34: {
----------------
NeHuang wrote:
> Can we combine these 3 relocation cases with `R_PPC64_PCREL34`? 
We can.
However, I don't think that this patch is the right place to do that. This is really just unrelated code cleanup. I would prefer to do it after in an NFC patch.


================
Comment at: lld/test/ELF/Inputs/ppc64-tls-vardef.s:1
+## Thread Local Storage variable definitions.
+.section .tbss,"awT", at nobits
----------------
NeHuang wrote:
> We can either use ` llvm-mc --defsym AUX=1` or `extract` to merge the variable definitions in `ppc64-tls-pcrel-ie.s`
I was actually planning to use this in another test in a later patch that requires a similar setup. As  a result I think I prefer to use a separate file for now.


================
Comment at: lld/test/ELF/ppc64-tls-pcrel-ie.s:10
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le %s -o %t.o
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le %p/Inputs/ppc64-tls-vardef.s -o %t-defs.o
+# RUN: ld.lld --shared %t-defs.o -o %t-defs.so
----------------
MaskRay wrote:
> Consider split-file
Sorry, I'm not sure what you mean.
Do you want me to split the SHARE and STATIC parts of this test into two tests?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86893/new/

https://reviews.llvm.org/D86893