[PATCH] D27055: [ELF] Refactor getDynRel to print error location

[Rui Ueyama via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM with these comments addressed.



================
Comment at: ELF/Relocations.cpp:722
+      AddDyn({P.first, &C, Offset, false, &Body, Addend});
       // MIPS ABI turns using of GOT and dynamic relocations inside out.
       // While regular ABI uses dynamic relocations to fill up GOT entries
----------------
ruiu wrote:
> Add a blank line between code and comment.
ping


================
Comment at: ELF/Relocations.cpp:716
       // linker to handle the relocation for us.
+      if (!Target->isPICRel(Type))
+        error(getLocation(C, Offset) + ": relocation " + getRelName(Type) +
----------------
We have `Config->Pic`, so it should be named `isPicRel`.


================
Comment at: ELF/Target.cpp:1250
 uint32_t AArch64TargetInfo::getDynRel(uint32_t Type) const {
-  if (Type == R_AARCH64_ABS32 || Type == R_AARCH64_ABS64)
-    return Type;
-  // Keep it going with a dummy value so that we can find more reloc errors.
-  errorDynRel(Type);
-  return R_AARCH64_ABS32;
+  return isPICRel(Type) ? Type : R_AARCH64_ABS32;
 }
----------------
You can just return `Type` because R_AARCH64_ABS32 was just a dummy value to return something in an error condition.


================
Comment at: ELF/Target.cpp:1980
+template <class ELFT> bool MipsTargetInfo<ELFT>::isPICRel(uint32_t Type) const {
+  return (Type == R_MIPS_32 || Type == R_MIPS_64);
+}
----------------
Remove () after `return`.


================
Comment at: ELF/Target.cpp:1985
 uint32_t MipsTargetInfo<ELFT>::getDynRel(uint32_t Type) const {
-  if (Type == R_MIPS_32 || Type == R_MIPS_64)
-    return RelativeRel;
-  // Keep it going with a dummy value so that we can find more reloc errors.
-  errorDynRel(Type);
-  return R_MIPS_32;
+  return isPICRel(Type) ? RelativeRel : R_MIPS_32;
 }
----------------
This is I think just a dummy value, too.


https://reviews.llvm.org/D27055