Closed Bug 1263811 Opened 5 years ago Closed 5 years ago

Differential Testing: Different output message involving arguments

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox45 --- affected
firefox46 --- affected
firefox47 --- affected
firefox48 --- affected
firefox49 --- fixed
firefox-esr45 --- affected

People

(Reporter: gkw, Assigned: arai)

References

Details

(Keywords: testcase)

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(arai.unmht)
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.
Flags: needinfo?(arai.unmht)
The testcase in the top of comment 1 still bisects to bug 1067049.
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.
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.
Blocks: 861596
Added ObjectMayHaveExtraIndexedProperties check both to Baseline and Ion.
Assignee: nobody → arai.unmht
Attachment #8744561 - Flags: review?(bhackett1024)
(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?
(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).
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...
(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 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...
Attachment #8744561 - Flags: review?(bhackett1024)
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.
Testing shape might be troublesome, as reading arguments element changes shape in slowpath.

Will try adding "element overridden"
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.
Attachment #8744561 - Attachment is obsolete: true
Attachment #8744582 - Flags: review?(jdemooij)
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");
Attachment #8744582 - Flags: review?(jdemooij) → review+
Thank you for reviewing :)

this is basically a regression from bug 861596 (Firefox 23), and it affects all supported branches.
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
https://hg.mozilla.org/mozilla-central/rev/6292f2786349
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.