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

This revision was automatically updated to reflect the committed changes. Closed by commit rC358968: [analyzer][CrossTU] Extend CTU to VarDecls with initializer (authored by r.stahl, committed by ). Changed prior to commit: https://reviews.llvm.org/D46421?vs=196075&id=196207#toc Repository:

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

xazax.hun added a comment. Looks good, thanks. Can you commit this or do you need someone to commit it on your behalf? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D46421/new/ https://reviews.llvm.org/D46421 ___ cfe

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

r.stahl updated this revision to Diff 196075. r.stahl added a comment. @xazax.hun good point and that actually fixes a bug since that branch should also do the deep check. Added that to the tests. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D46421/new/ https://

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

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. I have one question, once it is resolved I am fine with committing this. Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:352 +return true; +} e

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

r.stahl updated this revision to Diff 194285. r.stahl marked 4 inline comments as done. r.stahl added a comment. - addressed review comments - created cross_tu::containsConst - fixed bug that unions were not exported - added TODO test for constructor case Repository: rC Clang CHANGES SINCE LA

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

r.stahl added inline comments. Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:357 + return true; +if (!RTy->hasConstFields()) + return true; xazax.hun wrote: > I wonder what would happen with types that has const fields and

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

xazax.hun added a comment. I cannot think of other users, so I would prefer to put it in the CTU lib for now. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D46421/new/ https://reviews.llvm.org/D46421 ___ cfe-commits

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

r.stahl marked 3 inline comments as done. r.stahl added a comment. In D46421#1456171 , @xazax.hun wrote: > My last set of comments are also unresolved. Most of them are minor nits, but > I would love to get rid of the code duplication between ClangExtDefM

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

xazax.hun added a comment. In D46421#1456155 , @r.stahl wrote: > Okay so I would suggest to go ahead and commit this. Rebased it succeeds > without modification. > > Still leaves the open problems with the redecls. Should I add the failing > test from ht

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

r.stahl added a comment. Okay so I would suggest to go ahead and commit this. Rebased it succeeds without modification. Still leaves the open problems with the redecls. Should I add the failing test from https://reviews.llvm.org/D46421#1375147 in the same commit marked as expected to fail? Or

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

NoQ added a comment. Herald added a subscriber: Charusso. In D46421#1382017 , @martong wrote: > > I think you should change back to getInit() > > I am not entirely sure about this because the initalizer may not be attached > to the canonical decl. `getIni

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

martong added a comment. > I think you should change back to getInit() I am not entirely sure about this because the initalizer may not be attached to the canonical decl. `getInit()` gives the initializer of one given specific Decl, however, `getAnyInitializer()` searches through the whole rede

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

NoQ added a comment. Fxd the global variable problem in D57619 , thanks! I think you should change back to `getInit()` once D57619 lands. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D46421/new/

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

martong added a comment. > Previously this was not required since all VarDecls were canonical. Not sure > if this change was intended. I did some digging, but am not familiar enough > with the code base to figure out what changed. Does anyone have an idea about > this? We changed the ASTImport

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

r.stahl added a comment. Thanks for the comments! All good points. Nice idea with the constructor, but that can probably happen in a follow up patch. In D46421#1380619 , @xazax.hun wrote: > I know you probably tired of me making you split all the patches

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

xazax.hun added a comment. Thank you for working on this! I have two concerns: - The logic when should we import the initializer of a VarDecl is duplicated. I think to have better maintainability it would be better to have only one implementation of the decision in a function and call it multi

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

xazax.hun added a comment. Herald added a project: clang. In D46421#1375147 , @r.stahl wrote: > In D46421#1374807 , @NoQ wrote: > > > At the same time, i don't have any test cases for the actual change in > > behav

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

r.stahl added a comment. In D46421#1374807 , @NoQ wrote: > 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,

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

NoQ added a comment. In 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 imp

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

r.stahl updated this revision to Diff 181263. r.stahl marked 12 inline comments as done. r.stahl added a comment. Strip name changes (see D56441 ); addressed review comments In my old version I seemed to get away with the tests, but they failed after rebasing. I

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

