Closed Bug 1379936 Opened 7 years ago Closed 7 years ago

Differential Testing: Different output message involving RegExp

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: gkw, Assigned: jandem)

References

Details

(Keywords: testcase, Whiteboard: [fuzzblocker])

Attachments

(1 file)

print("".replace(/x/)); (function () { for (var i = 0; i < 2; ++i) { x = print(/[^]/g.exec()); } })() $ ./js-dbg-64-dm-linux-0e41d07a703f --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js u u $ ./js-dbg-64-dm-linux-0e41d07a703f --fuzzing-safe --no-threads --ion-eager testcase.js u n Tested this on m-c rev 0e41d07a703f. My configure flags are: AR=ar sh /home/ubuntu/trees/mozilla-central/js/src/configure --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 0e41d07a703f autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/76b0524a77e4 user: Jan de Mooij date: Sun Jul 09 11:46:57 2017 +0200 summary: Bug 1115355 - Optimize RegExpObject allocation in Ion. r=evilpie Jan, is bug 1115355 a likely regressor? Setting [fuzzblocker] because it is very difficult to ignore RegExp differential testing bugs in case of dupes.
Flags: needinfo?(jdemooij)
Attached patch Patch (deleted) — Splinter Review
This is annoying. Usually template objects are immutable, but Ion has this no-clone optimization for regular expressions where we can use the template object directly if doing this is not observable (when it doesn't escape to arbitrary JS). However a side-effect is that this can change the lastIndex slot of the template RegExpObject. This means that Ion background codegen can't use this slot because it can get a bogus value or race with the main thread updating it. Here's a patch to make sure we always initialize this slot to 0 instead of using the template object's value. The other slots are immutable and this matches what we do in CloneRegExpObject. After all the regexp cloning optimizations, it might be worth removing the no-clone optimization for global/sticky regular expressions at some point...
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8885243 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8885243 [details] [diff] [review] Patch Review of attachment 8885243 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MacroAssembler.cpp @@ +964,5 @@ > + // Template objects are not exposed to script and therefore immutable. > + // However, regexp template objects are sometimes used directly (when > + // the cloning is not observable), and therefore we can end up with a > + // non-zero lastIndex. Detect this case here and just substitute 0, to > + // avoid racing with the main thread updating this slot. Isn't there a case where the result of the match is attached as slots on the regexp object?
Attachment #8885243 - Flags: review?(nicolas.b.pierron) → review+
Jan, is this ready for landing?
Flags: needinfo?(jdemooij)
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ed1ef5a791 Make sure lastIndex slot is always initialized to 0 when cloning regexps in Ion. r=nbp
(In reply to Nicolas B. Pierron [:nbp] from comment #2) > Isn't there a case where the result of the match is attached as slots on the > regexp object? There's the RegExpStatics thing but it's shared. (In reply to Gary Kwong [:gkw] [:nth10sd] from comment #3) > Jan, is this ready for landing? Sorry for the delay!
Flags: needinfo?(jdemooij)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: