Repository navigation
don't kill borrows of #[thread_local] statics when their StaticRef temporaries go out of scope - #165291
don't kill borrows of #[thread_local] statics when their StaticRef temporaries go out of scope#165291dianne wants to merge 1 commit into
#[thread_local] statics when their StaticRef temporaries go out of scope#165291Conversation
rustbot
added
S-waiting-on-author
This comment was marked as outdated.
This comment was marked as outdated.
| mir::StatementKind::StorageDead(local) => { | ||
| // Make sure there are no remaining borrows for locals that | ||
| // are gone out of scope. | ||
| self.kill_borrows_on_place(state, Place::from(*local)); | ||
| &mir::StatementKind::StorageDead(local) => { | ||
| // Make sure there are no remaining borrows for locals that have gone out of scope, | ||
| // except for temporaries created to point to thread-local storage. Borrows of | ||
| // thread-locals are tracked to prevent them from living past the end of functions; | ||
| // we shouldn't kill them when the temporary they're accessed through dies. | ||
| if !self.body.local_decls[local].is_ref_to_thread_local() { | ||
| self.kill_borrows_on_place(state, Place::from(local)); | ||
| } |
There was a problem hiding this comment.
I think this should be equivalent to the logic before #165119: the old logic was in kill_borrows_on_place but it checked the place for no projections, which I think means it only applied to StorageDeads of shared ref-to-thread-local temporaries1. since it's effectively just for StorageDeads, I feel like it makes more sense to move the check here.
alternatively, in kill_borrows_on_place, we could use a shallow access depth when checking shallow accesses for conflicts with shared borrows, so that e.g. a StorageDead of a StaticRef temporary won't "conflict" with reborrows of the static. I think this only matters for thread-locals at the moment, since normal shared borrows aren't tracked and I don't think fake borrows are used in a way where it'd matter for them. for exclusive borrows, I think I'm convinced that checking for conflicts using a deep access depth is fine, but that seems too aggressive for shared borrows in the rare instance we need to track them (i.e. exactly this instance)
Footnotes
-
since other uses of
StaticRefs are implicitly derefed, normal shared static refs aren't tracked, and mutable statics are represented with raw pointers. in that sense, it's arbitrary whether we check.is_ref_to_static()or.is_ref_to_thread_local(). I've gone back and forth a bit on which to choose, but currently it's.is_ref_to_thread_local()for specificity. I could see opting for.is_ref_to_static()on the moral basis that the same static can be accessed through multipleStaticReftemporaries, but that argument suggests to me that this wouldn't just be to avoid killing shared borrows of thread locals on storage death. pursuing the moral argument, I'd rather either changekill_borrows_on_placeto handle shared borrows better or somehow make thread locals not borrowable as'static.. maybe then this could be done using normal region errors, so tracking real shared borrows wouldn't be necessary? ↩
|
r? borrowck or @oli-obk (since you reviewed #165119) or reassign multiple pings in case it'd be desirable to get this merged soon, but since it should require directly using |
rustbot
added
S-waiting-on-review
As I've just learned, borrows of
#[thread_local]s are prevented from escaping from functions by tracking them and checking that they're out-of-scope when the function exits. We shouldn't kill them prematurely. But without a case accounting either forStaticReftemporaries or shared borrows, borrows of a#[thread_local]will be considered out-of-scope upon reaching theStorageDeadfor theStaticReftemporary they were reborrowed through. Since theStorageDeaddoesn't actually conflict with any of those borrows, it would let them escape.This could be seen as reverting the mistake from #165119, then reorganizing and commenting it.
Fixes #165290