Add TargetOption::force_pic_relocation_model and use it for mips64

[draganmladjenovic]

Makes mips64 gnu targets use PIC relocation model regardless of what user specifies. See #49421.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[nagisa]

Forcing some option regardless of what user specifies is not a well behaved thing to do regardless of the intention.

User specification should take precedence over everything else, potentially emitting an error if the option cannot be possibly honoured – and reloc-model=static can.

[draganmladjenovic]

@nagisa Hi, sorry for the late replay. I agree on this. It can be confusing if a compiler ignores an option w/o even emitting a warning. You already have something like this in form of crt_static_respected target option. I have no strong opinion about this issue, but it kinda looks like an breaking change form LLVM 4 behavior. Then again I don't have a full picture of what is the common use case of user choosing other relocation model (static) other that target's default (pic), if there is one : producing ET_EXEC binaries, static c libraries ? In the end I can submit the PR to disable those tests (#49421) on mips64 targets.

[michaelwoerister]

r? @japaric

[bors]

☔ The latest upstream changes (presumably #49753) made this pull request unmergeable. Please resolve the merge conflicts.

[pietroalbini]

Ping from triage! Can @japaric or someone else from @rust-lang/compiler review this?

[nagisa]

Okay, so, in my opinion we should just disable the test for MIPS here and honour what the user explicitly specifies on the command line.

Adding an option to specifically not honour the user does not seem right.

[estebank]

@nagisa I would add to that a warning/error when targeting mips64_unknown_linux_gnuabi64 and get_reloc_model(sess) != llvm::RelocMode::PIC .

[nagisa]

Why? if the request is incompatible the users will get a linking failure and emiting a warning where there might be issues but also might not be any is a bad UX. Of course linker asserting is not a nice failure mode, but this is something that ought to be fixed within the linker not rustc.

…

On Tue, Apr 17, 2018, 00:34 Esteban Kuber ***@***.***> wrote: @nagisa <https://github.com/nagisa> I would add to that a warning/error when targeting mips64_unknown_linux_gnuabi64 and get_reloc_model(sess) != llvm::RelocMode::PIC. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49508 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApc0qVRhbzm41SNLAUTB0pLngyai7eRks5tpQ5VgaJpZM4TBq4p> .

[estebank]

@nagisa depends on how bad the actual linking error is. If we can easily avoid an error that no one understands by preempting it in rustc, I think it is a clear win, but I say this without knowing what the current output is.

[nagisa]

Currently the linker for MIPS crashes, but even outside that, relocation model mismatches yield fairly unhelpful errors and they are unhelpful universally (that is, unhelpful outside of MIPS too).

Alas, it is not possible to implement nicer errors properly without implementing a linker ourselves (or using a different linker) and a warning is even less feasible due to multiple models being used widely.

[pietroalbini]

Ping from triage @nagisa! What's the status of this PR?

[draganmladjenovic]

@pietroalbini, @nagisa I will submit the new PR to disable the failing relocation_model tests. Closing this then.