Remove NonEquivalentDecls from ASTImporter

[martong]

Currently ASTImporter::NonEquivalentDecls member keeps its state and shares it between consecutive calls of StructuralEquivalenceContesxt::IsEquivalent().

However, it seems like NonEquivalentDecls behaves similarly to other internal states of StructuralEquivalenceContext (TentativeEquivalences, DeclsToCheck). I.e this state too should be reset before each IsEquivalent() call.

The reason: I have observed, that when we compare two entities and they happen to be unequal via redecl chain mismatch, then, from a subsequent compare we may falsely use this cached inequality value. It is extremely hard to justify this statement, this is rather a suspicion. But it seems to be synchronized with other observations related to the inner state of StructuralEquivalenceContext. See, https://github.com/Ericsson/clang/pull/383

[martong]

Note, I hope that the majority of the ODR errors will disappear from the statistics by this.

[balazske]

Is there problem with the redecl chain if a "redecl chain mismatch" occurs, or is this a normal case during import (if a chain is partially imported)? If the problem is a consequence of another bug then maybe not the removal of NonEquivalentDecls is the best fix for it but can be made optional (it fixes now the problems and if these are fixed in other way we can "reverse" the change). Are there no problems with LLDB usage? Otherwise the NonEquivalentDecls can be really removed, or make a new "structural equivalence cache" struct that contains this and maybe other similar values.

[martong]

Well, I have noticed that the ODR problems did not cease with this change, in fact we got a bit more of them on bitcoin (+10%), protobuf remained unchanged. LLDB seems to be not effected.

The reason why I created this change is to try to find the root cause of the ODR errors. But having a shared cache is creating a too huge program state, and it is almost impossible to track back to the first false positive structrural nonequivalence. So consider this PR rather an experiment, so I attached the WIP tag too.

[martong]

Update: I checked this again. And seems like there is a bunch of ODR errors in bitcoin when we use a shared NonEquivalentDecls cache. However, applying this fix, all is gone.

If we think about it, we cache inequivalent pairs from a previous snapshot of the AST. We continuously build the AST and a pair which used to be inequivalent may be equivalent later (or vica-versa).

I did not experience any slowdown by removing the shared cache. Besides, debugging is way faster if we have a local cache for each structural eq check.

So, want this PR merged. :)

[martong]

ping