Closed
Bug 1136331
Opened 9 years ago
Closed 9 years ago
OdinMonkey: fix heap-resize no-builtin-call checking
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: luke, Assigned: luke)
Details
Attachments
(1 file)
7.68 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
Because of the order of evaluation of heap access expressions (the array view before the index expression or rhs of a store), we prevent calls in index/rhs expressions when heap resizing is used. Builtin-calls are fine, though, since they have fixed semantics and cannot reenter. This was almost implemented correctly, but I put the canCall check is in the wrong place which rules out some builtin calls.
Attachment #8568736 -
Flags: review?(benj)
Comment 1•9 years ago
|
||
Comment on attachment 8568736 [details] [diff] [review] fix-change-heap Review of attachment 8568736 [details] [diff] [review]: ----------------------------------------------------------------- Right, that makes sense. ::: js/src/asmjs/AsmJSValidate.cpp @@ +5144,5 @@ > RetType retType, MDefinition **def, Type *type) > { > + if (!f.canCall()) { > + return f.fail(callNode, "call expressions may not be nested inside heap expressions " > + "when the module contains a change-heap function"); Can you make the error message a static const char* variable, so that it can be reused by the three calls, instead of copy/pasto? (or even a small helper function) ::: js/src/jit-test/tests/asm.js/testResize.js @@ +152,5 @@ > asmCompile('glob', 'ffis', 'b', SETUP + 'function f() { i32[0] = 0 } return f'); > asmCompile('glob', 'ffis', 'b', SETUP + 'function f() { var i = 0; i32[i >> 2] } return f'); > asmCompile('glob', 'ffis', 'b', SETUP + 'function f() { var i = 0; i32[i >> 2] = 0 } return f'); > + asmCompile('glob', 'ffis', 'b', SETUP + 'function f() { var i = 0; i32[(imul(i,i)|0) >> 2] = 0 } return f'); > + asmCompile('glob', 'ffis', 'b', SETUP + 'function f() { var i = 0; i32[i >> 2] = (imul(i,i)|0) } return f'); Can you add a test case with a SIMD call: function f() { var i = 0; var v = SIMD_int32x4(1,2,3,4); i32[v.x >> 2] = 0 } And if any of the Atomic functions can return an integer, can you use it in a test as well, please?
Attachment #8568736 -
Flags: review?(benj) → review+
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #1) > Can you make the error message a static const char* variable, so that it can > be reused by the three calls, instead of copy/pasto? (or even a small helper > function) All three error messages are different (in the first few words) and I think it would complicate things to factor out the common part.
Assignee | ||
Comment 3•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/998842c5d5b5
Comment 4•9 years ago
|
||
Verified working on emscripten fuzz testcases that hit this.
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/998842c5d5b5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•