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

Move asm! and global_asm! to core::arch #1183

Merged
merged 1 commit into from
Jul 15, 2021
Merged

Conversation

bstrie
Copy link
Contributor

@bstrie bstrie commented Jul 14, 2021

Implements the first part of the libs team decision given at rust-lang/rust#84019 (comment) .

The first step is to define these items in stdarch. Once this PR is accepted, I have another patch prepared for the Rust repo that will simultaneously remove the definitions in libcore and bump the stdarch submodule.

cc @Amanieu

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@bstrie
Copy link
Contributor Author

bstrie commented Jul 14, 2021

error[E0773]: attempted to define built-in macro more than once

This error is expected. Because this is a submodule, there's no way to atomically remove the definitions in libcore and add the definitions here. One way or the other we have to either temporarily break stdarch or temporarily break the Rust repo, so we'll just have to live with HEAD here being broken until the Rust repo PR (which I already have written, and passes the tests) can be merged.

reason = "inline assembly is not stable enough for use and is subject to change"
)]
#[rustc_builtin_macro]
pub macro asm("assembly template", $(operands,)* $(options($(option),*))?) {
Copy link
Member

Choose a reason for hiding this comment

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

asm! is currently defined using macro_rules!. What are the differences of using pub macro compared to macro_rules!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To export a macro_rules macro you have to use #macro_export, which both makes the macro public and declares the macro in the crate root. Being declared in the crate root is undesirable because it means the macro is effectively "in the prelude" whether you want it to be or not. It also means that documentation lists the macro as being defined in the crate root, which is why the stdlib API docs display a giant wall of random macros (https://doc.rust-lang.org/std/index.html#macros).

In contrast, pub macro makes the macro public without defining it in the crate root. This is strictly more flexible, since we can add it to the prelude if we choose (which I will be doing with my aforementioned Rust repo PR, so as to not break nightly users). It also means we can have the documentation live somewhere sensible and avoid further cluttering the main stdlib API index.

As far as the implementation goes, all the actual macro machinery gets ignored since it's ultimately just a builtin. So the proper namespacing should be the only thing different here AFAIK.

@Amanieu Amanieu merged commit 9437b11 into rust-lang:master Jul 15, 2021
@Amanieu
Copy link
Member

Amanieu commented Jul 15, 2021

Great! I don't mind stdarch being broken for a bit, we don't update the submodule in rust-lang/rust very often.

bstrie added a commit to bstrie/stdarch that referenced this pull request Jul 18, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 19, 2021
Move asm! and global_asm! to core::arch

Follow-up to rust-lang/stdarch#1183 .

Implements the libs-api team decision from rust-lang#84019 (comment) .

In order to not break nightly users, this PR also adds the newly-moved items to the prelude. However, a decision will need to be made before stabilization as to whether these items should remain in the prelude. I will file an issue for this separately.

Fixes rust-lang#84019 .

r? `@Amanieu`
Amanieu pushed a commit that referenced this pull request Jul 20, 2021
nooop3 added a commit to LearningOS/lab0-1-run-os2-nooop3 that referenced this pull request Jul 10, 2022
nooop3 added a commit to LearningOS/lab0-1-run-os2-nooop3 that referenced this pull request Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants