[PATCH] D53408: [PPC64] Long branch thunks.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

First round of review.



================
Comment at: ELF/Arch/PPC64.cpp:735-736
+bool PPC64::inBranchRange(RelType Type, uint64_t Src, uint64_t Dst) const {
+  if (Type != R_PPC64_REL24)
+    llvm_unreachable("Unexepected relocation type used in branch");
+
----------------
`assert(Type == R_PPC64_REL24 && "Unexpected relocation type used in branch")` is more idiomatic in LLVM and lld.


================
Comment at: ELF/Arch/PPC64.cpp:739
+  int64_t Offset = Dst - Src;
+  return llvm::isInt<26>(Offset);
 }
----------------
Omit `llvm::`.


================
Comment at: ELF/Symbols.h:81
   uint32_t GotIndex = -1;
-  uint32_t PltIndex = -1;
+  // Linkage table index. If IsInPlt is set then it is an index into the
+  // procedure linkage table, otherwise it is an index into the long-branch
----------------
Can you insert blank lines between these lines so that it is clear that this comment applies only to `LtIndex`?


================
Comment at: ELF/Symbols.h:164
   bool isInGot() const { return GotIndex != -1U; }
-  bool isInPlt() const { return PltIndex != -1U; }
+  bool isInPlt() const { return IsInPlt; }
+  bool isLongBranchTarget() const { return !IsInPlt && LtIndex != -1U; }
----------------
We usually do not define trivial accessors but instead make member variables public.


================
Comment at: ELF/SyntheticSections.cpp:3072
+  // the section and fill it it.
+  if (!Config->Pic) {
+    for (const Symbol *Sym : Entries) {
----------------
Flip the condition to return early.


================
Comment at: ELF/SyntheticSections.h:967
 
-InputSection *createInterpSection();
+class LongBranchTargetSection final : public SyntheticSection {
+public:
----------------
This class needs a class comment.


================
Comment at: ELF/SyntheticSections.h:967
 
-InputSection *createInterpSection();
+class LongBranchTargetSection final : public SyntheticSection {
+public:
----------------
ruiu wrote:
> This class needs a class comment.
Is this a PPC64-only thing? If so, the name should include PPC64.


================
Comment at: ELF/SyntheticSections.h:974
+  bool empty() const override;
+  void finalizeContents(void) override { Finalized = true; }
+private:
----------------
`(void)` is C-ish. You don't need it in C++.


================
Comment at: ELF/SyntheticSections.h:975
+  void finalizeContents(void) override { Finalized = true; }
+private:
+  std::vector<const Symbol *> Entries;
----------------
nit: add a blank line before a label.


================
Comment at: ELF/Thunks.cpp:246-248
+protected:
+  PPC64LongBranchThunk(Symbol &Dest) : Thunk(Dest) {}
+
----------------
Protected members should be written after public members.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D53408