-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Make AllocId decoding thread-safe #50957
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,6 @@ pub use self::error::{EvalError, EvalResult, EvalErrorKind, AssertMessage}; | |
|
||
pub use self::value::{PrimVal, PrimValKind, Value, Pointer, ConstValue}; | ||
|
||
use std::collections::hash_map::Entry; | ||
use std::fmt; | ||
use mir; | ||
use hir::def_id::DefId; | ||
|
@@ -195,7 +194,21 @@ where | |
assert!(cache(encoder).insert(alloc_id, pos).is_none()); | ||
trace!("encoding {:?} with {:#?}", alloc_id, alloc); | ||
AllocKind::Alloc.encode(encoder)?; | ||
|
||
// Write placeholder for size | ||
let size_pos = encoder.position(); | ||
0usize.encode(encoder)?; | ||
|
||
let start = encoder.position(); | ||
alloc.encode(encoder)?; | ||
let end = encoder.position(); | ||
|
||
// Overwrite the placeholder with the real size | ||
let size: usize = end - start; | ||
encoder.set_position(size_pos); | ||
size.encode(encoder)?; | ||
encoder.set_position(end); | ||
|
||
} | ||
AllocType::Function(fn_instance) => { | ||
trace!("encoding {:?} with {:#?}", alloc_id, fn_instance); | ||
|
@@ -230,40 +243,23 @@ pub fn specialized_decode_alloc_id< | |
decoder.with_position(real_pos, AllocId::decode) | ||
}, | ||
AllocKind::Alloc => { | ||
let alloc_id = { | ||
let mut cache = global_cache(decoder); | ||
let entry = cache.entry(pos); | ||
match entry { | ||
Entry::Occupied(occupied) => { | ||
let id = occupied.get().0; | ||
|
||
// If the alloc id is fully loaded we just return here. | ||
if occupied.get().1 { | ||
return Ok(id) | ||
} | ||
|
||
// It was only partially loaded. | ||
// This may be loading further up the stack | ||
// or concurrently in another thread. | ||
id | ||
} | ||
Entry::Vacant(vacant) => { | ||
// Insert early to allow recursive allocs | ||
let id = tcx.alloc_map.lock().reserve(); | ||
vacant.insert((id, false)); | ||
id | ||
} | ||
} | ||
}; | ||
|
||
// Insert early to allow recursive allocs and ot indicate that the current | ||
// session will eventually fully load this alloc id | ||
if !local_cache(decoder).insert(alloc_id) { | ||
// Read the size of the allocation. | ||
// Used to skip ahead if we don't need to decode this. | ||
let alloc_size = usize::decode(decoder)?; | ||
|
||
let (alloc_id, fully_loaded) = *global_cache(decoder).entry(pos).or_insert_with(|| { | ||
// Create an id which is not fully loaded | ||
(tcx.alloc_map.lock().reserve(), false) | ||
}); | ||
if fully_loaded || !local_cache(decoder).insert(alloc_id) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if one thread tstarts decoding, the next thread takes over the CPU, gets here for the same AllocId, skips over and tries to access the allocation? It'll ICE about uncached alloc or error with dangling pointer, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, neat. Please make this explanation a comment on that if statement |
||
// We have started decoding this alloc id already, so just return it. | ||
// Its content is already filled in or will be filled in by functions | ||
// further up the stack. | ||
return Ok(alloc_id); | ||
|
||
|
||
// Skip the allocation | ||
let pos = decoder.position(); | ||
decoder.set_position(pos + alloc_size); | ||
return Ok(alloc_id) | ||
} | ||
|
||
let allocation = <&'tcx Allocation as Decodable>::decode(decoder)?; | ||
|
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.
This doesn't work because of variable-length integer encoding.
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.
rustc has some similar code elsewhere and works around this by using a 4 byte array that the size is encoded into. Somewhat space-wasteful though.
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.
It might be simpler to just remember the in the
global_cache
during decoding?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 doesn't work, because we need this value also when another thread hasn't finished decoding the allocation yet.