From ec4e48d4cbc28bcfd99e25842a90704e765b800f Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Wed, 1 Sep 2021 15:40:18 -0700 Subject: [PATCH] Stop doing fuzzy search for stack maps The new backends will not emit a stack map for a safepoint if there are zero live references. Our fuzzy search for stack maps, which was necessary for the old backend, caused us to use the wrong stack map for some PCs which would in turn cause us to treat arbitrary stack slots as reference types pointers. --- crates/wasmtime/src/module/registry.rs | 100 +++++++++++++------------ 1 file changed, 52 insertions(+), 48 deletions(-) diff --git a/crates/wasmtime/src/module/registry.rs b/crates/wasmtime/src/module/registry.rs index 08ebacdbe78e..89f851c488bf 100644 --- a/crates/wasmtime/src/module/registry.rs +++ b/crates/wasmtime/src/module/registry.rs @@ -122,61 +122,65 @@ impl ModuleInfo for RegisteredModule { let info = self.module.func_info(index); // Do a binary search to find the stack map for the given offset. - // - // Because GC safepoints are technically only associated with a single - // PC, we should ideally only care about `Ok(index)` values returned - // from the binary search. However, safepoints are inserted right before - // calls, and there are two things that can disturb the PC/offset - // associated with the safepoint versus the PC we actually use to query - // for the stack map: - // - // 1. The `backtrace` crate gives us the PC in a frame that will be - // *returned to*, and where execution will continue from, rather than - // the PC of the call we are currently at. So we would need to - // disassemble one instruction backwards to query the actual PC for - // the stack map. - // - // TODO: One thing we *could* do to make this a little less error - // prone, would be to assert/check that the nearest GC safepoint - // found is within `max_encoded_size(any kind of call instruction)` - // our queried PC for the target architecture. - // - // 2. Cranelift's stack maps only handle the stack, not - // registers. However, some references that are arguments to a call - // may need to be in registers. In these cases, what Cranelift will - // do is: - // - // a. spill all the live references, - // b. insert a GC safepoint for those references, - // c. reload the references into registers, and finally - // d. make the call. - // - // Step (c) adds drift between the GC safepoint and the location of - // the call, which is where we actually walk the stack frame and - // collect its live references. - // - // Luckily, the spill stack slots for the live references are still - // up to date, so we can still find all the on-stack roots. - // Furthermore, we do not have a moving GC, so we don't need to worry - // whether the following code will reuse the references in registers - // (which would not have been updated to point to the moved objects) - // or reload from the stack slots (which would have been updated to - // point to the moved objects). - let index = match info .stack_maps .binary_search_by_key(&func_offset, |i| i.code_offset) { - // Exact hit. + // Found it. Ok(i) => i, - // `Err(0)` means that the associated stack map would have been the - // first element in the array if this pc had an associated stack - // map, but this pc does not have an associated stack map. This can - // only happen inside a Wasm frame if there are no live refs at this - // pc. + // No stack map associated with this PC. + // + // Because we know we are in Wasm code, and we must be at some kind + // of call/safepoint, then the Cranelift backend must have avoided + // emitting a stack map for this location because no refs were live. + #[cfg(not(feature = "old-x86-backend"))] + Err(_) => return None, + + // ### Old x86_64 backend specific code. + // + // Because GC safepoints are technically only associated with a + // single PC, we should ideally only care about `Ok(index)` values + // returned from the binary search. However, safepoints are inserted + // right before calls, and there are two things that can disturb the + // PC/offset associated with the safepoint versus the PC we actually + // use to query for the stack map: + // + // 1. The `backtrace` crate gives us the PC in a frame that will be + // *returned to*, and where execution will continue from, rather than + // the PC of the call we are currently at. So we would need to + // disassemble one instruction backwards to query the actual PC for + // the stack map. + // + // TODO: One thing we *could* do to make this a little less error + // prone, would be to assert/check that the nearest GC safepoint + // found is within `max_encoded_size(any kind of call instruction)` + // our queried PC for the target architecture. + // + // 2. Cranelift's stack maps only handle the stack, not + // registers. However, some references that are arguments to a call + // may need to be in registers. In these cases, what Cranelift will + // do is: + // + // a. spill all the live references, + // b. insert a GC safepoint for those references, + // c. reload the references into registers, and finally + // d. make the call. + // + // Step (c) adds drift between the GC safepoint and the location of + // the call, which is where we actually walk the stack frame and + // collect its live references. + // + // Luckily, the spill stack slots for the live references are still + // up to date, so we can still find all the on-stack roots. + // Furthermore, we do not have a moving GC, so we don't need to worry + // whether the following code will reuse the references in registers + // (which would not have been updated to point to the moved objects) + // or reload from the stack slots (which would have been updated to + // point to the moved objects). + #[cfg(feature = "old-x86-backend")] Err(0) => return None, - + #[cfg(feature = "old-x86-backend")] Err(i) => i - 1, };