[PATCH] D18591: [ELF] - Separate assignAddressesScript method

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Writer.cpp:1413-1423
@@ +1412,13 @@
+
+    // We only assign VAs to allocated sections.
+    if (needsPtLoad<ELFT>(Sec)) {
+      VA = alignTo(VA, Align);
+      Sec->setVA(VA);
+      VA += Sec->getSize();
+    } else if (Sec->getFlags() & SHF_TLS && Sec->getType() == SHT_NOBITS) {
+      uintX_t TVA = VA + ThreadBssOffset;
+      TVA = alignTo(TVA, Align);
+      Sec->setVA(TVA);
+      ThreadBssOffset = TVA - VA + Sec->getSize();
+    }
+  }
----------------
grimar wrote:
> ruiu wrote:
> > This default behavior needs to be removed because all addresses are given by a linker script, no?
> That abstract Script->getSectionVA(Sec->getName()) function we discussed earlier today not exist yet, no ?
> So this logic like a stub now.
> 
> If we remove it now then all VA will be 0 ? Do you mean do that for now ?
You are not going to submit this without the other patches, right? I'd update this patch so that if I apply all patches to my local repository, I'd get the new code of the final form.

================
Comment at: ELF/Writer.cpp:1430-1432
@@ +1429,5 @@
+
+  // Update "_end" and "end" symbols so that they
+  // point to the end of the data segment.
+  ElfSym<ELFT>::End.st_value = VA;
+
----------------
grimar wrote:
> ruiu wrote:
> > This is duplicate.
> I know that is. Create one more method to move next code out ?
> 
> ```
> // Add space for section headers.
>   SectionHeaderOff = alignTo(FileOff, sizeof(uintX_t));
>   FileSize = SectionHeaderOff + getNumSections() * sizeof(Elf_Shdr);
> 
>   // Update "_end" and "end" symbols so that they
>   // point to the end of the data segment.
>   ElfSym<ELFT>::End.st_value = VA;
> ```
> 
> I did not moved it out to assignPhdrs() because assignPhdrs is not a good name for that IMO.
Is there any way to move this piece of code to fixAbsoluteSymbols? I don't think the last address of the image is available only here.

Also, not directly related to this patch, but the comment "they point to the end of the data segment" is incorrect. They point to the end of image in memory.


http://reviews.llvm.org/D18591