Closed Bug 1148232 Opened 5 years ago Closed 5 years ago

OdinMonkey: OOB and related cleanup patches

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(6 files)

In preparation for bug 1145676, I made several relatively minor patches.
This just updates an asm.js error message.
Attachment #8584197 - Flags: review?(luke)
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.
Attachment #8584199 - Flags: review?(luke)
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.
Attachment #8584201 - Flags: review?(luke)
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.
Attachment #8584208 - Flags: review?(luke)
This patch just deletes some unused code.
Attachment #8584209 - Flags: review?(luke)
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.
Attachment #8584211 - Flags: review?(luke)
Attachment #8584208 - Attachment description: asmjs → asmjs-put-mask-next-to-access.patch
Attachment #8584197 - Flags: review?(luke) → review+
Attachment #8584199 - Flags: review?(luke) → review+
Attachment #8584201 - Flags: review?(luke) → review+
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.
Attachment #8584208 - Flags: review?(luke) → review+
Attachment #8584209 - Flags: review?(luke) → review+
Attachment #8584211 - Flags: review?(luke) → review+
You need to log in before you can comment on or make changes to this bug.