Skip to content

Change Linker for x86_64-fortanix-unknown-sgx target to rust-lld - #66957

Merged
bors merged 1 commit into
rust-lang:masterfrom
parthsane:pvs/ftx_lld_linker
Dec 3, 2019
Merged

Change Linker for x86_64-fortanix-unknown-sgx target to rust-lld#66957
bors merged 1 commit into
rust-lang:masterfrom
parthsane:pvs/ftx_lld_linker

Conversation

@parthsane

@parthsaneparthsane commented Dec 2, 2019

Copy link
Copy Markdown
Contributor

Changed linker for x86_64-fortanix-unknown-sgx target to rust-lld
This change needed the RelaxELFRelocations flag to be set for it to work correctly

r? @jethrogb

@parthsane

parthsane commented Dec 2, 2019

Copy link
Copy Markdown
ContributorAuthor

r? @alexcrichton

jonas-schievink
jonas-schievink reviewed Dec 2, 2019
View reviewed changes
Comment threadsrc/librustc_codegen_ssa/back/linker.rs Outdated
jethrogb
jethrogb reviewed Dec 2, 2019
View reviewed changes
Comment threadsrc/librustc_codegen_ssa/back/linker.rs Outdated
jethrogb
jethrogb reviewed Dec 2, 2019
View reviewed changes
Comment threadsrc/rustllvm/PassWrapper.cpp Outdated

@jethrogbjethrogbDec 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing the effects of this is difficult, but I've filed fortanix/rust-sgx#202 to track possible future improvements.

@parthsaneparthsane changed the title Set RelaxELFRelocations if linker flavor is lldChange Linker for x86_64-fortanix-unknown-sgx target to rust-lldDec 2, 2019
@alexcrichton

alexcrichton commented Dec 2, 2019

Copy link
Copy Markdown
Member

Could relaxation of elf relocations be a separate parameter in the target configuration? There are existing targets using LLD and this may run a risk of breaking them if they weren't expecting this to be turned on.

@jethrogb

jethrogb commented Dec 2, 2019

Copy link
Copy Markdown
Contributor

From what I understand, the only reason to not use the relaxations is if you're using a linker that doesn't understand them. LLD does, so there is no reason to turn them off then. Code generation might be improved if the option is turned on.

I might be wrong since the feature isn't documented that well.

@alexcrichton

alexcrichton commented Dec 2, 2019

Copy link
Copy Markdown
Member

If you'd like to pursue this change as-is, then I'd have a similar comment I left on another PR. I do not have the confidence to r+ this change as-is, so you'll need to find a different reviewer. I unfortunately don't know who that different reviewer would be, but you'd likely want to start with the Compiler team.

@Centril

Centril commented Dec 2, 2019

Copy link
Copy Markdown
Contributor

cc @rust-lang/compiler-contributors for possible other reviewer

bjorn3
bjorn3 reviewed Dec 3, 2019
View reviewed changes
Comment threadsrc/rustllvm/PassWrapper.cpp Outdated
@parthsane
Change linker for x86_64-fortanix-unknown-sgx to rust-lld
54b2060
For SGX, the relocation using the relocation table is done by
the code in rust/src/libstd/sys/sgx/abi/reloc.rs and this code
should not require relocation. Setting RelaxELFRelocations flag
if allows this to happen, hence adding a Target Option for it.
@jethrogb

jethrogb commented Dec 3, 2019

Copy link
Copy Markdown
Contributor

LGTM

@parthsane

parthsane commented Dec 3, 2019

Copy link
Copy Markdown
ContributorAuthor

I have added relax_elf_relocation as a Target Option and it is false by default. This should prevent the risk of breaking other targets, just in case.
@alexcrichton

@alexcrichton

alexcrichton commented Dec 3, 2019

Copy link
Copy Markdown
Member

@bors: r+

👍

@bors

bors commented Dec 3, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 54b2060 has been approved by alexcrichton

@borsbors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 3, 2019
Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
@Centril
Rollup merge ofrust-lang#66957- parthsane:pvs/ftx_lld_linker, r=ale…
01b6be0
…xcrichton
Change Linker for x86_64-fortanix-unknown-sgx target to rust-lld
Changed linker for `x86_64-fortanix-unknown-sgx` target to `rust-lld`
This change needed the RelaxELFRelocations flag to be set for it to work correctly
r? @jethrogb
@CentrilCentril mentioned this pull request Dec 3, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
@Centril
Rollup merge ofrust-lang#66957- parthsane:pvs/ftx_lld_linker, r=ale…
69f1323
…xcrichton
Change Linker for x86_64-fortanix-unknown-sgx target to rust-lld
Changed linker for `x86_64-fortanix-unknown-sgx` target to `rust-lld`
This change needed the RelaxELFRelocations flag to be set for it to work correctly
r? @jethrogb
@CentrilCentril mentioned this pull request Dec 3, 2019
Merged
bors added a commit that referenced this pull request Dec 3, 2019
@bors
Auto merge of#66997- Centril:rollup-uraqpgu, r=Centril
7afe6d9
Rollup of 7 pull requests
Successful merges:
- #66750 (Update the `wasi` crate for `wasm32-wasi`)
- #66878 (Move Sessions into (new) librustc_session)
- #66903 (parse_enum_item -> parse_enum_variant)
- #66951 (miri: add throw_machine_stop macro)
- #66957 (Change Linker for x86_64-fortanix-unknown-sgx target to rust-lld)
- #66960 ([const-prop] Fix ICE calculating enum discriminant)
- #66973 (Update the minimum external LLVM to 7)
Failed merges:
r? @ghost
@bors
bors merged commit 54b2060 into rust-lang:masterDec 3, 2019
@jethrogbjethrogb mentioned this pull request Dec 9, 2019
Merged
bors added a commit that referenced this pull request Dec 12, 2019
@bors
Auto merge of#67172- jethrogb:jb/bootstrap-linker, r=alexcrichton
3eeb8d4
Bootstrap: change logic for choosing linker and rpath
This is a follow-up from #66957 and #67023. Apparently there was one more location with a hard-coded list of targets to influence linking.
I've filed #67171 to track this madness.
r? @alexcrichton
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjorn3bjorn3bjorn3 left review comments
+2 more reviewers
@jethrogbjethrogbjethrogb left review comments
@jonas-schievinkjonas-schievinkjonas-schievink left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@alexcrichtonalexcrichton

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@parthsane@alexcrichton@jethrogb@Centril@bors@jonas-schievink@bjorn3