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.

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

Add features gates for experimental asm features #90348

Merged
merged 2 commits into from
Nov 7, 2021

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Oct 27, 2021

This PR splits off parts of asm! into separate features because they are not ready for stabilization.

Specifically this adds:

  • asm_const for const operands.
  • asm_sym for sym operands.
  • asm_experimental_arch for architectures other than x86, x86_64, arm, aarch64 and riscv.

r? @nagisa

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 27, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 28, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@joshtriplett
Copy link
Member

Looks reasonable to me. r=me once it passes CI.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member Author

Amanieu commented Nov 1, 2021

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Nov 1, 2021

📌 Commit 53ff09f123d246c2e86ce9aba39027b9dfa82009 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 1, 2021
@bors
Copy link
Contributor

bors commented Nov 2, 2021

⌛ Testing commit 53ff09f123d246c2e86ce9aba39027b9dfa82009 with merge 5f7f78f5227ff3342c84fe250ac1e9b31a908c51...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 2, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 2, 2021
@bors
Copy link
Contributor

bors commented Nov 3, 2021

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

@Amanieu Amanieu force-pushed the asm_feature_gates branch 2 times, most recently from 8fabce2 to 752d4ae Compare November 3, 2021 12:26
@Amanieu
Copy link
Member Author

Amanieu commented Nov 3, 2021

@bors r=joshtriplett rollup=iffy

@bors
Copy link
Contributor

bors commented Nov 7, 2021

📌 Commit b88821c5abbc65dfb17eff7094b4b05e9ed523e6 has been approved by joshtriplett

@bors
Copy link
Contributor

bors commented Nov 7, 2021

⌛ Testing commit b88821c5abbc65dfb17eff7094b4b05e9ed523e6 with merge b0ee314d7a7490dd02bd701d23b5396c8b817e02...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 7, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 7, 2021
@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member Author

Amanieu commented Nov 7, 2021

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Nov 7, 2021

📌 Commit 87d0d64 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2021
@bors
Copy link
Contributor

bors commented Nov 7, 2021

⌛ Testing commit 87d0d64 with merge 90a273b...

@bors
Copy link
Contributor

bors commented Nov 7, 2021

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 90a273b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 7, 2021
@bors bors merged commit 90a273b into rust-lang:master Nov 7, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 7, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (90a273b): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@bnaecker
Copy link

bnaecker commented Nov 8, 2021

I know this has already been merged, but I'm a bit concerned about this change. For context, I maintain the usdt crate, which allows developers to insert DTrace probes into userland Rust code. For a variety of reasons, that crate works by generating declarative macros (macros-by-example) for each probe, whose expanded bodies contain inline assembly. Because the expansion site contains asm!, which requires a feature flag, developers using the usdt crate have previously been required to include a #![feature(asm)] in their own code that defines and uses probes. As of this change, there are now two features required for downstream users of the usdt crate, asm and (at least on macOS platforms), asm_sym.

More generally, this manner in which this new flag was included seems fraught. As I understand it, any code with a call to asm! that uses a symbol operand will not compile across this threshold. Existing code has been effectively hidden behind a new feature flag.

I'm not sure how to resolve this. Since there is no support for compile-time detection of the Rust toolchain (that I'm aware of!), it doesn't seem possible to write a library that uses symbol operands, and that can be compiled across this change. Including the asm_sym flag will generate compiler errors on an older toolchain, since that feature flag is unknown in that case. But omitting the asm_sym flag will generate compiler errors on this toolchain. How can one write the set of feature flags required to build such a library for a range of compiler versions?

cc @ahl

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 10, 2021
…=Mark-Simulacrum

kmc-solid: Avoid the use of `asm_const`

This PR removes the use of [the now-separated-out `asm_const` compiler feature][1] in `std::sys::solid` to fix the [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets.

[1]: rust-lang#90348
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 10, 2021
…=Mark-Simulacrum

kmc-solid: Avoid the use of `asm_const`

This PR removes the use of [the now-separated-out `asm_const` compiler feature][1] in `std::sys::solid` to fix the [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets.

[1]: rust-lang#90348
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants