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

Namespace the asm! macro #84019

Closed
bstrie opened this issue Apr 8, 2021 · 12 comments · Fixed by #87227
Closed

Namespace the asm! macro #84019

bstrie opened this issue Apr 8, 2021 · 12 comments · Fixed by #87227
Labels
A-inline-assembly Area: inline asm!(..) F-asm `#![feature(asm)]` (not `llvm_asm`) T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@bstrie
Copy link
Contributor

bstrie commented Apr 8, 2021

Click here for the final summary report.

Original issue preserved below:


As of today, most macros in Rust, despite being declared by the stdlib, are not properly namespaced in the sense that every other type, function, and trait in the stdlib is namespaced. Instead, historically, macros "live" in the crate root due to the technical limitations of Rust 1.0, which did not support namespacing for macros. This is distinct from items such as Option or Vec which are merely available as though they live in the crate root (via the prelude), but are actually defined at std::option::Option and std::vec::Vec. This has two primary downsides:

  1. The stdlib API index is polluted by a wall of random, unrelated macros, many of which are relatively unimportant or irrelevant: https://doc.rust-lang.org/std/index.html#macros
  2. Defining an item in the crate root is tantamount to exporting that item from the prelude, which means that adding a macro there totally bypasses the discussion of whether the macro should be exported from the prelude.

In Rust 1.51, ptr::addr_of became the first stable macro to not be defined in the crate root. The machinery exists to namespace macros, and there seems to be at least loose consensus that this is worth using for new macros in the stdlib.

One of the last remaining open questions for the stabilization of the asm macro is where it should live. Given the above, and the history of prior discussion on this topic, there are two options for consideration:

  1. core::arch::asm
  2. core::arch::foo64::asm, where foo64 is every architecture supported by the asm macro.

(For conciseness I will only be referring to asm in this document, but this decision also applies to any and all related macros, such as global_asm.)

First, the non-advantages of either option:

  1. Compile-time rejection of unsupported platforms: while arch::foo64::asm makes it immediately and syntactically obvious that a given platform is unsupported by dint of a nonexistent symbol, using arch::asm on an unsupported platform is still a compiler error.
  2. Inclusion in the prelude: while arch::asm is straightforwardly obvious to export from the prelude, arch::foo64::asm and friends could still possibly be exported from the prelude via #cfg.
  3. Architecture-dependent behavior: even aside from the literal assembly code itself, it is possible for the asm macro to have slightly different semantics on different architectures, e.g. architecture-specific register classes or clobbering behavior. While arch::foo64::asm is more explicit about this potential difference, it is not necessary for enabling such behavior.
  4. Target-specific stabilization or deprecation: while arch::foo64::asm is more straightforward to deprecate/stabilize on a per-target basis, #cfg can be used to the same effect even for arch::asm.

The relevant differences between the the two options:

  1. Importing the symbol: even assuming that asm is not added to the prelude, arch::asm would still be trivial to use in architecture-dependent code. Conversely, without a prelude addition, arch::foo64::asm would have to fall back on a few patterns when writing architecture-dependent code, some of which are more verbose than others. Below, Pattern add an IL type checker #4 is the one that comes closest to the ergonomics of arch::asm, but it may be non-obvious, and it involves using a glob-import, which some may find distasteful (or even be linting against):
// Pattern #1 ----------------------------------------
// fully-qualified names
#[cfg(foo64)]
fn qux() {
    std::arch::foo64::asm!(...);
}
#[cfg(bar64)]
fn qux() {
    std::arch::bar64::asm!(...);
}
// Pattern #2 ----------------------------------------
// doubled #cfg attributes
#[cfg(foo64)]
use std::arch::foo64::asm;
#[cfg(bar64)]
use std::arch::bar64::asm;

#[cfg(foo64)]
fn qux() {
    asm!(...);
}
#[cfg(bar64)]
fn qux() {
    asm!(...);
}
// Pattern #3 ----------------------------------------
// context-local use
#[cfg(foo64)]
fn qux() {
    use std::arch::foo64::asm;
    asm!(...);
}
#[cfg(bar64)]
fn qux() {
    use std::arch::bar64::asm;
    asm!(...);
}
// Pattern #4 ----------------------------------------
// glob import
use std::arch::*;

