Skip to content

Comments

Fix sleb128_decode overflow checking#2630

Merged
mergify[bot] merged 2 commits intomasterfrom
osa1/fix_sleb128_decode
Jun 29, 2021
Merged

Fix sleb128_decode overflow checking#2630
mergify[bot] merged 2 commits intomasterfrom
osa1/fix_sleb128_decode

Conversation

@osa1
Copy link
Contributor

@osa1 osa1 commented Jun 28, 2021

Fixes #2625

Refactors decode functions to allow checking overflows.

New tests added for checking edge cases + overflows.

Fixes #2625

New tests added for checking edge cases + overflows.
@osa1 osa1 requested a review from crusso June 28, 2021 10:50
@dfinity-ci
Copy link

This PR does not affect the produced WebAssembly code.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

LGTM.

Are the option returning versions used on hot paths? Would it be worth writing ones that either panic on overflow or return boolean out parameter indicating success instead of allocating an option?

@osa1
Copy link
Contributor Author

osa1 commented Jun 29, 2021

Are the option returning versions used on hot paths? Would it be worth writing ones that either panic on overflow or return boolean out parameter indicating success instead of allocating an option?

I didn't check the generated code, but in CanCan backend benchmark this reduced cycles by 0.00041% (42,055,832 to 42,055,656).

@osa1 osa1 added the automerge-squash When ready, merge (using squash) label Jun 29, 2021
@mergify mergify bot merged commit eea4e34 into master Jun 29, 2021
@mergify mergify bot deleted the osa1/fix_sleb128_decode branch June 29, 2021 11:58
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RTS: sleb128_decode overflow check is not right

3 participants