1148232 - OdinMonkey: OOB and related cleanup patches

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

In preparation for bug 1145676, I made several relatively minor patches.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: asmjs-error-message.patch

This just updates an asm.js error message.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: asmjs-legacy-cleanups.patch

Several tests were still using the legacy syntax for HEAP8 accesses without the shift. This patch adds comments mentioning this, and adds equivalent tests using the new syntax.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: asmjs-checkarrayaccess-cleanup.patch

This patch harmonizes the legacy path in CheckArrayAccess and the main path, and updates the error messages to point to the more relevant expression node.

Comment 4

[Dan Gohman [:sunfish]]

Attachment: asmjs-put-mask-next-to-access.patch

This patch splits out the index mask insertion code from CheckArrayAccess so that it can always be called immediately prior to the actual array access, instead of sometimes prior to the expansion of other operands.

The PrepareArrayIndex function this patch introduces will also be useful for inserting bounds checks in later patches.

Comment 5

[Dan Gohman [:sunfish]]

Attachment: delete-unused-stuff.patch

This patch just deletes some unused code.

Comment 6

[Dan Gohman [:sunfish]]

Attachment: asmjs-cold-disassembler.patch

This patch marks more of the disassembly code as cold, and uses more MOZ_RELEASE_ASSERTS so that more checks are included in release builds.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8584208 [details] [diff] [review]
asmjs-put-mask-next-to-access.patch

Review of attachment 8584208 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/AsmJSValidate.cpp
@@ +4493,5 @@
>                                        "change-heap function (0x%x - 0x%x)",
>                                        f.m().minHeapLength(), f.m().module().maxHeapLength());
>          }
>  
> +        *mask = -1; // no mask

How about define a 'static const int32_t NoMask = -1;' above?

@@ +4536,5 @@
>          // For legacy compatibility, accept Int8/Uint8 accesses with no shift.
>          if (TypedArrayShift(*viewType) != 0)
>              return f.fail(indexExpr, "index expression isn't shifted; must be an Int8/Uint8 access");
>  
> +        MOZ_ASSERT(*mask == -1);

Could be NoMask w/ abovementioned change.

@@ +4567,5 @@
> +}
> +
> +static void
> +PrepareArrayIndex(FunctionCompiler &f, MDefinition **def,
> +                  NeedsBoundsCheck needsBoundsCheck, int32_t mask = -1)

I think needsBoundsCheck can be on the previous line.  Also, I'd be a bit happier without a default arg (so the choice to leave off the mask is explicit); with 'NoMask' this will be more readable.

@@ +4573,3 @@
>      // Don't generate the mask op if there is no need for it which could happen for
> +    // a shift of zero or a SIMD access.
> +    if (mask != -1)

NoMask

@@ +4901,5 @@
>  static bool
>  CheckSharedArrayAtomicAccess(FunctionCompiler &f, ParseNode *viewName, ParseNode *indexExpr,
>                               Scalar::Type *viewType, MDefinition** pointerDef,
> +                             NeedsBoundsCheck *needsBoundsCheck,
> +                             int32_t *mask)

I think 'mask' can be on the previous line.

Comment 8

[Dan Gohman [:sunfish]]

Suggested fixes made. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/768fcbee6319
https://hg.mozilla.org/integration/mozilla-inbound/rev/f41fff953bf1
https://hg.mozilla.org/integration/mozilla-inbound/rev/da24bccfcaf0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2abf02e8df8
https://hg.mozilla.org/integration/mozilla-inbound/rev/982ae8f096bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/46872ab97179

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/768fcbee6319
https://hg.mozilla.org/mozilla-central/rev/f41fff953bf1
https://hg.mozilla.org/mozilla-central/rev/da24bccfcaf0
https://hg.mozilla.org/mozilla-central/rev/d2abf02e8df8
https://hg.mozilla.org/mozilla-central/rev/982ae8f096bd
https://hg.mozilla.org/mozilla-central/rev/46872ab97179