Make Allocation::bytes private #63561

HeroicKatora · 2019-08-14T16:15:01Z

Direct immutable access to the bytes is still possible but redirected through the new method raw_bytes_with_undef_and_ptr, similar to get_bytes_with_undef_and_ptr but without requiring an interpretation context and not doing any relocation or bounds checks. The size of the allocation is stored separately which makes access as Size and usize more ergonomic.

cc: @RalfJung

rust-highfive · 2019-08-14T16:15:05Z

r? @cramertj

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

HeroicKatora · 2019-08-14T16:21:26Z

r? @RalfJung

src/librustc/mir/interpret/allocation.rs

HeroicKatora · 2019-08-14T17:05:42Z

Changed pub(crate) to crate by amending the first commit.

RalfJung · 2019-08-14T21:04:35Z

I edited your OP to say "Fixes: #issue"; that will make GitHub auto-close it when the PR gets merged.

src/librustc/mir/interpret/allocation.rs

HeroicKatora · 2019-08-15T14:14:32Z

Removed crate visibility qualifier, only required rather simple changes to the stable hashing. Also removed the portion of comments on bytes and undef_mask that were not intended to be part of this PR yet.

HeroicKatora · 2019-08-15T19:44:41Z

The size field makes it much simpler to access the allocation extent as a Size, avoiding many additional conversions from the usize length of the byte vector. So it might be worth keeping?

src/librustc/ich/impls_ty.rs

src/librustc/mir/interpret/allocation.rs

src/librustc/ty/print/pretty.rs

RalfJung

This looks good overall, my notes are mostly editorial / ask for more comments.

Note that I'll be on conference next week and won't have time for reviews.

src/librustc_codegen_llvm/consts.rs

RalfJung · 2019-08-16T06:54:19Z

src/librustc/mir/interpret/allocation.rs

-    /// Note that the bytes of a pointer represent the offset of the pointer
-    pub bytes: Vec<u8>,
+    /// Note that the bytes of a pointer represent the offset of the pointer.
+    bytes: Vec<u8>,
    /// Maps from byte addresses to extra data for each pointer.
    /// Only the first byte of a pointer is inserted into the map; i.e.,
    /// every entry in this map applies to `pointer_size` consecutive bytes starting
    /// at the given offset.
    pub relocations: Relocations<Tag>,


Can you make this private and provide a read-only accessor, similar to what you did for undef_mask?

There already exists a method relocations, which returns relocations within a range specified by ptr and size, so I'd name the corresponding accessor all_relocations instead.

Also, interpret/memory.rs tries to access it directly. I'd move code in a similar manner as compressed undef mask.

Sounds good

I'm not available until Wednesday next week but the rest should be ready.

take all the time you need

The name fn relocations for the accessor is already taken:

rust/src/librustc/mir/interpret/allocation.rs

Lines 424 to 430 in 85ed538

/// Returns all relocations overlapping with the given ptr-offset pair.

