[PATCH] D29983: [LLD][ELF] Calculate sizes of SHF_ALLOC Synthetic Sections early

[Peter Smith via Phabricator via llvm-commits]

peter.smith updated this revision to Diff 89126.
peter.smith added a comment.

Thank you very much for the comments, I've updated the diff to incorporate the suggestions.

I will be on vacation with limited computer access till the 1st March, I'll pick this back up when I return.

I like the suggestion of splitting up the finalize into 3 separate parts. I've had a go at implementing that in this diff. This has a nice side-effect that the dynamic section finalize only has to run once as the dynamic symbol table early finalize means we don't need to update the entries. I think that this could be hidden inside a single finalize member but I think we might have to track the state to make sure finalize can be called more than once.

In the general case we will need to assign addresses multiple times as it is possible to write linker scripts where adding thunks causes address expressions in the script to change in ways that breaks the address calculations.

I've copied Rafael's comments into Phabricator for reference:

> +  if (In<ELFT>::GnuHashTab && In<ELFT>::DynSymTab)
>  +    In<ELFT>::DynSymTab->addSymbolsToGnuHashTab();

Having the gnu hash implies that we also have a dynamic symbol table,
no?

>   // Dynamic section must be the last one in this list and dynamic
>   // symbol table section (DynSymTab) must be the first one.

This comment should stay with the definition of the Synthetics array.

> +// Add remaining entries to complete .dynamic contents.
>  +template <class ELFT> void DynamicSection<ELFT>::updateAllocSize() {
>  +  std::vector<Entry> LateEntries = addLateEntries();
>  +  // +1 for DT_NULL
>  +  this->Size = (Entries.size() + LateEntries.size() + 1) * this->Entsize;
>  +}
>  +
>  +// Add remaining entries to complete .dynamic contents.
>  +template <class ELFT> void DynamicSection<ELFT>::finalize() {
>  +  this->Link = In<ELFT>::DynStrTab->OutSec->SectionIndex;
>  +  std::vector<Entry> LateEntries = addLateEntries();
>  +  Entries.insert(Entries.end(), LateEntries.begin(), LateEntries.end());
> 
>   this->OutSec->Entsize = this->Entsize;
>   this->OutSec->Link = this->Link;
> 
> - // +1 for DT_NULL this->Size = (Entries.size() + 1) * this->Entsize;

Can you call updateAllocSize instead? Why do you need to split
addLateEntries btw? The actual number of entries doesn't change when
adding thunks, right? I looks like it should be sufficient to just
populate Entries earlier.

> -template <class ELFT> void GnuHashTableSection<ELFT>::finalize() {
>  +template <class ELFT> void GnuHashTableSection<ELFT>::updateAllocSize() {
> 
>   unsigned NumHashed = Symbols.size();
>   NBuckets = calcNBuckets(NumHashed);
>   MaskWords = calcMaskWords(NumHashed);

Do you need to do this in every updateAllocSize?

If I understand it correctly, this patch is in an odd middle ground. I
think I would prefer to either call the full finalize multiple times if
thunks are added or split finalize is 3 methods:

- One that is called early and only once. It would do most of the work that finalize currently does.
- updateAllocSize, which is empty for most sections and just updates whatever can change the size in response to other sections changing size.
- A real finalize method, that is called only once and can set values that depend on the final address of the sections.

Cheers,
Rafael


https://reviews.llvm.org/D29983

Files:
  ELF/Relocations.cpp
  ELF/Relocations.h
  ELF/SyntheticSections.cpp
  ELF/SyntheticSections.h
  ELF/Writer.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D29983.89126.patch
Type: text/x-patch
Size: 18281 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170220/fbf1a1bb/attachment.bin>