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)

x86_64
macOS
defect
Not set
critical

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)

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)
Attached file stack
(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)
Attached patch disable-assert.patch (obsolete) — Splinter Review
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 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)
Spewing sounds good.
Attachment #8499836 - Attachment is obsolete: true
Attachment #8501302 - Flags: review?(nicolas.b.pierron)
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+
(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.