r.stahl added inline comments. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:120 +} +template static bool hasDefinition(const T *D) { + const T *Unused; martong wrote: > `hasDefinitionOrInit` ? I simply made it `hasBodyOrInit` now to be as clear as possible

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

NoQ added a comment. In D46421#1121080 , @r.stahl wrote: > It seems like a good idea to not do that, since non-const values are not > used. It might become useful if we ever do some kind of straight line > execution from static initialization to main. >

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

martong added a comment. Hi Rafael, This is a great patch and good improvement, thank you! I had a few minor comments. (Also, I was thinking about what else could we import in the future, maybe definitions of C++11 enum classes would be also worth to be handled by CTU.) Comm

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

xazax.hun added a comment. Sorry for the delay. The changes are looking good to me, although I think it might be worth to split this up into two patch, one NFC with the renaming, and one that actually introduces the changes. @martong could you also take a look? First, we would also like to tes

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

r.stahl added a comment. Herald added subscribers: dkrupp, donat.nagy, Szelethus, mikhail.ramalho, baloghadamsoftware. Please let me know if there is anything else that should be addressed. Note that even though this diff is quite large, most is just renaming things to stay consistent. CHANGE

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

r.stahl updated this revision to Diff 149959. r.stahl added a comment. - add missing AnalysisConsumer diff - only collect const qualified vardecls in the extdef index and adjust test https://reviews.llvm.org/D46421 Files: include/clang/Basic/DiagnosticCrossTUKinds.td include/clang/CrossTU/C

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

r.stahl added a comment. In https://reviews.llvm.org/D46421#1119098, @xazax.hun wrote: > Sorry for the limited activity. Unfortunately, I have very little time > reviewing patches lately. Thanks for getting around to it! > I think we need to answer the following questions: > > - Does this ch

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

xazax.hun added a comment. Sorry for the limited activity. Unfortunately, I have very little time reviewing patches lately. I think we need to answer the following questions: - Does this change affect the analysis of the constructors of global objects? If so, how? - Do we want to import non-con

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

r.stahl added inline comments. Comment at: include/clang/CrossTU/CrossTranslationUnit.h:114 + llvm::Expected + getCrossTUDefinition(const VarDecl *VD, StringRef CrossTUDir, + StringRef IndexName); xazax.hun wrote: > I wonder if we need the

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

r.stahl updated this revision to Diff 146789. r.stahl marked 3 inline comments as done. r.stahl edited the summary of this revision. r.stahl added a comment. Herald added subscribers: whisperity, mgorny. I looked through the original patches and found quite a few more occurrences of "function map

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

xazax.hun added inline comments. Comment at: include/clang/CrossTU/CrossTranslationUnit.h:117 - /// This function loads a function definition from an external AST - ///file. + /// \brief This function loads a definition from an external AST file. /// -

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

xazax.hun added a comment. The analyzer can only reason about const variables this way, right? Maybe we should only import the initializers for such variables to have better performance? What do you think? Also, I wonder what happens with user-defined classes. Will the analyzer evaluate the co

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

r.stahl updated this revision to Diff 146609. r.stahl marked an inline comment as done. r.stahl added a comment. - added tests - updated doc and var naming - addressed review comment https://reviews.llvm.org/D46421 Files: include/clang/CrossTU/CrossTranslationUnit.h lib/CrossTU/CrossTransla

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

a.sidorin added a comment. Hi Rafael! I like the change. Comment at: lib/CrossTU/CrossTranslationUnit.cpp:276 + ASTImporter &Importer = getOrCreateASTImporter(D->getASTContext()); + auto *ToDecl = cast(Importer.Import(const_cast(D))); + assert(HasDefinition(ToDecl)); ---

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

r.stahl added a comment. Tests and doc fixes pending. First I'm interested in your thoughts to this change. It allows to bind more symbolic values to constants if they have been defined and initialized in another TU: use.c: extern int * const p; extern struct S * const s; def.c: int *

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

r.stahl created this revision. r.stahl added reviewers: NoQ, dcoughlin, xazax.hun, george.karpenkov. Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet. The existing CTU mechanism imports FunctionDecls where the definition is available in another TU. This patch extends that to Var