[PATCH] D13562: [ELF2] Add a base set of PPC64 relocations
Rui Ueyama via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 8 10:38:43 PDT 2015
ruiu added inline comments.
================
Comment at: ELF/Target.cpp:213
@@ +212,3 @@
+
+static inline uint16_t applyPPClo(uint64_t value) { return value & 0xffff; }
+
----------------
ruiu wrote:
> No need for `inline`. Compilers should take care of them.
I'm not entirely sure, but the capitalization rule is usually applyPPCLo (not applyPPClo), no?
================
Comment at: ELF/Target.cpp:213-237
@@ +212,27 @@
+
+static inline uint16_t applyPPClo(uint64_t value) { return value & 0xffff; }
+
+static inline uint16_t applyPPChi(uint64_t value) {
+ return (value >> 16) & 0xffff;
+}
+
+static inline uint16_t applyPPCha (uint64_t value) {
+ return ((value + 0x8000) >> 16) & 0xffff;
+}
+
+static inline uint16_t applyPPChigher(uint64_t value) {
+ return (value >> 32) & 0xffff;
+}
+
+static inline uint16_t applyPPChighera (uint64_t value) {
+ return ((value + 0x8000) >> 32) & 0xffff;
+}
+
+static inline uint16_t applyPPChighest(uint64_t value) {
+ return (value >> 48) & 0xffff;
+}
+
+static inline uint16_t applyPPChighesta (uint64_t value) {
+ return ((value + 0x8000) >> 48) & 0xffff;
+}
+
----------------
No need for `inline`. Compilers should take care of them.
================
Comment at: ELF/Target.cpp:215
@@ +214,3 @@
+
+static inline uint16_t applyPPChi(uint64_t value) {
+ return (value >> 16) & 0xffff;
----------------
Arguments must start with uppercase, so Value. But I prefer just V, so that we can write these functions one line.
================
Comment at: ELF/Target.cpp:219
@@ +218,3 @@
+
+static inline uint16_t applyPPCha (uint64_t value) {
+ return ((value + 0x8000) >> 16) & 0xffff;
----------------
No space before '('
================
Comment at: ELF/Target.cpp:249-250
@@ +248,4 @@
+ uint64_t PltEntryAddr) const {
+ std::fill(Buf, Buf + getPltEntrySize(), 0);
+
+ // FIXME: See comments in relocateOne.
----------------
I wouldn't write zeros beforehand. Instead, you can add as many "0, 0 , 0, ..." as you want at end of Insts to fill the entire space with zeros.
================
Comment at: ELF/Target.cpp:255
@@ +254,3 @@
+ uint64_t TocBaseVA = TocVA - 0x8000;
+
+ uint64_t EntryOffset = GotEntryAddr - TocBaseVA;
----------------
Remove this blank line.
================
Comment at: ELF/Target.cpp:264-278
@@ +263,17 @@
+
+ const uint32_t Insts[] = {
+ 0xf8410000u, // std %r2, 40(%r1)
+ 0x3d620000u | applyPPCha(EntryOffset), // addis %r11, %r2, X at ha
+ 0xe98b0000u | applyPPClo(EntryOffset), // ld %r12, X at l(%r11)
+ 0xe96c0000u, // ld %r11,0(%r12)
+ 0x7d6903a6u, // mtctr %r11
+ 0xe84c0008u, // ld %r2,8(%r12)
+ 0xe96c0010u, // ld %r11,16(%r12)
+ 0x4e800420u // bctr
+ };
+
+ for (auto InstVal : Insts) {
+ write32be(Buf, InstVal);
+ Buf += 4;
+ }
+}
----------------
I wouldn't construct Insts. Instead I'd do
write32be(Buf, 0xf810000u); // std %r2 ...
write32be(Buf + 4, 0x3d62000u | apply...)
write32be(Buf + 8, ...
================
Comment at: ELF/Target.cpp:296-300
@@ -219,2 +295,7 @@
bool PPC64TargetInfo::relocNeedsPlt(uint32_t Type, const SymbolBody &S) const {
- return false;
+ switch (Type) {
+ default: return false;
+ case R_PPC64_REL24:
+ // These are function calls that need to be redirected through a PLT stub.
+ return S.isShared() || (S.isUndefined() && S.isWeak());
+ }
----------------
if (Type != R_PPC64_REL24)
return false;
return S.isShared() || (S....
================
Comment at: ELF/Target.cpp:304
@@ -222,3 +303,3 @@
void PPC64TargetInfo::relocateOne(uint8_t *Buf, const void *RelP, uint32_t Type,
uint64_t BaseAddr, uint64_t SymVA,
uint64_t GotVA) const {
----------------
BaseAddr -> Base
================
Comment at: ELF/Target.cpp:308
@@ -226,1 +307,3 @@
auto &Rel = *reinterpret_cast<const Elf_Rela *>(RelP);
+ int64_t Addend = Rel.r_addend;
+
----------------
Rename Addend -> A, just like the ELF spec.
================
Comment at: ELF/Target.cpp:323
@@ -227,3 +322,3 @@
uint64_t Offset = Rel.r_offset;
uint8_t *Loc = Buf + Offset;
----------------
Offset -> Off
================
Comment at: ELF/Target.cpp:326-346
@@ +325,23 @@
+
+ if (Type == R_PPC64_TOC16 ||
+ Type == R_PPC64_TOC16_DS ||
+ Type == R_PPC64_TOC16_LO ||
+ Type == R_PPC64_TOC16_LO_DS ||
+ Type == R_PPC64_TOC16_HI ||
+ Type == R_PPC64_TOC16_HA) {
+ switch (Type) {
+ case R_PPC64_TOC16: Type = R_PPC64_ADDR16; break;
+ case R_PPC64_TOC16_DS: Type = R_PPC64_ADDR16_DS; break;
+ case R_PPC64_TOC16_LO: Type = R_PPC64_ADDR16_LO; break;
+ case R_PPC64_TOC16_LO_DS: Type = R_PPC64_ADDR16_LO_DS; break;
+ case R_PPC64_TOC16_HI: Type = R_PPC64_ADDR16_HI; break;
+ case R_PPC64_TOC16_HA: Type = R_PPC64_ADDR16_HA; break;
+ default: llvm_unreachable("Wrong relocation type.");
+ }
+
+ Addend -= TocBaseVA;
+ } else if (Type == R_PPC64_TOC) {
+ write64be(Loc, TocBaseVA);
+ return;
+ }
+
----------------
This switch statement enclosed with if-else-if looks a bit weird. Can you remove outer if and just use switch instead?
================
Comment at: ELF/Target.cpp:343-346
@@ +342,6 @@
+ Addend -= TocBaseVA;
+ } else if (Type == R_PPC64_TOC) {
+ write64be(Loc, TocBaseVA);
+ return;
+ }
+
----------------
You want to move this piece of code just after "uint8_t *Loc = Buf + Offset;" to return early.
================
Comment at: ELF/Target.cpp:368
@@ +367,3 @@
+ (applyPPClo(SymVA + Addend) & Mask));
+ } break;
+ case R_PPC64_ADDR16_HI:
----------------
This style is not in line with others. Please don't put break after }
================
Comment at: ELF/Target.cpp:388
@@ +387,3 @@
+ case R_PPC64_ADDR14: {
+ assert(((SymVA + Addend) & 3) == 0);
+ // Preserve the AA/LK bits in the branch instruction
----------------
If you can trigger this assertion by giving broken object files, this shouldn't be an assert() but an error().
================
Comment at: ELF/Target.cpp:390
@@ +389,3 @@
+ // Preserve the AA/LK bits in the branch instruction
+ uint8_t aalk = *(Loc + 3);
+ write16be(Loc + 2, (aalk & 3) | ((SymVA + Addend) & 0xfffc));
----------------
All variable must starts with uppercase.
================
Comment at: ELF/Target.cpp:423-427
@@ +422,7 @@
+ case R_PPC64_REL32: {
+ uint64_t FinalAddress = (BaseAddr + Offset);
+ int32_t delta = static_cast<int32_t>(SymVA - FinalAddress + Addend);
+ if (SignExtend32<32>(delta) != delta)
+ error("Relocation R_PPC64_REL32 overflow");
+ write32be(Loc, delta);
+ } break;
----------------
Generally we have lots of very similar expressions to calculate relocation values, we want to write them down as simple as possible. So I don't want to use descriptive names for each temporary variables. Please use one-letter variables or don't use variables at all. For example, I'd write this as
int32_t V = int32(SymVA - Base + Off + A);
if (SignExtend32<32>(V) != V)
error(...)
write32be(Loc, V);
(By the way, the above error check seems odd. It would never fail, no?)
================
Comment at: ELF/Target.cpp:430-432
@@ +429,5 @@
+ case R_PPC64_REL64: {
+ uint64_t FinalAddress = (BaseAddr + Offset);
+ uint64_t Delta = SymVA - FinalAddress + Addend;
+ write64be(Loc, Delta);
+ } break;
----------------
write64be(Loc, SymVA - BaseAddr - Offset + Addend);
http://reviews.llvm.org/D13562
More information about the llvm-commits
mailing list