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

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/LinkerScript.cpp:36
@@ +35,3 @@
+
+class elf::LocCounterEvaluator {
+public:
----------------
ruiu wrote:
> It seems that we don't need this class because this class has only one member, LocCounter and you always reset LocCounter before calling evaluate. This class can just be a function that takes the current VA and an expression and returns a new VA.
Done.

================
Comment at: ELF/LinkerScript.cpp:72
@@ -31,1 +71,3 @@
 LinkerScript *elf::Script;
+LocCounterEvaluator Eval;
+
----------------
ruiu wrote:
> It is better to move this variable inside LinkerScript.
Since that class not exist, variable also not exist anymore.

================
Comment at: ELF/LinkerScript.cpp:125
@@ +124,3 @@
+      OutputSectionBase<ELFT> *Sec = *I;
+      S.erase(I);
+
----------------
ruiu wrote:
> 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.
Done. Made lambda as separate function + refactored code to have only one iteration.

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

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

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

================
Comment at: ELF/LinkerScript.h:52-54
@@ +51,5 @@
+
+  // For LocSet this keeps list of tokens to evaluate location.
+  // For LocOutSec first element is a output section name.
+  std::vector<StringRef> Args;
+};
----------------
ruiu wrote:
> I wouldn't use the same variable for completely different purposes. I'd define two fields, Expr and SectionName, instead.
Done.

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


http://reviews.llvm.org/D18499