Closed Bug 905148 Opened 6 years ago Closed 6 years ago

IonMonkey: Ensure a safepoint's live registers are not modified before its OsiPoint


(Core :: JavaScript Engine, defect)

Not set





(Reporter: jandem, Assigned: jandem)




(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
This patch stores all live registers before we perform a callVM. When we reach the OsiPoint, we check these registers still hold their original values.

With the patch for bug 905091 and after enabling these checks by default, it passes jit-tests with/without --ion.

I added a new shell function to enable these checks and added a call to this function to a few interesting jit-tests. Some of these tests will fail without the patch for bug 905091.

I tried to use the patch in bug 902690, but it's not complete. I also don't want to block this patch on that; it's an unrelated change and once we have a more generic way to set JIT options it will be very easy to change this.
Attachment #790189 - Flags: review?(nicolas.b.pierron)
Comment on attachment 790189 [details] [diff] [review]

Review of attachment 790189 [details] [diff] [review]:

Nice work :)

The register dump must be marked as part of the MarkJitActivation in case of an exit frame, by using the safepoint too.  Otherwise a moving GC might change the address of pointers which are captured by IonMonkey's registers and cause a failure which is unrelated.

::: js/src/jit/CodeGenerator.cpp
@@ +794,5 @@
> +#ifdef DEBUG
> +static void
> +OsiPointRegisterCheckFailed()
> +{
> +    MOZ_ASSUME_UNREACHABLE("Modified registers between VM call and OsiPoint");

Add a comment to explain that any register captured by a safepoint must remain unchanged.

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +455,5 @@
> +            // now on top of the stack), we need a second scratch register.
> +            Register scratch2 = allRegs.takeAny();
> +            masm.push(scratch2);
> +            masm.loadPtr(Address(StackPointer, sizeof(uintptr_t)), scratch2);
> +            masm.storePtr(scratch2, dest);

It seems that all the iteration logic of Verify & Store is all here.

Maybe we can avoid that much duplication by using functors. Also, If we remove the dependency on the safepoint, we can get some interesting primitives for testing any register invariant.

class Store
  void operator ()(Register reg, Address dump);
  void operator ()(FloatRegister reg, Address dump);

class Verify
  Label *failure_;
  void operator ()(Register reg, Address dump);
  void operator ()(FloatRegister reg, Address dump);

HandleActivationDump<Store>(masm, lir->safepoint()->liveRegs());
HandleActivationDump<Verify>(masm, lir->safepoint()->liveRegs());

::: js/src/vm/Stack.h
@@ +1290,5 @@
>  namespace ion {
> +#ifdef DEBUG
> +class RegisterDump
> +{

I do not think this is the best location for this class, I guess it would be better in Registers.h (before the MachineState). And I think we should add it as part of a separated patch which use it in the BailoutStack and InvalidationBailoutStack too and changle the MachineState::FromBailout function to use this class instead.
Attachment #790189 - Flags: review?(nicolas.b.pierron)
Attached patch Patch v2Splinter Review
Good catch about moving GCs causing failures! I fixed it by disabling the check for the current VM call in MarkJitActivation. Marking the pointers is a lot more complicated and doesn't help the register checks much.

I added a separate function to abstract the register store/verify. I tried to move more code into this function but that made it harder to understand what's going on.

RegisterDump is now in Registers.h. I can post another patch to change the bailout code to use this class when this patch is in.
Attachment #790189 - Attachment is obsolete: true
Attachment #790773 - Flags: review?(nicolas.b.pierron)
Comment on attachment 790773 [details] [diff] [review]
Patch v2

Review of attachment 790773 [details] [diff] [review]:

Sounds good, just a last remark and you can r=me.

Can you rename all the #ifdef DEBUG of this patch to #ifdef CHECK_SAFEPOINT_ARE_HONORED (or something similar) and define it to 1 at the bottom of IonTypes.h.
The reason being that it is easier to spot parts which are involve in this checking process, and also give a way to make an optimized build with this instrumentation.
Attachment #790773 - Flags: review?(nicolas.b.pierron) → review+
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment on attachment 790773 [details] [diff] [review]
Patch v2

Review of attachment 790773 [details] [diff] [review]:

::: js/src/builtin/TestingFunctions.cpp
@@ +897,5 @@
> +{
> +#ifdef DEBUG
> +    ion::js_IonOptions.osiPointRegisterChecks = true;
> +#endif
> +    JS_SET_RVAL(cx, vp, JSVAL_VOID);

cx? Interesting...
Depends on: 907117
You need to log in before you can comment on or make changes to this bug.