[PATCH] D49795: [PPC64] Position-independent long-branch thunks.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

This patch seems a bit too intrusive to me, but I'm not sure if it is unavoidable (and thus this is a straightforward implementation) or there's another way to implement it.



================
Comment at: ELF/Arch/PPC64.cpp:527
                        uint64_t BranchAddr, const Symbol &S) const {
-  // If a function is in the plt it needs to be called through
-  // a call stub.
-  return Type == R_PPC64_REL24 && S.isInPlt();
+  if (Type == R_PPC64_REL24) {
+    // If a function is in the plt it needs to be called through
----------------
Flip the condition to return early.


================
Comment at: ELF/Arch/PPC64.cpp:553
+  int64_t Offset = Dst - Src;
+  return Offset >= llvm::minIntN(26) && Offset <= llvm::maxIntN(26);
 }
----------------
Can you use IsInt<N> instead?


================
Comment at: ELF/Symbols.h:74
   uint32_t GlobalDynIndex = -1;
+  // If this symbol is the target of a long branch which needs a range extending
+  // thunk, this index will be set.
----------------
nit: add a blank line before a multi-line comment.


================
Comment at: ELF/Symbols.h:76
+  // thunk, this index will be set.
+  uint32_t LongBranchTargetIndex = -1;
 
----------------
Hmm, do you really need to add a new member to Symbol? I need to understand the spec better, but if we can, we should avoid adding a member to Symbol.


================
Comment at: ELF/Symbols.h:158-159
   uint64_t getSize() const;
+  uint64_t getLongBranchGotPltOffset() const;
+  uint64_t getLongBranchGotPltVA() const;
   OutputSection *getOutputSection() const;
----------------
Do you need to distinguish a .got.plt entry for a long branch thunk from a regular .got.plt entry? I wonder if you can treat a symbol that needs a range-extension thunk as a symbol that has a (regular) .got.plt entry.


================
Comment at: ELF/Thunks.cpp:223
+    assert(!Dest.IsPreemptible);
+    if (!Dest.isLongBranchTarget()) {
+      InX::GotPlt->addLongBranch(Dest);
----------------
Flip the condition to return early.


================
Comment at: ELF/Thunks.cpp:615-616
+   if (Config->Pic)
+     return make<PPC64PILongBranchThunk>(S);
+   else
+     fatal("Position dependant long_branch thunks not implemented yet!");
----------------
No `else` after `return`


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D49795