pub fn relocations(

&self,

cx: &impl HasDataLayout,

ptr: Pointer<Tag>,

size: Size,

) -> &[(Size, (Tag, AllocId))] {

I'd rename the existing method to get_relocations which would be aligned with get_bytes, check_bytes, check_relocations to be concerned with interpreter execution.

Sounds good.
Makes me wonder who will even need the general read-only general accessor. Maybe that should also be raw_relocations or so, to make it less appealing to call?

src/librustc_codegen_llvm/consts.rs

src/librustc/mir/interpret/allocation.rs

src/librustc_codegen_llvm/consts.rs

src/librustc_mir/interpret/snapshot.rs

src/librustc_codegen_llvm/consts.rs

Direct access to the bytes was previously a problem (rust-lang#62931) where components would read their contents without properly checking relocations and/or definedness. Making bytes private instead of purely renaming them also helps in allowing amendments to their allocation scheme (such as eliding allocation for undef of constant regions).

Requires a manual implementation for Relocations since dereferencing to SortedMap is not always implemented but that one is far more trivial. Added fields could otherwise be silently forgotten since private fields make destructing outside the module possible only with `..` pattern which would then also be applicable to newly added public fields.

This also means that the compressed representation chosen may be optimized together with any changes to the undef_mask.

HeroicKatora · 2019-08-17T19:56:12Z

Moved the HashStable implementation into the module itself, renamed the raw accessor method, added comments for calls to it.

src/librustc/mir/interpret/allocation.rs

RalfJung

LGTM, with the ntis fixed! However, I can't tell whether the arguments made in codegen_llvm are correct. Handing over to

r? @oli-obk

RalfJung · 2019-08-31T10:26:47Z

r? @oli-obk

(Doesn't look like the bots looks into reviews.)

src/librustc/mir/interpret/allocation.rs

This improves the clarity of the documentation a bit since they can reference each other when reading the member docs in sequence.

HeroicKatora · 2019-09-02T19:52:29Z

@oli-obk I fixed the remaining nits from Ralf now, there are some unresolved review comments where you commented and I wasn't certain they were fully addressed. And the review of codegen is still oustanding as mentioned. Status should now be waiting on review.

oli-obk · 2019-09-02T20:13:34Z

@bors r+

Oh, right. I reviewed those comments when you made them. Reviewed again, still all good.

bors · 2019-09-02T20:13:35Z

📌 Commit f3c435e has been approved by oli-obk

bors · 2019-09-02T22:07:47Z

⌛ Testing commit f3c435e with merge b505208...

@RalfJung

Make Allocation::bytes private Fixes #62931. Direct immutable access to the bytes is still possible but redirected through the new method `raw_bytes_with_undef_and_ptr`, similar to `get_bytes_with_undef_and_ptr` but without requiring an interpretation context and not doing *any* relocation or bounds checks. The `size` of the allocation is stored separately which makes access as `Size` and `usize` more ergonomic. cc: @RalfJung

bors · 2019-09-03T02:00:13Z

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing b505208 to master...

rust-highfive · 2019-09-03T02:01:27Z

📣 Toolstate changed by #63561!

Tested on commit b505208.
Direct link to PR: #63561

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch @flip1995, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch @flip1995, @rust-lang/infra).
💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).

@Manishearth

Tested on commit rust-lang/rust@b505208. Direct link to PR: <rust-lang/rust#63561> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch @flip1995, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch @flip1995, @rust-lang/infra). 💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).

Rustup to rust-lang/rust#63561 changelog: none

Avoid copying some undef memory in MIR Copying memory between allocations in MIR is not required to preserve the current representation of undef bytes. When the complete source of a copy is in undef state then there are no bytes that need to be copied and the function can return nearly immediately. In some cases this completely avoids writing to the allocation's memory with should have various benefits. This for example reduces required physical memory when interpreting `const fn` involving large uninitialized values, such as `MaybeUninit::uninit()`. (*This description has been changed from the original one after parts have been integrated into #63561 *)

rust-highfive assigned cramertj Aug 14, 2019

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

rust-highfive assigned RalfJung and unassigned cramertj Aug 14, 2019

Centril reviewed Aug 14, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

HeroicKatora force-pushed the alloc-private-bytes branch from e4b3045 to 0dcc038 Compare August 14, 2019 16:53

RalfJung reviewed Aug 14, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 14, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 14, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

HeroicKatora force-pushed the alloc-private-bytes branch from 0dcc038 to 6e30b33 Compare August 15, 2019 14:12

RalfJung reviewed Aug 16, 2019

View reviewed changes

src/librustc/ich/impls_ty.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 16, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 16, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 16, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 16, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 16, 2019

View reviewed changes

src/librustc/ty/print/pretty.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 16, 2019

View reviewed changes

src/librustc/ty/print/pretty.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 16, 2019

View reviewed changes

oli-obk reviewed Aug 16, 2019

View reviewed changes

src/librustc_codegen_llvm/consts.rs Outdated Show resolved Hide resolved

HeroicKatora added 3 commits August 17, 2019 18:43

Move copy of undef_mask into allocation

98cff69

This also means that the compressed representation chosen may be optimized together with any changes to the undef_mask.

HeroicKatora force-pushed the alloc-private-bytes branch from 6e30b33 to ddc7cc3 Compare August 17, 2019 19:54

RalfJung reviewed Aug 31, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 31, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Show resolved Hide resolved

RalfJung approved these changes Aug 31, 2019

View reviewed changes

rust-highfive assigned oli-obk and unassigned RalfJung Aug 31, 2019

RalfJung reviewed Aug 31, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 31, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Show resolved Hide resolved

Improve documentation around allocation accessors

823c3b9

RalfJung reviewed Aug 31, 2019

View reviewed changes

src/librustc/mir/interpret/allocation.rs Show resolved Hide resolved

Reorder AllocationDefinedness members

f3c435e

This improves the clarity of the documentation a bit since they can reference each other when reading the member docs in sequence.

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 Sep 2, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 3, 2019

bors merged commit f3c435e into rust-lang:master Sep 3, 2019

bors mentioned this pull request Sep 3, 2019

Get rid of special const intrinsic query in favour of const_eval #61626

Merged

rust-highfive mentioned this pull request Sep 3, 2019

miri no longer builds after rust-lang/rust#63561 #64109

Closed

bors mentioned this pull request Sep 3, 2019

Make sure interned constants are immutable #63955

Merged

flip1995 mentioned this pull request Sep 3, 2019

lint against MaybeUninit::uninit().assume_init() rust-lang/rust-clippy#4479

Merged

bors added a commit to rust-lang/rust-clippy that referenced this pull request Sep 3, 2019

Auto merge of #4493 - JohnTitor:fix-build-bytes, r=flip1995

2d7da04

Rustup to rust-lang/rust#63561 changelog: none

HeroicKatora deleted the alloc-private-bytes branch March 7, 2025 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Allocation::bytes private #63561

Make Allocation::bytes private #63561

HeroicKatora commented Aug 14, 2019 •

edited by RalfJung

Loading

rust-highfive commented Aug 14, 2019

HeroicKatora commented Aug 14, 2019

HeroicKatora commented Aug 14, 2019

RalfJung commented Aug 14, 2019

HeroicKatora commented Aug 15, 2019

HeroicKatora commented Aug 15, 2019

RalfJung left a comment

RalfJung Aug 16, 2019

HeroicKatora Aug 17, 2019

HeroicKatora Aug 17, 2019

oli-obk Aug 21, 2019

HeroicKatora Aug 22, 2019

oli-obk Aug 22, 2019

HeroicKatora Aug 29, 2019 •

edited

Loading

RalfJung Aug 31, 2019

HeroicKatora commented Aug 17, 2019

RalfJung left a comment

RalfJung commented Aug 31, 2019

HeroicKatora commented Sep 2, 2019

oli-obk commented Sep 2, 2019

bors commented Sep 2, 2019

bors commented Sep 2, 2019

bors commented Sep 3, 2019

rust-highfive commented Sep 3, 2019

	/// Returns all relocations overlapping with the given ptr-offset pair.
	pub fn relocations(
	&self,
	cx: &impl HasDataLayout,
	ptr: Pointer<Tag>,
	size: Size,
	) -> &[(Size, (Tag, AllocId))] {

Make Allocation::bytes private #63561

Make Allocation::bytes private #63561

Conversation

HeroicKatora commented Aug 14, 2019 • edited by RalfJung Loading

rust-highfive commented Aug 14, 2019

HeroicKatora commented Aug 14, 2019

HeroicKatora commented Aug 14, 2019

RalfJung commented Aug 14, 2019

HeroicKatora commented Aug 15, 2019

HeroicKatora commented Aug 15, 2019

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung Aug 16, 2019

Choose a reason for hiding this comment

HeroicKatora Aug 17, 2019

Choose a reason for hiding this comment

HeroicKatora Aug 17, 2019

Choose a reason for hiding this comment

oli-obk Aug 21, 2019

Choose a reason for hiding this comment

HeroicKatora Aug 22, 2019

Choose a reason for hiding this comment

oli-obk Aug 22, 2019

Choose a reason for hiding this comment

HeroicKatora Aug 29, 2019 • edited Loading

Choose a reason for hiding this comment

RalfJung Aug 31, 2019

Choose a reason for hiding this comment

HeroicKatora commented Aug 17, 2019

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung commented Aug 31, 2019

HeroicKatora commented Sep 2, 2019

oli-obk commented Sep 2, 2019

bors commented Sep 2, 2019

bors commented Sep 2, 2019

bors commented Sep 3, 2019

rust-highfive commented Sep 3, 2019

HeroicKatora commented Aug 14, 2019 •

edited by RalfJung

Loading

HeroicKatora Aug 29, 2019 •

edited

Loading