Remove asmCoercion family of functions from parseTools.js. NFC #14436
Conversation
|
I'm not sure about |
sbc100
force-pushed
the
remove_asmCoercion
branch
3 times, most recently
from
2999a68 to
1c5b605
Compare
These don't have a role to play any longer in a non-asm.js world.
sbc100
force-pushed
the
remove_asmCoercion
branch
from
1c5b605 to
a7e6159
Compare
| @@ -894,7 +842,7 @@ function getHeapForType(type, unsigned) { | |||
| // case external JS library code uses this name. | |||
| function makeStructuralReturn(values) { | |||
| assert(values.length == 2); | |||
| return 'setTempRet0(' + values[1] + '); return ' + asmCoercion(values[0], 'i32'); | |||
| return 'setTempRet0(' + values[1] + '); return ' + values[0]; | |||
There was a problem hiding this comment.
There is a possible risk here, as this turns return x | 0 into return x, and maybe x was unsigned.
The risk is probably low, but I'm not sure how low?
There was a problem hiding this comment.
Is this a risk just here for makeStructuralReturn? Or elsewhere too? I'm not sure I understand that the risk exactly.. maybe you could give an example of how it could break?
Internally makeStructuralReturn only has a single user __cxa_find_matching_catch and values[0] is always a pointer in that case.
There was a problem hiding this comment.
It could be in the others as well.
If we used to have
function foo() {
return x + y | 0;but now we have
function foo() {
return x + y;Then the result may be different, if it overflows beyond a 32-bit integer. It seems unlikely, but it's still a risk. Leaving the coercion is probably safest.
There was a problem hiding this comment.
It does sound like the single user here is fine, though. If you audit all the other code paths in the others as well, then sgtm.
| asmEnsureFloat(4294967296, 'double') + ')', 'double') + ', ' + | ||
| asmEnsureFloat(4294967295, 'double') + ')', 'i32') + '>>>0' + | ||
| 'Math.abs(VALUE) >= 1 ? (VALUE > 0 ? ' + | ||
| 'Math.min(Math.floor((VALUE)/4294967296), 4294967295)>>>0' + |
There was a problem hiding this comment.
This is one place that I'm not sure I understand the implications of removing the coercions.
In this change I've removed all usage of asmCoercion. I think all the other ones looks safe, but I don't grok this enough to know if it might have and effect.
There was a problem hiding this comment.
The double conversions (using +(x)) have no effect on doubles. But the i32 one might be important. It might be easiest to print out the output of this to see exactly what the |0 is on, for us to check.
|
This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant. |
|
ping |
|
Is the ping for me? The last two comments are questions from me to you: (1) to audit all other code paths, and (2) to print out the output from the one confusing location you mentioned. |
|
Sorry, no just a keepalive ping for the stalebot |
These don't have a role to play any longer in a non-asm.js world.