#[cfg(foo64)]
fn qux() {
    foo64::asm!(...);
}
#[cfg(bar64)]
fn qux() {
    bar64::asm!(...);
}
  1. Documentation: the documentation of arch::asm would have to document all architecture-dependent behavior, which could make it unwieldy. arch::foo64::asm would isolate architecture-dependent documentation, however, every module would be forced to duplicate all non-architecture-dependent asm documentation, which seems unfortunate in its own way.
  2. Error reporting in the event of unguarded asm: having arch::asm, or else having arch::foo64::asm but having asm in the prelude, increases the chances that someone will write an asm! invocation that is not guarded by any #cfg directive or any other mention of the original author's platform. Such an unguarded invocation would probably give unhelpful errors if the code is ever compiled for a platform that supports asm in general but was not considered by this specific asm invocation. Even worse, the code could compile but have unintended behavior. Conversely, arch::foo64::asm essentially guarantees that an author's code will have to mention their intended platform somewhere, either in a use or in an expression, and users on different platforms will receive obvious "cannot find value foo64::asm in this scope" errors when attempting to compile it. However, this benefit requires that asm never be added to the prelude (and never adding any other way of circumventing the need to mention a platform, e.g. having both arch::asm and arch::foo64::asm).
  3. Module proliferation: currently, asm supports architectures that do not have a corresponding module under std::arch. The arch::foo64::asm approach would require a module for all supported architectures going forward. This includes adding modules for targets that may not ever be supported by rustc itself (e.g. SPIR-V), but are supported by the asm! macro for the benefit of alternative backends. However, in either scenario alternative backends would still need to provide a patch to support new targets in asm!, and adding a new module for any unstably-supported target doesn't seem like a particularly onerous part of that process. However, given enough time (and enough alternative backends, e.g. a GCC one) this could lead to quite a few submodules under arch::, although to some degree that is the point of the arch module.
  4. Platform-agnostic assembly: it is possible for a single asm! invocation to properly support multiple targets. This is the flipside of point 3 stated above: where arch::asm is maximally permissive, arch::foo64::asm is maximally strict. With arch::foo64::asm, writing platform-agnostic assembly would require a pattern like the following:
#[cfg(foo64)]
use std::arch::foo64 as fooish;
#[cfg(foo32)]
use std::arch::foo32 as fooish;

fn qux() {
    fooish::asm!(...);
}

TL;DR: the benefits of either approach are fairly small. arch::asm is easier to use, but only if asm is not added to the prelude. arch::foo64::asm could lead to better error messages in some cases, but likewise only if asm is not added to the prelude. If the decision is made to add asm to the prelude, then there is essentially no advantage to either option.

@bstrie bstrie added A-inline-assembly Area: inline asm!(..) T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 8, 2021
@jonas-schievink jonas-schievink added the F-asm `#![feature(asm)]` (not `llvm_asm`) label Apr 8, 2021
@BartMassey
Copy link
Contributor

Strongly support std::arch::foo64::asm.

I find the kind of issue discussed in "3. Error reporting in the event of unguarded asm: having arch::asm" to be determinative.

I'm less worried about error reporting, though, than about having something accidentally cleanly assemble with different semantics because of the wrong architecture.

I have an example lying around somewhere in the issue tracker that will assemble for both x86_64 and aarch64: this is scary, since the operand order for the default x86_64 assembler is source, dest while aarch64 is dest, source. So now you've got probably UB or at least a surprising bug when somebody accidentally compiles the unqualified code for the wrong architecture.

@the8472
Copy link
Member

the8472 commented Apr 9, 2021

patterns

Another approach is to have arch-specific implementations in separate modules where the whole module is cfg'd, so you can use regular imports inside.

@jethrogb
Copy link
Contributor

jethrogb commented Apr 9, 2021

When you write std::arch I assume you mean core::arch, right?

@bstrie
Copy link
Contributor Author

bstrie commented Apr 9, 2021

Yes, this would technically live in core, I was merely ignoring the facade for simplicity.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Apr 9, 2021

