This is an archive of the discontinued LLVM Phabricator instance.

[lld] Infer relocation model from module flags in relocatable LTO link
ClosedPublic

Authored by eugenis on May 19 2017, 5:40 PM.

Details

Reviewers
pcc
davide
Summary

Fix for PR33096.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis created this revision.May 19 2017, 5:40 PM
davide added a subscriber: davide.May 19 2017, 5:46 PM
davide added inline comments.
test/ELF/lto/relocation-model-pic.ll
3–6 ↗(On Diff #99650)

can't this be a separate input in living in Inputs/ ?

eugenis added inline comments.May 19 2017, 5:48 PM
test/ELF/lto/relocation-model-pic.ll
3–6 ↗(On Diff #99650)

It can, but then the reader has to go and find this other file to understand what's going on in the test...
I can do it if you feel strongly about it.

davide added a subscriber: ruiu.May 19 2017, 5:51 PM
davide added inline comments.
ELF/LTO.cpp
76–82

I'll leave the final decision to @ruiu, but I feel

if ()
  [...]
else if()
  [...]
 else()

would be more readable in this case.
Not strong about this one either :)

test/ELF/lto/relocation-model-pic.ll
3–6 ↗(On Diff #99650)

Not necessarily, but this is generally what's done elsewhere in lld, so I'd keep it like that for consistency.

ruiu added inline comments.May 19 2017, 5:54 PM
ELF/LTO.cpp
76–82

I was about to say the same. :)

eugenis updated this revision to Diff 99656.May 19 2017, 5:59 PM
eugenis marked 2 inline comments as done.
davide accepted this revision.May 19 2017, 6:04 PM

LGTM. I see Peter confirmed this is the correct fix in the PR, and I think so too.

This revision is now accepted and ready to land.May 19 2017, 6:04 PM
eugenis closed this revision.May 22 2017, 2:18 PM

r303579