Page MenuHomePhabricator

[ASTImporter] Remove NonEquivalentDecls from ASTImporter
Needs ReviewPublic

Authored by martong on May 20 2019, 3:17 AM.

Details

Summary

Currently ASTImporter::NonEquivalentDecls member keeps its state and
shares it between consecutive calls of
StructuralEquivalenceContesxt::IsEquivalent(). Thus, we cache
inequivalent pairs from a previous snapshot of the AST. However, we
continuously build the AST and a pair which used to be inequivalent may
be equivalent later (or vica-versa). NonEquivalentDecls behaves
similarly to other internal states of StructuralEquivalenceContext
(TentativeEquivalences, DeclsToCheck). I.e this state too should be
reset before each IsEquivalent() call.

Event Timeline

martong created this revision.May 20 2019, 3:17 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

Hi Gabor,
Could you provide an example of an import sequence leading to this behavior? It's hard for me to imagine such a situation.

clang/test/ASTMerge/struct/test.c
37

It looks strange to me that these warnings are gone.

martong marked 2 inline comments as done.Fri, Jun 28, 8:10 AM

Hi Gabor,
Could you provide an example of an import sequence leading to this behavior? It's hard for me to imagine such a situation.

Alexei, thanks for the review again. I don't think I could create an import sequence for this. However, I have experienced this kind of poisonous cache behavior during the debugging of a mysterious false positive structural inequivalency (during the analysis of protobuf).
The following happened: During the analysis we compared two Decls which turned out to be inequivalent, so we cached them. Later during the analysis, however, we added a new node to the redecl chain of one of these Decls which we previously compared. Then another structural equivalent check followed for the two Decls. And this time they should have been considered structurally equivalent, but the cache already contained them as nonequivalent. This resulted in a false positive NameConflict error.

By this change the error had gone, and we did not recognize any analysis slowdown. Remember, we still have a cache, but not a global cache in the ASTImporter object.

clang/test/ASTMerge/struct/test.c
37

For me, these warnings seems just like noise, by themselves line 37-40 does not show where is the exact mismatch.
The exact mismatch is shown in line 34-36, those warnings indicate that DeeperError has incompatible fields in the different TUs with different types (int vs float). That is the lowest level where the mismatch happens, I don't think we should indicate a warning for all other types which contain the mismatched types.

The same is true below (line 50-52) with the struct and union.

The following happened: During the analysis we compared two Decls which turned out to be inequivalent, so we cached them. Later during the analysis, however, we added a new node to the redecl chain of one of these Decls which we previously compared. Then another structural equivalent check followed for the two Decls. And this time they should have been considered structurally equivalent, but the cache already contained them as nonequivalent. This resulted in a false positive NameConflict error.

Should we reset the non-equivalence relation after a decl is imported for this decl and its redecls?

By this change the error had gone, and we did not recognize any analysis slowdown. Remember, we still have a cache, but not a global cache in the ASTImporter object.

Hm, I wonder if our cache is really useful or not. Unfortunately, I never did any measures.

Still the lack of a test disturbs me, even despite the fact that I got the idea.