[PATCH] D16575: [ELF] - Redesign of program headers creation code.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Writer.cpp:50
@@ +49,3 @@
+    unsigned Flags;
+    std::list<OutputSectionBase<ELFT> *> OutSections;
+  };
----------------
ruiu wrote:
> Please do not use std::list unless you have a reason to do so because it is generally slower than std::vector.
Output section amount that should be attached to each phdr is unknown is the main reason for this. 
If there are many possible output sections then multiple reallocations can happen if using vector here. I have no argument for calling reserve(). 
If in opposite we assume that output sections count is little then there is no any perfomance hit during iterations in this case either, but we win a bit of memory probably that is not forwardly allocated as vector do. 

================
Comment at: ELF/Writer.cpp:58
@@ +57,3 @@
+  };
+  using PhdrMap = PhdrsContext;
+
----------------
ruiu wrote:
> What is this for?
Removed using, it was used before but no more needed.

================
Comment at: ELF/Writer.cpp:1100
@@ +1099,3 @@
+    std::unique_ptr<Phdr> U = std::make_unique<Phdr>(Type, Flags);
+    return Map.PhdrList.emplace(Map.PhdrList.end(), std::move(U))->get();
+  };
----------------
ruiu wrote:
> You are not using a return value.
Actually I am. In one place, but it is very useful there:

```
        uint32_t LoadType = (Config->EMachine == EM_AMDGPU) ? getAmdgpuPhdr(Sec)
                                                            : (uint32_t)PT_LOAD;
        Load = AddHdr(LoadType, NewFlags);
        Flags = NewFlags;
```

================
Comment at: ELF/Writer.cpp:1181
@@ +1180,3 @@
+  for (size_t I = 0, E = Map.PhdrList.size(); I != E; ++I) {
+    const auto &PHdr = Map.PhdrList[I];
+    Elf_Phdr *PH = &Phdrs[I];
----------------
ruiu wrote:
> Please do not use `auto` unless type is obvious by RHS.
Done.

================
Comment at: ELF/Writer.cpp:1201
@@ +1200,3 @@
+
+      for (OutputSectionBase<ELFT> *Sec : PHdr->OutSections) {
+        bool InRelRo = HasRelro && (PHdr->Flags & PF_W) && isRelroSection(Sec);
----------------
ruiu wrote:
> Hmm, I'm honestly not sure if this is net positive... This code hasn't changed that much, and this is what I wanted to simplify.
Generally this patch is not about simplification, main aim to have it is to split address assigning into scanHeaders() and assignAddressed() methods, what is needed to implement initial variant of linker script PHDRS I believe. After some futher tweaks for assignAddressed() it should be possible to use some scanHeadersLinkerScript() to affect output. I thinks it is good that code looks close to initial but at fact have significantly more possible functionality now.

But I cant agree that it was not simplified.
For example initial code has few local variables/flags in one place that affected execution flow in one huge method:

```
int PhdrIdx = 0;
Elf_Phdr *Interp = nullptr;
Elf_Phdr GnuRelroPhdr = {};
Elf_Phdr TlsPhdr{};
bool RelroAligned = false;
uintX_t ThreadBssOffset = 0;
```

This code has only one of them left:
```
bool RelroAligned = false;
```

And TLS address assigning was moved to separate method, what simplified logic a bit. 


http://reviews.llvm.org/D16575