625141 - JM: incorrect result for test case involving typed arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Consider this test case:
---
function f() {
    var arr = new Int32Array(10);
    for (var i = 0; i < ({y: function() { return arr.length; }}).y(); i++) {
        arr[i] = i;
    }
    assertEq(arr[5], 5);
}
f();
---
This fails on 32-bit with -m:

Error: Assertion failed: got 0, expected 5

Comment 1

[David Mandelin [:dmandelin]]

The general misbehavior here is pretty bad, but it seems that the weird loop condition is required to trigger the bug.

Comment 2

[Paul Biggar]

dvander on IRC: probably a good place to look is that the key and RHS are botht he same value, so mayeb some register fiddling edge case is misbehaving

Comment 3

[Paul Biggar]

(In reply to comment #1)
> The general misbehavior here is pretty bad, but it seems that the weird loop
> condition is required to trigger the bug.

I can cause it with a simpler loop condition too:

function f() {
  var arr = new Int32Array(10);
  function x() { return arr.length; };
  for (var i = 0; i < x(); i++) {
    arr[i] = i;
  }
  assertEq(arr[5], 5);
}
f();

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Hrm, simpler loop condition says this should probably still block. Calls flush register state so that's suspicious.

Comment 5

[Paul Biggar]

Attachment: WIP

Patch fixes it, but I think there are larger issues.

So the problem seems to be that in the register-swap edge case, we don't restore typereg before address is used, which is bad if typereg is a component of the address. I solved this by restoring the type in time, but I'm not sure about the whole swapping thing. It doesn't look safe here:

  vr = ValueRemat::FromRegisters(vr.typeReg(), newReg);


Why are we swapping the registers anyway? What does that get us?

Comment 6

[Paul Biggar]

(In reply to comment #4)
> Hrm, simpler loop condition says this should probably still block.

Dunno how I managed to set the flag back to ?, but I blame bugzilla.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #5)
> So the problem seems to be that in the register-swap edge case, we don't
> restore typereg before address is used, which is bad if typereg is a component
> of the address.

Nice catch. 

> Why are we swapping the registers anyway? What does that get us?

Trying to page this in since this code kind of looks like clownshoes to me now. There are potentially four registers in play: objReg, keyReg (these two form the LHS reference), typeReg, dataReg (these two form the RHS). The following conditions will always be true entering into StoreToTypedArray():
 * objReg != any other register   (obj[obj] won't IC)
 * typeReg != any other register  (all other regs are payload regs)

The RHS might need conversion, in which case we need a register that is free upon leaving the opcode (so we don't clobber anything live). The hard part is if there aren't any, we need to pick one and preserve it. We can't just pick anything, because if it's an Int8Array, we can *only* use (eax, ecx, edx). This is an x86 restriction.

We try to grab a register that's not in (objReg, typeReg, keyReg). We can't easily clobber objReg or keyReg, and the conversion process needs typeReg. So if all three of those registers take up the entire class suitable for Int8Array, we're in trouble. The swap trick is because we know typeReg will be in the correct class and dataReg is not. But as you've found, this doesn't correctly handle the case where (keyReg == dataReg).

If all that is correct, then I think your patch is correct. We could try to simplify it but it seems like no matter what you need a special case for colliding with the LHS and shuffling.

However, it's weird we hit this for the Int32Array case, where there are no register class restrictions. Would be good to fix that here too, all you need to do is change the two places where we restrict ourselves to C-ABI temporary regs. 

 (1) In Compiler::jsop_setelem, volatileMask should *not* start with "& Registers::TempRegs".
 (2) In StoreToTypedArray, allowMask should start out as AvailRegs, *not* TempRegs.

Comment 8

[David Mandelin [:dmandelin]]

Looks like the blocking flag got reset accidentally.

Comment 9

[Paul Biggar]

Attachment: Tidy up

THis is as above, except that it has better comments, adds the test and includes the jsop_setelem and allowMask fixes, and folds in some JaegerSpew fixes I did when trying to resolve FIXMEs.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 507270 [details] [diff] [review]
Tidy up

>         PreserveRegisters saveRHS(masm);
>+        PreserveRegisters saveType(masm);

Nit: maybe saveLHS?

>         GenConversionForIntArray(masm, tarray, vr, saveMask);
>+        // |GenConversionForIntArray| can overwrite |typeReg|, but
>+        // |typeReg| might be a component of |address|.
>+        saveType.restore();

Nit: house style wants a newline between the comment and GenConversion

Can you include another test case, identical to the first except using Int8Array instead of Int32Array?

r=me with that, thanks!

Comment 12

[Paul Biggar]

Attachment: A proper fix this time

My fix was broken, in enough ways to make it seem like it was working, and the test you suggested for the 8bit typed arrays picked it up.

The behaviour we want is to write the result of GenConversionForIntArray into |address|. So we need to restore all of the components of |address| which we're overwriting. GenConversionForArray stores and restores the registers in saveMask, but it may store the wrong one if we've gone down the register-swapping route.

The last patch worked by pure luck. I was trying to save typeReg, but forgot to mask the register, so it was randomly saving ecx - which happened to be the register I needed to save - rather than eax.

This patch does it properly. In addition, it doesn't save registers more than once, if the same register is required. (We could be duplicating the saves in saveLHS, saveRHS, and via saveMask). So this is optimal, AFAICT.


While working on this patch, I got hit by a few problems:
 - preserve() accepts unmasked registers. Can we use a bit of strong typing to avoid this?
 - I pushed and popped in the wrong order. Is there a checking mode where PreserveRegisters or the assembler checks this?


Just one more thing: why can vr.typeReg() also be part of the address? Copy propagation?

Comment 13

[Paul Biggar]

Attachment: With dvander's suggestions

Whoops, forgot to qrefresh.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 507377 [details] [diff] [review]
With dvander's suggestions

Argh, nice catch. Yeah, maybe in a separate bug we should make a RegisterMask struct or something, since RegisterID implicitly coerces to an integer.

> I pushed and popped in the wrong order. Is there a checking mode where
PreserveRegisters or the assembler checks this?

Nope, not yet.

Comment 15

[Paul Biggar]

http://hg.mozilla.org/tracemonkey/rev/ea2b1b5e3837

Comment 16

[Paul Biggar]

> [e-mail]: Whiteboard: [hardblocker][fixed-in-tracemonkey] ⇒ [hardblocker][fixed-in-tracemonkey][has patch]

I'm not terribly familiar with [has-patch]. Why did we add it back once it was [fixed-in-tracemonkey]?

Comment 17

[Asa Dotzler [:asa]]

Paul, I was trying to make easier tracking of the things that are no longer waiting on patches that still need to land in m-c for Firefox 4 beta and final.

Comment 18

[Paul Biggar]

(In reply to comment #17)
> Paul, I was trying to make easier tracking of the things that are no longer
> waiting on patches that still need to land in m-c for Firefox 4 beta and final.

Cool, thanks (wasn't a problem - I just didn't know the workflow :) ).

Comment 19

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/ea2b1b5e3837