[PATCH] D48090: [PPC64] global dynamic to initial exec relaxation

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: ELF/Arch/PPC64.cpp:418-422
+// Instruction                    Relocation                Symbol
+// addis r3, r2, x at got@tlsgd at ha   R_PPC64_GOT_TLSGD16_HA      x
+// addi  r3, r3, x at got@tlsgd at l    R_PPC64_GOT_TLSGD16_LO      x
+// bl __tls_get_addr(x at tlsgd)     R_PPC64_TLSGD               x
+//                                R_PPC64_REL24               __tls_get_addr
----------------
ruiu wrote:
> I wonder if this comment actually explains what this function is doing. This function handles three different relocation types, but this comment seems to cover only one type.
Part 1 is the addis/addi sequence which covers the R_PPC64_GOT_TLSGD16_HA and R_PPC64_GOT_TLSGD16_LO relocations on the first 2 instructions. Even though we handle them separately and they won't nessicariily be adjacent in the source it really only makes sense to talk about the pair together.

Part 2 and 3 cover the remaining R_PPC64_TLSGD relocation which affects the last 2 instructions in the sequence. 
(A call that crosses dso boundaries has to be followed by a nop on PPC  which is why we can handle both instructions with a single hint relocation).



================
Comment at: ELF/Arch/PPC64.cpp:437
+    //                      addis rT, r2, sym at got@tprel at ha.
+    write16(Loc, applyPPCHa(Val - getPPC64TocBase()));
+    return;
----------------
syzaara wrote:
> I think it would be more clear if we called relocateOne with the replacement relocation here.
Now I wish I hadn't jumped the gun on abandoning the R_RELAX_TLS_GD_TO_IE_GOT_OFF RelExpr :).

I like your idea `relocateOne(Loc, R_PPC64_TPREL16_HA/LO_DS, Val)` makes it clear that the relocation is handled the same as a normal TPREL16. Since we are getting the syms VA though, it would have to be `relocateOne(Loc, R_PPC64_TPREL16_HA/LO_DS, Val - InX::got->getVA())` which I think kills the clarity ...

I think we should pick one of the following:

1) Use a RelExpr of `R_RELAX_TLS_GD_TO_IE_GOT_OFF` so we get just the got offset of the symbol, then call relocateOne which handles adjusting by the toc-offset and performing the relocation.

2) Stick to the `R_RELAX_TLS_GD_TO_IE_GOT_ABS` RelExpr, adjust by the TocBase and relocate here (as the patch is currently doing). 

I would much prefer 1, as long as everyone is ok with adding the new RelExpr.




================
Comment at: ELF/Arch/PPC64.cpp:443
+    uint32_t EndianOffset = Config->EKind == ELF64BEKind ? 2U : 0U;
+    uint32_t InputRegister = (read32(Loc - EndianOffset) & (31 << 16));
+    write32(Loc - EndianOffset,
----------------
ruiu wrote:
> Perhaps 0x7fff0000 is easier to understand than 31<<16.
I picked this format to try to convey its a 5-bit mask and shifted over to bits 16-21. When I'm reading if I see 0x1F0000 I'll typically have to stop and count the 0's to know where the mask is.  Does `0x1f << 16` work for you? 


================
Comment at: ELF/Arch/PPC64.cpp:449
+  case R_PPC64_TLSGD:
+     write32(Loc, 0x60000000);       // bl __tls_get_addr(sym at tlsgd) --> nop
+     write32(Loc + 4, 0x7c636A14);   // nop --> add r3, r3, r13
----------------
ruiu wrote:
> Wrong indentation -- can you always apply clang-format?
 yes, sorry about that. I've been lazy with clang-format for a while because  my vim commands for it were broken. I've fixed them and will format the rest of this patch. 


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D48090