alexcrichton opened PR #2396 from use-right-stack-map
to main
:
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 aVMExternRef
when it wasn't. The fix was
to addNone
markers for "this range has no stack map" in the function
ranges map.Closes #2386
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
alexcrichton requested fitzgen for a review on PR #2396.
fitzgen submitted PR Review.
fitzgen submitted PR Review.
fitzgen created PR Review Comment:
If
info.code_offset
is0
then we could have duplicate entries (oneNone
for the start of the function and oneSome
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
is0
(because they should be sorted iirc) and only adding theNone
entry if the firstinfo.code_offset
is not0
.
alexcrichton updated PR #2396 from use-right-stack-map
to main
:
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 aVMExternRef
when it wasn't. The fix was
to addNone
markers for "this range has no stack map" in the function
ranges map.Closes #2386
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
alexcrichton submitted PR Review.
alexcrichton created PR Review 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 updated PR #2396 from use-right-stack-map
to main
:
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 aVMExternRef
when it wasn't. The fix was
to addNone
markers for "this range has no stack map" in the function
ranges map.Closes #2386
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
fitzgen submitted PR Review.
fitzgen submitted PR Review.
fitzgen created PR Review Comment:
Could make this a tiny bit less wordy (although perhaps some would argue less readable) like this:
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.
fitzgen submitted PR Review.
fitzgen created PR Review Comment:
Oh I guess this isn't actually even less wordy... nevermind!
alexcrichton merged PR #2396.
Last updated: Dec 23 2024 at 12:05 UTC