Baldr: switch traps to use actual hardware traps

RESOLVED FIXED in Firefox 61

Status

()

P1
normal
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(17 attachments, 3 obsolete attachments)

25.23 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
106.52 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
6.56 KB, patch
yury
: review+
Details | Diff | Splinter Review
4.01 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
93.67 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
23.30 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
22.99 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
3.84 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
28.20 KB, patch
luke
: review+
Details | Diff | Splinter Review
27.17 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
3.37 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
2.76 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
3.23 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
17.39 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
12.53 KB, patch
luke
: review+
Details | Diff | Splinter Review
133.00 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
60.23 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
Right now, traps jump (or in the special case of the out-of-bounds signal handler, are redirected to) out-of-line paths, one per trapping instruction, appended at the end of each function.  These out-of-line paths then jump to a shared stub (through a far-jump island) at the end of the module (one per trap).

This set of patches will convert wasm traps to actually use hardware traps (like ud2), creating one metadata record per trap site that can be looked up by the signal handler.  This should reduce code and metadata size and also simplify the interface between cretonne and SpiderMonkey.
(Assignee)

Comment 1

a year ago
Trivial patch that does code motion and removes a dead argument.
Attachment #8940335 - Flags: review?(bbouvier)
(Assignee)

Comment 2

a year ago
The patches in this bug will incrementally convert old-style trap handling to new-style, trap-by-trap.  Thus, there will be "old" trapping instructions and "new" trapping instructions, and they both end up wanting the same names (e.g., "TrapSite").  To make it quite clear which is old/new, this patch renames everything that will be deleted, at the end of the patchset, to be prefixed with "Old".
Attachment #8940336 - Flags: review?(bbouvier)
(Assignee)

Comment 3

a year ago
To help a later patch, this patch removes WasmFrameIter::debugTrapCallsite().
Attachment #8940337 - Flags: review?(ydelendik)
(Assignee)

Comment 4

a year ago
This patch removes WasmFrameIter::callsite_ by instead recording the one bit of info (lineOrBytecode_) that was actually pulled from the callsite.
Attachment #8940339 - Flags: review?(bbouvier)
(Assignee)

Comment 5

a year ago
Posted patch update-unreachable (obsolete) — Splinter Review
This patch forges the new path by converting Trap::Unreachable.
Attachment #8940340 - Flags: review?(bbouvier)
Comment on attachment 8940337 [details] [diff] [review]
rm-debug-trap-callsite

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

Looks good. I'm just wondering about tls->instance and frame->instance() relationship.

::: js/src/wasm/WasmBuiltins.cpp
@@ +108,5 @@
>      MOZ_ASSERT(site);
> +
> +    // Advance to the actual trapping frame.
> +    fp = fp->callerFP;
> +    DebugFrame* debugFrame = DebugFrame::from(fp);

Will it be useful to have an assert for `debugFrame.instance() == instance` here (and/or at DebugFrame::from)?
Attachment #8940337 - Flags: review?(ydelendik) → review+
Comment on attachment 8940335 [details] [diff] [review]
code-motion-an-dce

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

Thanks for doing a separate patch.

::: js/src/wasm/WasmCode.cpp
@@ +958,5 @@
> +    metadata_ = &metadata;
> +
> +    return cursor;
> +}
> +

nit: final empty line
Attachment #8940335 - Flags: review?(bbouvier) → review+
Comment on attachment 8940336 [details] [diff] [review]
rename-old-traps

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

::: js/src/jit/shared/Assembler-shared.h
@@ +853,3 @@
>  // exit at the end of the module when trap exits are emitted.
> +//
> +// TODO: these are in the process of being removed in lieu of wasm::TrapSite.

nit: maybe this metadata also will go under wasm::TrapSite, otherwise it might be a bad copy pasta?

::: js/src/wasm/WasmStubs.cpp
@@ +1000,5 @@
>      GenerateExitEpilogue(masm, framePushed, exitReason, offsets);
>      return FinishOffsets(masm, offsets);
>  }
>  
> +// Generate a stub that calls into OldReportTrap with the right trap reason.

