This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Some cleanup of Writer<ELFT>::createSections()
AbandonedPublic

Authored by grimar on Apr 26 2016, 9:58 AM.

Details

Reviewers
ruiu
rafael
Summary

Patch performs the next changes:

  • Create reserved symbols at one place.
  • Add predefined sections at one place to output list.
  • Separate processing commons to addCommonSymbols().
  • Added fillSymTables() to keep logic about filling SymTab and DynSymTab.
  • Moved scan relocations logic to scanRelocs().

I am trying to clean up this method as it do too much now, and not just what
it's name says. That hopefully will help to reuse sub-methods later for redesigning
linkerscript functionality.

Diff Detail

Event Timeline

grimar updated this revision to Diff 55024.Apr 26 2016, 9:58 AM
grimar retitled this revision from to [ELF] - Some cleanup of Writer<ELFT>::createSections().
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: grimar, llvm-commits.
rafael added inline comments.Apr 26 2016, 12:12 PM
ELF/Writer.cpp
883

Please use the same structure we use for the other comparisons:

AIsFoo = ...;
BIsFoo = ...;
if (AIsFoo != BIsFoo)
return ...;

I wonder if it wouldn't be nicer to just put Interp and BuildId at the start of the section list in addPredefinedSections and avoid having this special cases in the comparison function.

ruiu edited edge metadata.Apr 26 2016, 5:10 PM

I'm not sure if how this patch would work towards full linker script support. Can you describe your plan? Thanks!

In D19538#413090, @ruiu wrote:

I'm not sure if how this patch would work towards full linker script support. Can you describe your plan? Thanks!

There is no real plan yet, I am still investigating possible redesign of LS. This patch is just clean up as I noticed we are creating symbols, adding sections in different places/methods now,
what is in my opinion does not look clean when we have everything to localize that in one place.
So patch does not directly related to linkerscript support except one moment:
void Writer<ELFT>::createSections() is used now not only for just creating some sections. It do a lot of things: updates symbol tables, scans relocations, finalizes the sections, something else probably. Some of these parts I hope can be reused after delegating create sections functionality to linkerscript. So I guess this function needs some cleanup and first step I would do is splitting it. But before doing that initial cleanup can be done and that is what patch do.

grimar updated this revision to Diff 55176.Apr 27 2016, 3:32 AM
grimar updated this object.
grimar edited edge metadata.
  • Addressed review comment.
  • Separate processing commons to addCommonSymbols().
  • Added fillSymTables() to keep logic about filling SymTab and DynSymTab.
  • Moved scan relocations logic to scanRelocs().
ELF/Writer.cpp
883

I am not sure, because that is not so obvious, but since we had very close logic before
and its really shorter, lets try your suggestion.

In D19538#413090, @ruiu wrote:

I'm not sure if how this patch would work towards full linker script support. Can you describe your plan? Thanks!

So, I upgraded this patch to make it's general usefulness (hopefully) more visible.

Something wrong with this ? I thought that it at least useful cleanup.

grimar abandoned this revision.May 5 2016, 9:13 AM

Let's start from litle: D19981