[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 28 17:17:18 PST 2019


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





More information about the cfe-commits mailing list