944196 - Rest arguments JIT code doesn't set ObjectElements::length

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Testcase from comment 0

The following testcase (also attached) passes without --ion-eager, fails with:

  var std_Array_join = Array.prototype.join;
  var std_Array_slice = Array.prototype.slice;

  function callFunction(f, thisv, ...args)
  {
    if (f === std_Array_join)
      assertEq(args.length !== 0, true, "rest args wrong");
    return f.apply(thisv, args);
  }

  function CanonicalizeLanguageTag(locale)
  {
    var subtags = locale.split("-");
    var i = 2;
    var normal = callFunction(std_Array_join, callFunction(std_Array_slice, subtags, 0, i), "-");
    return normal;
  }

  var r1 = CanonicalizeLanguageTag("en-US");
  var r2 = CanonicalizeLanguageTag("en-US");

  assertEq(r1, "en-US", "r1 is wrong");
  assertEq(r2, "en-US", "r2 is wrong");

I think the problem is that IonBuilder::jsop_rest() in the inlining case stores all the provided arguments/elements, then updates the initialized length -- but it never updates the length!  And the length is the result of BaselineCompiler::emit_JSOP_REST(), which creates a template object with length 0.

Many algorithms look solely to the initialized length, because it's a representation invariant that initializedLength < length -- but not all.  And it happens that if the rest array gets passed as the arguments array to Function.prototype.apply, that method uses GetLengthProperty, which has a fast-path for ArrayObject which reads out ObjectElements::length, and hilarity ensues.

This may or may not be a partial cause of bug 944080.  I took the self-hosted code at issue and consed up a very-minimally-reduced testcase from it, but this testcase's emulated callFunction *has* to use rest arguments, whereas I don't *think* the Intl code uses rest args to hit exactly this same issue.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch and tests

Nope, doesn't fix bug 944080.  Yaaaaaaaaaaaaaaaaaaaaks!

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8339683 [details] [diff] [review]
Patch and tests

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

Looks good to me.

r=me with comments addressed.

::: js/src/jit-test/tests/arrays/rest-array.js
@@ +79,5 @@
> +{
> +  twoWithLeading(0, 1, 2);
> +}
> +
> +ttwoWithLeading(); ttwoWithLeading(); ttwoWithLeading();

I think this would be better under tests/arguments/ with "inlining" in the name somewhere.

::: js/src/jit/IonBuilder.cpp
@@ +7768,5 @@
>      current->add(array);
>  
> +    if (numRest == 0) {
> +        // No more updating to do.  (Note that in this one case the length
> +        // from the template object is already correct.)

Comment is incorrect. The template object's length is still 0 and possibly incorrect, but the VM call takes care of all the actual object creation and will set the right length.

@@ +7791,5 @@
>      }
>  
> +    // The array's length is incorrectly 0 now, from the template object
> +    // created by BaselineCompiler::emit_JSOP_REST() before the actual argument
> +    // count was known.  Set the correct length now that we know that count.

Tiny nit: Here and elsewhere, Ion style doesn't really use 2 spaces after period.

::: js/src/jit/Lowering.cpp
@@ +2228,5 @@
> +{
> +    JS_ASSERT(ins->elements()->type() == MIRType_Elements);
> +    JS_ASSERT(ins->index()->type() == MIRType_Int32);
> +
> +    JS_ASSERT(ins->index()->isConstant());

As an aside, I'm not sure why we assert this in MSetInitializedLength (and here). Doesn't hurt anything right now, but seems restrictive for no real reason.

::: js/src/jit/MIR.h
@@ +5289,5 @@
> +class MSetArrayLength
> +  : public MAryInstruction<2>
> +{
> +    MSetArrayLength(MDefinition *elements, MDefinition *index)
> +    {

Nit: I think we put the { on the same line as the function if there's no initializer list. You probably copied the skeleton of the class from MSetInitializedLength, since I see the same thing there. Mind doing a drive-by style fix?

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8339683 [details] [diff] [review]
Patch and tests

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

::: js/src/jit/IonBuilder.cpp
@@ +7768,5 @@
>      current->add(array);
>  
> +    if (numRest == 0) {
> +        // No more updating to do.  (Note that in this one case the length
> +        // from the template object is already correct.)

This comment is fine. I misread the code, my bad.

Comment 4

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a1d2338a794

Comment 5

[Jeff Walden [:Waldo]]

This appears to be a regression from bug 935027, judging purely by this diff, no actual attempt made to test the hypothesis:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f88f1e41372

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a1d2338a794

Comment 7

[Ioana (away)]

https://hg.mozilla.org/mozilla-central/diff/6a1d2338a794/js/src/jit-test/tests/arguments/inline-rest-array-creation.js