[optimizer] Fix in-block jump offsets resolved wrong inside blocks with NOP statements. #1042
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was playing around with implementing stuff in
gencode.c
when I encountered two bugs that can occur when you have a block with in-block jumps (statements with jt and jf pointing to other statements in the block) that also has NOPs.The problem stems from the
offset
array, the size of the array is calculated by the functionslength
which does not count NOPs even though the array is populated with NOPs.This causes two things:
offset
array does not contains all of the statements in the block, and thus trying to jump to one of the missing statements at the end of the block fails unless-O
is given.The naïve solution would be to not include the NOPs in the offset table, and even though that will solve these two problems, it will cause a different bug where you cannot jump to a statement that is being optimized out.
Assuming all optimizations take
n
statement and replace firstn-1
with NOPs, if we jump to a NOP I think the wanted behavior is to jump to then
th statement.So, since the
offset
array is generated "for convenience", and since we can see it both causes bugs and takes up more lines of code then the trivial approach, I think that removing it is the right fix for these problems.I have created two minimal examples of the bugs in this branch
without fix:
with fix: