[PATCH] D18499: [ELF] - Implemented prototype of location counter support.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/LinkerScript.cpp:125
@@ +124,3 @@
+      OutputSectionBase<ELFT> *Sec = *I;
+      S.erase(I);
+
----------------
grimar wrote:
> ruiu wrote:
> > Why do you have to remove the element here?
> I need to visit all output sections to assign addresses for each.
> Script can mention only some of them.
> There were 2 choices I saw:
> 
> 1) Make this lambda and do 2 iterations. First iteration visits everything that was directly mentioned in script. During that it removes the sections if found from global list. After that we will have all unvisited sections in this global list. Then second iteration visits them all.
> That gives 2 iterations, but complexity is O(n) in total.
> 
> 2) In this variant it is possible to insert all sections that were not mentioned in script to Locations list at the begining of method and avoid using lambda. That would be one loop. Problem here that is that if we have OutputSectionBase list and have Locations lists then how to find those sections for which LocationNode was not created to vizit ?
> That would be something like (pseudocode):
> 
> ```
> for (OutputSectionBase<ELFT> *Sec : S) {
>   if (S NOT IN Locations) {
>     Locations.push_back(create location for S);
>   }
> }
> ```
> 
> That is O(N^2) part of code that can be inserted at the begining of this method, that can help to get rid of labmda and do all in one iteration.
This is a bit too convoluted. It'd need a few trial-and-errors, but it should be able to be written in a straightforward way. I'd probably start making this lambda a separate function.

================
Comment at: ELF/LinkerScript.cpp:610
@@ -483,2 +609,3 @@
   ScriptParser(&Alloc, MB.getBuffer(), isUnderSysroot(Path)).run();
+  Exist = true;
 }
----------------
Exist -> Exists

================
Comment at: ELF/LinkerScript.cpp:629
@@ +628,3 @@
+template void
+LinkerScript::doLayout(std::vector<OutputSectionBase<ELF32LE> *> S);
+template void
----------------
Remove ` S`.

================
Comment at: ELF/LinkerScript.h:48
@@ +47,3 @@
+// LocOutSec is a description of output section, like ".data :..."
+enum LocNodeType { LocSet, LocOutSec };
+struct LocationNode {
----------------
I'd make this a enum class and rename members as shown below.

LocNodeType -> Command
LocSet -> Expr
LocOutSet -> Section

================
Comment at: ELF/Writer.cpp:228
@@ +227,3 @@
+    if (Script->Exist) {
+      Script->doLayout(OutputSections);
+    } else {
----------------
Let's name this `assignAddresses` for consistency with the Writer.


http://reviews.llvm.org/D18499