700613 - interpreter convert_u wrong on 64-bit

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: Run Array's push and a copy of it on borderline objects

When you try to use apply Array's generic push method to a non-array Object, you get weirdness, and at least some of it is arising from within the JIT.

Here's the basic code you start with:

      var o1 = {};
      o1.length = uint.MAX_VALUE - 2;
      Array.prototype.public::push.call(o1,1,2,3,4,5);

The 32-bit interpreter, 32-bit jit, and 64-bit jit all agree on this result for 'o1' after the push:

o1: {"length":2,
     "0":4,"1":5,
     "4294967293":1,"4294967294":2,"4294967295":3}

But the 64-bit interpreter says this instead:

o1: {"length":4294967298,
     "4294967293":1,"4294967294":2,"4294967295":3,
     "4294967296":4,"4294967297":5}

That's not the end of it though.  After copying the implementation of Array's prototype push method from Array.as to its own file (attached) and running it on a similar object 'o2', I get yet another result:

      var o2 = {};
      o2.length = uint.MAX_VALUE - 2;
      push.call(o2,1,2,3,4,5);

The 32-bit interpreter, 32-bit jit, and 64-bit jit all agree on this result for 'o2' after the push:

o2: {"length":4294967298,
     "0":4,"1":5,
     "4294967293":1,"4294967294":2,"4294967295":3}

But the 64-bit interpreter says that this is 'o2':
o2: {"length":4294967298,
     "4294967293":1,"4294967294":2,"4294967295":3,
     "4294967296":4,"4294967297":5}

