1263811 - Differential Testing: Different output message involving arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

x = [];
x.splice(1, this.v, "", 0);
x.sort(function() {
    y = arguments;
});
Object.defineProperty(y, 1, {
    value: 2
});
for (var z of y) {
    print(z);
}

$ ./js-dbg-64-dm-clang-darwin-29d5a4175c8b --fuzzing-safe --no-threads --baseline-eager testcase.js

0

$ ./js-dbg-64-dm-clang-darwin-29d5a4175c8b --fuzzing-safe --no-threads --ion-eager testcase.js

2

Tested this on m-c rev 29d5a4175c8b.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin14.5.0 --disable-jemalloc --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 29d5a4175c8b

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/f7aa719c43c9
parent:      278712:4103127ecd87
user:        Tooru Fujisawa
date:        Wed Jan 06 17:53:21 2016 +0900
summary:     Bug 1067049 - Implement arguments[@@iterator]. r=evilpie

arai-san, is bug 1067049 a likely regressor?

Comment 1

[Tooru Fujisawa [:arai]]

The underlying bug seems to be more old.

The testcase can be reduced to following:

  (function() {
    Object.defineProperty(arguments, 1, { value: 30 });
    for (var z of arguments) {
      print(z);
    }
  })(10, 20);

and the output is following:

  $ ./js --fuzzing-safe --no-threads --baseline-eager testcase.js
  10
  20

  $ ./js --fuzzing-safe --no-threads --ion-eager testcase.js
  10
  30


Then, the underlying issue can be observed with following code:

  function f() {
    Object.defineProperty(arguments, 1, { value: 30 });
    print(arguments[1]);
  }
  for (let i = 0; i < 6; i++)
    f(10, 20);

and the output is following:

  $ ./js  --no-threads --ion-eager ~/Desktop/a.js
  30
  20
  30
  20
  20
  20

arguments[1] should always be 30, but it sometimes returns 20.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The testcase in the top of comment 1 still bisects to bug 1067049.

Comment 3

[Tooru Fujisawa [:arai]]

Yes, because arguments[@@iterator] was added in bug 1067049.
it just exposed the underlying issue, the 2nd testcase in comment #1, I believe.
anyway, I'll investigate it tonight.

Comment 4

[Tooru Fujisawa [:arai]]

Here's the output and the code executed for each.

  Baseline GetElemFallback - GetElementOperation
  30
  Baseline ICGetElem_Arguments
  20
  Ion GetPropertyIC::update - GetObjectElementOperation
  30
  Ion GetPropertyIC::tryAttachArgumentsElement
  20
  Ion GetPropertyIC::tryAttachArgumentsElement
  20
  Ion GetPropertyIC::tryAttachArgumentsElement
  20

So, this is the behavior difference between GetElementOperation/GetObjectElementOperation and optimized arguments element access both in baseline/ion.

I think, we shouldn't attach arguments element IC if the argument object have extra indexed property.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Do not attach optimized IC for arguments element access if the arguments object have extra indexed property.

Added ObjectMayHaveExtraIndexedProperties check both to Baseline and Ion.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #4)
> I think, we shouldn't attach arguments element IC if the argument object
> have extra indexed property.

Will that do the right thing if we first attach the IC and *then* add the extra property?

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Tooru Fujisawa [:arai] from comment #4)
> > I think, we shouldn't attach arguments element IC if the argument object
> > have extra indexed property.
> 
> Will that do the right thing if we first attach the IC and *then* add the
> extra property?

Oh, good point.

in that case, we should either:
  a) add check to arguments element IC and fallback, maybe adding some shape guard?
  b) invalidate the IC when arguments object is modified

will check and try implementing either one, maybe (a).

Comment 8

[Jan de Mooij [:jandem]]

Arguments objects have "length overridden" and "iterator overridden" flags. Maybe we could also add "element overridden"? We'd probably have to implement obj_defineProperty for that though...

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> Arguments objects have "length overridden" and "iterator overridden" flags.
> Maybe we could also add "element overridden"? We'd probably have to
> implement obj_defineProperty for that though...

I guess a shape guard is simpler, and let's us get rid of the Class check.

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 8744561 [details] [diff] [review]
Do not attach optimized IC for arguments element access if the arguments object have extra indexed property.

In any way ObjectMayHaveExtraIndexedProperties was too strong and it disabled all optimization, as ClassMayResolveId is true for arguments...

Comment 11

[Tooru Fujisawa [:arai]]

before that, isIndexed also doesn't work for that check...

> ArgumentsObject*
> ArgumentsObject::createTemplateObject(JSContext* cx, bool mapped)
> {
> ...
>     RootedShape shape(cx, EmptyShape::getInitialShape(cx, clasp, TaggedProto(proto),
>                                                       FINALIZE_KIND, BaseShape::INDEXED));
>     if (!shape)
>         return nullptr;

we need another way to check the overridden property.

Comment 12

[Tooru Fujisawa [:arai]]

Testing shape might be troublesome, as reading arguments element changes shape in slowpath.

Will try adding "element overridden"

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Do not attach optimized IC for arguments element access if the arguments object have extra indexed property.

Added ELEMENT_OVERRIDDEN_BIT, hasOverriddenElement, and markElementOverridden.

NativeDefineProperty now marks ELEMENT_OVERRIDDEN_BIT if the property `id` is int,
as we're only optimizing element access.

Both in Baseline and Ion, ELEMENT_OVERRIDDEN_BIT is checked at 2 places, before attaching IC, and inside the IC.
if ELEMENT_OVERRIDDEN_BIT is marked before attaching IC, it just avoid attaching.
and if ELEMENT_OVERRIDDEN_BIT is marked after attaching IC, it fallbacks, as same as LENGTH_OVERRIDDEN_BIT.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8744582 [details] [diff] [review]
Do not attach optimized IC for arguments element access if the arguments object have extra indexed property.

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

Nice.

::: js/src/vm/ArgumentsObject.h
@@ +127,5 @@
>    public:
>      static const uint32_t LENGTH_OVERRIDDEN_BIT = 0x1;
>      static const uint32_t ITERATOR_OVERRIDDEN_BIT = 0x2;
> +    static const uint32_t ELEMENT_OVERRIDDEN_BIT = 0x4;
> +    static const uint32_t PACKED_BITS_COUNT = 3;

Please add a static assert here:

static_assert(ARGS_LENGTH_MAX <= (UINT32_MAX >> PACKED_BITS_COUNT),
              "Max arguments length must fit in available bits");

Comment 15

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

this is basically a regression from bug 861596 (Firefox 23), and it affects all supported branches.

Comment 16

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6292f27863498e6367b679f37e8c710ab41bfff4
Bug 1263811 - Do not attach optimized IC for arguments element access if any arguments element has been overridden. r=jandem

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6292f2786349