[PATCH] D18119: [ELF][MIPS] Factor out SumVA adjustments into a couple of separate functions. NFC.

[Rui Ueyama via llvm-commits]

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

Since this is a refactoring patch to split the function, LGTM.

But I really want you and all other people who are working on MIPS to not only add code to the support current MIPS ABI but also make efforts to fix the ABI. Some parts of the ABI are very odd and sometimes even broken (e.g. the mixed endian header field), and supporting it sometimes really complicates stuff (like the paired relocations.)


================
Comment at: ELF/InputSection.cpp:290
@@ -265,3 +289,3 @@
+      else
         SymVA = Body.getGotVA<ELFT>() + A;
       if (Body.IsTls)
----------------
grimar wrote:
> I don't like this place because it has "if (Config->EMachine == EM_MIPS)"
> I would do next:
> 1) rename static uintX_t getMipsGotVA -> static uintX_t getGotVA
> 2) remove uintX_t SymVA argument, but add uintX_t A
> 
> and do:
> 
> ```
> template <class ELFT, class uintX_t>
> static uintX_t getGotVA(const SymbolBody &Body, uintX_t A,
>                             uint8_t *BufLoc, uint8_t *PairedLoc) {
> 
> if (Config->EMachine != EM_MIPS)
>  return Body.getGotVA<ELFT>() + A;
> ...
> ```
> 
> ```
> else if (Target->needsGot(Type, Body)) {
>   SymVA = getGotVA<ELFT>(Body, A, BufLoc, PairedLoc);
> ...
> ```
I prefer the other way. This function would not have been written if MIPS ABI were not so weird. I want to keep the code in the form that you can ignore all function calls including "mips" if you are not interested in MIPS. If you move that code into the function, you have to jump to that function and read that function only to know that that was basically separated for MIPS oddity.


Repository:
  rL LLVM

http://reviews.llvm.org/D18119