My hypothesis is that there are actually two bugs here, and that the first (which is causing the discrepancy in what indexes are used when pushing the values into the object's properties) probably has something to do with how we deal with the statement 
  this[n++] = v
when n has type :uint and the result of the addition overflows.  (A good question, if you ask me.)

The second bug is how the jit/interpreter distinction is affecting the setting of the length property, but *only* for the Array builtin code.  The setting of the length property seems like it is being handled identically for jit/interp on the code when it has been copied into its own file here.

(I did experiment last night with making 'o2' an instance of a class with length getter/setter, since that is something that the array code is doing that the attached code for 'o2' is not doing; but that did not seem to reproduce the problem we see with 'o1'.)

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

(btw just ignore the following comment in attachment 572772 [details].  I believe I was terribly mistaken and was not making correct observations at the time when I wrote it last night.)

          // This branch seems pointless but its not; if you reduce it
          // to the single statement, the bug with the length wrap-around
          // in the JIT goes away.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Oh, duh.  The 'push' in attachment 572772 [details] is *way* more complicated than it should be (it originates from the 'push' i have been developing in Bug 681803).

Will fix.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #2)
> Oh, duh.  The 'push' in attachment 572772 [details] is *way* more
> complicated than it should be (it originates from the 'push' i have been
> developing in Bug 681803).

And obviously that means that the 'push' in attachment 572772 [details] doesn't match the Array.prototype.push that currently lives in TR.

The fix of putting the actual current Array.prototype.push into the file brings 'o1' and 'o2' into sync, in the sense that the 64-bit interpreter produces:

o1: {"length":4294967298,
     "4294967293":1,"4294967294":2,"4294967295":3,
     "4294967296":4,"4294967297":5}

but the 64-bit jit (and 32-bit jit and 32-bit interpreter) produces:

o1: {"length":2,
     "0":4,"1":5,
     "4294967293":1,"4294967294":2,"4294967295":3}

So this is not as great a mystery as I had thought.

But still a definite jit/interpreter bug.

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: push jit/interp borderline case

Here's the updated test with the much simpler 'push' that focuses on the bug.  There's nothing special about the builtins for this bug, its just the jit/interpreter mishandling this code:

      var push = function(...args)
      {
          var n:uint = uint(this.length)
          for (var i:uint=0, argc:uint=args.length; i < argc; i++)
              this[n++] = args[i]
          this.length = n
          return n
      }

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: much simpler test

I think the prior test really can be narrowed down to just this:

      var pu = function(arg)
      {
          var n:uint = uint(arg);
          n++;
          return n;
      }
      pu(uint.MAX_VALUE);

as illustrated in the uploaded test case (once again: 64-bit interp does not wrap around; 64-bit jit + 32-bit jit+interp all wrap).

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Compared how the interpreter behaves on 32-bit vs 64-bit debug build.

The two behaviors differ at this case:

            INSTR(convert_u) {
            ABC_CODE_ONLY( convert_u_impl: )
                a1 = sp[0];
                if (!IS_INTEGER(a1) || a1 < 0) { // <-- difference here
                    SAVE_EXPC;                   // <-- 32-bit goes here,
                    sp[0] = core->uintAtom(a1);  //       64-bit skips
                }
                NEXT;
            }

Comment 7

[Edwin Smith]

On 64-bit, kIntptrType atoms can hold large integer values -- the logic above assumes Atom can't hold big unsigned values.  So, definite interpreter bug.  I'd vote to just fix with no versioning.

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A: handle large 64-bit atoms.

This seems like the obvious patch.

(UINT32_T_MAX seems safe to use since we define it ourselves in a couple headers.)

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #8)
> Created attachment 572795 [details] [diff] [review] [diff] [details] [review]
> patch A: handle large 64-bit atoms.
> 
> This seems like the obvious patch.
> 

(of course, its entirely possible that some compilers might emit warnings about the unsatisfiable comparison when compiling for 32-bit targets.  But I'd prefer to wait to see such events occur before I clutter up the code with another ifdef AVMPLUS_64BIT conditional in here.)

Comment 10

[Edwin Smith]

Comment on attachment 572795 [details] [diff] [review]
patch A: handle large 64-bit atoms.

a1 is an Atom, so the check (a1 > UINT32_T_MAX) is overly conservative; is that what you want?  I think this is right, please double check me

   (!IS_INTEGER(a1) || atomGetIntptr(a1) > intptr_t(UINT32_T_MAX))

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Edwin Smith from comment #10)
> Comment on attachment 572795 [details] [diff] [review] [diff] [details] [review]
> patch A: handle large 64-bit atoms.
> 
> a1 is an Atom, so the check (a1 > UINT32_T_MAX) is overly conservative; is
> that what you want?

Just checking: You're talking about conservative w.r.t. overall efficiency, not semantic correctness, right?

(But yes, assuming I understand you correctly, there are cases that unnecessarily go down the slow path.)

>  I think this is right, please double check me
> 
>    (!IS_INTEGER(a1) || atomGetIntptr(a1) > intptr_t(UINT32_T_MAX))

I'll figure out the right condition to use; IIRC (intptr_t(UINT32_T_MAX) == -1), so I think you want to put the cast on the other side of the >

Comment 12

[Edwin Smith]

(In reply to Felix S Klock II from comment #11)
> (In reply to Edwin Smith from comment #10)
> > Comment on attachment 572795 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch A: handle large 64-bit atoms.
> > 
> > a1 is an Atom, so the check (a1 > UINT32_T_MAX) is overly conservative; is
> > that what you want?
> 
> Just checking: You're talking about conservative w.r.t. overall efficiency,
> not semantic correctness, right?

right

> (But yes, assuming I understand you correctly, there are cases that
> unnecessarily go down the slow path.)

it wouldn't be worth complicating the check to catch a few more cases, granted.  but
if an equally fast check can handle the full uint32 range on 64-bit, all the better.

> >  I think this is right, please double check me
> > 
> >    (!IS_INTEGER(a1) || atomGetIntptr(a1) > intptr_t(UINT32_T_MAX))
> 
> I'll figure out the right condition to use; IIRC (intptr_t(UINT32_T_MAX) ==
> -1), so I think you want to put the cast on the other side of the >

cool.  from today's comment stream, other opcodes with IS_INTEGER() fast paths
might have bugs too. (increment?)

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v2: handle large 64-bit atoms via general helper

Delegates to atomCanBeUint32(Atom atom).

atomCanBeUint32 assumes (and debug asserts) that atomIsIntptr(atom), and has isolated 64/32-bit paths.

So if it is not as fast possibly can be for this use case, then IMO the fix would be to change the implementation of atomCanBeUint32.

Comment 14

[Tamarin Bot]

changeset: 6712:96f8dd33480f
user:      Felix S Klock II <[e-mail]>
summary:   Bug 700613: handle large 64-bit atoms via general helper (r=edwsmith).

http://hg.mozilla.org/tamarin-redux/rev/96f8dd33480f

Comment 15

[Tamarin Bot]

changeset: 6713:f18f44930816
user:      Felix S Klock II <[e-mail]>
summary:   Bug 700613: regression test for bug (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/f18f44930816