821735 - IonMonkey: fix integrity checking for safepoint register and slot information

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

The AllocationIntegrityState structure added by 812945 is intended to both check the correctness of register allocations and optionally fill in safepoint information for allocators which do not do that themselves (stupid and backtracking).  The latter bit (and corresponding checking code) is not correct, populating safepoints according to what is live after an instruction rather than before.  This is a combination of missing documentation, outright broken documentation, and poor coverage in jit-tests for what should go into safepoints.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch and cleanup

Fix LSafepoint comments and AllocationIntegrityState behavior per discussion in IRC.  This also cleans up a bunch of stuff in AllocationIntegrityState, which stored the initial allocation info in a fairly ad hoc manner.  This passes jit-tests --ion both with LSRA (for which AllocationIntegrityState will validate the correctness of safepoint info) and with the backtracking allocator (for which AllocationIntegrityState will populate the safepoint info directly).  Nicolas, can you see if the comments on LSafepoint and calls to checkSafepointAllocation look right?

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 692464 [details] [diff] [review]
patch and cleanup

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

This sounds good, add more assertions to ensure that safepoint are correctly filled.

::: js/src/ion/LIR.h
@@ +938,5 @@
>      typedef Vector<NunboxEntry, 0, IonAllocPolicy> NunboxList;
>  
>    private:
> +    // The information in a safepoint describes the registers and gc related
> +    // values that are live at the start of the associated instruction.

A safepoint contains allocations which are live at the time of the call.  A call can use any temporary and input registers which are not only used at the start of the instruction, and produces outputs which are marked as part of the call mechanism.

@@ +943,5 @@
> +
> +    // The set of registers which are live at an OOL call made within the
> +    // instruction. This includes any registers for inputs which are not
> +    // use-at-start, any registers for temps, and any registers live after the
> +    // call except outputs of the instruction.

This is good.

@@ +949,5 @@
> +    // For call instructions, the live regs are empty. Call instructions may
> +    // have register inputs or temporaries, which will *not* be in the live
> +    // registers: if passed to the call, the values passed will be marked via 
> +    // MarkIonExitFrame, and no registers can be live after the instruction
> +    // except its outputs.

This should be asserted somewhere.  Otherwise we may need to add saveLive & restoreLive around normal VM calls.

nit: remove trailing whitespace after "via".

::: js/src/ion/RegisterAllocator.cpp
@@ +150,5 @@
>  
> +                uint32_t vreg = oldInput.toUse()->virtualRegister();
> +
> +                if (safepoint && !oldInput.toUse()->usedAtStart())
> +                    checkSafepointAllocation(ins, vreg, **alloc, populateSafepoints);

can you assert that usedAtStart allocation are not present in the safepoint if the allocation is not reused by any temporary or by the output?

@@ +258,5 @@
> +    LSafepoint *safepoint = ins->safepoint();
> +    JS_ASSERT(safepoint);
> +
> +    if (ins->isCall() && alloc.isRegister()) {
> +        JS_ASSERT_IF(!populateSafepoints, !safepoint->liveRegs().has(alloc.toRegister()));

“For call instructions, the live regs are empty”.

Assert that here, and maybe that populateSafepoints is always false?

@@ +267,5 @@
> +        AnyRegister reg = alloc.toRegister();
> +        if (populateSafepoints)
> +            safepoint->addLiveRegister(reg);
> +        else
> +            JS_ASSERT(safepoint->liveRegs().has(reg));

nit: remove "else".

@@ +282,5 @@
> +                    vreg, ins->id(), alloc.toString());
> +            if (!safepoint->addGcPointer(alloc))
> +                return false;
> +        } else {
> +            JS_ASSERT(safepoint->hasGcPointer(alloc));

nit: remove "else"

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 692464 [details] [diff] [review]
patch and cleanup

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

r=me with comments addressed.

::: js/src/ion/LIR.cpp
@@ +224,5 @@
>          // float or a double. Should we steal a bit in LUse for help? For now,
>          // nothing defines any fixed xmm registers.
>          JS_snprintf(buf, size, "v%d:%s", use->virtualRegister(),
>                      Registers::GetName(Registers::Code(use->registerCode())));
> +      default:

Nit: missing "break;" before "default:"

::: js/src/ion/RegisterAllocator.cpp
@@ +136,5 @@
>  
> +            LSafepoint *safepoint = ins->safepoint();
> +            if (safepoint) {
> +                for (size_t i = 0; i < ins->numTemps(); i++) {
> +                    checkSafepointAllocation(ins, info.temps[i].virtualRegister(),

checkSafepointAllocation is fallible

@@ +150,5 @@
>  
> +                uint32_t vreg = oldInput.toUse()->virtualRegister();
> +
> +                if (safepoint && !oldInput.toUse()->usedAtStart())
> +                    checkSafepointAllocation(ins, vreg, **alloc, populateSafepoints);

checkSafepointAllocation is fallible

@@ +205,3 @@
>                  JS_ASSERT(*def->output() == alloc);
>  
>                  // Found the original definition, done scanning.

Would be good to assert here that outputs are not stored in the safepoint (if we don't do that yet).

@@ +217,5 @@
>                  JS_ASSERT(*temp->output() != alloc);
>          }
>  
> +        if (ins->safepoint())
> +            checkSafepointAllocation(ins, vreg, alloc, populateSafepoints);

checkSafepointAllocation is fallible.

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a99043b80e7

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9a99043b80e7