[PATCH] D32612: Remove LinkerScript::flush

[Peter Smith via llvm-commits]

Just tried to rebase the the long range thunks on top of this change
and have come across an interesting design choice that it would be
useful to get your input on?

The short form of the question is:
Should createThunks() insert Thunks into the OutputSection::Sections
and call synchronize(), or should it insert into
InputSectionDescriptions::Sections and do a manual synchronization
from InputSectionDescription() back to OutputSection::Sections?

My preference is to insert directly into
InputSectionDescriptions::Sections as that is where the direction of
travel seems to be going, calling synchonize() after inserting Thunks
would make synchronize() not a last minute cleanup.

Longer form:

The original range thunk implementation ( reviews D31656 - D31666 )
inserted thunks into OutputSection::Sections, then merged to
InputSectionDescriptions (D31656) much like the synchronize() function
used here does.

The review comment on the original RFC email (last patch referring to D31656)
"This last past seems to be the messier. The issue is not with the
patch, is with the existing infrastructure that uses a completely
different representation for linker scripts and non linker scripts.

What I think is needed is for the writer to create a dummy "script"
and use what is now LinkerScript::assignAddresses. That "script" would

* Contain only OutputSectionCommand.
* All string manipulations would have been moved before assignAddress.
* All the orphan handling would have been made explicit before assignAddress.
* Each OutputSectionCommand would contain just a InputSectionDescription.

With this the thunk creation should be able to add thunk to a single location."

I have rewritten the Thunk implementation to use and merge into
InputSectionDescription::Sections rather than OutputSection::Sections
with the original intention to avoid the need for synchronization, I
still needed to insert the Thunks into OutputSection::Sections, it
just didn't matter about the order as I could put them at the end.

It turns out that by putting in the synchronize() to fix the
SHF_LINK_ORDER problem the insertion of thunks into
InputSectionDescription::Sections can be broken as the order in
OutputSection::Sections is preferred.

I can fix this problem by either:
1. Going back to my initial Thunk implementation, insert into
OutputSection::Section and using synchronize() after each iteration.
2. Keeping with the insert into InputSectionDescription::Section and
manually synchronize the InputSectionDescription::Sections back to
OutputSection::Sections (only one synch is needed after all Thunks
have been added).

Other alternatives that I'm guessing are less likely:
3. Only call synchronize() on OutputSections containing
SHF_LINK_ORDER, these should not contain Thunks
4. Implement the SHF_LINK_ORDER change and addition of Sections in a
similar way to D32233 , then in theory the call to synchronize() isn't
needed.

I'll post a conversion of the existing Thunk Implementation to use 2.)
but I'm happy to go down any of the routes and I'd prefer to only have
to do the work once if there is a preference.

Thanks in advance for any opinions, and apologies for the length of the mail.

Peter

On 2 May 2017 at 11:03, Peter Smith <peter.smith at linaro.org> wrote:
> Yes that approach would fail in the current implementation with
> synchronize being done after SHF_LINK_ORDER reordering. When I was
> experimenting with Thunks, the synchronize() was done before the
> SHF_LINK_ORDER sections were reordered, I then reordered the
> InputSectionDescription view of the SHF_LINK_ORDER sections.
>
>
> On 28 April 2017 at 21:29, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>> On 27 April 2017 at 19:30, Peter Smith via Phabricator
>> <reviews at reviews.llvm.org> wrote:
>>> peter.smith added a comment.
>>>
>>> This looks good to me too.
>>>
>>> One thing I observed when writing the code in the initial Thunks submission to insert the Thunks back into the InputSectionDescriptions was that the order of OutputSections::Sections should match the order of InputSectionDescription::Sections (on the assumption that any additional Sections added to OutputSections::Sections were added on the end). If I'm correct I think you may be able to:
>>>
>>> - Extract the last InputSectionBase from the last InputSectionDescription
>>> - Find this section in OutputSection::Sections
>>> - From the next section to the end of OutputSection::Sections should be your missing sections.
>>>
>>> I may have missed something though.
>>
>> For SHF_LINK_ORDER this fails, no?
>>
>>> Looking forward we'll need to make some changes to OutputSections::finalize(), if there are orphan link-order sections (the sentinel .ARM.exidx) for example they will get added to a new InputSectionDescription. The current sort of link order sections will only work on a single InputSectionDescription. When I get back to the office on Tuesday I'll rebase https://reviews.llvm.org/D32233, and my Thunks patches on top of this.
>>>
>>>
>>> https://reviews.llvm.org/D32612
>>>
>>>
>>>