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

Decide whether asm! and/or global_asm! should be exported from the prelude. #87228

Closed
bstrie opened this issue Jul 17, 2021 · 53 comments
Closed
Labels
A-inline-assembly Area: inline asm!(..) C-discussion Category: Discussion or questions that doesn't represent real issues. disposition-postpone This issue / PR is in PFCP or FCP with a disposition to postpone it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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 Jul 17, 2021

In #84019 it was decided that asm! and global_asm! should be defined in core::arch rather than be defined in the crate root. Whereas anything defined in the crate root is essentially "in the prelude" by necessity, being defined elsewhere means that we have the option of deciding whether or not these macros should be exported from the prelude.

In the original Zulip thread regarding which module these macros should be defined in, there was a small amount of discussion as to whether or not these should additionally be exported from the prelude: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/namespacing.20.60asm!.60/near/233407632 . There did not seem to be a clear prevailing opinion.

Furthermore, I faintly recall from the most recent libs-api meeting someone mentioning that adding new macros to the prelude (or even the crate root) might be a compatibility hazard, and should only be done during an edition. I'm not sure if this is an official policy or not.

Note that this is not a blocker for asm! stabilization, as the item can be stabilized at arch::asm and exported from the prelude any time in the future.

@bstrie bstrie added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-inline-assembly Area: inline asm!(..) labels Jul 17, 2021
@inquisitivecrystal inquisitivecrystal added C-discussion Category: Discussion or questions that doesn't represent real issues. I-needs-decision Issue: In need of a decision. labels Jul 18, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Aug 11, 2021

The prelude should probably just contain things that are widely used. While asm!() has a pretty clear meaning, there's not going to be a lot of users.

However, @dtolnay mentioned that it might be good to have it in the prelude as a 'marketing' feature: People looking through the prelude documentation will discover the existence of inline assembly, showing that Rust is also a low level language.

On nightly, they're already in the prelude and seemingly not breaking anything, so that's not much of a concern.

Pinging @rust-lang/libs-api for more opinions.

@BurntSushi
Copy link
Member

For me personally, it feels a little weird to put it in the prelude, especially if it's really just for marketing purposes. Inline assembly is a rarely used feature, so tucking it away into a module feels like the right thing to me.

Of course, making it easy to discover features is important too. My hope is that after inline assembly lands, doing a web search for "rust inline assembly" will (eventually) bring you to the right place. Maybe that's good enough?

@joshtriplett
Copy link
Member

"marketing" is an interesting angle I hadn't considered.

Personally, I think these should be in the prelude for a different reason: to make them feel like innate language capabilities. You don't have to import a name from a module to use things like async and await, or closures, or (in the future) generators. I think inline assembly should feel the same way.

@thomcc
Copy link
Member

thomcc commented Aug 12, 2021

My gut feeling is that inline assembly uses macro syntax might mean that it won't feel like an innate language capability even if it is in the prelude.

I think putting it in a module1 is the right balance given how rarely it's used, and I say this as someone who uses it frequently, even in its current unstable state.

1: Well, fine so long as it's not a different module for each arch, which I believe is no longer being considered.

@Amanieu
Copy link
Member

Amanieu commented Aug 13, 2021

I feel ambivalent about this: I am probably one of the bigger users of asm! (78 uses over 10 files), but adding an import line for this is really very little effort that I don't mind it. In the end, I am happy with both options.

@npmccallum
Copy link
Contributor

I'm a pretty big user of this (at least a few dozen invocations across a bunch of crates). I also don't mind an import line. I suspect that use of this feature is somewhat uncommon and I'd hate to litter the prelude for my minor inconvenience. Anyone who needs this feature knows they need it and can google it. I don't see a reason to advertise it.

I use TryFrom a lot more and it isn't (yet) in the prelude (this one does bug me...).

@thomcc
Copy link
Member

thomcc commented Aug 13, 2021

adding an import line for this is really very little effort that I don't mind it.

Ultimately, the fact that you'll be able to use it as core::arch::asm! is why I don't mind. If it required a separate import line (the way traits do), I'd not be a fan. (That said, this would be terribly inconsistent with how other macros work, so I don't think anybody else would be either)

@joshtriplett
Copy link
Member

joshtriplett commented Aug 13, 2021 via email

@BurntSushi
Copy link
Member

@joshtriplett Could you say more about why you want it to feel like an innate language feature? Is it primarily because it feels that way in C?

@joshtriplett
Copy link
Member

@BurntSushi Sure. Off the top of my head, trying to introspect on that feeling: I'm thinking about the mindset associated with one of the types of development that commonly invokes inline assembly: low-level OS/firmware/systems/etc programming. Much like many other compiler attributes that control binary sections, linker attributes, and similar, this feels like something that should have a direct straightforward meaning in code generation, with zero flexibility for the compiler. This is a "drop down to low-level assembly" mechanism, and it fits well into a mindset of thinking of all of your code in terms of the semantics of the underlying assembly.

In addition, assembly is a feature that fundamentally must integrate natively with the compiler's code generation, and can't be done purely using library mechanisms, because it has to handle things like clobbers, and arrange for the right inputs to be in the right registers. So it feels right for it to be built into the language. And something that's available via a specific library path doesn't feel like "part of the language".

(The other common case for inline assembly is optimization, for which I'd expect this to feel like less of an issue.)

@bstrie
Copy link
Contributor Author

bstrie commented Aug 16, 2021

For people who would like to see asm in the prelude, what is your opinion on having global_asm in the prelude? Is only the former important enough to warrant it, or do you see them as a package deal?

(Also to reiterate, there is no urgency to this issue as it's not a blocker for stabilizing asm.)

@joshtriplett
Copy link
Member

@bstrie I personally think the same logic applies to global_asm, and it's not like the name global_asm is likely to conflict with anything. I don't consider them a package deal, though, and I place more importance on having asm in the prelude.

@joshtriplett
Copy link
Member

@rfcbot merge

In advance of the next time this gets discussed in a meeting, I'd like to use rfcbot to gauge consensus on keeping asm! (and global_asm!) in the prelude.

I personally believe they should be available without an import, to give them the appearance of a built-in language mechanism (which they inherently have to be).

@rfcbot
Copy link

rfcbot commented Aug 17, 2021

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 17, 2021
@rfcbot
Copy link

rfcbot commented Sep 22, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Sep 22, 2021
@workingjubilee
Copy link
Member

From the perspective of someone who previously used Python, having to import to do IO, when Python has open as a builtin, feels odd. From the perspective of someone who previously used JavaScript, having even trivial math and logging functions be (somewhat) scoped feels normal, but the rules around strings, arrays, and Vecs get quite strange. Rust makes println! available in the prelude, even though its cousin requires #include <stdio.h> in C. All of these have caused me odd little impedances when context switching between each of these languages and Rust. I always get the C compiler to yell at me about using a function that's not declared, and I try looking in a library instead of the builtins for open(), and... you get the idea.

And for x86 assembly... For Rust, libcore is really required to make things "go", but the parts that are injected into the prelude are rather arbitrary, based largely on frequency of use. A rather lot of things in libcore degrade to intrinsics which wind up being pretty direct invocations to the compiler, yet not all such things in libcore are in the prelude. I do not see how asm! or global_asm! is truly different from the compiler intrinsic functions in core::arch, for example, most of which have a direct 1:1 lowering, and are functionally almost-equivalent to injecting the equivalent assembly, even linking against LLVM IR that is target-arch-scoped.

I do not have any fundamental objection, however, to choosing either way.

@mark-i-m
Copy link
Member

I would also like to see asm! in the prelude. My "feeling" is similar to Josh's. As a low-level programmer, asm! is a fundamental low-level utility; I feel about asm! how others might feel about Vec or Option. Requiring a special import for asm! makes it feel like systems program is a second class concern for rust. I'm not sure if that's reasonable or not, but that's my gut reaction.

Also, the primary objection seems to be cluttering up the macro namespace, but I don't think I have ever seen another macro called asm! or global_asm! before, so I'm skeptical this should be a concern.

@BurntSushi
Copy link
Member

I personally did not tick my box because I'm not really convinced at all that either of these macros should be in the prelude, now that we have the ability to namespace macros. I just do not think inline assembly is anywhere near common enough to be deserving of it. I do not think comparisons to Vec or Option are particularly good here, because the frequency of use across all domains is no where near the same.

I've stopped short of registering a blocking concern because I don't know that I feel strongly enough to go be a lone voice against this if everyone else wants it.

@workingjubilee
Copy link
Member

workingjubilee commented Sep 23, 2021

I would also like to see asm! in the prelude. My "feeling" is similar to Josh's. As a low-level programmer, asm! is a fundamental low-level utility; I feel about asm! how others might feel about Vec or Option. Requiring a special import for asm! makes it feel like systems program is a second class concern for rust. I'm not sure if that's reasonable or not, but that's my gut reaction.

C requires #include <stdlib.h> for malloc and free, Rust does not for Vec.
Actually, now I do have an objection, and not just a comment:

All of these comparisons propose Rust should define itself in terms of other languages when it is not those languages.
However, if we wish to make Rust more like other languages, such as C, then I see no deep reason not to.
Let us drop std::vec::Vec from the prelude and namespace println! when we add asm!, so we can make Rust more like C.

@workingjubilee
Copy link
Member

Less flippantly: "I feel so-and-so is very fundamental to my concerns" applies for every single function, type, and macro in std. Why not Error? Why not Arc, or Mutex, as those are very often used in multithreaded programming? That's something Rust is uniquely good at, why not market those? Sure, I guess we don't like our Mutex much. How about the Atomics instead?

All of the proposed Simd types and the existing std::arch compiler intrinsic functions certainly require a lot of compiler assistance, so they fill that criteria. And for the portable versions, it will be a distinct capability, too, not something all languages get as strong a hand in, so it's quite "marketable". And graphics programming and digital signal processing use SIMD a lot,

And it occurs to me that moving something into the prelude has a serious drawback that no one seems to have discussed:
It makes it much harder to figure out where it is coming from.

It's quite possible nothing as rare in use as asm! should be in the prelude's namespace, because then it will not be immediately obvious from the file and tracing it to that fateful use std::arch::asm; where it comes from, and thus which module to look in.

Rust is a programming language already used by millions. Even if we see a few familiar faces in these discussions again and again, the question should be what we want all Rust programmers to find easier, and what legacy we wish to leave for the next generation of programmers, which will be far more numerous.

I am not sure that introducing something into the prelude's namespace is not merely about convenience. I think it is expressing an expectation that Rust programmers know what it is and where they can find it if they need to read up on it. This is a mental toll on learning Rust, and the toll is already high, with understanding the semantics of Vec, String, Option, and Result, From and Into.

@mark-i-m
Copy link
Member

I actually wish those things were in the prelude, along with all the collection types. Importing them feels like a completely unnecessary papercut to me. (I'm inclined to say that nearly everything in std should be in prelude, but I haven't thought through the implications enough to go that far).

I don't really feel that putting something in the prelude makes it harder to find because I almost always search rustdoc by the name of the item, not the module. I think it should be considered poor style to define a type with the same name as a std type.

Amanieu added a commit to Amanieu/rust that referenced this issue Dec 12, 2021
They are also removed from the prelude as per the decision in
rust-lang#87228.

stdarch and compiler-builtins are updated to work with the new, stable
asm! and global_asm! macros.
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 14, 2021
Stabilize asm! and global_asm!

Tracking issue: rust-lang#72016

It's been almost 2 years since the original [RFC](rust-lang/rfcs#2850) was posted and we're finally ready to stabilize this feature!

The main changes in this PR are:
- Removing `asm!` and `global_asm!` from the prelude as per the decision in rust-lang#87228.
- Stabilizing the `asm` and `global_asm` features.
- Removing the unstable book pages for `asm` and `global_asm`. The contents are moved to the [reference](rust-lang/reference#1105) and [rust by example](rust-lang/rust-by-example#1483).
  - All links to these pages have been removed to satisfy the link checker. In a later PR these will be replaced with links to the reference or rust by example.
- Removing the automatic suggestion for using `llvm_asm!` instead of `asm!` if you're still using the old syntax, since it doesn't work anymore with `asm!` no longer being in the prelude. This only affects code that predates the old LLVM-style `asm!` being renamed to `llvm_asm!`.
- Updating `stdarch` and `compiler-builtins`.
- Updating all the tests.

r? `@joshtriplett`
@m-ou-se m-ou-se removed the I-needs-decision Issue: In need of a decision. label Dec 15, 2021
flip1995 pushed a commit to flip1995/rust that referenced this issue Dec 17, 2021
Stabilize asm! and global_asm!

Tracking issue: rust-lang#72016

It's been almost 2 years since the original [RFC](rust-lang/rfcs#2850) was posted and we're finally ready to stabilize this feature!

The main changes in this PR are:
- Removing `asm!` and `global_asm!` from the prelude as per the decision in rust-lang#87228.
- Stabilizing the `asm` and `global_asm` features.
- Removing the unstable book pages for `asm` and `global_asm`. The contents are moved to the [reference](rust-lang/reference#1105) and [rust by example](rust-lang/rust-by-example#1483).
  - All links to these pages have been removed to satisfy the link checker. In a later PR these will be replaced with links to the reference or rust by example.
- Removing the automatic suggestion for using `llvm_asm!` instead of `asm!` if you're still using the old syntax, since it doesn't work anymore with `asm!` no longer being in the prelude. This only affects code that predates the old LLVM-style `asm!` being renamed to `llvm_asm!`.
- Updating `stdarch` and `compiler-builtins`.
- Updating all the tests.

r? `@joshtriplett`
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 19, 2021
@rfcbot
Copy link

rfcbot commented Dec 19, 2021

The final comment period, with a disposition to postpone, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Dec 19, 2021
@Amanieu Amanieu closed this as completed Dec 19, 2021
linkmauve added a commit to linkmauve/luma that referenced this issue Dec 19, 2021
Since the 19th of December, these two macros are not exported from the
prelude any longer, and must be imported from core::arch instead.

See rust-lang/rust#87228
linkmauve added a commit to linkmauve/luma that referenced this issue Dec 19, 2021
Since the 19th of December, these two macros are not exported from the
prelude any longer, and must be imported from core::arch instead.

See rust-lang/rust#87228
linkmauve added a commit to linkmauve/luma that referenced this issue Dec 19, 2021
These features are now stable, as of the 19th of December! See
rust-lang/rust#87228
linkmauve added a commit to linkmauve/luma that referenced this issue Dec 19, 2021
These features are now stable, as of the 19th of December! See
rust-lang/rust#87228
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 23, 2021
bjorn3 pushed a commit to bjorn3/rustc_codegen_gcc that referenced this issue Dec 30, 2021
They are also removed from the prelude as per the decision in
rust-lang/rust#87228.

stdarch and compiler-builtins are updated to work with the new, stable
asm! and global_asm! macros.
antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this issue Dec 30, 2021
* Rebase fallout.

* Move rustc_middle::middle::cstore to rustc_session.

* Create more accurate debuginfo for vtables.

Before this commit all vtables would have the same name "vtable" in
debuginfo. Now they get a name that identifies the implementing type
and the trait that is being implemented.

* Remove alloc::prelude

As per the libs team decision in #58935.

Closes #58935

* Make hash_result an Option.

* Add LLVM CFI support to the Rust compiler

This commit adds LLVM Control Flow Integrity (CFI) support to the Rust
compiler. It initially provides forward-edge control flow protection for
Rust-compiled code only by aggregating function pointers in groups
identified by their number of arguments.

Forward-edge control flow protection for C or C++ and Rust -compiled
code "mixed binaries" (i.e., for when C or C++ and Rust -compiled code
share the same virtual address space) will be provided in later work as
part of this project by defining and using compatible type identifiers
(see Type metadata in the design document in the tracking issue #89653).

LLVM CFI can be enabled with -Zsanitizer=cfi and requires LTO (i.e.,
-Clto).

* Properly check `target_features` not to trigger an assertion

* Remove workaround for the forward progress handling in LLVM

* Feat: make cg_ssa get_param borrow the builder mutable

* fix sparc64 ABI for aggregates with floating point members

* rustc_codegen_gcc: error on unwinding inline asm

* rustc_codegen_gcc: proper check for may_unwind

* Implement inline asm! for AVR platform

* Use object crate for .rustc metadata generation

We already use the object crate for generating uncompressed .rmeta
metadata object files. This switches the generation of compressed
.rustc object files to use the object crate as well. These have
slightly different requirements in that .rmeta should be completely
excluded from any final compilation artifacts, while .rustc should
be part of shared objects, but not loaded into memory.

The primary motivation for this change is #90326: In LLVM 14, the
current way of setting section flags (and in particular, preventing
the setting of SHF_ALLOC) will no longer work. There are other ways
we could work around this, but switching to the object crate seems
like the most elegant, as we already use it for .rmeta, and as it
makes this independent of the codegen backend. In particular, we
don't need separate handling in codegen_llvm and codegen_gcc.
codegen_cranelift should be able to reuse the implementation as
well, though I have omitted that here, as it is not based on
codegen_ssa.

This change mostly extracts the existing code for .rmeta handling
to allow using it for .rustc as well, and adjust the codegen
infrastructure to handle the metadata object file separately: We
no longer create a backend-specific module for it, and directly
produce the compiled module instead.

This does not fix #90326 by itself yet, as .llvmbc will need to be
handled separately.

* Remove the reg_thumb register class for asm! on ARM

Also restricts r8-r14 from being used on Thumb1 targets as per #90736.

* Remove redundant [..]s

* Stabilize asm! and global_asm!

They are also removed from the prelude as per the decision in
rust-lang/rust#87228.

stdarch and compiler-builtins are updated to work with the new, stable
asm! and global_asm! macros.

* Use `OutputFilenames` to generate output file for `-Zllvm-time-trace`

The resulting profile will include the crate name and will be stored in
the `--out-dir` directory.

This implementation makes it convenient to use LLVM time trace together
with cargo, in the contrast to the previous implementation which would
overwrite profiles or store them in `.cargo/registry/..`.

* Remove unnecessary sigils around `Symbol::as_str()` calls.

* Rustup to rustc 1.59.0-nightly (78fd0f633 2021-12-29)

* Import std::arch::asm

* Add missing feature gate

* Disable portable-simd test

Support for portable-simd isn't implemented yet

* Disable long running libcore tests

These only finish in reasonable time with optimizations enabled.

This patch file is copied from cg_clif.

* Ignore new failing test_is_sorted test

Co-authored-by: Camille GILLOT <gillot.camille@gmail.com>
Co-authored-by: Michael Woerister <michaelwoerister@posteo>
Co-authored-by: bors <bors@rust-lang.org>
Co-authored-by: Amanieu d'Antras <amanieu@gmail.com>
Co-authored-by: Ramon de C Valle <rcvalle@users.noreply.github.com>
Co-authored-by: Yuki Okushi <yuki.okushi@huawei.com>
Co-authored-by: Andreas Jonson <andjo403@users.noreply.github.com>
Co-authored-by: rdambrosio <rdambrosio016@gmail.com>
Co-authored-by: Petr Sumbera <petr.sumbera@oracle.com>
Co-authored-by: cynecx <me@cynecx.net>
Co-authored-by: Andrew Dona-Couch <hi@andrewcou.ch>
Co-authored-by: Nikita Popov <nikita.ppv@gmail.com>
Co-authored-by: est31 <MTest31@outlook.com>
Co-authored-by: Matthias Krüger <matthias.krueger@famsik.de>
Co-authored-by: Tomasz Miąsko <tomasz.miasko@gmail.com>
Co-authored-by: Nicholas Nethercote <n.nethercote@gmail.com>
Demindiro added a commit to Norost/norost-b that referenced this issue Feb 23, 2022
It has been removed from the prelude during stabilization.
See rust-lang/rust#87228
vladimir-ea pushed a commit to vladimir-ea/compiler-builtins that referenced this issue Mar 8, 2022
It is going to be removed from the prelude due to the decision in
rust-lang/rust#87228
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!(..) C-discussion Category: Discussion or questions that doesn't represent real issues. disposition-postpone This issue / PR is in PFCP or FCP with a disposition to postpone it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests