Differential Testing: Different output message involving arguments

RESOLVED FIXED in Firefox 49

Status

()

defect
--
major
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gkw, Assigned: arai)

Tracking

(Blocks 2 bugs, {testcase})

Trunk
mozilla49
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox46 affected, firefox47 affected, firefox48 affected, firefox49 fixed, firefox-esr45 affected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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)
(Assignee)

Comment 1

3 years ago
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)
(Reporter)

Comment 2

3 years ago
The testcase in the top of comment 1 still bisects to bug 1067049.
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 861596
(Assignee)

Comment 5

3 years ago
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?
(Assignee)

Comment 7

3 years ago
(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.
(Assignee)

Comment 10

3 years ago
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)
(Assignee)

Comment 11

3 years ago
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.
(Assignee)

Comment 12

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

Will try adding "element overridden"
(Assignee)

Comment 13

3 years ago
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+
(Assignee)

Comment 15

3 years ago
Thank you for reviewing :)

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

Comment 16

3 years ago
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6292f2786349
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.