NoQ added a comment.

In D46421#1354188 <https://reviews.llvm.org/D46421#1354188>, @r.stahl wrote:

> In my old version I seemed to get away with the tests, but they failed after 
> rebasing. It seems like earlier there was only a single VarDecl for the 
> imported ones with InitExpr. Now after importing there is one without init 
> and a redecl with the init. This is why I changed getInit() in RegionStore to 
> getAnyInititializer. I think these three should be enough, but I'm not sure 
> where else in the analyzer this would have to be changed.


Hmm, that is actually pretty interesting and sounds pretty bad: it seems that 
we are constructing a `VarRegion` with a non-canonical `VarDecl`. In other 
words, it turns out that `VarRegion` that is constructed for a `DeclRefExpr` 
may depend on which of the re-declarations of the variable does `DeclRefExpr` 
refer to. Which means that we potentially construct two different `VarRegion`s 
for the same variable during analysis. It should always refer to the canonical 
declaration (which should, as far as i understand, be the one that has an 
initializer on it, if any).

The problem can be easily demonstrated by adding `assert(d->isCanonicalDecl()` 
to `DeclRegion`'s constructor and running tests (a few should fail, including 
`ctu-main.c`), though i'd rather prefer the constructor to automatically 
canonicalize the declaration - which statically ensures that this sort of stuff 
doesn't happen (for a tiny performance cost). I also wonder if `DeclRefExpr` in 
a fully constructed AST should always point to the canonical declaration - 
maybe that's the thing that's broken.

At the same time, i don't have any test cases for the actual change in behavior 
that such canonicalization causes. If the test case that you had in mind is 
indeed demonstrating this problem, i'd love to have it. If it turns out that 
your test case doesn't allow us to demonstrate the problem without CTU, then 
probably it has something to do with `ASTImporter` accidentally canonicalizing 
the the declaration in `DeclRefExpr` more rarely than the vanilla AST.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D46421/new/

https://reviews.llvm.org/D46421



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to