Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not assume dynamic linking for musl/mips[el] targets #47663

Merged
merged 6 commits into from
Jan 29, 2018
Merged

Do not assume dynamic linking for musl/mips[el] targets #47663

merged 6 commits into from
Jan 29, 2018

Conversation

malbarbo
Copy link
Contributor

All musl targets except mips[el] assume static linking by default. This can be confusing.

When the musl/mips[el] targets was added, dynamic linking was chosen because of binary size concerns, and probably also because libunwind didn't supported mips.

Now that we have crt-static target-feature (the user can choose dynamic link for musl targets), and libunwind 6.0 add support to mips, we do not need to assume dynamic linking.

@malbarbo
Do not assume dynamic linking for musl/mips[el] targets
68db72d 
All musl targets except mips[el] assume static linking by default. This
can be confusing
https://users.rust-lang.org/t/static-cross-compiled-binaries-arent-really-static/6084

When the musl/mips[el] targets was
[added](rust-lang#31298), dynamic linking
was chosen because of binary size concerns, and probably also because
libunwind
[didn't](https://users.rust-lang.org/t/static-cross-compiled-binaries-arent-really-static/6084/8)
supported mips.

Now that we have `crt-static` target-feature (the user can choose
dynamic link for musl targets), and libunwind
[6.0](https://github.com/llvm-mirror/libunwind/commits/release_60) add
support to mips, we do not need to assume dynamic linking.
@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@malbarbo malbarbo mentioned this pull request Jan 22, 2018
Merged
@malbarbo
Copy link
Contributor Author

Waiting rust-lang/libc#908

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned aturon Jan 22, 2018
@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2018
@alexcrichton
Copy link
Member

cc @japaric I think you originally added these targets, right? If so, do you have thoughts on this?

@japaric
Copy link
Member

japaric commented Jan 23, 2018

I think it's a good idea to support both static linking and dynamic linking now that libunwind supports MIPS.

Does this change the default linking mode, @malbarbo? Will mips-musl continue to link dynamically after this PR? I can't tell from the diff.

I don't know if we want to change it so that musl = static by default everywhere; that may be less confusing (cf. PR description) but it would be a breaking change to change the default for this target as people could be relying on dynamic linking for their CI deployments, etc.

@malbarbo
Copy link
Contributor Author

Yes, this changes the default linking mode to static.

It is a breaking change. But we some times do breaking changes, like when the android arm target was changed (Off course, it was included in the release notes).

We need to evaluate if the breaking change here worth it. I think so, mainly because the others musl targets uses static linking by default.

Anyway, we can revert to dynamic linking by default setting crt_static_default = false in the target definition, while still allowing static linking.

@alexcrichton
Copy link
Member

@malbarbo I wonder, could we perhaps land this without changing the defaults, and then consider that separately? Perhaps with a post to internals to try to catch users of the current target?

@malbarbo
Copy link
Contributor Author

Ok, I changed crt_static_default = false.

@alexcrichton
Copy link
Member

Ok! As a final question, could we use the more recent version of the unwinder for all targets? (AFAIK there's no technical reason to stick to an older version)

@malbarbo
Copy link
Contributor Author

Done.

@malbarbo
Copy link
Contributor Author

Internals post: https://internals.rust-lang.org/t/default-linking-mode-for-musl-mips-el-targets/6634

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 23, 2018

📋 Looks like this PR is still in progress, ignoring approval

@malbarbo
Copy link
Contributor Author

We should wait for rust-lang/libc#908

@alexcrichton
Copy link
Member

Ah ok! I thought that with the change in defaults this may no longer need the PR. I left a comment over there

@malbarbo
Copy link
Contributor Author

Add -fPIC. Without it, dynamic linking fails.

@alexcrichton
Copy link
Member

Ok! Want to update libc here too?

@malbarbo
Copy link
Contributor Author

Update libc.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 24, 2018

📋 Looks like this PR is still in progress, ignoring approval

@malbarbo malbarbo changed the title [WIP] Do not assume dynamic linking for musl/mips[el] targets Do not assume dynamic linking for musl/mips[el] targets Jan 24, 2018
@malbarbo
Copy link
Contributor Author

Remove WIP from title issue... Is this the reason bors is complaining?

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 24, 2018

📌 Commit cd47ddf has been approved by alexcrichton

@alexcrichton
Copy link
Member

Apparently so!

@bors
Copy link
Contributor

bors commented Jan 27, 2018

⌛ Testing commit cd47ddf with merge c188e448eb8729aa4ea1ef7a458522d08f54c875...

@bors
Copy link
Contributor

bors commented Jan 27, 2018

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Jan 27, 2018

ui/test-should-panic-attr.rs failed on musl due to "fatal runtime error: failed to initiate panic, error 5", legit.

[00:56:16] ---- [ui] ui/test-should-panic-attr.rs stdout ----
[00:56:16] 	
[00:56:16] error: test run failed!
[00:56:16] status: signal: 6
[00:56:16] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/test-should-panic-attr.stage2-i686-unknown-linux-musl"
[00:56:16] stdout:
[00:56:16] ------------------------------------------
[00:56:16] 
[00:56:16] running 5 tests
[00:56:16] 
[00:56:16] ------------------------------------------
[00:56:16] stderr:
[00:56:16] ------------------------------------------
[00:56:16] fatal runtime error: failed to initiate panic, error 5
[00:56:16] 
[00:56:16] ------------------------------------------
[00:56:16] 
[00:56:16] thread '[ui] ui/test-should-panic-attr.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:2883:9
[00:56:16] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:56:16] 
[00:56:16] 
[00:56:16] failures:
[00:56:16]     [ui] ui/test-should-panic-attr.rs

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2018
@malbarbo
Copy link
Contributor Author

I think we do not need the x86 patch anymore: llvm-mirror/libunwind@aa805e4

I will test it in my machine...

@malbarbo
Copy link
Contributor Author

i686 patch removed (it worked locally).

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 28, 2018

📌 Commit 2875f82 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jan 28, 2018

⌛ Testing commit 2875f82 with merge 385ef15...

bors added a commit that referenced this pull request Jan 28, 2018
@bors
Auto merge of #47663 - malbarbo:mips-crt-static, r=alexcrichton
385ef15 
Do not assume dynamic linking for musl/mips[el] targets

All musl targets except mips[el] assume static linking by default. This can be [confusing](https://users.rust-lang.org/t/static-cross-compiled-binaries-arent-really-static/6084).

When the musl/mips[el] targets was [added](#31298), dynamic linking was chosen because of binary size concerns, and probably also because libunwind [didn't](https://users.rust-lang.org/t/static-cross-compiled-binaries-arent-really-static/6084/8) supported mips.

Now that we have `crt-static` target-feature (the user can choose dynamic link for musl targets), and libunwind [6.0](https://github.com/llvm-mirror/libunwind/commits/release_60) add support to mips, we do not need to assume dynamic linking.
@bors
Copy link
Contributor

bors commented Jan 29, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 385ef15 to master...

@bors bors merged commit 2875f82 into rust-lang:master Jan 29, 2018
@bors bors mentioned this pull request Jan 29, 2018
Merged
@petrochenkov petrochenkov mentioned this pull request Feb 28, 2021
Closed
@12101111 12101111 mentioned this pull request May 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@malbarbo @rust-highfive @alexcrichton @japaric @bors @kennytm @carols10cents @aturon