Alignment of the `bytes` of `Allocation` to match `align` parameter #100467

emarteca · 2022-08-12T22:25:56Z

Aligning the bytes field of Allocation so that it is aligned matching the align parameter.

Main changes are to:

in the Allocation constructors from_bytes and uninit
in the adjust_from_tcx method in Allocation

Instead of just converting the relevant [u8] to a Box<[u8]> directly we now:

create a std::alloc::Layout of the alignment the Allocation is specified to have
allocate a buffer with this layout
get the corresponding [u8] for this buffer

Then we write the relevant bytes to it, check for uninit, etc -- same process as previously.

NOTE
With this current code, when the bytes field is deallocated, even though the size is right, in the layout the alignment might be under-required: for example we might have allocated it with alignment 4 and deallocate it with alignment 2. We know this is undefined behaviour violating the memory fitting requirements of dealloc -- we're going to work around this by adding a wrapper struct for the Box<[u8]> that stores the alignment it was allocated with, and manually implements deallocation with the right alignment.

Why are we doing this?

We want the address of the bytes field of an Allocation to be accessible, and represent the actual address of the [u8]. This is part of an effort to pass pointers in Miri to the C FFI.

Punch a hole to allow direct access to bytes - this API should be changed to provide a `Size` value corresponding to an `AllocId`. Additionally, force bytes to be appropriately aligned - the current trick is a hack, and over-aligns. We should use the `Align` parameter to appropriately align these buffers, as otherwise they'll never be able to be used by native code. The "adjust" callback used on deserialized allocations might not have that information - if it doesn't, just overalign to max alignment

…he check make sense (should be len of bytes) for real addrs

…ompilation

rustbot · 2022-08-12T22:25:59Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

The Miri submodule was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2022-08-12T22:25:59Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

Please see the contribution instructions for more information.

rust-highfive · 2022-08-12T22:26:00Z

⚠️ Warning ⚠️

These commits modify submodules.

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

RalfJung · 2022-08-13T02:59:32Z

The Miri submodule was changed

Please take the submodule update out of this MR. :) That will be done separately.

davidtwco · 2022-08-15T11:27:24Z

r? @RalfJung

RalfJung · 2022-08-15T12:49:41Z

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

+        let size = Size::from_bytes(slice.len());
+        let align_usize: usize = align.bytes().try_into().unwrap();
+        let layout = std::alloc::Layout::from_size_align(slice.len(), align_usize).unwrap();
+        let bytes = unsafe {


I think my biggest concern is that this introduces unsafe code deep in the core of the interpreter engine. This is a rather significant cost and risk that the interpreter has to carry, even the compile-time interpreter that definitely does not want or need this.

At the least, we should find a way to factor this such that all the unsafe code lives in the Miri repository. Even that is still not great though, it is exactly the kind of global cost I was worried about.

Ah, I see your point, but I don't think we can align the bytes of the allocation without unsafe code.
If we want to factor this out into Miri, then one option could be to refactor Allocation to expose bytes so that the bytes alignment can be done on the Miri side. Although, this would be a bigger change to Allocation.
What do you think?

No we certainly don't want to expose bytes. But we can make Allocation generic over how bytes are allocated.

You can add a new type parameter for that:

pub struct Allocation<Prov = AllocId, Extra = (), Bytes = Box<[u8]>> { bytes: Bytes

and then define a suitable trait

trait AllocationBytes

and add a bound Bytes: AllocationBytes whereveer needed.

The trait needs to then contain all the operations you need. The implementation in rustc can use Box<[u8]> and be completely safe, and that way only Miri needs unsafe code.

Ah that's smart -- I had tried earlier to make Allocation generic over the allocator for bytes which didn't work. But this makes sense!

RalfJung · 2022-08-15T12:51:21Z

This is part of rust-lang/miri#2365.

So this is a first step of a many-step plan -- would be good to have at least some idea of the overall plan, rather than going blindly into some direction without knowing whether it will work out in the end.

rustbot · 2022-08-15T16:47:30Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

oli-obk · 2022-09-09T13:45:55Z

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

+pub trait AllocBytes:
+    Clone + core::fmt::Debug + Eq + PartialEq + PartialOrd + Ord + core::hash::Hash


If you make this also have Deref<Target = [u8]>, you can remove everything but adjust_to_align, from_bytes and uninit and should have to adjust much less code as it would pick up the slice methods that Box<[u8]> also exposes via Deref.

Ah good point! I was hoping to keep AllocBytes more general though, so I can use it to represent foreign memory in Miri, that isn't represented by a [u8]. Given that, do you think it's ok to keep these extra functions in AllocBytes?

What kind of memory are you thinking of? You already support get_slice_from_range, which returns a &[u8]. Most of what I'm proposing is to get rid of that method and require the implementor of AllocBytes to put the logic into the Deref::deref method instead. Or are you planning to change the return type of the get_slice methods?

Ah, if we can make a custom get_slice method with the same return type then that should work! In my implementation these methods still return &[u8] -- my current representation of foreign memory is represented as a pair of machine address and length, and the get_slice methods build the slice from std::slice::from_raw_parts.

yea, so you could implement Deref and DerefMut for your foreign memory representation and have the deref and deref_mut methods use from_raw_parts. Then you can remove the get_slice* methods and others from the trait, as you get them for free once Deref is a super trait

Oh I see -- that's smart, thanks for the suggestion! (Will push these changes soon)

I agree the trait has a lot more methods than I was expecting so this sounds good.

RalfJung · 2022-09-21T18:52:11Z

compiler/rustc_const_eval/src/interpret/memory.rs

+    /// Get the base address for the bytes in an `Allocation` specified by the
+    /// `AllocID` passed in; error if no such allocation exists.
+    pub fn get_alloc_base_addr(&self, id: AllocId) -> InterpResult<'tcx, usize> {


Suggested change

/// Get the base address for the bytes in an `Allocation` specified by the

/// `AllocID` passed in; error if no such allocation exists.

pub fn get_alloc_base_addr(&self, id: AllocId) -> InterpResult<'tcx, usize> {

/// Get the base address for the bytes in an `Allocation` specified by the

/// `AllocID` passed in; error if no such allocation exists. The address will

/// be exposed (on the host system!).

///

/// 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 expose_alloc_base_addr(&self, id: AllocId) -> InterpResult<'tcx, usize> {

Also note that this only allows read-only access to the bytes. If C code writes to this memory, it's writing to a shared reference! So I think you probably want to make this mut.

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

RalfJung · 2022-09-21T18:55:36Z

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

+
+// Default `bytes` for `Allocation` is a `Box<[u8]>`.
+impl AllocBytes for Box<[u8]> {
+    fn uninit<'tcx, F: Fn() -> InterpError<'tcx>>(


It's not uninit though, it returns a zeroed buffer?

Huh that's true -- I just took this code from here where it was originally in the file

The entire allocation is considered uninit based on its InitMask, yeah. But looking just at the bytes, which is the level of abstraction you have here, this is zeroed. The bytes cannot be uninit.

RalfJung · 2022-09-21T18:55:49Z

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

+    /// Adjust the bytes to the specified alignment -- by default, this is a no-op.
+    fn adjust_to_align(self, _align: Align) -> Self {
+        self
+    }


What is this about? Alignment should be set at allocation time.

JohnCSimon · 2022-11-06T01:19:26Z

@emarteca
Ping from triage:
Can you please address the comments from the reviewer?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

…hods from trait

emarteca · 2022-11-09T22:34:09Z

Note: the build is failing b/c the Miri machine doesn't specify its Bytes type, but I don't think I'm meant to be editing the submodules in this PR (correct me if I'm wrong though!)
Sorry for the delay in PR edits, grad school really picked up.

@rustbot ready

rust-log-analyzer · 2022-11-10T00:21:27Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

RalfJung · 2022-11-10T07:04:18Z

Miri is a subtree now so yes you are meant to fix Miri in the same PR. :)

bors · 2022-11-13T21:44:19Z

☔ The latest upstream changes (presumably #104370) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2022-11-26T08:59:00Z

@rustbot author
waiting for the Miri fixes

RalfJung · 2022-11-26T09:01:45Z

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

+    // REEEEEEE TODO ELLEN
+    // make a new constructor that just takes a Bytes
+    // then, make the allocation this way in ffi_support, and add it to the memory with allocate_raw_ptr


this doesn't look like it should land in this form ;)

RalfJung · 2022-11-26T09:02:56Z

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

+    /// The address of the bytes.
+    fn expose_addr(&self) -> u64;


I think it would be better to make this return a raw pointer, and leave exposing and things like that to the caller.

And in fact... why is this even needed? One can use the deref to get such a pointer, right?

RalfJung · 2022-11-26T09:11:37Z

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

+    + core::fmt::Debug
+    + Eq
+    + PartialEq
+    + PartialOrd
+    + Ord
+    + core::hash::Hash


Please remove the Ord bounds, they should not be needed. No idea why Allocation derives PartialOrd, but #104933 removes that.

Dylan-DPC · 2023-01-23T13:23:34Z

@emarteca any updates on this?

Dylan-DPC · 2023-02-23T09:30:06Z

Closing this due to inactivity. Feel free to create a new pr if you wish to continue with this

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

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

maurer and others added 12 commits August 2, 2022 15:01

Stop x.py from being mean

0fe75b8

point to my miri fork

fde4235

no more direct hole punch to alloc bytes

e0e8e00

no more holepunch to bytes

a7b7f19

commented out alignment check for int-aligned addrs -- need to make t…

f7a991b

…he check make sense (should be len of bytes) for real addrs

proper check for size of allocation being aligned

e2ed272

using the align parameter to properly align allocations

51269b8

redoing allocation bytes realignment -- still testing with miri

bca203e

allocation bytes alignment, and cleanup .gitmodules

0ddff36

double free detected in tcache 2: could not compile core in stage 1 c…

a72a057

…ompilation

using slice directly, no intermediate vec

04f29dc

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 12, 2022

rust-highfive assigned davidtwco Aug 12, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2022

the8472 reviewed Aug 12, 2022

View reviewed changes

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

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

rust-highfive assigned RalfJung and unassigned davidtwco Aug 15, 2022

RalfJung reviewed Aug 15, 2022

View reviewed changes

removing miri submodule updates

2fd7606

emarteca force-pushed the alloc-align branch from b8b041f to 2fd7606 Compare August 15, 2022 16:54

Ellen Arteca added 2 commits August 15, 2022 17:00

removing miri submodule updates

ab1a61f

removing miri submodule updates

b87f5ef

oli-obk requested changes Sep 9, 2022

View reviewed changes

propagating Allocation Bytes type

f75649b

RalfJung reviewed Sep 21, 2022

View reviewed changes

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

RalfJung reviewed Sep 21, 2022

View reviewed changes

emarteca added 2 commits November 9, 2022 17:19

adding deref and derefmut to allocbytes, removing now unnecessary met…

8db066f

…hods from trait

nit

f075a12

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 9, 2022

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 26, 2022

RalfJung reviewed Nov 26, 2022

View reviewed changes

CraftSpider mentioned this pull request Feb 14, 2023

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

Merged

Dylan-DPC closed this Feb 23, 2023

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alignment of the `bytes` of `Allocation` to match `align` parameter #100467

Alignment of the `bytes` of `Allocation` to match `align` parameter #100467

emarteca commented Aug 12, 2022

rustbot commented Aug 12, 2022

rust-highfive commented Aug 12, 2022

rust-highfive commented Aug 12, 2022

RalfJung commented Aug 13, 2022

davidtwco commented Aug 15, 2022

RalfJung Aug 15, 2022

emarteca Aug 16, 2022

RalfJung Aug 21, 2022

emarteca Aug 23, 2022

RalfJung commented Aug 15, 2022 •

edited

Loading

rustbot commented Aug 15, 2022

oli-obk Sep 9, 2022

emarteca Sep 13, 2022

oli-obk Sep 14, 2022

emarteca Sep 14, 2022 •

edited

Loading

oli-obk Sep 14, 2022

emarteca Sep 14, 2022

RalfJung Sep 21, 2022

RalfJung Sep 21, 2022

RalfJung Sep 21, 2022

emarteca Nov 9, 2022

RalfJung Nov 26, 2022

RalfJung Sep 21, 2022

JohnCSimon commented Nov 6, 2022

emarteca commented Nov 9, 2022

rust-log-analyzer commented Nov 10, 2022

RalfJung commented Nov 10, 2022 via email

bors commented Nov 13, 2022

RalfJung commented Nov 26, 2022

RalfJung Nov 26, 2022

RalfJung Nov 26, 2022

RalfJung Nov 26, 2022

Dylan-DPC commented Jan 23, 2023

Dylan-DPC commented Feb 23, 2023

		pub trait AllocBytes:
		Clone + core::fmt::Debug + Eq + PartialEq + PartialOrd + Ord + core::hash::Hash

-    /// Get the base address for the bytes in an `Allocation` specified by the
-    /// `AllocID` passed in; error if no such allocation exists.
-    pub fn get_alloc_base_addr(&self, id: AllocId) -> InterpResult<'tcx, usize> {
+    /// Get the base address for the bytes in an `Allocation` specified by the
+    /// `AllocID` passed in; error if no such allocation exists. The address will
+    /// be exposed (on the host system!).
+    ///
+    /// 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 expose_alloc_base_addr(&self, id: AllocId) -> InterpResult<'tcx, usize> {

Alignment of the bytes of Allocation to match align parameter #100467

Alignment of the bytes of Allocation to match align parameter #100467

Conversation

emarteca commented Aug 12, 2022

Main changes are to:

Why are we doing this?

rustbot commented Aug 12, 2022

rust-highfive commented Aug 12, 2022

rust-highfive commented Aug 12, 2022

RalfJung commented Aug 13, 2022

davidtwco commented Aug 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Aug 15, 2022 • edited Loading

rustbot commented Aug 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emarteca Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnCSimon commented Nov 6, 2022

emarteca commented Nov 9, 2022

rust-log-analyzer commented Nov 10, 2022

RalfJung commented Nov 10, 2022 via email

bors commented Nov 13, 2022

RalfJung commented Nov 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dylan-DPC commented Jan 23, 2023

Dylan-DPC commented Feb 23, 2023

Alignment of the `bytes` of `Allocation` to match `align` parameter #100467

Alignment of the `bytes` of `Allocation` to match `align` parameter #100467

RalfJung commented Aug 15, 2022 •

edited

Loading

emarteca Sep 14, 2022 •

edited

Loading