Fix a case of using the wrong stack map during gcs #2396
Conversation
crates/runtime/src/externref.rs
Outdated
| // Add a marker between functions indicating that this function's pc | ||
| // starts with no stack map so when our binary search later on finds | ||
| // a pc between the start of the function and the function's first | ||
| // stack map it doesn't think the previous stack map is our stack | ||
| // map. | ||
| if pc_to_stack_map.len() > 0 { | ||
| pc_to_stack_map.push((range.start, None)); | ||
| } | ||
|
|
||
| for info in infos { | ||
| assert!((info.code_offset as usize) < len); | ||
| pc_to_stack_map.push(( | ||
| range.start + (info.code_offset as usize), | ||
| Rc::new(info.stack_map.clone()), | ||
| Some(Rc::new(info.stack_map.clone())), |
There was a problem hiding this comment.
If info.code_offset is 0 then we could have duplicate entries (one None for the start of the function and one Some for this info).
I don't think this should ever happen though, so perhaps we can just assert that info.code_offset > 0?
The alternative is checking ahead of time whether the first info.code_offset is 0 (because they should be sorted iirc) and only adding the None entry if the first info.code_offset is not 0.
There was a problem hiding this comment.
Oh nice catch, I refactored a bit which I think will handle this correctly, mind double-checking?
I also went ahead and did a small tweak where consecutive None entries are now coalesced.
alexcrichton
force-pushed
the
use-right-stack-map
branch
from
af72028 to
2f468ca
Compare
This commit fixes an issue where when looking up the stack map for a pc within a function we might end up reading the *previous* function's stack maps. This then later caused asserts to trip because we started interpreting random data as a `VMExternRef` when it wasn't. The fix was to add `None` markers for "this range has no stack map" in the function ranges map. Closes bytecodealliance#2386
alexcrichton
force-pushed
the
use-right-stack-map
branch
from
2f468ca to
3ef1002
Compare
| // marker, in which case the starting pc already has no stack map. | ||
| // This is also skipped if the first `code_offset` is zero since | ||
| // what we'll push applies for the first pc anyway. | ||
| if !last_is_none_marker && (infos.is_empty() || infos[0].code_offset > 0) { |
There was a problem hiding this comment.
Could make this a tiny bit less wordy (although perhaps some would argue less readable) like this:
| if !last_is_none_marker && (infos.is_empty() || infos[0].code_offset > 0) { | |
| if !last_is_none_marker && (infos.get(0).map_or(true, |i| i.code_offset > 0) { |
I would probably use map_or personally but it doesn't really matter either way.
There was a problem hiding this comment.
Oh I guess this isn't actually even less wordy... nevermind!
This commit fixes an issue where when looking up the stack map for a pc within a function we might end up reading the *previous* function's stack maps. This then later caused asserts to trip because we started interpreting random data as a `VMExternRef` when it wasn't. The fix was to add `None` markers for "this range has no stack map" in the function ranges map. Closes #2386
This commit fixes an issue where when looking up the stack map for a pc
within a function we might end up reading the previous function's
stack maps. This then later caused asserts to trip because we started
interpreting random data as a
VMExternRefwhen it wasn't. The fix wasto add
Nonemarkers for "this range has no stack map" in the functionranges map.
Closes #2386