Conversation
sbc100
force-pushed
the
checked_makeSetValue
branch
2 times, most recently
from
9e8d169 to
beb3478
Compare
sbc100
force-pushed
the
checked_makeSetValue
branch
2 times, most recently
from
8770c7c to
74b851d
Compare
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
The finding here is good but I think maybe the assertions are excessive? It is a reasonable thing for an optimizer to do HEAP32[x] = doubleValueThatWeWantLowerBitsFrom, that is, rather than manually mask off the lower bits, depend on the semantics of the JS operation that does it automatically.
If you want to do These assertions only apply the We can always revert if we find genuine uses for the truncating/lossy behaviour in |
|
(also its hard to imagine more heavy user of |
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Fair enough, as this is just for makeSetValue the assertion seems reasonable, and it doesn't limit the optimizer later.
src/parseTools.js
Outdated
| } | ||
| return slab + '[' + getHeapOffset(offset, type) + '] = ' + value; | ||
| var rtn = slab + '[' + getHeapOffset(offset, type) + '] = ' + value; | ||
| return rtn; |
There was a problem hiding this comment.
This change (adding rtn) looks unnecessary?
sbc100
force-pushed
the
checked_makeSetValue
branch
2 times, most recently
from
6c9459d to
6553053
Compare
sbc100
force-pushed
the
checked_makeSetValue
branch
from
6553053 to
129d158
Compare
Trying to store a value outside the range of the type being stored to can result in unexpected behaviour. For example, storing 256 to a u8 will end up storing 1. Adding these checks discovered real bug in out library code in `getaddrinfo`. I ran the full other and core test suite with these checks enabled at ASSERTIONS=1 before deciding to use ASSERTIONS=2, at least for now.
sbc100
force-pushed
the
checked_makeSetValue
branch
from
129d158 to
0af6c79
Compare
2 participants
Trying to store a value outside the range of the type being stored to can result in unexpected behaviour. For example, storing 256 to a u8 will end up storing 1.
I decided to write this as a followup to #19239 to make it extra clear what the expected behaviour of makeSetValue is.
Adding these checks discovered real bug in out library code in
getaddrinfo.I ran the full other and core test suite with these checks enabled at ASSERTIONS=1 before deciding to use ASSERTIONS=2 (at least for now).