Skip to content

Deprecate the asm! macro in favor of llvm_asm! #71007

New issue

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

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

Already on GitHub? # to your account

Merged
merged 3 commits into from
Apr 20, 2020

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Apr 10, 2020

Since we will be changing the syntax of asm! soon, deprecate it and encourage people to use llvm_asm! instead (which preserves the old syntax). This will avoid breakage when asm! is changed.

RFC: rust-lang/rfcs#2843

@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

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

@rust-highfive
Copy link
Contributor

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 10, 2020
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

I have not reviewed the stdarch submodule bump, but this otherwise looks good. I think we should hold off on landing this until we have a replacement in nightly for some time (say, 2 weeks) just to give people time to migrate without generating lots of warnings in their code.

(Have we landed llvm_asm yet? I think no, but I don't remember. If we have, my concern is moot of course.)

@Amanieu
Copy link
Member Author

Amanieu commented Apr 10, 2020

llvm_asm! landed in #68404 about 2 weeks ago.

@Amanieu
Copy link
Member Author

Amanieu commented Apr 10, 2020

The stdarch submodule bump also includes additional ARM NEON intrinsics, which are all unstable.

@Mark-Simulacrum
Copy link
Member

I think the current note is slightly better, as it directly suggests why we're doing it and gives the user a chance to try to find out why the asm! syntax will change soon. I suspect that ideally we'd include a link to some tracking issue or whatever, but that can happen separately.

@Amanieu -- does the "suggestion" in the deprecation make cargo fix work?

@Mark-Simulacrum Mark-Simulacrum 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 Apr 11, 2020
@Amanieu
Copy link
Member Author

Amanieu commented Apr 11, 2020

It should work with cargo fix but I haven't tried it yet.

@Mark-Simulacrum
Copy link
Member

Okay, that's good enough for me (and Centril has separately indicated to me that he's okay with the existing comment), so @bors r+

@bors
Copy link
Collaborator

bors commented Apr 11, 2020

📌 Commit 43ee31f has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 11, 2020
Centril added a commit to Centril/rust that referenced this pull request Apr 11, 2020
…acrum

Deprecate the asm! macro in favor of llvm_asm!

Since we will be changing the syntax of `asm!` soon, deprecate it and encourage people to use `llvm_asm!` instead (which preserves the old syntax). This will avoid breakage when `asm!` is changed.

RFC: rust-lang/rfcs#2843
@bors
Copy link
Collaborator

bors commented Apr 13, 2020

⌛ Testing commit 43ee31f with merge c9f88ad146aa20172d3a0e13098c707238fa0be5...

@bors
Copy link
Collaborator

bors commented Apr 13, 2020

💔 Test failed - checks-azure

@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 Apr 13, 2020
@Amanieu
Copy link
Member Author

Amanieu commented Apr 14, 2020

Seems spurious @bors retry

@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 Apr 14, 2020
@bors
Copy link
Collaborator

bors commented Apr 14, 2020

⌛ Testing commit 43ee31f with merge 71bccf0e8e6d51e37fda2d367be79597df0413f2...

@bors
Copy link
Collaborator

bors commented Apr 14, 2020

💔 Test failed - checks-azure

@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 Apr 14, 2020
@Amanieu
Copy link
Member Author

Amanieu commented Apr 15, 2020

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Apr 15, 2020

📌 Commit ce83b49 has been approved by Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Apr 15, 2020

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@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 Apr 15, 2020
@bors
Copy link
Collaborator

bors commented Apr 20, 2020

⌛ Testing commit ce83b49 with merge 9b2f8db...

@bors
Copy link
Collaborator

bors commented Apr 20, 2020

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing 9b2f8db to master...

@berkus
Copy link
Contributor

berkus commented Apr 24, 2020

Is #![feature(asm)] also going to be renamed?

@Amanieu
Copy link
Member Author

Amanieu commented Apr 24, 2020

llvm_asm! is gated behind the llvm_asm feature.

@samuela
Copy link

samuela commented May 31, 2020

The error message when switching from asm! to llvm_asm! is confusing:

error[E0658]: use of unstable library feature 'llvm_asm': LLVM-style inline assembly will never be stabilized, prefer using asm! instead
   --> util_linux/volume_id/sysv.rs:253:17
    |
253 |                 llvm_asm!("bswap $0" : "=r" (fresh4) : "0"
    |                 ^^^^^^^^
    |
    = note: see issue #70173 <https://github.com/rust-lang/rust/issues/70173> for more information
    = help: add `#![feature(llvm_asm)]` to the crate attributes to enable

even though I just got an error message telling me to use llvm_asm! instead of asm!. This is resolved with #![feature(llvm_asm)] as expected, but confusing nonetheless.

@mark-i-m
Copy link
Member

@samuela Haha, yes, could you please open an issue for that? The asm macro's syntax changed. What the error means is to use the new asm style, rather than llvm/gcc style inline asm.

@Amanieu
Copy link
Member Author

Amanieu commented May 31, 2020

I opened #72825 to clarify the messages.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 2, 2020
Clarify errors and warnings about the transition to the new asm!

Hopefully addresses the concerns from rust-lang#71007 (comment).
# for free to join this conversation on GitHub. Already have an account? # 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants