This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Cleanup of processSectionCommands().
ClosedPublic

Authored by grimar on Oct 19 2017, 7:35 AM.

Details

Summary

The way we handle ONLY_IF_RO/OLY_IF_RW constraints in
processSectionCommands is a bit tricky. If input sections
does no satisfy given constraint we remove command from
commands list. It seems too complex, what we can do instead
is to make the OutputCommand empty. So that at later steps
LLD will remove it just like it deal with other empty output commands.
That allows to simplify the loop a bit.

Diff Detail

Event Timeline

grimar created this revision.Oct 19 2017, 7:35 AM
ruiu edited edge metadata.Oct 24 2017, 9:24 PM

Your change to ELF/LinkerScript.cpp looks good, but the other change needs improving.

ELF/OutputSections.cpp
119–120

This function is addSection, which is supposed to add a section to an output section, right? Adding this piece of code which is not related to adding sections to this function is not a good idea. This is how this function organically grown (and I took quite a long time to clean it up), so when you add code to some function, please stop and think if what you are about to do is semantically correct.

grimar added inline comments.Oct 25 2017, 2:01 AM
ELF/OutputSections.cpp
119–120

This function not only adds section, but also initializes and changes the Type.
And that happened before my change already, so I assume it is proper place to set Type because
it probably desirable to change it in a single place of code, no ?

We can split this code (lines 97-120) to something like setSectionType, but that would be separate refactoring change.
Will such change be OK for you ?

ruiu added inline comments.Oct 25 2017, 7:44 AM
ELF/OutputSections.cpp
119–120

George, I knew that was the reason why you added this piece of code to this function, but this is not right. If addSection mutates its class' Type based on a given argument, it is fine, but if it does something that is not related to a given section every time you pass a section to the function, something's not correct.

grimar updated this revision to Diff 120615.Oct 27 2017, 8:13 AM
  • Addressed review comment.
ELF/OutputSections.cpp
119–120

I see. Fixed.

ruiu accepted this revision.Oct 31 2017, 8:42 PM

LGTM

This revision is now accepted and ready to land.Oct 31 2017, 8:42 PM
This revision was automatically updated to reflect the committed changes.