Add debug range checks for makeSetValue #19251
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
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 |
src/parseTools.js
Outdated
| @@ -381,7 +392,8 @@ function makeSetValue(ptr, pos, value, type, noNeedFirst, ignore, align, sep) { | |||
| if (slab == 'HEAPU64' || slab == 'HEAP64') { | |||
| value = `BigInt(${value})`; | |||
| } | |||
| 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
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).