[PATCH] D16887: Avoid code duplication when creating dynamic relocations

[Rui Ueyama via llvm-commits]

ruiu added a comment.

Looks like this is towards the direction that I wanted to see. Thank you for doing this. First round of review.


================
Comment at: ELF/OutputSections.cpp:231-249
@@ -312,21 +230,21 @@
 
-    bool LazyReloc =
-        Body && Target->UseLazyBinding && Target->needsPlt(Type, *Body);
-
-    unsigned Reloc;
-    if (!CBP)
-      Reloc = Target->RelativeRel;
-    else if (LazyReloc)
-      Reloc = Target->PltRel;
-    else if (NeedsGot)
-      Reloc = Body->isTls() ? Target->getTlsGotRel() : Target->GotRel;
-    else
-      Reloc = Target->getDynRel(Type);
-    P->setSymbolAndType(CBP ? Body->DynsymIndex : 0, Reloc, Config->Mips64EL);
-
-    if (LazyReloc)
-      P->r_offset = Body->getGotPltVA<ELFT>();
-    else if (NeedsGot)
-      P->r_offset = Body->getGotVA<ELFT>();
-    else
-      P->r_offset = C.getOffset(RI.r_offset) + C.OutSec->getVA();
+    auto GetOffset = [&]() -> uintX_t {
+      switch (Rel.OKind) {
+      case DynamicReloc<ELFT>::Off_GTlsIndex:
+        return Out<ELFT>::Got->getGlobalDynAddr(*Rel.Sym);
+      case DynamicReloc<ELFT>::Off_GTlsOffset:
+        return Out<ELFT>::Got->getGlobalDynAddr(*Rel.Sym) + sizeof(uintX_t);
+      case DynamicReloc<ELFT>::Off_LTlsIndex:
+        return Out<ELFT>::Got->getLocalTlsIndexVA();
+      case DynamicReloc<ELFT>::Off_Sec:
+        return Rel.OffsetSec->getOffset(Rel.OffsetInSec) +
+               Rel.OffsetSec->OutSec->getVA();
+      case DynamicReloc<ELFT>::Off_Bss:
+        return cast<SharedSymbol<ELFT>>(Rel.Sym)->OffsetInBss +
+               Out<ELFT>::Bss->getVA();
+      case DynamicReloc<ELFT>::Off_Got:
+        return Rel.Sym->template getGotVA<ELFT>();
+      case DynamicReloc<ELFT>::Off_GotPlt:
+        return Rel.Sym->template getGotPltVA<ELFT>();
+      }
+    };
----------------
This is a cute way to write a switch-case without lots of "break"s, but I would probably make this a regular static function in this file. Or, you may want to make it a member function of DynamicReloc?

================
Comment at: ELF/OutputSections.cpp:234
@@ +233,3 @@
+      case DynamicReloc<ELFT>::Off_GTlsIndex:
+        return Out<ELFT>::Got->getGlobalDynAddr(*Rel.Sym);
+      case DynamicReloc<ELFT>::Off_GTlsOffset:
----------------
You may want to define a variable of type SymbolBody * for Rel.Sym, so that you need to repeat Rel.Sym many times in this function.

================
Comment at: ELF/OutputSections.h:206
@@ +205,3 @@
+        OffsetInSec(OffsetInSec), UseSymVA(UseSymVA), Addend(Addend) {}
+  DynamicReloc(uint32_t Type, InputSectionBase<ELFT> *OffsetSec,
+               uintX_t OffsetInSec, InputSectionBase<ELFT> *TargetSec,
----------------
I'd add newlines between constructors.

================
Comment at: ELF/Writer.cpp:420
@@ -402,3 +419,3 @@
 
     // If we get here, the code we are handling is not PIC. We need to copy
     // relocations from object files to the output file, so that the
----------------
Could you update this comment for me?

================
Comment at: ELF/Writer.cpp:428-429
@@ -410,4 +427,4 @@
     // at a fixed address, so we don't copy relocations.
     if (Config->Shared && !Target->isRelRelative(Type) &&
-        !Target->isSizeRel(Type))
-      Out<ELFT>::RelaDyn->addReloc({&C, &RI});
+        !Target->isSizeRel(Type)) {
+      uintX_t Addend = getAddend<ELFT>(RI);
----------------
Flip the condition and use early return.

================
Comment at: ELF/Writer.cpp:435
@@ +434,3 @@
+                                      (uintX_t)getPPC64TocBase() + Addend});
+      } else if (Body) {
+        Out<ELFT>::RelaDyn->addReloc(
----------------
I'd use early return instead of `else if` or `else`.


Repository:
  rL LLVM

http://reviews.llvm.org/D16887