Closed Bug 1136331 Opened 9 years ago Closed 9 years ago

OdinMonkey: fix heap-resize no-builtin-call checking

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(1 file)

Attached patch fix-change-heapSplinter 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 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+
(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.
Verified working on emscripten fuzz testcases that hit this.
https://hg.mozilla.org/mozilla-central/rev/998842c5d5b5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: