1088655 - OdinMonkey: add a pref that throws (with stack trace) on unaligned or null heap access

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Luke Wagner [:luke]]

This could save some serious developer time tracking down bugs that are rooted in these two error sources.  And it's way easier to flip a pref than rebuilding an app with SAFE_HEAP.

Comment 1

[Luke Wagner [:luke]]

Alon: do you know what the lowest address is that Emscripten starts allocating globals?  In a "hello, world" program I compiled a while ago, it was 8.  It'd be nice if Emscripten left more of an unused zone to catch field accesses which will occur at null+small-constant.

Comment 2

[Alon Zakai (:azakai)]

8 is the default, but that is customizable, and in different modes we work differently. For example in the emterpreter, we currently start at 0 (!). Perhaps the pref here could get the value from which to consider accesses valid?

Comment 3

[Luke Wagner [:luke]]

Yeah, we can certainly have it pref-defined where to start.  I was thinking it'd be nice if there was a generous default (like 4096).  That way, you wouldn't have to recompile if you see problems; you can just flip the pref and go.

Comment 4

[Luke Wagner [:luke]]

Attachment: tidy-lookup

Tiny cleanup

Comment 5

[Luke Wagner [:luke]]

Attachment: fix-lowering

While testing this patch I found a pre-existing regalloc bug in x64, but fortunately only when JS_NO_SIGNALS=1.

Comment 6

[Luke Wagner [:luke]]

Attachment: rm-option-check

I happened across this code in the next patch.  The extra conjunct is a no-op now that funbox->useAsm is false in the function when validation fails.

Comment 7

[Luke Wagner [:luke]]

Attachment: add-debug-modes

This patch does all the actual work: it adds an AsmJSOptions structs which contains the options throw-on-oob and throw-on-misalignment and conditions these things when set.  The next patch hooks this up to the browser.

Comment 8

[Luke Wagner [:luke]]

Attachment: add-debug-prefs

This patch adds the browser prefs:
 pref("javascript.options.asmjs.throw_on_misaligned", false);
 pref("javascript.options.asmjs.throw_on_oob", false);
 pref("javascript.options.asmjs.throw_on_oob.lower_bound", 0);
In particular, the lower_bound is inclusive so 0 effectively disables low-memory-access checking.

Comment 9

[Luke Wagner [:luke]]

Attachment: add-debug-modes

Comment 10

[Luke Wagner [:luke]]

