Support allocations with non-Box<[u8]> bytes #108022

CraftSpider · 2023-02-14T04:21:05Z

This is prep work for allowing miri to support passing pointers to C code, which will require Allocations to be correctly aligned. Currently, it just makes Allocation generic and plumbs the necessary changes through the right places.

The follow-up to this will be adding a type in the miri interpreter which correctly aligns the bytes, using that for the Miri engine, then allowing Miri to pass pointers into these allocations to C calls.

Based off of #100467, credit to @emarteca for the code

rustbot · 2023-02-14T04:21:12Z

r? @nagisa

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-02-14T04:21:14Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

compiler/rustc_middle/src/mir/interpret/allocation.rs

rustbot · 2023-02-14T20:28:12Z

The Miri subtree was changed

cc @rust-lang/miri

CraftSpider · 2023-02-14T20:54:38Z

Baffled by the tidy failure - x tidy passes locally

oli-obk · 2023-02-15T08:55:10Z

looks like a rustfmt difference between your system and CI. Try rebasing over latest master and running x test tidy again

…ons for miri. Credit to emarteca for the code.

compiler/rustc_middle/src/mir/interpret/allocation.rs

nagisa · 2023-02-26T23:38:52Z

r? rust-lang/miri

oli-obk · 2023-03-02T09:49:17Z

@bors r+

bors · 2023-03-02T09:49:20Z

📌 Commit f26b0a2 has been approved by oli-obk

It is now in the queue for this repository.

Support allocations with non-Box<[u8]> bytes This is prep work for allowing miri to support passing pointers to C code, which will require `Allocation`s to be correctly aligned. Currently, it just makes `Allocation` generic and plumbs the necessary changes through the right places. The follow-up to this will be adding a type in the miri interpreter which correctly aligns the bytes, using that for the Miri engine, then allowing Miri to pass pointers into these allocations to C calls. Based off of rust-lang#100467, credit to `@emarteca` for the code

Support allocations with non-Box<[u8]> bytes This is prep work for allowing miri to support passing pointers to C code, which will require `Allocation`s to be correctly aligned. Currently, it just makes `Allocation` generic and plumbs the necessary changes through the right places. The follow-up to this will be adding a type in the miri interpreter which correctly aligns the bytes, using that for the Miri engine, then allowing Miri to pass pointers into these allocations to C calls. Based off of rust-lang#100467, credit to ``@emarteca`` for the code

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#108022 (Support allocations with non-Box<[u8]> bytes) - rust-lang#108367 (Re-apply "switch to the macos-12-xl builder") - rust-lang#108557 (Point error span at Some constructor argument when trait resolution fails) - rust-lang#108573 (Explain compile-time vs run-time difference in env!() error message) - rust-lang#108584 (Put backtick content from rustdoc search errors into a `<code>` elements) - rust-lang#108624 (Make `ExprKind` the first field in `thir::Expr`) - rust-lang#108644 (Allow setting hashmap toml values in `./configure`) - rust-lang#108672 (Feed queries on impl side for RPITITs when using lower_impl_trait_in_trait_to_assoc_ty) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

RalfJung · 2023-03-13T21:39:39Z

compiler/rustc_const_eval/src/interpret/memory.rs

+    /// It is up to the caller to take sufficient care when using this address:
+    /// there could be provenance or uninit memory in there, and other memory
+    /// accesses could invalidate the exposed pointer.
+    pub fn alloc_base_addr(&self, id: AllocId) -> InterpResult<'tcx, *const u8> {


It is very confusing to talk about getting an "address" when the return type is a pointer types. I would expect an "address" to be usize-typed, like ptr.addr().

Also the docs don't say what the caller is allowed to do with this pointer: for sure no mutation of the pointed-to memory is allowed; for how long may that memory be read? The provenance of the returned raw pointer will become invalid at some point.

Why do we need such an extremely unsafe-to-use function deep inside the interpreter?

This is used in FFI code to get a pointer to the base address, however, it likely isn't necessary - we can get a byte slice through other methods that are a lot clearer and actually check various things we probably want to check anyways. I'd be in favor of removing this if I can alter my current draft PR to not use it, to confirm it's practical to avoid.

It being used for FFI does not absolve it from following the usual Rust pointer rules, so all the questions about mutability, lifetime, and provenance still remain.

Also I don't see why FFI support would need this as a new functionality inside the core interpreter. As you said the existing slice methods could be used, or alternatively we might want a function that gives access to the underling M::Bytes inside an allocation, and then Miri can do whatever it has to do directly with that type.

RalfJung · 2023-03-13T21:40:34Z

compiler/rustc_middle/src/mir/interpret/allocation.rs

