[PATCH] D17601: [ELF] - Define special symbols _etext and _edata

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Writer.cpp:906
@@ -905,1 +905,3 @@
 
+  auto defineWithAlias = [this](StringRef Name, StringRef Alias, Elf_Sym &Sym) {
+    if (Symtab.find(Name))
----------------
ruiu wrote:
> Local variable names need to start with a uppercase letter. Maybe just Define should suffice.
Done.

================
Comment at: ELF/Writer.cpp:914-921
@@ -906,11 +913,10 @@
+
   // If the "_end" symbol is referenced, it is expected to point to the address
   // right after the data segment. Usually, this symbol points to the end
   // of .bss section or to the end of .data section if .bss section is absent.
   // We don't know the final address of _end yet, so just add a symbol here,
   // and fix ElfSym<ELFT>::End.st_value later.
-  if (Symtab.find("_end"))
-    Symtab.addAbsolute("_end", ElfSym<ELFT>::End);
-
   // Define "end" as an alias to "_end" if it is used but not defined.
   // We don't want to define that unconditionally because we don't want to
   // break programs that uses "end" as a regular symbol.
+  defineWithAlias("_end", "end", ElfSym<ELFT>::End);
----------------
ruiu wrote:
> Need to update this comment.
Done.

================
Comment at: ELF/Writer.cpp:1357
@@ -1349,2 +1356,3 @@
 
-    if (Sec->getType() != SHT_NOBITS)
+    const bool HasBits = Sec->getType() != SHT_NOBITS;
+    if (HasBits)
----------------
ruiu wrote:
> Can you define the variables at once here?
> 
>   bool Alloc = Sec->getFlags() & SHF_ALLOC;
>   bool NoBits = Sec->getFlags() & SHF_NOBITS;
> 
That code is not needed anymore as I moved it to 
fixAbsoluteSymbols() after your hint, but 
(just in case it will be returned back):

HasBits IMO better than NoBits:
```
bool HasBits = Sec->getType() != SHT_NOBITS;
```
because double negation not looking good:

```
    if (!NoBits)
      FileOff = alignTo(FileOff, Align);
    Sec->setFileOffset(FileOff);
    if (!NoBits)
      FileOff += Sec->getSize();
```

================
Comment at: ELF/Writer.cpp:1379-1382
@@ -1367,1 +1378,6 @@
+    // _edata points to the end of the last non SHT_NOBITS section.
+    if (Alloc && !(Sec->getFlags() & SHF_WRITE))
+      ElfSym<ELFT>::Etext.st_value = VA;
+    if (Alloc && HasBits)
+      ElfSym<ELFT>::Edata.st_value = VA;
   }
----------------
ruiu wrote:
> Both symbols get the same address. Is this correct?
> 
> Is there any way to move this piece of code to fixAbsoluteSymbols? I don't want to add more code to this function.
Yes it is expected code here. Finally they will get different address. You can see that _etext and _edata are different in testcase.
Values for them will be updated on each iteration and stop updating at different moments.
```
   // _etext points to location after the last read-only loadable segment.
   // _edata points to the end of the last non SHT_NOBITS section.
```

moved that code to fixAbsoluteSymbols.


http://reviews.llvm.org/D17601