I just took this for a test drive:
 - No problems with any of the humble bundle games I tested (but I didn't test them all)
 - Unity DT2 demo seems to fail with an access at 0, but otherwise succeeds if lower_bound = 0.
 - BananaBench fails with both access at 0 and misalignment.

Bugs ahoy!

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8513084 [details] [diff] [review]
add-debug-modes

Actually, this patch needs a bit more work: it doesn't do the necessary work to produce a stack trace containing asm.js frames and it also doesn't push shadow-stack space for win64.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8513073 [details] [diff] [review]
add-debug-prefs

r=me

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8513048 [details] [diff] [review]
fix-lowering

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

Good catch, and nice simplifications.

::: js/src/jit/MIR.h
@@ +11662,5 @@
>  
>  class MAsmJSHeapAccess
>  {
>      Scalar::Type viewType_;
> +    bool needsBoundsCheck_;

Thanks for this renaming. Not having double negations (!skipBoundsCheck()) is so much better.

@@ +11665,5 @@
>      Scalar::Type viewType_;
> +    bool needsBoundsCheck_;
> +
> +  public:
> +    explicit MAsmJSHeapAccess(Scalar::Type vt, bool needsBoundsCheck)

no need to keep it explicit anymore, but not mandatory to delete it

@@ +11692,5 @@
>    public:
>      INSTRUCTION_HEADER(AsmJSLoadHeap);
>  
> +    static MAsmJSLoadHeap *New(TempAllocator &alloc, Scalar::Type vt,
> +                               MDefinition *ptr, bool needsBoundsCheck) {

nit: { on next line

::: js/src/jit/x64/Lowering-x64.cpp
@@ +140,5 @@
> +    // getting maximum performance in these cases) only allow constant
> +    // opererands when skipping bounds checks.
> +    LAllocation ptrAlloc = ins->needsBoundsCheck()
> +                           ? useRegisterAtStart(ptr)
> +                           : useRegisterOrNonNegativeConstantAtStart(ptr);

Nice, much simpler!

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8513070 [details] [diff] [review]
rm-option-check

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

::: js/src/frontend/FoldConstants.cpp
@@ +289,5 @@
> +        // Note: pn_body is nullptr for functions which are being lazily parsed.
> +        MOZ_ASSERT(pn->getKind() == PNK_FUNCTION);
> +        if (pn->pn_body) {
> +            if (!Fold(cx, &pn->pn_body, handler, options, pn->pn_funbox->inGenexpLambda,
> +                      SyntacticContext::Other))

nit: 'if' condition on several lines => {} around return false;

@@ -883,5 @@
>  {
>      // Don't fold constants if the code has requested "use asm" as
>      // constant-folding will misrepresent the source text for the purpose
>      // of type checking. (Also guard against entering a function containing
>      // "use asm", see PN_FUNC case below.)

This comment could benefit some updating: the mentioned case is PN_CODE and is now above.

Comment 15

[Luke Wagner [:luke]]

Just the preceding clean-ups:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39ed1cb415ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/9da6797b8d46
https://hg.mozilla.org/integration/mozilla-inbound/rev/38772a560533

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38772a560533
https://hg.mozilla.org/mozilla-central/rev/9da6797b8d46
https://hg.mozilla.org/mozilla-central/rev/39ed1cb415ab

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Luke, the last patch in this bug is r+'d. Needinfo'ing you in case it dropped off your radar; maybe there was a reason not to land it?

Comment 19

[Jukka Jylänki]

Ooh, thanks for remembering this, this would be very useful to have!

Comment 20

[Luke Wagner [:luke]]

IIRC, the patches are incomplete (specifically in the heap codegen) and required a bit more work before they could land.  Perhaps we could throw in a pref that throws on validation failure as well as suggested in the other bug?

Comment 21

[Jukka Jylänki]

A Mozilla partner reported hitting odd problems today in their codebase, and I realize if we had the prefs from comment 8, that would significantly ease debugging the issue. I see this landed partially, but the final part is still missing?

Comment 22

[Luke Wagner [:luke]]

Only refactorings landed; the meat isn't landed and would require a bit of work to finish.  We should get to this when we have time; I just don't have any at this moment.

Comment 23

[Jukka Jylänki]

Oh, no worries, it's not a blocking trouble, since we can communicate users to rebuild with the Emscripten -s SAFE_HEAP=1, so this is not the most urgent feature.

Comment 24

[Jukka Jylänki]

Hmm, this would still be useful, we regularly run into people who have alignment trouble in their titles, this would make it simpler for them to debug. Luke, how does this look like in WebAssembly? Did I remember correctly that wasm will have support for unaligned loads and stores? (trapping alignment faults and emulating if needed?)

Comment 25

[Luke Wagner [:luke]]

wasm will improve the situation on both axes: unaligned access is well-defined to work and oob access is well-defined to throw.

Comment 26

[Benjamin Bouvier [:bbouvier] (inactive)]

I think the case for null heap access with stack trace need a bit of work for showing the stack trace. Unaligned accesses should work, per spec. Is there something else to do in this bug?

Comment 27

[Jukka Jylänki]

It would be cool to have a pref even in wasm that would make unaligned memory loads/stores throw a JS exception with a callstack, because that will allow us to identify where Emscripten compiler emitted an unaligned load/store that might affect ARM performance. That would be more for optimization reasons rather than correctness reasons.

Comment 28

[Luke Wagner [:luke]]

FWIW, one thing I've been thinking we should add to our devtools, once we have the basic wasm-debugger integration working, is a checkbox for "first-chance trap handling", i.e., breaking at the trap-site before the exception is thrown.  A "break on unaligned" checkbox could pair well with that.

Comment 29

[Jukka Jylänki]

+1 on that(!!)

Comment 30

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:luke, maybe it's time to close this bug?

Comment 32

[Luke Wagner [:luke]]

Right.  With wasm we have trapping out-of-bounds, so I'll propose this is WONTFIX.