This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Do --hash-style=both by default.
ClosedPublic

Authored by grimar on Sep 29 2017, 8:31 AM.

Details

Summary

Its PR34712,

GNU linkers recently changed default values to "both" of "sysv".
It seems reasonable for me to do the same in LLD,
patch do the same for all targets except MIPS, where .gnu.hash
section is not yet supported.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Sep 29 2017, 8:31 AM
ruiu edited edge metadata.Sep 29 2017, 12:37 PM

Could you start a thread in the mailing list to propose a change of the default settings first then create a patch?

I don't want you to update that many tests (which probably needed a lot of labor work) before getting any consensus, even if reaching a consensus is obvious to you. A rule of thumb is that, code review is not a place to discuss policy changes.

grimar updated this revision to Diff 117532.Oct 3 2017, 8:31 AM
  • Use --hash-style=sysv to simulate previous behavior in testcases instead of modifying values checked.
ruiu added a comment.Oct 3 2017, 3:52 PM

Please do *not* update a patch until you declare we reach an conclusion in the discussion thread.

ruiu added inline comments.Oct 3 2017, 3:57 PM
ELF/Driver.cpp
259–261 ↗(On Diff #117532)

This function is not a place to put this code, as described in the function comment. This function checks for validity of option combinations. You shouldn't set a new value to a Config member.

grimar updated this revision to Diff 117642.Oct 4 2017, 2:29 AM
  • Addressed review comments.
ELF/Driver.cpp
259–261 ↗(On Diff #117532)

I updated this function name and description. Problem is that I need to know EMachine type,
which is updated after we scan input files, what happens after reading normal config reading flow.
The place where this function is located and called and the fact that it is the place where
"the .gnu.hash section is not compatible with the MIPS target." error is reported at the same time
makes me think it is a right place to update as it allows to localize such a bit hacky situation.

Alternative probably could be to intoduce one more function to set up options that have different
defaults basing on target, but it does not seem it worth to do for a single option we have atm.

ruiu added inline comments.Oct 4 2017, 11:56 AM
ELF/Driver.cpp
267 ↗(On Diff #117642)

No, please do not make a change to this function like this, but instead keep it side-effect-free except calling error(). This change hurts readability.

ruiu added a comment.Oct 4 2017, 8:30 PM

I created https://reviews.llvm.org/D38573 to show you what I meant by my previous comment. Please apply it to your patch.

Basically, if you want to name a function "finalize", it is likely a sign that that is not a good name nor a good function. It just says "it runs at last" and doesn't really explain what the function does. Such function is easy to become a kitchen sink. So, if you try to name your function "finalize" next time, you probably should stop and think about what your function actually does. In most cases, you shouldn't name a function "finalize".

grimar updated this revision to Diff 117784.Oct 5 2017, 1:40 AM
  • Applied code from D38573 as requested.
ruiu accepted this revision.Oct 5 2017, 11:47 AM

LGTM

This revision is now accepted and ready to land.Oct 5 2017, 11:47 AM
This revision was automatically updated to reflect the committed changes.