OdinMonkey: OOB and related cleanup patches

RESOLVED FIXED in Firefox 39

Status

()

enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

4 years ago
In preparation for bug 1145676, I made several relatively minor patches.
Assignee

Comment 1

4 years ago
This just updates an asm.js error message.
Attachment #8584197 - Flags: review?(luke)
Assignee

Comment 2

4 years ago
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)
Assignee

Comment 3

4 years ago
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)
Assignee

Comment 4

4 years ago
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)
Assignee

Comment 5

4 years ago
This patch just deletes some unused code.
Attachment #8584209 - Flags: review?(luke)
Assignee

Comment 6

4 years ago
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)
Assignee

Updated

4 years ago
Attachment #8584208 - Attachment description: asmjs → asmjs-put-mask-next-to-access.patch

Updated

4 years ago
Attachment #8584197 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #8584199 - Flags: review?(luke) → review+

Updated

4 years ago
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+

Updated

4 years ago
Attachment #8584209 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #8584211 - Flags: review?(luke) → review+
You need to log in before you can comment on or make changes to this bug.