-
Notifications
You must be signed in to change notification settings - Fork 13.5k
interpret/allocation: expose init + write_wildcards on a range #143634
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rustbot author
There's also something missing: either you need an operation that fills uninit bytes with 0 without doing the mark_foreign_write
steps, or you need to patch write_uninit
to enforce the general invariant that uninit bytes are 0. I think we should do the latter.
compiler/rustc_middle/src/mir/interpret/allocation/provenance_map.rs
Outdated
Show resolved
Hide resolved
Reminder, once the PR becomes ready for a review, use |
I think I implemented that in rust-lang/miri#4456 directly in Miri; under // Prepare for possible write from native code if mutable.
if info.mutbl.is_mut() {
let alloc = this.get_alloc_raw_mut(alloc_id)?.0;
if tracing {
let full_range =
AllocRange { start: Size::ZERO, size: Size::from_bytes(alloc.len()) };
// Overwrite uninitialized bytes with 0, to ensure we don't leak whatever their value happens to be.
for chunk in alloc.init_mask().clone().range_as_init_chunks(full_range) {
if !chunk.is_init() {
let uninit_bytes = unsafe {
let start = chunk.range().start.bytes_usize();
let len = chunk.range().end.bytes_usize().strict_sub(start);
let ptr = alloc.get_bytes_unchecked_raw_mut().add(start);
std::slice::from_raw_parts_mut(ptr, len)
};
uninit_bytes.fill(0);
}
}
} else {
// FIXME: Make this take an arg to determine whether it actually
// writes wildcard prov & marks init, so we don't duplicate code above.
alloc.prepare_for_native_access();
}
// Also expose *mutable* provenance for the interpreter-level allocation.
std::hint::black_box(alloc.get_bytes_unchecked_raw_mut().expose_provenance());
} Though there is a FIXME about this, I considered moving |
Please don't. That's just unnecessary code duplication. Also, just because you now can access all these private parts of the allocation from Miri doesn't mean you should.
Indeed, which is why I suggested a way that it can be avoided. :) |
The Miri subtree was changed cc @rust-lang/miri |
Can't test locally but seems to build @rustbot ready |
compiler/rustc_middle/src/mir/interpret/allocation/provenance_map.rs
Outdated
Show resolved
Hide resolved
compiler/rustc_middle/src/mir/interpret/allocation/provenance_map.rs
Outdated
Show resolved
Hide resolved
/// Initialize all previously uninitialized bytes in the entire allocation, and set | ||
/// provenance of everything to `Wildcard`. Before calling this, make sure all | ||
/// Initialize all previously uninitialized bytes in the entire allocation, but | ||
/// do not actually mark them as init. Before calling this, make sure all | ||
/// provenance in this allocation is exposed! | ||
pub fn prepare_for_native_access(&mut self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we had agreed that this function should just disappear entirely? I guess I was not clear enough, so here we go:
- please adjust
write_uninit
to fill the range with 0s - then remove
prepare_for_native_access
, it is not not needed any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Sorry, I thought you meant the opposite - I was concerned that having write_uninit()
write zeroes by default would be a perf hit. But sure, I'll do that if you think it's an okay tradeoff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that was the misunderstanding then. :)
write_uninit
already does some things that are probably more expensive than writing a few 0s, so I think it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should mark_init(false)
also inherit this behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a private function... no, it doesn't need to.
@rustbot author |
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
(ignore CI, there was an oopsie and everything is broken) |
33a6382
to
de15924
Compare
Thanks! @bors r+ rollup |
Oh, damn... rust/compiler/rustc_const_eval/src/interpret/memory.rs Lines 1465 to 1477 in 93f1201
@bors r- I am not sure what to do about that. The comment there explicitly says we do not want to write a bunch of 0s into the buffer, which is exactly what we are doing now. |
de15924
to
60e7104
Compare
Let's see if we see this in perf. @bors2 try |
This comment has been minimized.
This comment has been minimized.
interpret/allocation: expose init + write_wildcards on a range Part of rust-lang/miri#4456, so that we can mark down when a foreign access to our memory happened. Should this also move `prepare_for_native_access()` itself into Miri, given that everything there can be implemented on Miri's side? r? `@RalfJung`
Why not just gate the zero write behind some condition (e.g. being in Miri proper and not the CTFE machine)? Seems like that function gets an |
Actually it seems like only Miri sets |
Even Miri only needs this if native-lib mode is enabled... |
I don't see a decent way to do this directly; best I can think of is extending |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (e6c1026): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -1.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 464.444s -> 464.79s (0.07%) |
...well I can't say I expected that but I'm not complaining! |
The I will take this mostly as a null result, this is not that much above the usual noise level. @oli-obk what do you think? Is it worth preserving the optimization where if you copy a large amount of uninitialized memory somewhere, we avoid actually touching the target memory and filling it with 0s? This was apparently added in #67658. I guess it was added for a reason but we don't have a benchmark for a large uninit array being copied around. Seems like a shame to regress this. Maybe we should just say that it's entirely fine for native code to see non-0 leftover values in uninit memory. It's uninit after all... |
yes, I think that is the right behaviour. I haven't read the rest of the PR yet, will do so now |
Yea we should just keep the bytes at whatever they were, even if that exposes some non-zero bytes. |
@nia-e okay, so can you remove the filling-with-0s, and the comment stating that as an invariant, and add a comment in Miri in the native-lib code saying that yes this exposes whatever bytes happen to be in the "uninitialized" part to the C code but, well, it's uninitialized so it's arbitrary garbage anyway. (The invariant about there being no provenance on uninit bytes is still true AFAIK. I don't think we need it but it's probably a good invariant to have and it doesn't hurt documenting it.) |
@rustbot author |
60e7104
to
8d0e0c6
Compare
Force-pushed, hope that's ok given it's tiny? |
@rustbot ready |
Part of rust-lang/miri#4456, so that we can mark down when a foreign access to our memory happened. Should this also move
prepare_for_native_access()
itself into Miri, given that everything there can be implemented on Miri's side?r? @RalfJung