I'd like to see it in core::arch, so asm! macro's stabilisation is not tight to a specific ISA's core::arch::<isa> being stable. There are also archs that Rust can target but does not yet have a presence in core::arch, e.g. RISC-V.

@bstrie
Copy link
Contributor Author

bstrie commented Apr 9, 2021

I'd like to see it in core::arch, so asm! macro's stabilisation is not tight to a specific ISA's core::arch:: being stable.

I don't think this is actually a requirement; arch::x86::asm could be stabilized independently of arch::riscv::asm. The converse is also true: arch::asm could be stabilized only on certain targets via selective use of #cfg.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Apr 9, 2021

Oh another think I think it probably does make sense to have them in core::arch::<isa>, as asm! does not support all ISAs at the moment. It seems that nothing prevents us from having core::arch::riscv stablised with just asm! in it.

@bstrie
Copy link
Contributor Author

bstrie commented Apr 11, 2021

Note that there is also an extensive Zulip thread where discussion is taking place: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/namespacing.20.60asm!.60

@repnop
Copy link
Contributor

repnop commented Apr 14, 2021

As a somewhat tangential benefit to namespacing asm! under the specific architecture modules is something like rust-lang/rust-analyzer#6031 where tooling would benefit from understanding which architecture the asm block is for

@bstrie
Copy link
Contributor Author

bstrie commented May 23, 2021

One thing worth keeping in mind is that this decision may influence future macro namespacing. Consider these unstable macros:

is_aarch64_feature_detected
is_arm_feature_detected
is_mips64_feature_detected
is_mips_feature_detected
is_powerpc64_feature_detected
is_powerpc_feature_detected

It would be a bit of a reach to suggest that these macros are universal enough that they need to live in the prelude; they really ought to be namespaced. But should they be namespaced under std::arch or under std::arch::their_own_platform? Whatever is decided for asm and global_asm could very well set precedent for that decision.

@bstrie
Copy link
Contributor Author

bstrie commented Jul 6, 2021

Now that an asm! MVP may be on the horizon, I propose that this issue has spent long enough percolating. I've reviewed the comments from all the prior threads and will here summarize what I believe to be the status of this issue, so that the libs team can make a final decision.

Question

Should the asm! macro be defined as core::arch::asm, or should each supported architecture in core::arch instead export its own copy of the macro (core::arch::x86_64::asm, etc.)?

The most important fundamental difference between these two is that the first does not strictly require the user to have any #cfg directive in their code (though it is conceivable that they often (perhaps even usually) will as part of writing multi-platform code)), and the latter does strictly require it.

Whatever is decided for asm! will also apply to all of its (potential) siblings, e.g. global_asm!, asm_snippet!.

Option 1: one public item, core::arch::asm

Pros:

  1. Trivial to import and invoke; no #cfg is necessary.
  2. Because no #cfg is necessary, writing cross-platform asm! does not require any extra effort.
  3. The list of submodules in core::arch does not need to be tightly coupled to the platforms supported by asm!.
  4. There is no complication regarding the decision of whether to ever add asm! to the prelude.

Cons:

  1. Because there is no strict requirement that a #cfg directive appears anywhere in the user's code, it is possible that a library author might write an asm! block intended for one platform that is then later executed on another platform, which at best would fail to compile, but at worst could run with unknown results.
  2. If there are platform-specific asm! features, the documentation for asm! will have to mention all of them for all platforms.

Option 2: A public item per arch, core::arch::x86_64::asm, etc.

Pros:

  1. Because the presence of a #cfg somewhere is necessary to invoke the macro, users will have no choice but to explicitly gate their invocations to their intended platforms. There is no risk that a library author might write an asm! block for x86 that is then accidentally compiled for (and potentially even runs on, with unknown results) ARM.

