Closed Bug 1122856 Opened 9 years ago Closed 9 years ago

Odin: split needsBoundsChecks into a provenWithinBounds and canUseSigsegv flags

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: dougc, Assigned: sunfish)

References

Details

Attachments

(1 file, 1 obsolete file)

See bug 1073096 comment 33 and comment 34. It seems unfortunate that the needsBoundsCheck flag conflates two matters. The compiler has a number of phases and may be able to prove that an index in within bounds at multiple stages and want to flag this without destroying a backend specific flag such as a canUseSigsegv flag.

It appears to be the x64 backend which is affected. It has the option to use memory protection to catch OOB accesses or to use inline bounds checks. In bug 1073096 it was necessary to flag a requirement to use the inline bounds checks (to avoid a sigsegv) - it looks ok for the compiler to clear this flag when the access is proven within bounds, but perhaps not optimal.

If the access is proven to be within bounds then it would be useful to avoid noting it irrespective of canUseSigsegv. In other words there might be more than two options that need flagging, for example:

 canUseSigsegv   | provenWithinBounds | 
---------------------------------------------
   *             | yes                | no bounds check, no notes.
   yes           | no                 | inline check, don't note code location, note cmp offset
   no            | no                 | no bounds check, note code location for sigsegv
---------------------------------------------

Separating this into two flags might be simpler and easier to understand and maintain.
Blocks: 986981
Attached patch oob-label.patch (obsolete) — Splinter Review
Part of the counter-intuitive part for me is that information about whether signal handlers are available is being stored in each MAsmJSHeapAccess. This is really a property of the entire compilation, rather than of an access. The attached patch moves this information to the MIRGenerator, which seems to be a more natural place. This also deconflates it with the needsBoundsCheck() flag, so that it works the same way as flags of the same name in other MIR classes.
Assignee: nobody → sunfish
Attachment #8566827 - Flags: review?(luke)
Comment on attachment 8566827 [details] [diff] [review]
oob-label.patch

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

Nice, I really like it better than my original approach.  Storing attributes in the MIRGenerator for global compilation state makes so much sense... (sorry for the drive-by review, I couldn't help but looking at this patch)

::: js/src/jit/MIRGraph.cpp
@@ +40,5 @@
>      modifiesFrameArguments_(false),
>      instrumentedProfiling_(false),
>      instrumentedProfilingIsCached_(false),
>      nurseryObjects_(*alloc),
> +    options(options),

nit: please fix initialization order to avoid a warning (options should go to the end)
Attached patch oob-label.patchSplinter Review
(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Comment on attachment 8566827 [details] [diff] [review]
> oob-label.patch
> 
> Review of attachment 8566827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, I really like it better than my original approach.  Storing attributes
> in the MIRGenerator for global compilation state makes so much sense...
> (sorry for the drive-by review, I couldn't help but looking at this patch)

Drive-by reviews are welcome!

> ::: js/src/jit/MIRGraph.cpp
> @@ +40,5 @@
> >      modifiesFrameArguments_(false),
> >      instrumentedProfiling_(false),
> >      instrumentedProfilingIsCached_(false),
> >      nurseryObjects_(*alloc),
> > +    options(options),
> 
> nit: please fix initialization order to avoid a warning (options should go
> to the end)

Fixed.

This also fixes the non-SIMD case to check usesSignalHandlersForOOB too.
Attachment #8566827 - Attachment is obsolete: true
Attachment #8566827 - Flags: review?(luke)
Attachment #8567072 - Flags: review?(luke)
Comment on attachment 8567072 [details] [diff] [review]
oob-label.patch

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +2872,5 @@
>      {
>          if (inDeadCode())
>              return nullptr;
>  
> +        bool needsBoundsCheck = chk == NEEDS_BOUNDS_CHECK;

I feel like the rhs is readable enough that you could remove the variable and inline the rhs into the New calls (here and below).  We could also rename 'chk' to 'needsBoundsCheck' and use it as a bool directly (perhaps adding '= false' and '= true' to the enumerators to document the dependency).
Attachment #8567072 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/12ea42444af9
Status: NEW → 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.