Closed Bug 1088655 Opened 7 years ago Closed 3 years ago

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


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






(Reporter: luke, Assigned: luke)



(Whiteboard: [gaming-tools][games:p1][platform-rel-Games])


(4 files, 2 obsolete files)

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.
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.
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?
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.
Attached patch tidy-lookupSplinter Review
Tiny cleanup
Attachment #8513046 - Flags: review?(benj)
Attached patch fix-loweringSplinter Review
While testing this patch I found a pre-existing regalloc bug in x64, but fortunately only when JS_NO_SIGNALS=1.
Attachment #8513048 - Flags: review?(benj)
Attached patch rm-option-checkSplinter Review
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.
Attachment #8513070 - Flags: review?(benj)
Attached patch add-debug-modes (obsolete) — Splinter Review
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.
Attachment #8513072 - Flags: review?(benj)
Attached patch add-debug-prefsSplinter Review
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.
Attachment #8513073 - Flags: review?(bzbarsky)
Attached patch add-debug-modes (obsolete) — Splinter Review
Attachment #8513072 - Attachment is obsolete: true
Attachment #8513072 - Flags: review?(benj)
Attachment #8513084 - Flags: review?(benj)
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 on attachment 8513084 [details] [diff] [review]

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.
Attachment #8513084 - Attachment is obsolete: true
Attachment #8513084 - Flags: review?(benj)
Comment on attachment 8513073 [details] [diff] [review]

Attachment #8513073 - Flags: review?(bzbarsky) → review+
Attachment #8513046 - Flags: review?(benj) → review+
Comment on attachment 8513048 [details] [diff] [review]

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:
> +    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!
Attachment #8513048 - Flags: review?(benj) → review+
Comment on attachment 8513070 [details] [diff] [review]

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.
Attachment #8513070 - Flags: review?(benj) → review+
Duplicate of this bug: 879891
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?
Flags: needinfo?(luke)
Ooh, thanks for remembering this, this would be very useful to have!
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?
Flags: needinfo?(luke)
Whiteboard: [gaming-tools]
Priority: -- → P2
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?
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.
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.
Whiteboard: [gaming-tools] → [gaming-tools][games:p1]
Whiteboard: [gaming-tools][games:p1] → [gaming-tools][games:p1][platform-rel-Games]
platform-rel: --- → ?
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?)
wasm will improve the situation on both axes: unaligned access is well-defined to work and oob access is well-defined to throw.
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?
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.
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.
+1 on that(!!)
Priority: P2 → P3
platform-rel: ? → ---
Per policy at If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Closed: 4 years ago
Resolution: --- → INACTIVE
Resolution: INACTIVE → ---
The leave-open keyword is there and there is no activity for 6 months.
:luke, maybe it's time to close this bug?
Flags: needinfo?(luke)
Right.  With wasm we have trapping out-of-bounds, so I'll propose this is WONTFIX.
Closed: 4 years ago3 years ago
Flags: needinfo?(luke)
Resolution: --- → WONTFIX
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.