[PATCH] D15383: [ELF] - R_386_GOTOFF relocation implemented.

[Rui Ueyama via llvm-commits]

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

LGTM with a few nits.


================
Comment at: ELF/OutputSections.cpp:24-28
@@ -23,1 +23,7 @@
 
+namespace lld {
+namespace elf2 {
+bool HasGotOffRel = false;
+}
+}
+
----------------
bool lld::elf2::HasGotOffRel = false;


================
Comment at: ELF/OutputSections.h:39
@@ -38,1 +38,3 @@
 
+// Flag to force got to be in output if we have relocations
+// that relies on its address.
----------------
got -> GOT

================
Comment at: ELF/Target.cpp:364
@@ -360,1 +363,3 @@
 
+bool X86TargetInfo::relocAddressesGot(uint32_t Type) const {
+  // This relocation does not require got entry,
----------------
I'd name this isGotRelative.

================
Comment at: ELF/Writer.cpp:771-779
@@ -767,6 +770,11 @@
 
   // We add the .got section to the result for dynamic MIPS target because
   // its address and properties are mentioned in the .dynamic section.
-  if (!Out<ELFT>::Got->empty() ||
-      (isOutputDynamic() && Config->EMachine == EM_MIPS))
+  bool needsGot = (isOutputDynamic() && Config->EMachine == EM_MIPS);
+  // If we have a relocation that is relative to GOT (such as GOTOFFREL),
+  // we need to emit a GOT even if it's empty.
+  if (HasGotOffRel)
+    needsGot = true;
+
+  if (needsGot || !Out<ELFT>::Got->empty())
     OutputSections.push_back(Out<ELFT>::Got);
----------------
I think this flow is slightly better in readability (because special cases are now in `if`s.)

  bool needsGot = !Out<ELFT>::Got->empty();
  // We add the .got section to the result for dynamic MIPS target because
  // its address and properties are mentioned in the .dynamic section.
  if (Config->EMachine == EM_MIPS)
    needsGot = needsGot || isOutputDynamic();
  // If we have a relocation that is relative to GOT (such as GOTOFFREL),
  // we need to emit a GOT even if it's empty.
  if (HasGotOffRel)
    needsGot = true; 
 


http://reviews.llvm.org/D15383