preexisting nit: WasmOldReportTrap
Attachment #8940336 - Flags: review?(bbouvier) → review+
Comment on attachment 8940339 [details] [diff] [review]
rm-frame-iter-callsite

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

Nice.
Attachment #8940339 - Flags: review?(bbouvier) → review+
Comment on attachment 8940340 [details] [diff] [review]
update-unreachable

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

Thanks, looks good; below are suggestions that seem valuable enough to have a second look later.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +5401,5 @@
> +
> +void
> +MacroAssembler::wasmTrap(wasm::Trap trap, wasm::BytecodeOffset bytecodeOffset)
> +{
> +    append(trap, wasm::TrapSite(as_illegal_trap().getOffset(), bytecodeOffset));

Could there be one MacroAssembler function that does this line, with as_illegal_trap() being another masm function that's defined by platform? Then the common part is abstracted over the platform and only the illegal instruction generation would be platform dependent.

::: js/src/vm/Stack.cpp
@@ +1715,5 @@
>  
>      void* pc = unwindState.pc;
>      MOZ_ASSERT(wasm::LookupCode(pc)->lookupRange(pc)->isFunction());
>  
> +    rt->wasmUnwindData.ref().construct<wasm::InterruptData>(pc, state.pc);

aww this is not pretty. Having the wasmUnwindData public in the runtime unhides a lot of implementation details about this data (in particular, the atommic stuff perspires in any access to wasmUnwindData through the ref() call, and then the MaybeOneOf can be observed through construct<> et al.) Keeping the previous operations seems enough to have cleaner, more separate interfaces here. Can we do this instead?

::: js/src/wasm/WasmFrameIter.cpp
@@ +114,5 @@
>  
>      if (unwind_ == Unwind::True) {
>          if (activation_->isWasmInterrupted())
>              activation_->finishWasmInterrupt();
> +        if (activation_->isWasmTrapping())

nit: else if

::: js/src/wasm/WasmSignalHandlers.h
@@ +97,5 @@
> +      : unwindPC(unwindPC), resumePC(resumePC)
> +    {}
> +};
> +
> +struct TrapData

I wonder if instead of having InterruptData and TrapData, we could just use a single tagged union. The unwindPC is common to both, and the MaybeOneOf just seems to obfuscate the code of its users (in particular, both structs are POD, so the MaybeOneOf::construct<> usage seems a bit overkill here). What do you think?

::: js/src/wasm/WasmTypes.h
@@ +974,5 @@
> +struct TrapSiteVectorArray : EnumeratedArray<Trap, Trap::Limit, TrapSiteVector>
> +{
> +    bool empty() const;
> +    void clear();
> +    void swap(TrapSiteVectorArray& rhs);

nit: use a pointer argument to make it apparent on call sites that rhs is being mutated?
Attachment #8940340 - Flags: review?(bbouvier)
(Assignee)

Comment 11

a year ago
(In reply to Yury Delendik (:yury) from comment #6)
> > +    DebugFrame* debugFrame = DebugFrame::from(fp);
> 
> Will it be useful to have an assert for `debugFrame.instance() == instance`
> here (and/or at DebugFrame::from)?

Sure, I'll add that to DebugFrame::from().
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 12

a year ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02fc87285373
Baldr: move code and remove dead argument/field (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5e24467641e
Baldr: prefix current trap mechanism names with 'Old' (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf36ca9dd013
Baldr: remove WasmFrameIter::debugTrapCallsite() (r=yury)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e9f7e84f6b
Baldr: remove WasmFrameIter::callsite_ (r=bbouvier)
(Assignee)

Comment 13

a year ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #10)
> > +    rt->wasmUnwindData.ref().construct<wasm::InterruptData>(pc, state.pc);
> 
> aww this is not pretty. Having the wasmUnwindData public in the runtime
> unhides a lot of implementation details about this data (in particular, the
> atommic stuff perspires in any access to wasmUnwindData through the ref()
> call, and then the MaybeOneOf can be observed through construct<> et al.)
> Keeping the previous operations seems enough to have cleaner, more separate
> interfaces here. Can we do this instead?

It's more verbose at the (encapsulated) use sites in JitActivation, sure, but the actual type MaybeOneOf<InterruptData,TrapData> provides pretty much the exact same logical interface as the ad hoc thing before, so I don't think it gives up any real encapsulation: clients of JSRuntime can do basically the same thing as before.  Rather, I think when you look at the wasmUnwindData field in Runtime.h, it's more clear to read because you can see how the field works at a glance from the type instead of having to read the comment + method bodies.  I'm not sure what you were getting at with 'the atomic stuff perspires in any access ...' sentence, though.

> ::: js/src/wasm/WasmSignalHandlers.h
> @@ +97,5 @@
> > +      : unwindPC(unwindPC), resumePC(resumePC)
> > +    {}
> > +};
> > +
> > +struct TrapData
> 
> I wonder if instead of having InterruptData and TrapData, we could just use
> a single tagged union. The unwindPC is common to both, and the MaybeOneOf
> just seems to obfuscate the code of its users (in particular, both structs
> are POD, so the MaybeOneOf::construct<> usage seems a bit overkill here).
> What do you think?

I started that way (having unwindPC in a common field), but I think it's more clear to say that the Runtime is in one of 3 states: trapping, interrupting, or none and the non-None states each have a Data struct.  That both TrapData/InterruptData have the same field unwindPC is just incidental; in fact one patch I started, and now have postponed, changed InterruptData to have something else instead of wasmUnwindPC.

> > +    void swap(TrapSiteVectorArray& rhs);
> 
> nit: use a pointer argument to make it apparent on call sites that rhs is
> being mutated?

Agreed on general idiom, but in the specific case of swap(), there seems to be a convention (stemming from Vector and other shared types) of swap() taking a & and so if you look at CompiledCode::swap(), everything takes a & so I didn't want to make this one stick out.
(In reply to Luke Wagner [:luke] from comment #13)
> It's more verbose at the (encapsulated) use sites in JitActivation, sure,
> but the actual type MaybeOneOf<InterruptData,TrapData> provides pretty much
> the exact same logical interface as the ad hoc thing before, so I don't
> think it gives up any real encapsulation: clients of JSRuntime can do
> basically the same thing as before.  Rather, I think when you look at the
> wasmUnwindData field in Runtime.h, it's more clear to read because you can
> see how the field works at a glance from the type instead of having to read
> the comment + method bodies.

But it's much unclear when reading the users: ref() and sometimes also another templated ref to get the right type (that's what I meant by "perspire"; my bad for the poor choice of words). See for example from your patch:

rt->wasmUnwindData.ref().ref<wasm::InterruptData>().unwindPC;

Having an helper that does the ref().ref<T>() would be nice to hide verbosity on the callees; another one to construct (that would also MOZ_ASSERT the MaybeOneOf was empty in the first place) would be nice to have, but not mandatory. Does it sound reasonable?

(the above line could read as simply as rt->wasmInterruptData().unwindPC, or rt->wasmUnwindData<wasm::InterruptData>().unwindPC, which both look much better IMO)

> I started that way (having unwindPC in a common field), but I think it's
> more clear to say that the Runtime is in one of 3 states: trapping,
> interrupting, or none and the non-None states each have a Data struct.  That
> both TrapData/InterruptData have the same field unwindPC is just incidental;
> in fact one patch I started, and now have postponed, changed InterruptData
> to have something else instead of wasmUnwindPC.

It just seems like a lot of verbosity (having 2 new classes + 1 MaybeOneOf vs a single tagged union with 3 tags) for very few gain, but I guess that's just C++ being regularly overly verbose. I don't care too much here.
 
> Agreed on general idiom [...].

Fair enough.
(Assignee)

Comment 16

a year ago
Ok, two little helpers to make the repeated lines more concise do sound nice.  Several of the uses (constructing, testing) are only used once so having a helper for that one use seemed to unnecessarily bloat an already-bloating class (JSRuntime).
Attachment #8941215 - Flags: review?(bbouvier)
(Assignee)

Updated

a year ago
Attachment #8940340 - Attachment is obsolete: true
Comment on attachment 8941215 [details] [diff] [review]
update-unreachable

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

Thanks, looks nice!

::: js/src/jit/arm/Simulator-arm.cpp
@@ +260,5 @@
>          return typeValue() == 7 && bit(24) == 1 && svcValue() >= kStopCode;
>      }
>  
> +    // Test for a udf instruction, which falls under type 3.
> +    inline bool isUDF() const {

(preexisting) nit: I see you're following the code style here, but the inline keyword isn't needed since the method is defined in the class definition.

::: js/src/wasm/WasmBuiltins.cpp
@@ +291,5 @@
>  
>  static void
> +WasmReportTrap()
> +{
> +    Trap trap = TlsContext.get()->runtime()->wasmUnwindData.ref().ref<TrapData>().trap;

nit: we can use the helper here

::: js/src/wasm/WasmCode.cpp
@@ +838,5 @@
> +        return trapSites[index].pcOffset;
> +    }
> +};
> +
> +

nit: two blank lines

@@ +842,5 @@
> +
> +bool
> +Code::lookupTrap(void* pc, Trap* trapOut, BytecodeOffset* bytecode) const
> +{
> +    for (auto t : tiers()) {

nit: for (Tiers& t : tiers()) {

maybe even `const Tiers&`?

Should we remove Tiers' copy ctor to avoid misusage in range-based for loops?
Attachment #8941215 - Flags: review?(bbouvier) → review+
Status: ASSIGNED → NEW
Priority: -- → P1
(Assignee)

Comment 18

a year ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> > +    for (auto t : tiers()) {
> 
> nit: for (Tiers& t : tiers()) {
> 
> maybe even `const Tiers&`?
> 
> Should we remove Tiers' copy ctor to avoid misusage in range-based for loops?

This is iterating over (word-sized) Tier values, not Tiers; Tiers is the container being iterated over.  If we were iterating over Tiers, you're right, const auto& would be appropriate but since Tier values are words, by-value is appropriate.  I'll reeinterpret this nit as a request to use 'Tier t : tiers()' instead of 'auto' :)

Comment 19

a year ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f56ef640d6c0
Baldr: use hardware traps for Trap::Unreachable (r=bbouvier)
(Assignee)

Comment 20

a year ago
With that framework in place, pretty easy to do the integer traps.
Attachment #8941618 - Flags: review?(bbouvier)
(Assignee)

Comment 21

a year ago
Attachment #8941619 - Flags: review?(bbouvier)
(Assignee)

Comment 22

a year ago
Attachment #8941620 - Flags: review?(bbouvier)
Comment on attachment 8941619 [details] [diff] [review]
update-int-div-zero

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

::: js/src/jit/mips64/CodeGenerator-mips64.cpp
@@ +361,5 @@
>      Label done;
>  
>      // Handle divide by zero.
> +    if (lir->canBeDivideByZero()) {
> +        Lable nonZero;

nit: Label
Attachment #8941619 - Flags: review?(bbouvier) → review+
Comment on attachment 8941618 [details] [diff] [review]
update-int-overflow

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

Nice

::: js/src/jit/arm/Assembler-arm.h
@@ +1173,5 @@
>          GreaterThanOrEqual = GE,
>          LessThan = LT,
>          LessThanOrEqual = LE,
>          Overflow = VS,
> +        NoOverflow = VC,

nit: unused

::: js/src/jit/none/MacroAssembler-none.h
@@ +108,5 @@
>          GreaterThanOrEqual,
>          LessThan,
>          LessThanOrEqual,
>          Overflow,
> +        NoOverflow,

nit: unused

::: js/src/wasm/WasmFrameIter.cpp
@@ +1111,5 @@
>  
>      size_t offsetInModule = ((uint8_t*)pc) - codeSegment.base();
>      if (offsetInModule < codeRange->funcNormalEntry() + SetFP)
>          return nullptr;
> +    if (offsetInModule >= codeRange->ret() - PoppedFP && offsetInModule <= codeRange->ret())

Was this trying to use FP from an OOL path and thus failing? If so, this fix makes sense.
Attachment #8941618 - Flags: review?(bbouvier) → review+
Attachment #8941620 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 26

a year ago
Ah hah, it would appear jit-tests simply weren't running on automation on OSX, which is why my try runs which only did "-u jittests" were green.  (The authorities have been notified.)  So OSX undefined-instruction signal handling had never actually been tested.  Good thing we had the wpt tests running the .wast files.

Trivial fix, though, just needed to add "| EXC_MASK_BAD_INSTRUCTION" to the thread_set_exception_ports() call.
Flags: needinfo?(luke)

Comment 27

a year ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/947a058e42b9
Baldr: use hardware traps for Trap::Unreachable (r=bbouvier)
(Assignee)

Comment 28

a year ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #25)
> > +    if (offsetInModule >= codeRange->ret() - PoppedFP && offsetInModule <= codeRange->ret())
> 
> Was this trying to use FP from an OOL path and thus failing? If so, this fix
> makes sense.

Yeah: an out-of-line (as in Ion OutOfLineCode) path was causing a wasm trap and so the PC is > codeRange->ret() but we still want LookupFaultingInstance() to find it (b/c fp is totally valid etc).
Posted patch wasm_traps_mips.patch (obsolete) — Splinter Review
Hi,

This patch implements MacroAssembler::illegalInstruction() for MIPS platforms. I chose to use integer overflow exception instead of illegal instruction one. I've also added a set of conditional trap instructions that would later allow us to conditional branch / raise trap sequences with single instruction.
Attachment #8943887 - Flags: feedback?(luke)
(Assignee)

Comment 31

a year ago
Comment on attachment 8943887 [details] [diff] [review]
wasm_traps_mips.patch

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

Thanks!
Attachment #8943887 - Flags: feedback?(luke) → feedback+
Ok! then. 
This patch adds simulator support atop the previous one and renames MacroAssembler::illegalInstruction to MacroAssembler::wasmTrapInstruction.
NOTE that any missing wasm support for simulators besides this feature will be added in Bug 1420838.
Attachment #8943887 - Attachment is obsolete: true
Attachment #8944366 - Flags: review?(luke)
(Assignee)

Updated

a year ago
Attachment #8944366 - Flags: review?(luke) → review+
Can somebody land bug1428453_mips.patch ?

Thanks in advance.
(In reply to Dragan Mladjenovic from comment #33)
> Can somebody land bug1428453_mips.patch ?

I'll take care of it.

Comment 35

a year ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db980507149b
[MIPS] Implement hardware wasm traps support; patch=dragan.mladjenovic, r=luke, push=lth
(Assignee)

Updated

a year ago
Depends on: 1435360
(Assignee)

Comment 37

a year ago
This patch converts the stack overflow trap.
Attachment #8949908 - Flags: review?(bbouvier)
(Assignee)

Updated

a year ago
Duplicate of this bug: 1436811
Comment on attachment 8949908 [details] [diff] [review]
update-stack-overflow

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

Looks good, proposed one simplification below, thanks.

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +1487,5 @@
>      // and it uses only a small amount of stack space, it doesn't need a
>      // stack overflow check. Note that the actual number here is somewhat
>      // arbitrary, and codegen actually uses small bounded amounts of
>      // additional stack space in some cases too.
> +    return frameSize() < MAX_UNCHECKED_LEAF_FRAME_SIZE && !gen->needsOverrecursedCheck();

The check against MAX_UNCHECKED_LEAF_FRAME_SIZE is already done in GenerateFunctionPrologue, so you could even remove this function and use IsLeaf isLeaf = !gen->needsOverrecursedCheck(); in the wasm caller. Maybe the only other jit caller could embed the constant, instead of defining it in IonTypes.h.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +1178,5 @@
> +
> +    void allocStack(Register tmp, BytecodeOffset trapOffset) {
> +        stackAddOffset_ = masm.sub32FromStackPtrWithPatch(tmp);
> +        Label ok;
> +        masm.branchPtr(Assembler::Below,

Can you MOZ_ASSERT(!IsHiddenSP(tmp))? Otherwise, the behavior might be different from the previous one (that required using branchStackPtrRhs).

::: js/src/wasm/WasmFrameIter.cpp
@@ +553,5 @@
>      masm.haltingAlign(CodeAlignment);
> +
> +    GenerateCallablePrologue(masm, &offsets->begin);
> +
> +    // If this frame will be exiting compiled code to C++, record the fp and

nit: we can remove the "If" at the beginning of this sentence
Attachment #8949908 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #39)
> Comment on attachment 8949908 [details] [diff] [review]
> update-stack-overflow
> 
> Review of attachment 8949908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, proposed one simplification below, thanks.
> 
> ::: js/src/wasm/WasmBaselineCompile.cpp
> @@ +1178,5 @@
> > +
> > +    void allocStack(Register tmp, BytecodeOffset trapOffset) {
> > +        stackAddOffset_ = masm.sub32FromStackPtrWithPatch(tmp);
> > +        Label ok;
> > +        masm.branchPtr(Assembler::Below,
> 
> Can you MOZ_ASSERT(!IsHiddenSP(tmp))? Otherwise, the behavior might be
> different from the previous one (that required using branchStackPtrRhs).

If so it would have to be !IsHiddenSP(RegisterOrSP(tmp)) but that is false on every platform except ARM64 and it could only be true on ARM64 if the type of tmp was already RegisterOrSP, since Register can't represent the SP.  I think such an assertion would be redundant.

Comment 41

a year ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6eac58c8e76
Baldr: use new traps for stack overflow (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/18e4d706ecdf
Baldr: use new traps for integer overflow (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b54df6ca0ab8
Baldr: use new traps for idiv (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca928e3c740
Baldr: use new traps for float-to-int conversions (r=bbouvier)
(Assignee)

Comment 43

a year ago
Easy
Attachment #8951088 - Flags: review?(bbouvier)
(Assignee)

Comment 44

a year ago
Another easy one.
Attachment #8951094 - Flags: review?(bbouvier)
(Assignee)

Comment 45

a year ago
Posted patch update-call-null (obsolete) — Splinter Review
Punting the optimization of using SIGSEGV to avoid the need for the null check altogether off to bug 1340235 (it probably won't be noticeable until the other call_indirects anyway).
Attachment #8951103 - Flags: review?(bbouvier)
Attachment #8951088 - Flags: review?(bbouvier) → review+
Attachment #8951094 - Flags: review?(bbouvier) → review+
Comment on attachment 8951103 [details] [diff] [review]
update-call-null

Empty changeset (forgot to qref?); that being said, no code means no bugs...
Attachment #8951103 - Flags: review?(bbouvier)
(Assignee)

Comment 47

a year ago
D'oh, sorry, forgot to qref.
Attachment #8951103 - Attachment is obsolete: true
Attachment #8951273 - Flags: review?(bbouvier)
Comment on attachment 8951273 [details] [diff] [review]
update-call-null

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

Thanks
Attachment #8951273 - Flags: review?(bbouvier) → review+

Comment 49

a year ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b25860d4f9
Baldr: use new traps for SIMD conversion trap (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6ab11ede4c
Baldr: use new traps for the already-reported trap (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1616ad6c800b
Baldr: use new traps for indirect call to null trap (r=bbouvier)
(Assignee)

Comment 51

a year ago
This converts the bad-signature-mismatch trap.  The challenge here is the trap occurs in the table entry which is pre-prologue and thus there is no valid frame.  After considering a few options, the simplest thing at this point is to drop the invariant that LookupFaultingInstance(pc) returns an Instance containing pc and instead have the caller ensure this property.  Most paths can assert this immediately, but traps have to do a StartUnwinding() in JitActivation::startWasmTrap(), symmetric to startWasmInterrupt().

The only thing left now is to turn OOB/unaligned faults into regular traps.  I think this will allow some refactorings that remove the above hack.
Attachment #8951744 - Flags: review?(bbouvier)
Comment on attachment 8951744 [details] [diff] [review]
update-sig-mismatch

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

Looks good, thanks!

::: js/src/vm/Stack.cpp
@@ +1837,5 @@
> +    void* pc = unwindState.pc;
> +    wasm::Frame* fp = unwindState.fp;
> +
> +    const wasm::Code& code = fp->tls->instance->code();
> +    MOZ_RELEASE_ASSERT(&code == wasm::LookupCode(pc));

Why does this hold when a wasm::Module A calls into a table that contains a function from a wasm::Module B with a different code? (since fp->tls still points to the caller's TlsData)

::: js/src/wasm/WasmFrameIter.cpp
@@ +1299,1 @@
>      //MOZ_RELEASE_ASSERT(&instance->code() == &codeSegment.code());

nit: time to remove the assert? (or keep the TODO)
Attachment #8951744 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 53

a year ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #52)
> > +    void* pc = unwindState.pc;
> > +    wasm::Frame* fp = unwindState.fp;
> > +
> > +    const wasm::Code& code = fp->tls->instance->code();
> > +    MOZ_RELEASE_ASSERT(&code == wasm::LookupCode(pc));
> 
> Why does this hold when a wasm::Module A calls into a table that contains a
> function from a wasm::Module B with a different code? (since fp->tls still
> points to the caller's TlsData)

Because, after the patch, 'pc' is the unwound pc, not the original trap pc.

> nit: time to remove the assert? (or keep the TODO)

Yeah, I'll keep the TODO b/c the point is this will change (or I'll remove the TODO and commented-out release assert).

Comment 54

a year ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a95360977f76
Baldr: use new traps for bad indirect call signature trap (r=bbouvier)

Comment 56

a year ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa95a3a1d4bd
Baldr: use new traps for bad indirect call signature trap (r=bbouvier)

Comment 57

a year ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/665ae9280a32
Baldr: fix tests/wasm/profiling.js (r=me)
I've applied the changes from ARM simulator to MIPS ones and removed calling the single_step_callback on mips64 simulator inside the delay slots which can confuse new ProfilingFrameIterator.
Attachment #8953121 - Flags: review?(luke)
(Assignee)

Comment 60

a year ago
Comment on attachment 8953121 [details] [diff] [review]
1428453_mips_sim.patch

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

Thanks!
Attachment #8953121 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #60)
> Comment on attachment 8953121 [details] [diff] [review]
> 1428453_mips_sim.patch
> 
> Review of attachment 8953121 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!

Ok, thanks!

Can somebody land this for me? I don't have the sufficient rights in this issue.
Will land.
Flags: needinfo?(luke)

Comment 63

a year ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74307e838816
Fix mips32/mips64 simulator build; r=luke
(Assignee)

Comment 65

a year ago
This patch removes the last old trap.  To keep the patch legible, I saved all the code removal for the next patch, so please ignore the tons of dead types/functions/paths/etc.

As predicted, this is an enormous size savings and nice baseline compile-time savings.

Measuring webassembly.org/demo in about:memory (recall that this is two tiers of code+metadata):

          code     metadata
 Before:  60.0MB   51.8MB
 After:   50.2MB   23.0MB

So an overall 34.5% reduction!  (A bit of the metadata shrinkage was the unrelated fix of adding back callSites.podResizeToFit().)

Comparing shell compile times with --thread-count=2, I see a 9.1% speedup (330ms to 300ms).
Attachment #8961596 - Flags: review?(bbouvier)
(Assignee)

Comment 66

a year ago
Posted patch rm-old-trapsSplinter Review
And this, the last patch, removes all the OldTrap* stuff renamed by the first patch.
Attachment #8961851 - Flags: review?(bbouvier)
(Assignee)

Updated

a year ago
Keywords: leave-open
Comment on attachment 8961596 [details] [diff] [review]
remove-oob-trap

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

LGTM, thanks.
Attachment #8961596 - Flags: review?(bbouvier) → review+
Comment on attachment 8961851 [details] [diff] [review]
rm-old-traps

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

Skimmed the patch, LGTM, thanks.
Attachment #8961851 - Flags: review?(bbouvier) → review+

Comment 69

a year ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11913418a907
Baldr: use new traps for out-of-bounds (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/91a702815131
Baldr: remove old traps (r=bbouvier)

Comment 70

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/11913418a907
https://hg.mozilla.org/mozilla-central/rev/91a702815131
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1514143
You need to log in before you can comment on or make changes to this bug.