Closed
Bug 1263811
Opened 5 years ago
Closed 5 years ago
Differential Testing: Different output message involving arguments
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
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)
Assignee | ||
Comment 1•5 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•5 years ago
|
||
The testcase in the top of comment 1 still bisects to bug 1067049.
Assignee | ||
Comment 3•5 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•5 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 | ||
Comment 5•5 years ago
|
||
Added ObjectMayHaveExtraIndexedProperties check both to Baseline and Ion.
Assignee: nobody → arai.unmht
Attachment #8744561 -
Flags: review?(bhackett1024)
Comment 6•5 years ago
|
||
(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•5 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).
Comment 8•5 years ago
|
||
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•5 years ago
|
||
(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•5 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•5 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•5 years ago
|
||
Testing shape might be troublesome, as reading arguments element changes shape in slowpath. Will try adding "element overridden"
Assignee | ||
Comment 13•5 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 14•5 years ago
|
||
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•5 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•5 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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6292f2786349
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•