This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Separate assignAddressesScript method
AbandonedPublic

Authored by grimar on Mar 30 2016, 2:39 AM.

Details

Reviewers
ruiu
rafael
Summary

This adds assignAddressesScript method as separate one.
It is used when script is present.

Diff Detail

Event Timeline

grimar updated this revision to Diff 52032.Mar 30 2016, 2:39 AM
grimar retitled this revision from to [ELF] - Separate assignAddressesScript method.
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
grimar added inline comments.Mar 30 2016, 2:44 AM
ELF/Writer.cpp
1417

What is changed here in compare with assignAddresses() is:

  1. There is no PageAlign relative stuff
  2. uintX_t VA = 0
emaste added a subscriber: emaste.Mar 30 2016, 6:34 AM
ruiu edited edge metadata.Mar 30 2016, 10:12 AM

The new code is mostly copy of the old code, so it would make the situation worse. The common part needs to be factored out instead of copying it.

grimar updated this revision to Diff 52089.Mar 30 2016, 11:21 AM
grimar edited edge metadata.
  • Trying to address review comments :)
In D18591#387126, @ruiu wrote:

The new code is mostly copy of the old code, so it would make the situation worse. The common part needs to be factored out instead of copying it.

Something like that ?

ruiu added inline comments.Mar 30 2016, 12:48 PM
ELF/Writer.cpp
1410–1420

This default behavior needs to be removed because all addresses are given by a linker script, no?

1427–1429

This is duplicate.

1431

Move this function call to Writer::run().

grimar added inline comments.Mar 30 2016, 2:35 PM
ELF/Writer.cpp
1410–1420

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 ?

1427–1429

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.

ruiu added inline comments.Mar 30 2016, 2:46 PM
ELF/Writer.cpp
1410–1420

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.

1427–1429

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.

grimar abandoned this revision.Apr 1 2016, 3:34 AM

Will do several little patches instead of this one.