+    Clone + fmt::Debug + Eq + PartialEq + Hash + Deref<Target = [u8]> + DerefMut<Target = [u8]>
+{
+    /// Adjust the bytes to the specified alignment -- by default, this is a no-op.
+    fn adjust_to_align(self, _align: Align) -> Self;


That's a strange API. Shouldn't alignment be set when the Self is created?

This is called in adjust_from_tcx, it would be necessary if it could accept another bytes type that may not be aligned, but as is I think you're right - we always create the item with the desired alignment and don't need to re-align it later.

adjust_from_tcx will always have to convert a Box<[u8]> into an aligned byte storage, so that is probably the API we want in the trait.

RalfJung · 2023-03-13T21:40:58Z

compiler/rustc_middle/src/mir/interpret/allocation.rs

+    fn from_bytes<'a>(slice: impl Into<Cow<'a, [u8]>>, _align: Align) -> Self;
+
+    /// Create a zeroed `AllocBytes` of the specified size and alignment;
+    /// call the callback error handler if there is an error in allocating the memory.


Which "callback error handler"?

This function used to return a Result and generate the error using a callback - this was effectively the same as returning an Option and calling ok_or_else on it, so it was changed to do that, but I missed updating the docs.

rustbot assigned nagisa Feb 14, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 14, 2023

This comment has been minimized.

Sign in to view

oli-obk reviewed Feb 14, 2023

View reviewed changes

compiler/rustc_middle/src/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

CraftSpider added 5 commits February 20, 2023 13:47

Allow non-Box allocations in preparation for aligned const allocati…

6f407d6

…ons for miri. Credit to emarteca for the code.

Add mentioned from_raw_bytes constructor

b2455dc

Fix alloc_base_addr type

f1f40ac

Remove unused lifetime

871c1de

Minimal changes to make miri work

936b567

CraftSpider force-pushed the align-bytes branch from 3e7bf2d to 936b567 Compare February 20, 2023 18:50

oli-obk requested changes Feb 21, 2023

View reviewed changes

compiler/rustc_middle/src/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

rustbot assigned oli-obk and unassigned nagisa Feb 26, 2023

Make zeroed return an Option

3a87a18

This comment has been minimized.

Sign in to view

Format

f26b0a2

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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2023

compiler-errors mentioned this pull request Mar 2, 2023

Rollup of 7 pull requests #108661

Closed

matthiaskrgr mentioned this pull request Mar 2, 2023

Rollup of 7 pull requests #108666

Closed

matthiaskrgr mentioned this pull request Mar 2, 2023

Rollup of 8 pull requests #108677

Merged

bors merged commit f75f440 into rust-lang:master Mar 3, 2023

rustbot added this to the 1.69.0 milestone Mar 3, 2023

RalfJung reviewed Mar 13, 2023

View reviewed changes

RalfJung mentioned this pull request Apr 10, 2023

Clean up support for "aligning the backing store of allocations" rust-lang/miri#2836

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support allocations with non-Box<[u8]> bytes #108022

Support allocations with non-Box<[u8]> bytes #108022

CraftSpider commented Feb 14, 2023

rustbot commented Feb 14, 2023

rustbot commented Feb 14, 2023

This comment has been minimized.

rustbot commented Feb 14, 2023

CraftSpider commented Feb 14, 2023

This comment has been minimized.

oli-obk commented Feb 15, 2023

nagisa commented Feb 26, 2023

This comment has been minimized.

oli-obk commented Mar 2, 2023

bors commented Mar 2, 2023

RalfJung Mar 13, 2023 •

edited

Loading

CraftSpider Apr 10, 2023

RalfJung Apr 13, 2023

RalfJung Mar 13, 2023

CraftSpider Apr 10, 2023

RalfJung Apr 13, 2023

RalfJung Mar 13, 2023

CraftSpider Apr 10, 2023

Support allocations with non-Box<[u8]> bytes #108022

Support allocations with non-Box<[u8]> bytes #108022

Conversation

CraftSpider commented Feb 14, 2023

rustbot commented Feb 14, 2023

rustbot commented Feb 14, 2023

This comment has been minimized.

rustbot commented Feb 14, 2023

CraftSpider commented Feb 14, 2023

This comment has been minimized.

oli-obk commented Feb 15, 2023

nagisa commented Feb 26, 2023

This comment has been minimized.

oli-obk commented Mar 2, 2023

bors commented Mar 2, 2023

RalfJung Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

CraftSpider Apr 10, 2023

Choose a reason for hiding this comment

RalfJung Apr 13, 2023

Choose a reason for hiding this comment

RalfJung Mar 13, 2023

Choose a reason for hiding this comment

CraftSpider Apr 10, 2023

Choose a reason for hiding this comment

RalfJung Apr 13, 2023

Choose a reason for hiding this comment

RalfJung Mar 13, 2023

Choose a reason for hiding this comment

CraftSpider Apr 10, 2023

Choose a reason for hiding this comment

RalfJung Mar 13, 2023 •

edited

Loading