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

[George Rimar via llvm-commits]

grimar added inline comments.

================
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);
----------------
ruiu wrote:
> 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; 
>  
Ok.
The only distinction - I used |= :

```

  if (Config->EMachine == EM_MIPS)
    needsGot |= isOutputDynamic();
```


Repository:
  rL LLVM

http://reviews.llvm.org/D15383