This is an archive of the discontinued LLVM Phabricator instance.

Infer relocation model from module flags in relocatable LTO link
ClosedPublic

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

Details

Reviewers
pcc
Summary

Fix for PR33096.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis created this revision.May 19 2017, 5:25 PM

LLD fix coming in a separate patch.
I've opted not to change current behavior for shared and executable linking, but now I'm having second thoughts. Non-LTO pipeline allows the user to put PIC code in a non-PIE executable, or non-PIC code in a shared library if they wish so. Maybe we should completely ignore the output type here and strictly go by module flags, to be consistent.

pcc edited edge metadata.May 19 2017, 6:38 PM

Non-LTO pipeline allows the user to put PIC code in a non-PIE executable, or non-PIC code in a shared library if they wish so. Maybe we should completely ignore the output type here and strictly go by module flags, to be consistent.

Disabling PIC can sometimes act as an optimization. We may want to replace the reloc model field in lto::Config with a boolean that gives the backend permission to generate position-dependent code. Before we do that though, I think we will need to decide what to do about the other reloc models (dynamic-no-pic, rwpi, ropi, ropi-rwpi).

lib/LTO/LTO.cpp
117–120

I'd rewrite this with if/else and drop the .hasValue() here.

lib/LTO/LTOBackend.cpp
139

Can you pass the module into createTargetMachine and inline this function into it?

140

Drop .hasValue().

joerg added a subscriber: joerg.May 20 2017, 4:13 AM
In D33372#760060, @pcc wrote:

Non-LTO pipeline allows the user to put PIC code in a non-PIE executable, or non-PIC code in a shared library if they wish so. Maybe we should completely ignore the output type here and strictly go by module flags, to be consistent.

Disabling PIC can sometimes act as an optimization.

Disabling PIC was a performance hack on 32bit x86 due to the extreme register starvation of the architecture.
It is also generally agreed that it is fundamentally broken from a design perspective. I don't see good reasons for supporting that, frankly.

pcc added a comment.May 22 2017, 11:20 AM
In D33372#760060, @pcc wrote:

Non-LTO pipeline allows the user to put PIC code in a non-PIE executable, or non-PIC code in a shared library if they wish so. Maybe we should completely ignore the output type here and strictly go by module flags, to be consistent.

Disabling PIC can sometimes act as an optimization.

Disabling PIC was a performance hack on 32bit x86 due to the extreme register starvation of the architecture.
It is also generally agreed that it is fundamentally broken from a design perspective. I don't see good reasons for supporting that, frankly.

Disabling PIC on its own doesn't seem that problematic to me. The problematic part is the assumption in the backend that symbols are DSO-local, which leads to hacks such as copy relocations. But with LTO the linker knows whether each symbol is DSO-local, and it could in principle pass that information to the backend, which could then generate appropriate code for accessing each symbol. This is blocked on something like D20217 though.

eugenis updated this revision to Diff 99803.May 22 2017, 1:38 PM
eugenis marked 3 inline comments as done.
pcc accepted this revision.May 22 2017, 1:43 PM

LGTM with one nit

lib/LTO/LTOBackend.cpp
119–120

Remove TheTriple argument, it can always be computed from the module.

This revision is now accepted and ready to land.May 22 2017, 1:43 PM
eugenis updated this revision to Diff 99806.May 22 2017, 1:47 PM
eugenis marked an inline comment as done.
eugenis closed this revision.May 22 2017, 2:18 PM

r303578