Closed Bug 1360263 Opened 2 years ago Closed 2 years ago

Remove resumePC from the WasmActivation

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(1 file)

It can be stored in the runtime: it's only used by the interrupt handler, which isn't reentrant and thus can't clobber the resumePC value.
Simple and obvious.
Attachment #8862505 - Flags: review?(luke)
Comment on attachment 8862505 [details] [diff] [review]
remove-resumepc.patch

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

Yay!

::: js/src/vm/Runtime.h
@@ +1050,5 @@
> +
> +  private:
> +    // When wasm is interrupted, the pc at which we should return if the
> +    // interrupt hasn't stopped execution of the current running code. Since
> +    // this is used only by the interrupt handler and the latter is not

nit: capitalize 'this'

@@ +1051,5 @@
> +  private:
> +    // When wasm is interrupted, the pc at which we should return if the
> +    // interrupt hasn't stopped execution of the current running code. Since
> +    // this is used only by the interrupt handler and the latter is not
> +    // reentrant, this value can't be clobbered and is unique to the runtime.

nit: instead of saying the value is unique to the runtime, maybe say "so there is at most one resume PC at a time"

@@ +1056,5 @@
> +    js::ActiveThreadData<void*> wasmResumePC_;
> +
> +  public:
> +    void* wasmResumePC() const { return wasmResumePC_; }
> +    void setWasmResumePC(void* resumePC) { wasmResumePC_ = resumePC; }

Can you assert !wasmResumePC_?
Attachment #8862505 - Flags: review?(luke) → review+
I noticed, from try servering some of my patches with this patch applied, a crash with the active-cx checks on OSX.  The reason is that HandleMachException(), which touches some cx fields, executes on a special mach thread.  I think the fix, which I tested locally, is to put an AutoNoteSingleThreadedRegion in HandleMachException.
Thank you for the review!

(In reply to Luke Wagner [:luke] from comment #2)
> ::: js/src/vm/Runtime.h
> @@ +1050,5 @@
> > +
> > +  private:
> > +    // When wasm is interrupted, the pc at which we should return if the
> > +    // interrupt hasn't stopped execution of the current running code. Since
> > +    // this is used only by the interrupt handler and the latter is not
> 
> nit: capitalize 'this'

(didn't: "Since" just above is the first word in this sentence.)

> 
> @@ +1056,5 @@
> > +    js::ActiveThreadData<void*> wasmResumePC_;
> > +
> > +  public:
> > +    void* wasmResumePC() const { return wasmResumePC_; }
> > +    void setWasmResumePC(void* resumePC) { wasmResumePC_ = resumePC; }
> 
> Can you assert !wasmResumePC_?
Almost: !!resumePC == !wasmResumePC_, since this method is used to reset.


(In reply to Luke Wagner [:luke] from comment #3)
> I noticed, from try servering some of my patches with this patch applied, a
> crash with the active-cx checks on OSX.  The reason is that
> HandleMachException(), which touches some cx fields, executes on a special
> mach thread.  I think the fix, which I tested locally, is to put an
> AutoNoteSingleThreadedRegion in HandleMachException.

Thanks for the heads-up, added it just before the call to HandleMemoryAccess (which can call startInterrupt and thus sets Runtime::wasmResumePC_).
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08a538847683
Move WasmActivation::resumePC to the Runtime; r=luke
https://hg.mozilla.org/mozilla-central/rev/08a538847683
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1367871
You need to log in before you can comment on or make changes to this bug.