From b75dfa8a2bac745d7d09212e3e28cb4f0bc28fdf Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 25 Jul 2019 19:29:48 +0200 Subject: [PATCH 1/7] Don't access a static just for its size and alignment --- src/librustc_mir/interpret/memory.rs | 27 ++++++++++++------------ src/test/ui/consts/static-cycle-error.rs | 11 ++++++++++ 2 files changed, 24 insertions(+), 14 deletions(-) create mode 100644 src/test/ui/consts/static-cycle-error.rs diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 3f2a76a77be36..674ae29070644 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -535,6 +535,19 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, liveness: AllocCheck, ) -> InterpResult<'static, (Size, Align)> { + // Allocations of `static` items + // Can't do this in the match argument, we may get cycle errors since the lock would + // be held throughout the match. + let alloc = self.tcx.alloc_map.lock().get(id); + match alloc { + Some(GlobalAlloc::Static(did)) => { + // Use size and align of the type + let ty = self.tcx.type_of(did); + let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); + return Ok((layout.size, layout.align.abi)); + } + _ => {} + } // Regular allocations. if let Ok(alloc) = self.get(id) { return Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)); @@ -548,20 +561,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { Ok((Size::ZERO, Align::from_bytes(1).unwrap())) }; } - // Foreign statics. - // Can't do this in the match argument, we may get cycle errors since the lock would - // be held throughout the match. - let alloc = self.tcx.alloc_map.lock().get(id); - match alloc { - Some(GlobalAlloc::Static(did)) => { - assert!(self.tcx.is_foreign_item(did)); - // Use size and align of the type - let ty = self.tcx.type_of(did); - let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); - return Ok((layout.size, layout.align.abi)); - } - _ => {} - } // The rest must be dead. if let AllocCheck::MaybeDead = liveness { // Deallocated pointers are allowed, we should be able to find diff --git a/src/test/ui/consts/static-cycle-error.rs b/src/test/ui/consts/static-cycle-error.rs new file mode 100644 index 0000000000000..9ce050aae2181 --- /dev/null +++ b/src/test/ui/consts/static-cycle-error.rs @@ -0,0 +1,11 @@ +// check-pass + +struct Foo { + foo: Option<&'static Foo> +} + +static FOO: Foo = Foo { + foo: Some(&FOO), +}; + +fn main() {} From d9ac0c67ed5a3ea7d708894a4636a6e83c5aec49 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 25 Jul 2019 23:53:05 +0200 Subject: [PATCH 2/7] Rewrite `get_size_and_align` so it doesn't duplicate work --- src/librustc_mir/interpret/memory.rs | 69 ++++++++++++++-------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 674ae29070644..c8e09ca4a1a47 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -535,40 +535,41 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, liveness: AllocCheck, ) -> InterpResult<'static, (Size, Align)> { - // Allocations of `static` items - // Can't do this in the match argument, we may get cycle errors since the lock would - // be held throughout the match. - let alloc = self.tcx.alloc_map.lock().get(id); - match alloc { - Some(GlobalAlloc::Static(did)) => { - // Use size and align of the type - let ty = self.tcx.type_of(did); - let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); - return Ok((layout.size, layout.align.abi)); - } - _ => {} - } - // Regular allocations. - if let Ok(alloc) = self.get(id) { - return Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)); - } - // Function pointers. - if let Ok(_) = self.get_fn_alloc(id) { - return if let AllocCheck::Dereferencable = liveness { - // The caller requested no function pointers. - err!(DerefFunctionPointer) - } else { - Ok((Size::ZERO, Align::from_bytes(1).unwrap())) - }; - } - // The rest must be dead. - if let AllocCheck::MaybeDead = liveness { - // Deallocated pointers are allowed, we should be able to find - // them in the map. - Ok(*self.dead_alloc_map.get(&id) - .expect("deallocated pointers should all be recorded in `dead_alloc_map`")) - } else { - err!(DanglingPointerDeref) + let alloc_or_size_align = self.alloc_map.get_or(id, || -> Result<_, InterpResult<'static, (Size, Align)>> { + // Can't do this in the match argument, we may get cycle errors since the lock would + // be held throughout the match. + let alloc = self.tcx.alloc_map.lock().get(id); + Err(match alloc { + Some(GlobalAlloc::Static(did)) => { + // Use size and align of the type + let ty = self.tcx.type_of(did); + let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); + Ok((layout.size, layout.align.abi)) + }, + Some(GlobalAlloc::Memory(alloc)) => + // this duplicates the logic on the `match alloc_or_size_align`, but due to the + // API of `get_or` there's no way around that. + Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), + Some(GlobalAlloc::Function(_)) => if let AllocCheck::Dereferencable = liveness { + // The caller requested no function pointers. + err!(DerefFunctionPointer) + } else { + Ok((Size::ZERO, Align::from_bytes(1).unwrap())) + }, + // The rest must be dead. + None => if let AllocCheck::MaybeDead = liveness { + // Deallocated pointers are allowed, we should be able to find + // them in the map. + Ok(*self.dead_alloc_map.get(&id) + .expect("deallocated pointers should all be recorded in `dead_alloc_map`")) + } else { + err!(DanglingPointerDeref) + }, + }) + }); + match alloc_or_size_align { + Ok((_, alloc)) => Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), + Err(done) => done, } } From 34e7a3cc4dcb7ddd404b9566047a78d1e234f137 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 26 Jul 2019 08:10:09 +0200 Subject: [PATCH 3/7] Fix tidy --- src/librustc_mir/interpret/memory.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index c8e09ca4a1a47..4ac5920c60e1a 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -535,7 +535,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, liveness: AllocCheck, ) -> InterpResult<'static, (Size, Align)> { - let alloc_or_size_align = self.alloc_map.get_or(id, || -> Result<_, InterpResult<'static, (Size, Align)>> { + let alloc_or_size_align = self.alloc_map.get_or(id, || { // Can't do this in the match argument, we may get cycle errors since the lock would // be held throughout the match. let alloc = self.tcx.alloc_map.lock().get(id); From 3bc1d01bb9d81fa3682186d8ace06becaa8cac8c Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 26 Jul 2019 10:34:54 +0200 Subject: [PATCH 4/7] Clear up `get_size_and_align` --- src/librustc_mir/interpret/memory.rs | 71 +++++++++++++++------------- 1 file changed, 37 insertions(+), 34 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 4ac5920c60e1a..9140c90bed376 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -535,41 +535,44 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { id: AllocId, liveness: AllocCheck, ) -> InterpResult<'static, (Size, Align)> { - let alloc_or_size_align = self.alloc_map.get_or(id, || { - // Can't do this in the match argument, we may get cycle errors since the lock would - // be held throughout the match. - let alloc = self.tcx.alloc_map.lock().get(id); - Err(match alloc { - Some(GlobalAlloc::Static(did)) => { - // Use size and align of the type - let ty = self.tcx.type_of(did); - let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); - Ok((layout.size, layout.align.abi)) - }, - Some(GlobalAlloc::Memory(alloc)) => - // this duplicates the logic on the `match alloc_or_size_align`, but due to the - // API of `get_or` there's no way around that. - Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), - Some(GlobalAlloc::Function(_)) => if let AllocCheck::Dereferencable = liveness { - // The caller requested no function pointers. - err!(DerefFunctionPointer) - } else { - Ok((Size::ZERO, Align::from_bytes(1).unwrap())) - }, - // The rest must be dead. - None => if let AllocCheck::MaybeDead = liveness { - // Deallocated pointers are allowed, we should be able to find - // them in the map. - Ok(*self.dead_alloc_map.get(&id) - .expect("deallocated pointers should all be recorded in `dead_alloc_map`")) - } else { - err!(DanglingPointerDeref) - }, - }) - }); - match alloc_or_size_align { + // Don't use `self.get` here as that will + // a) cause cycles in case `id` refers to a static + // b) duplicate a static's allocation in miri + match self.alloc_map.get_or(id, || Err(())) { Ok((_, alloc)) => Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), - Err(done) => done, + Err(()) => { + // Can't do this in the match argument, we may get cycle errors since the lock would + // be held throughout the match. + let alloc = self.tcx.alloc_map.lock().get(id); + match alloc { + Some(GlobalAlloc::Static(did)) => { + // Use size and align of the type + let ty = self.tcx.type_of(did); + let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); + return Ok((layout.size, layout.align.abi)); + }, + Some(GlobalAlloc::Memory(alloc)) => + // Need to duplicate the logic here, because the global allocations have + // different associated types than the interpreter-local ones + Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), + Some(GlobalAlloc::Function(_)) => if let AllocCheck::Dereferencable = liveness { + // The caller requested no function pointers. + return err!(DerefFunctionPointer); + } else { + return Ok((Size::ZERO, Align::from_bytes(1).unwrap())); + }, + // The rest must be dead. + None => return if let AllocCheck::MaybeDead = liveness { + // Deallocated pointers are allowed, we should be able to find + // them in the map. + Ok(*self.dead_alloc_map.get(&id) + .expect("deallocated pointers should all be recorded in \ + `dead_alloc_map`")) + } else { + err!(DanglingPointerDeref) + }, + } + } } } From 796e7a8d7ca1b06e07112579cdd74acc7b1c102b Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 26 Jul 2019 17:10:21 +0200 Subject: [PATCH 5/7] Address review comments --- src/librustc_mir/interpret/memory.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 9140c90bed376..934fa7f6f877b 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -541,6 +541,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { match self.alloc_map.get_or(id, || Err(())) { Ok((_, alloc)) => Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), Err(()) => { + // Not a local allocation, check the global `tcx.alloc_map`. + // Can't do this in the match argument, we may get cycle errors since the lock would // be held throughout the match. let alloc = self.tcx.alloc_map.lock().get(id); @@ -549,20 +551,22 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { // Use size and align of the type let ty = self.tcx.type_of(did); let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); - return Ok((layout.size, layout.align.abi)); + Ok((layout.size, layout.align.abi)) }, Some(GlobalAlloc::Memory(alloc)) => // Need to duplicate the logic here, because the global allocations have // different associated types than the interpreter-local ones Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), - Some(GlobalAlloc::Function(_)) => if let AllocCheck::Dereferencable = liveness { - // The caller requested no function pointers. - return err!(DerefFunctionPointer); - } else { - return Ok((Size::ZERO, Align::from_bytes(1).unwrap())); + Some(GlobalAlloc::Function(_)) => { + if let AllocCheck::Dereferencable = liveness { + // The caller requested no function pointers. + err!(DerefFunctionPointer) + } else { + Ok((Size::ZERO, Align::from_bytes(1).unwrap())) + } }, // The rest must be dead. - None => return if let AllocCheck::MaybeDead = liveness { + None => if let AllocCheck::MaybeDead = liveness { // Deallocated pointers are allowed, we should be able to find // them in the map. Ok(*self.dead_alloc_map.get(&id) From 6e04ca7fb6865f1481a7b2e635fd67b586dc0216 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 26 Jul 2019 17:43:49 +0200 Subject: [PATCH 6/7] Update src/librustc_mir/interpret/memory.rs Co-Authored-By: Ralf Jung --- src/librustc_mir/interpret/memory.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 934fa7f6f877b..1981a239a1196 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -548,7 +548,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { let alloc = self.tcx.alloc_map.lock().get(id); match alloc { Some(GlobalAlloc::Static(did)) => { - // Use size and align of the type + // Use size and align of the type. let ty = self.tcx.type_of(did); let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); Ok((layout.size, layout.align.abi)) From 0cd71678e17973ed40f898101d01588bf6f6757a Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Fri, 26 Jul 2019 17:44:11 +0200 Subject: [PATCH 7/7] Update src/librustc_mir/interpret/memory.rs Co-Authored-By: Ralf Jung --- src/librustc_mir/interpret/memory.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 1981a239a1196..4575784ac3703 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -555,7 +555,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { }, Some(GlobalAlloc::Memory(alloc)) => // Need to duplicate the logic here, because the global allocations have - // different associated types than the interpreter-local ones + // different associated types than the interpreter-local ones. Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)), Some(GlobalAlloc::Function(_)) => { if let AllocCheck::Dereferencable = liveness {