Skip to content

Fix updating srclocs in truncate_last_branch - #3014

Merged
cfallin merged 1 commit into
bytecodealliance:mainfrom
uweigand:fix-srcloc
Jun 22, 2021
Merged

cfallin merged 1 commit into
bytecodealliance:mainfrom
uweigand:fix-srcloc

Conversation

@uweigand

Copy link
Copy Markdown
Member

The truncate_last_branch removes an instruction that had already
been added to the buffer, and must update various bookkeeping.

However, updating the "srclocs" field is incorrect: if there is
a srclocs entry that spans both the removed branch and some
previous instruction
, that whole srclocs entry is removed,
which makes those previous instructions now uncovered by any
srclocs record. This can cause subsequent problems e.g. if
one of those instructions traps.

Fixed by just truncating instead of fully removing the srclocs
record in this case.

The truncate_last_branch removes an instruction that had already
been added to the buffer, and must update various bookkeeping.

However, updating the "srclocs" field is incorrect: if there is
a srclocs entry that spans both the removed branch *and some
previous instruction*, that whole srclocs entry is removed,
which makes those previous instructions now uncovered by any
srclocs record.  This can cause subsequent problems e.g. if
one of those instructions traps.

Fixed by just truncating instead of fully removing the srclocs
record in this case.
@github-actions github-actions Bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. labels Jun 22, 2021

@cfallin cfallin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find, thanks!

@cfallin
cfallin merged commit 4b25e3e into bytecodealliance:main Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants