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

Tracking Issue for raw_ref_macros #73394

Closed
1 of 3 tasks
RalfJung opened this issue Jun 16, 2020 · 28 comments · Fixed by #80886
Closed
1 of 3 tasks

Tracking Issue for raw_ref_macros #73394

RalfJung opened this issue Jun 16, 2020 · 28 comments · Fixed by #80886
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-raw_ref_op `#![feature(raw_ref_op)]` finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jun 16, 2020

This is a tracking issue for the macro version of raw_ref_op (#64490).
The feature gate for the issue is #![feature(raw_ref_macros)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

Implementation history

@RalfJung RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-raw_ref_op `#![feature(raw_ref_op)]` labels Jun 16, 2020
@jonas-schievink jonas-schievink added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Jun 16, 2020
@jonas-schievink
Copy link
Contributor

rustdoc displays the macro in core::raw_mut https://doc.rust-lang.org/nightly/core/macro.raw_mut.html

@RalfJung
Copy link
Member Author

RalfJung commented Jul 15, 2020

Sounds like a rustdoc bug, thanks for pointing that out.

Reported as #74355.

@RalfJung
Copy link
Member Author

Stabilization report

(Is there a template for these? I am entirely making this up.^^)

I'd like to propose stabilizing the raw_ref_macros feature. This feature guard two macros, core::ptr::raw_const! and core::ptr::raw_mut!. Both of these macros take path expressions and their effect is to create a raw pointer as described in RFC 2582.

This exposes a new primitive language ability: creating a raw pointer to something without going through an intermediate reference. This operation has been talked about in the Rust community for at least as long as I am around (I recall @arielb1 suggesting something like it). Example use cases that require such an operation are a general offset_of! macro, and creating pointers to unaligned fields of a packed struct. In particular, stabilizing this feature one way or another is crucial to unblock progress on one of the oldest open soundness bugs, #27060.

The aforementioned RFC proposes a new primitive syntax for these macros. However, we are not yet ready to commit to a stable syntax for these operations -- but we still would like to expose this ability to stable code. Following precedent like the try! macro, we thus propose to stabilize this feature through macros first, which is what the raw_ref_macros feature does.

Existing users

The raw_ref operation (whether through the syntax or the macro) is already seeing some use in-tree, e.g. in #73845 and #73971. Out-of-tree, Gilnaa/memoffset#43 ports the popular memoffset crate to use this operation.

An earlier crater experiment found around 50 crates that created references to unaligned fields of a packed struct. Some of those can probably be fixed by making copies instead of creating references (a common issue when using such fields in println! or comparison operations), but other likely truly need a pointer to that field, which currently cannot be created in a UB-free way.

Implementation history

Potential blockers/issues

@nikomatsakis
Copy link
Contributor

@RalfJung there is no template I'm aware of, I've been meaning to make one for some time

@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

Following @RalfJung's excellent report, I propose that we stabilize the raw_const and raw_mut macros.

@rfcbot
Copy link

rfcbot commented Jul 27, 2020

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

Concerns:

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 Jul 27, 2020
@nikomatsakis
Copy link
Contributor

One thing to note here is that we are stabilizing things implemented with a macro, I believe -- I would appreciate it if @petrochenkov would cast an eye over the definitions to "approve" them =)

I see they include a #[rustc_macro_transparency = "semitransparent"] attribute, which I think @petrochenkov requested, though I have no real idea just what it does. I would presume that, in terms of hygiene, these macros would be a rather simple case, since they don't introduce any binders. =)

rust/src/libcore/ptr/mod.rs

Lines 1477 to 1542 in 4a90e36

/// Create a `const` raw pointer to a place, without creating an intermediate reference.
///
/// Creating a reference with `&`/`&mut` is only allowed if the pointer is properly aligned
/// and points to initialized data. For cases where those requirements do not hold,
/// raw pointers should be used instead. However, `&expr as *const _` creates a reference
/// before casting it to a raw pointer, and that reference is subject to the same rules
/// as all other references. This macro can create a raw pointer *without* creating
/// a reference first.
///
/// # Example
///
/// ```
/// #![feature(raw_ref_macros)]
/// use std::ptr;
///
/// #[repr(packed)]
/// struct Packed {
/// f1: u8,
/// f2: u16,
/// }
///
/// let packed = Packed { f1: 1, f2: 2 };
/// // `&packed.f2` would create an unaligned reference, and thus be Undefined Behavior!
/// let raw_f2 = ptr::raw_const!(packed.f2);
/// assert_eq!(unsafe { raw_f2.read_unaligned() }, 2);
/// ```
#[unstable(feature = "raw_ref_macros", issue = "73394")]
#[rustc_macro_transparency = "semitransparent"]
#[allow_internal_unstable(raw_ref_op)]
pub macro raw_const($e:expr) {
&raw const $e
}
/// Create a `mut` raw pointer to a place, without creating an intermediate reference.
///
/// Creating a reference with `&`/`&mut` is only allowed if the pointer is properly aligned
/// and points to initialized data. For cases where those requirements do not hold,
/// raw pointers should be used instead. However, `&mut expr as *mut _` creates a reference
/// before casting it to a raw pointer, and that reference is subject to the same rules
/// as all other references. This macro can create a raw pointer *without* creating
/// a reference first.
///
/// # Example
///
/// ```
/// #![feature(raw_ref_macros)]
/// use std::ptr;
///
/// #[repr(packed)]
/// struct Packed {
/// f1: u8,
/// f2: u16,
/// }
///
/// let mut packed = Packed { f1: 1, f2: 2 };
/// // `&mut packed.f2` would create an unaligned reference, and thus be Undefined Behavior!
/// let raw_f2 = ptr::raw_mut!(packed.f2);
/// unsafe { raw_f2.write_unaligned(42); }
/// assert_eq!({packed.f2}, 42); // `{...}` forces copying the field instead of creating a reference.
/// ```
#[unstable(feature = "raw_ref_macros", issue = "73394")]
#[rustc_macro_transparency = "semitransparent"]
#[allow_internal_unstable(raw_ref_op)]
pub macro raw_mut($e:expr) {
&raw mut $e
}

@KodrAus KodrAus added Libs-Tracked Libs issues that are tracked on the team's project board. A-raw-pointers Area: raw pointers, MaybeUninit, NonNull labels Jul 29, 2020
@comex
Copy link
Contributor

comex commented Aug 6, 2020

So, stabilizing this will allow offset_of! from memoffset to work without UB, but making it work as a constant expression will require two other features:

This is somewhat unfortunate, since the above-mentioned operations all have the issue that they're sometimes illegal depending on their inputs (or in the case of ptr-to-usize, sometimes produce an output that's then illegal to perform arithmetic on), which means that stabilizing those operations is blocked on figuring out how "unconst" should work. Yet offset_of! itself never uses them in a way that could be illegal.

It would be nice if we figured out "unconst" soon, but I'm wondering if we should consider just stabilizing an offset_of! macro in libcore.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 6, 2020

In the const lang-team meeting with @rust-lang/wg-const-eval we did make some plans towards unconst operations, but nothing has happened since then I think. See the meeting notes. The first step would be an RFC specifying what "UB during CTFE" means.

It would be nice if we figured out "unconst" soon, but I'm wondering if we should consider just stabilizing an offset_of! macro in libcore.

That seems reasonable as well.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 6, 2020

@Amanieu @SimonSapin @cramertj @sfackler @withoutboats friendly reminder that there's an FCP here waiting for you. :)

@withoutboats
Copy link
Contributor

@rfcbot concern simon's concern

#73394 (comment)

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

I see they include a #[rustc_macro_transparency = "semitransparent"] attribute, which I think petrochenkov requested, though I have no real idea just what it does. I would presume that, in terms of hygiene, these macros would be a rather simple case, since they don't introduce any binders. =)

@nikomatsakis: I believe this makes the macro use normal macro_rules! hygiene (def-site for locals, call-site otherwise), rather than the def-site hygiene normally used by macros 2.0.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 20, 2020

The aforementioned RFC proposes a new primitive syntax for these macros. However, we are not yet ready to commit to a stable syntax for these operations -- but we still would like to expose this ability to stable code.

Stabilizing these macros but keeping the language level syntax unstable does allow us to change the exact sigils used to spell &raw, but it does lock us into the design of RFC 2582 that focuses on avoiding the need for & / &mut operators. I’m sorry to say it this late in the process but as commented in #64490 (comment) I feel that we should instead consider designs that focus on avoiding the * operator to "dereference" pointers that may not point to valid memory.

@SimonSapin this concern is currently preventing us from entering FCP. I think it has been conclusively answered here and here by pointing out that the concern rests on a misunderstanding of what the RFC is about. If you still have concerns left, I'd appreciate if you could let us know what they are, so that we can make progress on this. :)

@SimonSapin
Copy link
Contributor

Yeah it looks like I misunderstood the RFC and my concern does not really apply. It can be considered resolved, but rfcbot seems to have ignored my command and @withoutboats formally filed a concern with the bot for me, so they need to give the resolve command. Sorry for the delay here.

(I still have a feeling that the approach of this RFC is not the one I would like most. But I don’t have bandwidth at the moment to look into it more and make a better-informed opinion, and it’s already late in the process anyway to argue for a very different design.)

@withoutboats
Copy link
Contributor

@rfcbot resolve simon's concern

@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 Oct 26, 2020
@rfcbot
Copy link

rfcbot commented Oct 26, 2020

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

@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 Nov 5, 2020
@rfcbot
Copy link

rfcbot commented Nov 5, 2020

The final comment period, with a disposition to merge, 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.

The RFC will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Nov 5, 2020
@nikomatsakis
Copy link
Contributor

Woohoo! I guess we need a stabilization PR now?

@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Nov 5, 2020
@RalfJung
Copy link
Member Author

RalfJung commented Nov 5, 2020

🎉

However, if we stabilize while #74355 is unresolved, its docs will be rendered incorrectly.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 26, 2020

FWIW, I am still having doubt about the macro names, but no better names have been suggested yet either. raw_const! in particular sounds like const is the subject here... usually this will probably be written as ptr::raw_const!, but that is still strange.

const_raw! might be better? At least now const is unambiguously an adjective. That also works for mut_raw!. But usually, mut is a suffix, not a prefix. @rust-lang/libs any ideas?

@RalfJung
Copy link
Member Author

RalfJung commented Jan 4, 2021

Another option might be ptr::const_addr_of! and ptr::mut_addr_of!. These at least indicate what the operation is doing.

@bors bors closed this as completed in b94d84d Jan 30, 2021
eggyal pushed a commit to eggyal/copse that referenced this issue Jan 9, 2023
Stabilize raw ref macros

This stabilizes `raw_ref_macros` (rust-lang/rust#73394), which is possible now that rust-lang/rust#74355 is fixed.

However, as I already said in rust-lang/rust#73394 (comment), I am not particularly happy with the current names of the macros. So I propose we also change them, which means I am proposing to stabilize the following in `core::ptr`:
```rust
pub macro const_addr_of($e:expr) {
    &raw const $e
}

pub macro mut_addr_of($e:expr) {
    &raw mut $e
}
```

The macro name change means we need another round of FCP. Cc `````@rust-lang/libs`````
Fixes #73394
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-raw_ref_op `#![feature(raw_ref_op)]` finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-lang Relevant to the language team, which will review and decide on the PR/issue. 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.