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

Add a raw "address of" operator #64588

Merged
merged 8 commits into from Dec 20, 2019
Merged

Conversation

matthewjasper
Copy link
Contributor

  • Parse and feature gate &raw [const | mut] expr (feature gate name is raw_address_of)
  • Add mir::Rvalue::AddressOf
  • Use the new Rvalue for:
    • the new syntax
    • reference to pointer casts
    • drop shims for slices and arrays
  • Stop using mir::Rvalue::Cast with a reference as the operand
  • Correctly evaluate mir::Rvalue::{Ref, AddressOf} in constant propagation

cc @Centril @RalfJung @oli-obk @eddyb
cc #64490

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2019
@rust-highfive

This comment has been minimized.

src/librustc/hir/mod.rs Outdated Show resolved Hide resolved
src/librustc/hir/mod.rs Outdated Show resolved Hide resolved
src/librustc/hir/mod.rs Outdated Show resolved Hide resolved
src/librustc/mir/mod.rs Outdated Show resolved Hide resolved
src/librustc/mir/visit.rs Outdated Show resolved Hide resolved
_ => None,
};

if let Some((ctx, place)) = borrow_ctx {
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @ecstatic-morse @oli-obk re. the qualification code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I think none of it should be changed and we should just treat raw borrows as unpromotable (because any potentially promotable things had to error out before)

src/librustc_mir/hair/mod.rs Show resolved Hide resolved
src/librustc_mir/borrow_check/mod.rs Show resolved Hide resolved
src/librustc_codegen_ssa/mir/rvalue.rs Show resolved Hide resolved
@Centril

This comment has been minimized.

@rust-highfive rust-highfive assigned RalfJung and unassigned cramertj Sep 18, 2019
@Centril
Copy link
Contributor

Centril commented Sep 18, 2019

Thanks for the PR. I've made an initial review above ^--- (but Ralf should also review this).

Some more remarks on the PR description:

  • Use the new Rvalue for:

    • reference to pointer casts

This is not part of the RFC and should remain UB.

  • drop shims for slices and arrays

Can you elaborate on this please? Is this a spec change or just internal refactoring?

  • Stop using mir::Rvalue::Cast with a reference as the operand

Seems like the same as &x as *const _ using Rvalue::AddrOf (i.e. not part of the RFC).


Can you please also extend the unused_parens lint to handle &raw [const|mut] $place_expr?

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Sep 19, 2019

@matthewjasper You need to handle Rvalue::AddressOf(_, _) here and here as well. This is the part of the code that forbids taking &mut references in const bodies. We need to forbid anything that allows mutation through indirection, since that can cause an Option<NeedsDropType> to go from None to Some.

@matthewjasper
Copy link
Contributor Author

matthewjasper commented Sep 19, 2019

This is not part of the RFC and should remain UB.

What should be UB? &x as *const _ still is and x as *const _ could never cause UB.

Can you elaborate on this please? Is this a spec change or just internal refactoring?

It's a refactoring. If the precise stacked borrows effects of slice/array drop shims is defined, then maybe it's a change, but not a very big one since the parameter is a mutable reference, and the calls to drop the elements create mutable references.

Can you please also extend the unused_parens lint to handle &raw [const|mut] $place_expr?

This can be a follow up. The (lack of) behavior here is consistent with & AFAICT.

@Centril
Copy link
Contributor

Centril commented Sep 19, 2019

What should be UB? &x as *const _ still is and x as *const _ could never cause UB.

Oh I see; seems I misread. Can you add a mir-opt test to that effect?

This can be a follow up. The (lack of) behavior here is consistent with & AFAICT.

Sure, can you add a ticket to the tracking issue when this PR is merged?

@matthewjasper

This comment has been minimized.

@Centril

This comment has been minimized.

@bors

This comment has been minimized.

@JohnCSimon

This comment has been minimized.

@ecstatic-morse
Copy link
Contributor

@matthewjasper @RalfJung Some tests need to be added to ensure that &raw mut is forbidden in const contexts. Also, there's now two validation passes (one uses dataflow to support #49146) that are checked against one another for compatibility. Once &raw mut references are handled, I'm happy to port the changes to the new validator in librustc_mir/transform/check_consts/validation.rs.

@matthewjasper matthewjasper added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2019
@matthewjasper

This comment has been minimized.

Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
tmandry added a commit to tmandry/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
tmandry added a commit to tmandry/rust that referenced this pull request Oct 1, 2019
syntax: cleanup param, method, and misc parsing

Do some misc cleanup of the parser:
- Method and parameter parsing is refactored.
- A parser for `const | mut` is introduced that rust-lang#64588 can reuse.
- Some other misc parsing.

Next up in a different PR:
- ~Implementing rust-lang#64252 -- maybe some other time...
- Heavily restructuring up `item.rs` which is a mess (hopefully, no promises ^^).

r? @petrochenkov
@matthewjasper
Copy link
Contributor Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Dec 19, 2019

📌 Commit a749116 has been approved by oli-obk

@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 Dec 19, 2019
@bors

This comment has been minimized.

@Centril

This comment has been minimized.

Centril added a commit to Centril/rust that referenced this pull request Dec 20, 2019
…-obk

Add a raw "address of" operator

* Parse and feature gate `&raw [const | mut] expr` (feature gate name is `raw_address_of`)
* Add `mir::Rvalue::AddressOf`
* Use the new `Rvalue` for:
    * the new syntax
    * reference to pointer casts
    * drop shims for slices and arrays
* Stop using `mir::Rvalue::Cast` with a reference as the operand
* Correctly evaluate `mir::Rvalue::{Ref, AddressOf}` in constant propagation

cc @Centril @RalfJung @oli-obk @eddyb
cc rust-lang#64490
Centril added a commit to Centril/rust that referenced this pull request Dec 20, 2019
…-obk

Add a raw "address of" operator

* Parse and feature gate `&raw [const | mut] expr` (feature gate name is `raw_address_of`)
* Add `mir::Rvalue::AddressOf`
* Use the new `Rvalue` for:
    * the new syntax
    * reference to pointer casts
    * drop shims for slices and arrays
* Stop using `mir::Rvalue::Cast` with a reference as the operand
* Correctly evaluate `mir::Rvalue::{Ref, AddressOf}` in constant propagation

cc @Centril @RalfJung @oli-obk @eddyb
cc rust-lang#64490
bors added a commit that referenced this pull request Dec 20, 2019
Rollup of 5 pull requests

Successful merges:

 - #64588 (Add a raw "address of" operator)
 - #67031 (Update tokio crates to latest versions)
 - #67131 (Merge `TraitItem` & `ImplItem into `AssocItem`)
 - #67354 (Fix pointing at arg when cause is outside of call)
 - #67363 (Fix handling of wasm import modules and names)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

@bors bors merged commit a749116 into rust-lang:master Dec 20, 2019
@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 20, 2019
@matthewjasper matthewjasper deleted the mir-address-of branch December 20, 2019 19:45
@RalfJung
Copy link
Member

This is awesome, thanks so much. :)

Comment on lines +139 to 142
// Retag-as-raw after escaping to a raw pointer.
StatementKind::Assign(box (ref place, Rvalue::AddressOf(..))) => {
(RetagKind::Raw, place.clone())
}
Copy link
Member

Choose a reason for hiding this comment

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

So this is doing that for all &raw now, right? Certainly a reasonable start, though it doesn't seem to be entirely equivalent to the previous operation which also made sure that the "input" is a reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There wasn't any way to write &raw const *x where x is a raw pointer before.

bors added a commit to rust-lang/miri that referenced this pull request Jan 30, 2020
Test raw-ptr-cast without reborrow

With rust-lang/rust#64588 landed, we can finally test these things adequately. :)
bors added a commit to rust-lang/miri that referenced this pull request Jan 30, 2020
Test raw-ptr-cast without reborrow

With rust-lang/rust#64588 landed, we can finally test these things adequately. :)
@RalfJung RalfJung mentioned this pull request Jul 27, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-raw_ref_op `#![feature(raw_ref_op)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet