[PATCH] D31528: [ELF][MIPS] Multi-GOT implementation

[Simon Atanasyan via Phabricator via llvm-commits]

atanasyan added a comment.

I'm going to submit updated patch soon.



================
Comment at: ELF/DriverUtils.cpp:31
 using namespace llvm;
+using namespace llvm::opt;
 using namespace llvm::sys;
----------------
grimar wrote:
> Seems unused.
Without this line I get the following error message because Options.td includes the `HelpHidden` flag:
```
In file included from /home/simon/work/lld/git/tools/lld/ELF/DriverUtils.cpp:48:
tools/lld/ELF/Options.inc:201:87: error: use of undeclared identifier 'HelpHidden'; did you mean 'llvm::opt::HelpHidden'?
```


================
Comment at: ELF/InputFiles.h:116
+  // Index of MIPS GOT built for this file.
+  size_t MipsGotIndex = -1;
+
----------------
grimar wrote:
> May be type could be Optional<> ?
OK


================
Comment at: ELF/SyntheticSections.cpp:729
+    auto E = G.Tls.find(Sym);
+    assert(E != G.Tls.end());
+    return E->second * Config->Wordsize;
----------------
grimar wrote:
> Does not seem assert is needed here. It anyways will either assert or crash on error,
> not a huge difference probably. I would simplify:
> 
> ```
> if (Sym->isTls())
>     return G.Tls.find(Sym)->second * Config->Wordsize;
> if (Sym->IsPreemptible)
>     return G.Global.find(Sym)->second * Config->Wordsize;
> ...
> 
> ```
> 
> The same in `getGlobalDynOffset` and other places.
OK. Good point.


================
Comment at: ELF/SyntheticSections.h:337
+  // holds primary ans secondaty GOT's.
+  std::vector<FileGot> Gots;
+
----------------
grimar wrote:
> Looks it can be `MapVector<InputFile *, FileGot>`.
> Then probably you will not need to add `InputFile::MipsGotIndex` member.
> 
While we collect requests for GOT entries, each input file containing GOT relocations has its own entry in the `Gots` container. But in the `MipsGotSection::build` method number of separated GOTs is reduced my merging and multiple files might start to refer a single GOT. So we need something like `MapVector<InputFile *, FileGot *>`. That makes more complicated at least GOT writing because we will not be able to iterate the `Gots` container and write each entry but have to extract unique GOT.


================
Comment at: ELF/SyntheticSections.h:403
+  DynamicReloc(uint32_t Type, const InputSectionBase *InputSec,
+               uint64_t  OffsetInSec, const OutputSection *OutputSec,
+               int64_t Addend)
----------------
grimar wrote:
> Excessive space: `uint64_t  OffsetInSec`
OK


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D31528