Cons:

  1. This introduces tight coupling between the submodules in core::arch and the list of platforms supported by asm!. This would be a departure from the current policy, where the possible submodules in core::arch are determined by available values of the target_arch argument to #cfg, which is part of rustc’s own target stability considerations, which are under the control of the compiler team rather than the libs team. The asm! macro may support platforms that are highly backend-specific, such as SPIR-V via the rust-gpu backend; such modules will nonetheless need to be added to core::arch. This may also complicate the situation of people using target specifications to target unsupported platforms, although many of these may already require changes to the compiler/stdlib in order to make asm! usable in the first place.
  2. Importing and invoking is slightly complicated, due to the need for #cfg. You must either not use the item at all and refer to it only via its fully-qualified name, or #cfg-guard every import and invocation individually, or introduce local use in each #cfg'd context, or glob import std::arch::*, or have distinct #cfg'd modules for every platform.
  3. Because each public item is specific to one #cfg, writing cross-platform asm! has added friction, however this can be addressed via creative use aliases.
  4. Although this approach does not technically prevent Rust from ever adding asm! to the prelude, ever doing so would allow users to use the macro without the presence of a mandatory #cfg directive, which would totally obviate the main advantage of this approach. Thus this is essentially a promise that asm! will never be in the prelude (or any other location that would allow users to avoid #cfg; in particular this is the reason we cannot do both approaches).
  5. Every individual item needs to duplicate a large portion (likely the majority) of the asm! documentation.

@joshtriplett
Copy link
Member

joshtriplett commented Jul 7, 2021

We discussed this in today's @rust-lang/libs-api meeting. Our consensus was to put asm! in core::arch::asm!.

We felt that per-arch paths for it would not provide substantive protection for the issues that are more likely to arise in practice; it addresses the mixing of top-level architectures, but does not address architecture variants that aren't reflected in target_arch, CPU features, OS-specific assembly, or many other similar things. Given that, we felt that we shouldn't inflict the additional pain of the arch-specific paths on the users of asm!.

@bstrie plans to send in a PR that moves asm! to core::arch::asm! and re-exports it from the prelude. We can decide before stabilization whether to keep the re-export in the prelude.

(Thank you to @bstrie for the detailed summary of the alternatives and tradeoffs.)

@bors bors closed this as completed in 65b7aa9 Jul 19, 2021
simonschoening added a commit to simonschoening/trapframe-rs that referenced this issue Feb 16, 2022
The macros are now namespaced:
rust-lang/rust#84019
simonschoening added a commit to simonschoening/trapframe-rs that referenced this issue Feb 25, 2022
The macros are now namespaced:
rust-lang/rust#84019

In addition, llvm_asm was removed.
simonschoening added a commit to simonschoening/trapframe-rs that referenced this issue Feb 25, 2022
The macros are now namespaced:
rust-lang/rust#84019

In addition, llvm_asm was removed.
wangrunji0408 pushed a commit to rcore-os/trapframe-rs that referenced this issue Feb 25, 2022
The macros are now namespaced:
rust-lang/rust#84019

In addition, llvm_asm was removed.
kwindla pushed a commit to daily-co/synthetic-network that referenced this issue Jul 3, 2022
danbev added a commit to danbev/drogue-boot that referenced this issue Oct 29, 2022
This commit removes the feature attribute asm and add a namespace for
the usage of the asm macro.

Refs: rust-lang/rust#84019

Signed-off-by: Daniel Bevenius <daniel.bevenius@gmail.com>
danbev added a commit to danbev/drogue-boot that referenced this issue Oct 29, 2022
This commit removes the feature attribute asm and add a namespace for
the usage of the asm macro.

Currently the following build error is generated when using
rustc 1.66.0-nightly (c0983a9aa 2022-10-12):

$ cargo b
   Compiling drogue-boot v0.1.2 (/iot/drogue/drogue-boot)
error: cannot find macro `asm` in this scope
   --> src/lib.rs:100:9
    |
100 |         asm! {
    |         ^^^
    |
    = note: consider importing this macro:
            core::arch::asm

warning: the feature `asm` has been stable since 1.59.0 and no longer
requires an attribute to enable

 --> src/lib.rs:4:12
  |
4 | #![feature(asm)]
  |            ^^^
  |
  = note: `#[warn(stable_features)]` on by default

warning: `drogue-boot` (lib) generated 1 warning
error: could not compile `drogue-boot` due to previous error; 1 warning emitted

Refs: rust-lang/rust#84019

Signed-off-by: Daniel Bevenius <daniel.bevenius@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inline-assembly Area: inline asm!(..) F-asm `#![feature(asm)]` (not `llvm_asm`) T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants