Closed
Bug 1075266
Opened 10 years ago
Closed 10 years ago
Assertion failure: congruent == l->congruentTo(k) (congruentTo relation is not symmetric), at jit/ValueNumbering.cpp
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
Tracking | Status | |
---|---|---|
firefox35 | --- | affected |
People
(Reporter: gkw, Assigned: sunfish)
References
Details
(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])
Attachments
(2 files, 1 obsolete file)
7.48 KB,
text/plain
|
Details | |
1.19 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
for (var j = 0; j < 1; j++) { (function(x) { false + 0 + x | x + false })(0 >> []) } asserts js debug shell on m-c changeset 2ae57957e4bb with --ion-eager --no-threads at Assertion failure: congruent == l->congruentTo(k) (congruentTo relation is not symmetric), at jit/ValueNumbering.cpp. Debug configure flags: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests === Tinderbox Build Bisection Results by autoBisect === The "good" changeset has the timestamp "20140917100058" and the hash "33bef5d1f9bb". The "bad" changeset has the timestamp "20140917102756" and the hash "ce0a75f9481b". Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=33bef5d1f9bb&tochange=ce0a75f9481b Dan, is bug 1029830 a possible regressor?
Flags: needinfo?(sunfish)
Reporter | ||
Comment 1•10 years ago
|
||
(lldb) bt 5 * thread #1: tid = 0x2e69c2, 0x000000010043ebd3 js-dbg-opt-64-dm-nsprBuild-darwin-14665b1de5ee`js::detail::HashTable<js::jit::MDefinition* const, js::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::IonAllocPolicy>::SetOps, js::jit::IonAllocPolicy>::match(js::detail::HashTableEntry<js::jit::MDefinition* const>&, js::jit::MDefinition const* const&) [inlined] js::detail::HashTableEntry<js::jit::MDefinition* const>::get(this=<unavailable>, k=<unavailable>, l=<unavailable>) + 106 at HashTable.h:716, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) * frame #0: 0x000000010043ebd3 js-dbg-opt-64-dm-nsprBuild-darwin-14665b1de5ee`js::detail::HashTable<js::jit::MDefinition* const, js::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::IonAllocPolicy>::SetOps, js::jit::IonAllocPolicy>::match(js::detail::HashTableEntry<js::jit::MDefinition* const>&, js::jit::MDefinition const* const&) [inlined] js::detail::HashTableEntry<js::jit::MDefinition* const>::get(this=<unavailable>, k=<unavailable>, l=<unavailable>) + 106 at HashTable.h:716 frame #1: 0x000000010043eb69 js-dbg-opt-64-dm-nsprBuild-darwin-14665b1de5ee`js::detail::HashTable<js::jit::MDefinition* const, js::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::IonAllocPolicy>::SetOps, js::jit::IonAllocPolicy>::match(e=<unavailable>, l=<unavailable>) + 89 at HashTable.h:1215 frame #2: 0x000000010043e97c js-dbg-opt-64-dm-nsprBuild-darwin-14665b1de5ee`js::detail::HashTable<js::jit::MDefinition* const, js::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::IonAllocPolicy>::SetOps, js::jit::IonAllocPolicy>::lookup(this=0x00007fff5fbfea60, l=0x00007fff5fbfe840, keyHash=3894351730, collisionBit=1) const + 236 at HashTable.h:1265 frame #3: 0x000000010043e7d2 js-dbg-opt-64-dm-nsprBuild-darwin-14665b1de5ee`js::detail::HashTable<js::jit::MDefinition* const, js::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::IonAllocPolicy>::SetOps, js::jit::IonAllocPolicy>::lookupForAdd(this=0x00007fff5fbfea60, l=0x00007fff5fbfe840) const + 82 at HashTable.h:1557 frame #4: 0x000000010040eeb9 js-dbg-opt-64-dm-nsprBuild-darwin-14665b1de5ee`js::jit::ValueNumberer::leader(js::jit::MDefinition*) [inlined] js::HashSet<js::jit::MDefinition*, js::jit::ValueNumberer::VisibleValues::ValueHasher, js::jit::IonAllocPolicy>::lookupForAdd(l=0x000000010411da88) const + 89 at HashTable.h:372 (lldb)
Assignee | ||
Comment 2•10 years ago
|
||
There are some obscure cases where type inference isn't marking an integer add as commutative due to the presence of boolean values. Simple fixes seemed to introduce other problems, and it's not super important right now, so I'm inclined to just disable this assertion for now.
Assignee: nobody → sunfish
Attachment #8499836 -
Flags: review?(nicolas.b.pierron)
Flags: needinfo?(sunfish)
Comment 3•10 years ago
|
||
Comment on attachment 8499836 [details] [diff] [review] disable-assert.patch Review of attachment 8499836 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer if we could keep this relation in the code, as it deserve to be here, could we spew something if they are different? Maybe by using JitSpewEnabled(JitSpew_GVN), what do you think?
Attachment #8499836 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 4•10 years ago
|
||
Spewing sounds good.
Attachment #8499836 -
Attachment is obsolete: true
Attachment #8501302 -
Flags: review?(nicolas.b.pierron)
Comment 5•10 years ago
|
||
Comment on attachment 8501302 [details] [diff] [review] disable-assert.patch Review of attachment 8501302 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/ValueNumbering.cpp @@ +54,5 @@ > // does not depend on the same store, the instructions are not congruent. > if (k->dependency() != l->dependency()) > return false; > > bool congruent = k->congruentTo(l); // Ask the values themselves what they think. nit: As the symmetry does not hold, then we should read: bool congruent = k->congruentTo(l) || l->congruentTo(k); And update the condition below to report all cases.
Attachment #8501302 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #5) > Comment on attachment 8501302 [details] [diff] [review] > disable-assert.patch > > Review of attachment 8501302 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/ValueNumbering.cpp > @@ +54,5 @@ > > // does not depend on the same store, the instructions are not congruent. > > if (k->dependency() != l->dependency()) > > return false; > > > > bool congruent = k->congruentTo(l); // Ask the values themselves what they think. > > nit: As the symmetry does not hold, then we should read: > > bool congruent = k->congruentTo(l) || l->congruentTo(k); > > And update the condition below to report all cases. Given that the assertion was in mozilla-central for 2 weeks before this failure was found, and that it was found by fuzzing, asymmetry is pretty rare in practice. I'm more concerned that adding an extra congruentTo call here could slow down compile time in cases with high hash table collision rates. Checked in here accordingly, but feel free to ask me to change it if you disagree: https://hg.mozilla.org/integration/mozilla-inbound/rev/1cc7fa64a589
https://hg.mozilla.org/mozilla-central/rev/1cc7fa64a589
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•