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

[George Rimar via llvm-commits]

grimar added a comment.

In http://reviews.llvm.org/D16575#336638, @rafael wrote:

> The idea is that with linker script scanHeaders is not used and instead we use the data from the script to construct a mapping and the rest of the code is not changed? If so, I like the general direction.


Yes, script could use some special version of scanHeaders(), like scanHeadersLinkerScript() which will generate some structure (header map) with all required parameters based on options parsed by LS. And then assignAddresses should be able to handle it without changes in other code.
This is of course initial implemention and things are subject to change during work on LS (assignAddresses will need tweaking I think), but I think that whole approach should work.


================
Comment at: ELF/Writer.cpp:103
@@ +102,3 @@
+    unsigned Flags;
+    std::list<OutputSectionBase<ELFT> *> OutSections;
+  };
----------------
rafael wrote:
> Why std::list and not std::vector?
Because I dont know the amount of sections to be assigned to header. So I cant reserve() and dont want multiple reallocates that can happen when using the vector. Also vector can bring overhead here because of reserving memory upfront.
And I also dont need any indexed access so just see no reasons to use vector here.

================
Comment at: ELF/Writer.cpp:106
@@ +105,3 @@
+
+  using PhdrMap = std::list<std::unique_ptr<Phdr>>;
+  PhdrMap scanHeaders();
----------------
rafael wrote:
> Thin can be a std::list<Phdr> or a std::vector<std::unique_ptr<Phdr>>, no?
Changed to std::list<Phdr>.

================
Comment at: ELF/Writer.cpp:1056
@@ +1055,3 @@
+  auto AddIf = [this, &Phdrs](unsigned Type, bool C, unsigned Flags = 0) {
+    if (!C)
+      return (Phdr *)nullptr;
----------------
rafael wrote:
> Not sure if having this if in here is actually more readable.
Changed AddIf -> AddHdr, moved condition check outside.

================
Comment at: ELF/Writer.cpp:1122-1130
@@ +1121,11 @@
+
+  uintX_t VA = Target->getVAStart() + sizeof(Elf_Ehdr);
+  uintX_t FileOff = sizeof(Elf_Ehdr);
+  uintX_t TlsVA = 0;
+  uintX_t TlsFileOff = 0;
+  Elf_Phdr *InterpHdr = nullptr;
+  Elf_Phdr GnuRelroPhdr = {};
+  bool RelroAligned = false;
+  bool FirstLoad = true;
+  uintX_t I = 0;
+  for (auto &PHdr : Map) {
----------------
ruiu wrote:
> Do you think you can reduce number of variables and boolean flags used in this function by separating into small functions?
> 
> The fundamental reason why the previous code was not easy to read is because too many flags are entangled. If the code makes a decision based on n boolean flags, the possible combination is 2^n, which is exponential. Moreover, some variables are updated in the middle of a process, that made it hard to follow the logic. (If a variable gets a new value in one path but don't in the other, you'll get 2^n combinations, again.) We'd like to use less information. I think it is acceptable to iterate over the same data structure more than once because I don't think this is a performance-critical path.
Was able to get rid of some variables. Also separated TLS adresses asigning logic into another method. Now there are 3 passes logically separated.

================
Comment at: ELF/Writer.cpp:1143
@@ +1142,3 @@
+
+      bool CreateLoad = !FirstLoad;
+      bool isW = PHdr->Flags & PF_W;
----------------
rafael wrote:
> Do you really need this flag? Looks like the only special case is that the first load starts at 0, not at the start of its first section.
> 
> Is that the same behaviour one gets with linker scripts?
assignAddresses() definetely will requre some tweaking for linker script logic I am sure, currently it is the base concept that assigns addresses in according to some pre-generated map.
It seems that first load will be special anyways, just the parameters will be different. That way probably parameters from linkersript will be used for first load. During implementation of linker script it will be more clear what exactly changes required.

I placed that flag to header description and also moved the code from inside the loop. That added few lines but probaby looks more readable now.


http://reviews.llvm.org/D16575