[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
NoQ added a comment. Aha, yeah, i see. It only invalidates the current stack frame, and additionally it's impossible to bring the reference into the current stack frame by reference, because, well, it's already a reference and you can't mutate a reference. Ok then! Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris added a comment.
In https://reviews.llvm.org/D47044#1130339, @NoQ wrote:
> I'm still curious whether this also works:
>
> void foo() {
> const A = B();
> bar();
> }
>
> void bar() {
> for (int i = 0; i < 10; ++i) {}
> }
>
>
> Though we can land this patch and deal with this later.
@NoQ The above example doesn't crash.
Repository:
rC Clang
https://reviews.llvm.org/D47044
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris added a comment. Thanks @george.karpenkov . I was wondering why it didn't close automatically. Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
george.karpenkov added a comment. > This change has been committed, so I'm closing this review. @ormris The process which should be followed here is to add a line (exactly) "Differential Revision: https://reviews.llvm.org/D47044; to the bottom of your commit message, so that the phabricator can cross-reference the review and the commit. Alternatively, you could use the "arc" tool which would do that for you. Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris closed this revision. ormris added a comment. This change has been committed, so I'm closing this review. https://reviews.llvm.org/rC334554 Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris added a comment. Hmm... I'll take a look. Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
NoQ accepted this revision.
NoQ added a comment.
I'm still curious whether this also works:
void foo() {
const A = B();
bar();
}
void bar() {
for (int i = 0; i < 10; ++i) {}
}
Though we can land this patch and deal with this later.
Repository:
rC Clang
https://reviews.llvm.org/D47044
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris marked an inline comment as done. ormris added a comment. Sounds good. I'll go ahead and commit this. Thanks for the review! Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris updated this revision to Diff 151046.
ormris added a comment.
Remove unneeded header
Repository:
rC Clang
https://reviews.llvm.org/D47044
Files:
lib/StaticAnalyzer/Core/LoopWidening.cpp
test/Analysis/loop-widening-preserve-reference-type.cpp
Index: test/Analysis/loop-widening-preserve-reference-type.cpp
===
--- /dev/null
+++ test/Analysis/loop-widening-preserve-reference-type.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1
-analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4
-analyzer-config widen-loops=true -verify %s
+
+void clang_analyzer_eval(int);
+
+struct A {
+ ~A() {}
+};
+struct B : public A {};
+
+void invalid_type_region_access() {
+ const A = B();
+ for (int i = 0; i < 10; ++i) { }
+ clang_analyzer_eval( != 0); // expected-warning{{TRUE}}
+} // expected-warning@-1{{reference cannot be
bound to dereferenced null pointer in well-defined C++ code; comparison may be
assumed to always evaluate to true}}
Index: lib/StaticAnalyzer/Core/LoopWidening.cpp
===
--- lib/StaticAnalyzer/Core/LoopWidening.cpp
+++ lib/StaticAnalyzer/Core/LoopWidening.cpp
@@ -14,10 +14,16 @@
///
//===--===//
+#include "clang/AST/AST.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h"
using namespace clang;
using namespace ento;
+using namespace clang::ast_matchers;
+
+const auto MatchRef = "matchref";
/// Return the loops condition Stmt or NULL if LoopStmt is not a loop
static const Expr *getLoopCondition(const Stmt *LoopStmt) {
@@ -49,6 +55,7 @@
// TODO Nested loops are currently widened as a result of the invalidation
// being so inprecise. When the invalidation is improved, the handling
// of nested loops will also need to be improved.
+ ASTContext = LCtx->getAnalysisDeclContext()->getASTContext();
const StackFrameContext *STC = LCtx->getCurrentStackFrame();
MemRegionManager = PrevState->getStateManager().getRegionManager();
const MemRegion *Regions[] = {MRMgr.getStackLocalsRegion(STC),
@@ -60,6 +67,18 @@
RegionAndSymbolInvalidationTraits::TK_EntireMemSpace);
}
+ // References should not be invalidated.
+ auto Matches =
match(findAll(stmt(hasDescendant(varDecl(hasType(referenceType())).bind(MatchRef,
+ *LCtx->getDecl()->getBody(), ASTCtx);
+ for (BoundNodes Match : Matches) {
+const VarDecl *VD = Match.getNodeAs(MatchRef);
+assert(VD);
+const VarRegion *VarMem = MRMgr.getVarRegion(VD, LCtx);
+ITraits.setTrait(VarMem,
+ RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+ }
+
+
// 'this' pointer is not an lvalue, we should not invalidate it. If the loop
// is located in a method, constructor or destructor, the value of 'this'
// pointer shoule remain unchanged.
Index: test/Analysis/loop-widening-preserve-reference-type.cpp
===
--- /dev/null
+++ test/Analysis/loop-widening-preserve-reference-type.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
+
+void clang_analyzer_eval(int);
+
+struct A {
+ ~A() {}
+};
+struct B : public A {};
+
+void invalid_type_region_access() {
+ const A = B();
+ for (int i = 0; i < 10; ++i) { }
+ clang_analyzer_eval( != 0); // expected-warning{{TRUE}}
+} // expected-warning@-1{{reference cannot be bound to dereferenced null pointer in well-defined C++ code; comparison may be assumed to always evaluate to true}}
Index: lib/StaticAnalyzer/Core/LoopWidening.cpp
===
--- lib/StaticAnalyzer/Core/LoopWidening.cpp
+++ lib/StaticAnalyzer/Core/LoopWidening.cpp
@@ -14,10 +14,16 @@
///
//===--===//
+#include "clang/AST/AST.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h"
using namespace clang;
using namespace ento;
+using namespace clang::ast_matchers;
+
+const auto MatchRef = "matchref";
/// Return the loops condition Stmt or NULL if LoopStmt is not a loop
static const Expr *getLoopCondition(const Stmt *LoopStmt) {
@@ -49,6 +55,7 @@
// TODO Nested loops are currently widened as a result of the invalidation
// being so inprecise. When the invalidation is improved, the handling
// of nested loops will also need to be improved.
+ ASTContext =
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
george.karpenkov accepted this revision. george.karpenkov added a comment. This revision is now accepted and ready to land. Looks good to me, apart from the nit on an unused header. Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:21 #include "clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h" +#include "llvm/ADT/SmallSet.h" Seems unused now? Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris updated this revision to Diff 151043.
ormris added a comment.
- Use match function iterators rather than a callback class
- Update test
Repository:
rC Clang
https://reviews.llvm.org/D47044
Files:
lib/StaticAnalyzer/Core/LoopWidening.cpp
test/Analysis/loop-widening-preserve-reference-type.cpp
Index: test/Analysis/loop-widening-preserve-reference-type.cpp
===
--- /dev/null
+++ test/Analysis/loop-widening-preserve-reference-type.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1
-analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4
-analyzer-config widen-loops=true -verify %s
+
+void clang_analyzer_eval(int);
+
+struct A {
+ ~A() {}
+};
+struct B : public A {};
+
+void invalid_type_region_access() {
+ const A = B();
+ for (int i = 0; i < 10; ++i) { }
+ clang_analyzer_eval( != 0); // expected-warning{{TRUE}}
+} // expected-warning@-1{{reference cannot be
bound to dereferenced null pointer in well-defined C++ code; comparison may be
assumed to always evaluate to true}}
Index: lib/StaticAnalyzer/Core/LoopWidening.cpp
===
--- lib/StaticAnalyzer/Core/LoopWidening.cpp
+++ lib/StaticAnalyzer/Core/LoopWidening.cpp
@@ -14,10 +14,17 @@
///
//===--===//
+#include "clang/AST/AST.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h"
+#include "llvm/ADT/SmallSet.h"
using namespace clang;
using namespace ento;
+using namespace clang::ast_matchers;
+
+const auto MatchRef = "matchref";
/// Return the loops condition Stmt or NULL if LoopStmt is not a loop
static const Expr *getLoopCondition(const Stmt *LoopStmt) {
@@ -49,6 +56,7 @@
// TODO Nested loops are currently widened as a result of the invalidation
// being so inprecise. When the invalidation is improved, the handling
// of nested loops will also need to be improved.
+ ASTContext = LCtx->getAnalysisDeclContext()->getASTContext();
const StackFrameContext *STC = LCtx->getCurrentStackFrame();
MemRegionManager = PrevState->getStateManager().getRegionManager();
const MemRegion *Regions[] = {MRMgr.getStackLocalsRegion(STC),
@@ -60,6 +68,18 @@
RegionAndSymbolInvalidationTraits::TK_EntireMemSpace);
}
+ // References should not be invalidated.
+ auto Matches =
match(findAll(stmt(hasDescendant(varDecl(hasType(referenceType())).bind(MatchRef,
+ *LCtx->getDecl()->getBody(), ASTCtx);
+ for (BoundNodes Match : Matches) {
+const VarDecl *VD = Match.getNodeAs(MatchRef);
+assert(VD);
+const VarRegion *VarMem = MRMgr.getVarRegion(VD, LCtx);
+ITraits.setTrait(VarMem,
+ RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+ }
+
+
// 'this' pointer is not an lvalue, we should not invalidate it. If the loop
// is located in a method, constructor or destructor, the value of 'this'
// pointer shoule remain unchanged.
Index: test/Analysis/loop-widening-preserve-reference-type.cpp
===
--- /dev/null
+++ test/Analysis/loop-widening-preserve-reference-type.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
+
+void clang_analyzer_eval(int);
+
+struct A {
+ ~A() {}
+};
+struct B : public A {};
+
+void invalid_type_region_access() {
+ const A = B();
+ for (int i = 0; i < 10; ++i) { }
+ clang_analyzer_eval( != 0); // expected-warning{{TRUE}}
+} // expected-warning@-1{{reference cannot be bound to dereferenced null pointer in well-defined C++ code; comparison may be assumed to always evaluate to true}}
Index: lib/StaticAnalyzer/Core/LoopWidening.cpp
===
--- lib/StaticAnalyzer/Core/LoopWidening.cpp
+++ lib/StaticAnalyzer/Core/LoopWidening.cpp
@@ -14,10 +14,17 @@
///
//===--===//
+#include "clang/AST/AST.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h"
+#include "llvm/ADT/SmallSet.h"
using namespace clang;
using namespace ento;
+using namespace clang::ast_matchers;
+
+const auto MatchRef = "matchref";
/// Return the loops condition Stmt or NULL if LoopStmt is not a loop
static const Expr *getLoopCondition(const Stmt *LoopStmt) {
@@ -49,6 +56,7 @@
// TODO Nested loops are currently widened as a result of the invalidation
// being so inprecise. When the
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris added inline comments.
Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:89
+new Callback(LCtx, MRMgr, ITraits));
+ Finder.matchAST(ASTCtx);
+
NoQ wrote:
> ormris wrote:
> > ormris wrote:
> > > NoQ wrote:
> > > > NoQ wrote:
> > > > > george.karpenkov wrote:
> > > > > > ormris wrote:
> > > > > > > george.karpenkov wrote:
> > > > > > > > IMO using the iterator directly (e.g. like it was done in
> > > > > > > > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp#L213)
> > > > > > > > leads to a much cleaner code and saves you from having to
> > > > > > > > define a callback class.
> > > > > > > Hmm... I think that's a better approach. Let me see if I can get
> > > > > > > that working.
> > > > > > @ormris Yeah I'm really not sure why all examples use the callback
> > > > > > API by default.
> > > > > Also, please match only the local AST, i.e. the body of the function
> > > > > under analysis, which can be obtained as
> > > > > `LCtx->getDecl()->getBody()`. There's no need to scan the whole
> > > > > translation unit.
> > > > Actually not sure, would widening screw the previous stack frames as
> > > > well? We should test that, i guess. And even then, it's better to match
> > > > a few stack frames in the current stack trace than to match the whole
> > > > translation unit.
> > > I can't seem to get the new syntax to work. The following assert(0) is
> > > never triggered.
> > >
> > > ```
> > > auto Matches = match(varDecl(hasType(referenceType())).bind(MatchRef),
> > >*LCtx->getDecl()->getBody(), ASTCtx);
> > > for (BoundNodes Match : Matches) {
> > > assert(0 && "anything");
> > > const VarDecl *VD = Match.getNodeAs(MatchRef);
> > > const VarRegion *VarMem = MRMgr.getVarRegion(VD, LCtx);
> > > ITraits.setTrait(VarMem,
> > >
> > > RegionAndSymbolInvalidationTraits::TK_PreserveContents);
> > > }
> > >
> > > ```
> > It appears that "decl()" produces no matches...
> Mmm, i think when you're matching using `match` rather than `matchAST`, you
> need to write a match for the exact statement rather than any sub-statement.
> I.e., those are like "match" vs. "find". I.e., try wraping your matcher into
> `stmt(hasDescendant(...))`, where `stmt()` would match the whole function
> body (most likely a `CompoundStmt` for the curly braces around the function
> body, but there are some other cases).
Looks like that worked. Thanks!
Repository:
rC Clang
https://reviews.llvm.org/D47044
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:89
+new Callback(LCtx, MRMgr, ITraits));
+ Finder.matchAST(ASTCtx);
+
ormris wrote:
> ormris wrote:
> > NoQ wrote:
> > > NoQ wrote:
> > > > george.karpenkov wrote:
> > > > > ormris wrote:
> > > > > > george.karpenkov wrote:
> > > > > > > IMO using the iterator directly (e.g. like it was done in
> > > > > > > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp#L213)
> > > > > > > leads to a much cleaner code and saves you from having to define
> > > > > > > a callback class.
> > > > > > Hmm... I think that's a better approach. Let me see if I can get
> > > > > > that working.
> > > > > @ormris Yeah I'm really not sure why all examples use the callback
> > > > > API by default.
> > > > Also, please match only the local AST, i.e. the body of the function
> > > > under analysis, which can be obtained as `LCtx->getDecl()->getBody()`.
> > > > There's no need to scan the whole translation unit.
> > > Actually not sure, would widening screw the previous stack frames as
> > > well? We should test that, i guess. And even then, it's better to match a
> > > few stack frames in the current stack trace than to match the whole
> > > translation unit.
> > I can't seem to get the new syntax to work. The following assert(0) is
> > never triggered.
> >
> > ```
> > auto Matches = match(varDecl(hasType(referenceType())).bind(MatchRef),
> >*LCtx->getDecl()->getBody(), ASTCtx);
> > for (BoundNodes Match : Matches) {
> > assert(0 && "anything");
> > const VarDecl *VD = Match.getNodeAs(MatchRef);
> > const VarRegion *VarMem = MRMgr.getVarRegion(VD, LCtx);
> > ITraits.setTrait(VarMem,
> >
> > RegionAndSymbolInvalidationTraits::TK_PreserveContents);
> > }
> >
> > ```
> It appears that "decl()" produces no matches...
Mmm, i think when you're matching using `match` rather than `matchAST`, you
need to write a match for the exact statement rather than any sub-statement.
I.e., those are like "match" vs. "find". I.e., try wraping your matcher into
`stmt(hasDescendant(...))`, where `stmt()` would match the whole function body
(most likely a `CompoundStmt` for the curly braces around the function body,
but there are some other cases).
Repository:
rC Clang
https://reviews.llvm.org/D47044
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris added inline comments.
Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:89
+new Callback(LCtx, MRMgr, ITraits));
+ Finder.matchAST(ASTCtx);
+
ormris wrote:
> NoQ wrote:
> > NoQ wrote:
> > > george.karpenkov wrote:
> > > > ormris wrote:
> > > > > george.karpenkov wrote:
> > > > > > IMO using the iterator directly (e.g. like it was done in
> > > > > > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp#L213)
> > > > > > leads to a much cleaner code and saves you from having to define a
> > > > > > callback class.
> > > > > Hmm... I think that's a better approach. Let me see if I can get that
> > > > > working.
> > > > @ormris Yeah I'm really not sure why all examples use the callback API
> > > > by default.
> > > Also, please match only the local AST, i.e. the body of the function
> > > under analysis, which can be obtained as `LCtx->getDecl()->getBody()`.
> > > There's no need to scan the whole translation unit.
> > Actually not sure, would widening screw the previous stack frames as well?
> > We should test that, i guess. And even then, it's better to match a few
> > stack frames in the current stack trace than to match the whole translation
> > unit.
> I can't seem to get the new syntax to work. The following assert(0) is never
> triggered.
>
> ```
> auto Matches = match(varDecl(hasType(referenceType())).bind(MatchRef),
>*LCtx->getDecl()->getBody(), ASTCtx);
> for (BoundNodes Match : Matches) {
> assert(0 && "anything");
> const VarDecl *VD = Match.getNodeAs(MatchRef);
> const VarRegion *VarMem = MRMgr.getVarRegion(VD, LCtx);
> ITraits.setTrait(VarMem,
> RegionAndSymbolInvalidationTraits::TK_PreserveContents);
> }
>
> ```
It appears that "decl()" produces no matches...
Repository:
rC Clang
https://reviews.llvm.org/D47044
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris added inline comments.
Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:89
+new Callback(LCtx, MRMgr, ITraits));
+ Finder.matchAST(ASTCtx);
+
NoQ wrote:
> NoQ wrote:
> > george.karpenkov wrote:
> > > ormris wrote:
> > > > george.karpenkov wrote:
> > > > > IMO using the iterator directly (e.g. like it was done in
> > > > > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp#L213)
> > > > > leads to a much cleaner code and saves you from having to define a
> > > > > callback class.
> > > > Hmm... I think that's a better approach. Let me see if I can get that
> > > > working.
> > > @ormris Yeah I'm really not sure why all examples use the callback API by
> > > default.
> > Also, please match only the local AST, i.e. the body of the function under
> > analysis, which can be obtained as `LCtx->getDecl()->getBody()`. There's no
> > need to scan the whole translation unit.
> Actually not sure, would widening screw the previous stack frames as well? We
> should test that, i guess. And even then, it's better to match a few stack
> frames in the current stack trace than to match the whole translation unit.
I can't seem to get the new syntax to work. The following assert(0) is never
triggered.
```
auto Matches = match(varDecl(hasType(referenceType())).bind(MatchRef),
*LCtx->getDecl()->getBody(), ASTCtx);
for (BoundNodes Match : Matches) {
assert(0 && "anything");
const VarDecl *VD = Match.getNodeAs(MatchRef);
const VarRegion *VarMem = MRMgr.getVarRegion(VD, LCtx);
ITraits.setTrait(VarMem,
RegionAndSymbolInvalidationTraits::TK_PreserveContents);
}
```
Repository:
rC Clang
https://reviews.llvm.org/D47044
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:89 +new Callback(LCtx, MRMgr, ITraits)); + Finder.matchAST(ASTCtx); + NoQ wrote: > george.karpenkov wrote: > > ormris wrote: > > > george.karpenkov wrote: > > > > IMO using the iterator directly (e.g. like it was done in > > > > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp#L213) > > > > leads to a much cleaner code and saves you from having to define a > > > > callback class. > > > Hmm... I think that's a better approach. Let me see if I can get that > > > working. > > @ormris Yeah I'm really not sure why all examples use the callback API by > > default. > Also, please match only the local AST, i.e. the body of the function under > analysis, which can be obtained as `LCtx->getDecl()->getBody()`. There's no > need to scan the whole translation unit. Actually not sure, would widening screw the previous stack frames as well? We should test that, i guess. And even then, it's better to match a few stack frames in the current stack trace than to match the whole translation unit. Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
NoQ added a comment.
Thanks, this looks very reasonable!
I agree that the syntax pointed out by @george.karpenkov is much cleaner.
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h:30
ProgramStateRef getWidenedLoopState(ProgramStateRef PrevState,
+ASTContext ,
const LocationContext *LCtx,
You can obtain the `ASTContext` either from `PrevState->getStateManager()` or
from `LCtx->getAnalysisDeclContext()`, there's no need to pass it separately.
Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:46-48
+ explicit Callback(const LocationContext *LCtx_, MemRegionManager _,
+RegionAndSymbolInvalidationTraits _)
+ : LCtx(LCtx_), MRMgr(MRMgr_), ITraits(ITraits_) {}
We usually just write `X(Y y): y(y) {}` and don't bother about name collisions.
Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:89
+new Callback(LCtx, MRMgr, ITraits));
+ Finder.matchAST(ASTCtx);
+
george.karpenkov wrote:
> ormris wrote:
> > george.karpenkov wrote:
> > > IMO using the iterator directly (e.g. like it was done in
> > > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp#L213)
> > > leads to a much cleaner code and saves you from having to define a
> > > callback class.
> > Hmm... I think that's a better approach. Let me see if I can get that
> > working.
> @ormris Yeah I'm really not sure why all examples use the callback API by
> default.
Also, please match only the local AST, i.e. the body of the function under
analysis, which can be obtained as `LCtx->getDecl()->getBody()`. There's no
need to scan the whole translation unit.
Comment at: test/Analysis/loop-widening-preserve-reference-type.cpp:13
+ for (int i = 0; i < 10; ++i) { }
+ clang_analyzer_eval( == ); // expected-warning{{TRUE}}
+}
The expression is trivially true, i don't think it's exactly the thing we want
to be testing.
I'm not sure how to come up with a better test here. One good thing to test,
which i'd prefer, would be ` != 0` - which should be true because stack
objects can't have address `0` and the analyzer is supposed to know that, but
the symbolic pointer that would have overwritten `x` during over-aggressive
widening could as well be null.
Other alternatives are to add some sort of `clang_analyzer_getMemorySpace()`
and check that the variable is still on the stack (which tests more, but is
also more work) or use `clang_analyzer_dump()`/`clang_analyzer_explain()` to
verify the exact value (but that'd test too much as it'd also test the dump
format, which is redundant).
Repository:
rC Clang
https://reviews.llvm.org/D47044
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
george.karpenkov added inline comments. Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:89 +new Callback(LCtx, MRMgr, ITraits)); + Finder.matchAST(ASTCtx); + ormris wrote: > george.karpenkov wrote: > > IMO using the iterator directly (e.g. like it was done in > > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp#L213) > > leads to a much cleaner code and saves you from having to define a > > callback class. > Hmm... I think that's a better approach. Let me see if I can get that working. @ormris Yeah I'm really not sure why all examples use the callback API by default. Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris added inline comments. Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:89 +new Callback(LCtx, MRMgr, ITraits)); + Finder.matchAST(ASTCtx); + george.karpenkov wrote: > IMO using the iterator directly (e.g. like it was done in > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp#L213) > leads to a much cleaner code and saves you from having to define a callback > class. Hmm... I think that's a better approach. Let me see if I can get that working. Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
george.karpenkov added inline comments. Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:89 +new Callback(LCtx, MRMgr, ITraits)); + Finder.matchAST(ASTCtx); + IMO using the iterator directly (e.g. like it was done in https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp#L213) leads to a much cleaner code and saves you from having to define a callback class. Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris marked 2 inline comments as done. ormris added a comment. Thanks for the review @MTC! I'll wait for @NoQ's feedback. Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
MTC added a comment. LGTM, @NoQ May have further feedback. Thanks! Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris marked 3 inline comments as done.
ormris added a comment.
Thanks for the comments so far.
Comment at: test/Analysis/loop-widening-invalid-type.cpp:1
+// RUN: %clang_cc1 -analyze
-analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4
-analyzer-config widen-loops=true -verify %s
+
MTC wrote:
> I think it's better to add more expressive tests. Like:
>
> ```
> struct A {
> int x;
> A(int x) : x(x) {}
> };
>
> void invalid_type_region_access() {
> const A = A(10);
> for(int i = 0; i < 10; ++i) {}
> clang_analyzer_eval(a.x ==10); // expected-warning{{TRUE}}
> }
> ```
>
> I think should use more related names instead of
> `loop-widening-invalid-type.cpp`, like `loop-widening-reference-type`.
Agreed. Fixed.
Comment at: test/Analysis/loop-widening-invalid-type.cpp:8
+
+void invalid_type_region_access() { // expected-no-diagnostics
+ const A = B();
MTC wrote:
> I don't know what the purpose of the test is, is the comment `no-crash`
> better?
I've changed the test to (hopefully) look for a valid address for "x".
Repository:
rC Clang
https://reviews.llvm.org/D47044
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris updated this revision to Diff 150567.
ormris added a comment.
Herald added a subscriber: mikhail.ramalho.
- Reformat with clang-format-diff.py
- Rename test
- Modify test to use clang_analyzer_eval
Repository:
rC Clang
https://reviews.llvm.org/D47044
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/LoopWidening.cpp
test/Analysis/loop-widening-preserve-reference-type.cpp
Index: test/Analysis/loop-widening-preserve-reference-type.cpp
===
--- /dev/null
+++ test/Analysis/loop-widening-preserve-reference-type.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
+
+void clang_analyzer_eval(int);
+
+struct A {
+ ~A() {}
+};
+struct B : public A {};
+
+void invalid_type_region_access() {
+ const A = B();
+ for (int i = 0; i < 10; ++i) { }
+ clang_analyzer_eval( == ); // expected-warning{{TRUE}}
+}
Index: lib/StaticAnalyzer/Core/LoopWidening.cpp
===
--- lib/StaticAnalyzer/Core/LoopWidening.cpp
+++ lib/StaticAnalyzer/Core/LoopWidening.cpp
@@ -15,9 +15,15 @@
//===--===//
#include "clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h"
+#include "clang/AST/AST.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
+#include "llvm/ADT/SmallSet.h"
using namespace clang;
using namespace ento;
+using namespace clang::ast_matchers;
/// Return the loops condition Stmt or NULL if LoopStmt is not a loop
static const Expr *getLoopCondition(const Stmt *LoopStmt) {
@@ -33,10 +39,26 @@
}
}
+struct Callback : public MatchFinder::MatchCallback {
+ const LocationContext *LCtx;
+ MemRegionManager
+ RegionAndSymbolInvalidationTraits
+ explicit Callback(const LocationContext *LCtx_, MemRegionManager _,
+RegionAndSymbolInvalidationTraits _)
+ : LCtx(LCtx_), MRMgr(MRMgr_), ITraits(ITraits_) {}
+ virtual void run(const MatchFinder::MatchResult ) override {
+const VarDecl *VD = Result.Nodes.getNodeAs("match");
+const VarRegion *VarMem = MRMgr.getVarRegion(VD, LCtx);
+ITraits.setTrait(VarMem,
+ RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+ }
+};
+
namespace clang {
namespace ento {
ProgramStateRef getWidenedLoopState(ProgramStateRef PrevState,
+ASTContext ,
const LocationContext *LCtx,
unsigned BlockCount, const Stmt *LoopStmt) {
@@ -60,6 +82,12 @@
RegionAndSymbolInvalidationTraits::TK_EntireMemSpace);
}
+ // References should not be invalidated.
+ MatchFinder Finder;
+ Finder.addMatcher(varDecl(hasType(referenceType())).bind("match"),
+new Callback(LCtx, MRMgr, ITraits));
+ Finder.matchAST(ASTCtx);
+
// 'this' pointer is not an lvalue, we should not invalidate it. If the loop
// is located in a method, constructor or destructor, the value of 'this'
// pointer shoule remain unchanged.
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1854,8 +1854,8 @@
return;
// Widen.
const LocationContext *LCtx = Pred->getLocationContext();
-ProgramStateRef WidenedState =
-getWidenedLoopState(Pred->getState(), LCtx, BlockCount, Term);
+ProgramStateRef WidenedState = getWidenedLoopState(
+Pred->getState(), AMgr.getASTContext(), LCtx, BlockCount, Term);
nodeBuilder.generateNode(WidenedState, Pred);
return;
}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
@@ -27,6 +27,7 @@
/// Widen the loop by invalidating anything that might be modified
/// by the loop body in any iteration.
ProgramStateRef getWidenedLoopState(ProgramStateRef PrevState,
+ASTContext ,
const LocationContext *LCtx,
unsigned BlockCount, const Stmt *LoopStmt);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
MTC added a comment.
I didn't test the code, but the code seems correct. Thanks!
Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:88
+ MatchFinder Finder;
+ Finder.addMatcher(varDecl(hasType(referenceType())).bind("match"), new
Callback(LCtx, MRMgr, ITraits));
+ Finder.matchAST(ASTCtx);
The code should fit within 80 columns of text.
Comment at: test/Analysis/loop-widening-invalid-type.cpp:1
+// RUN: %clang_cc1 -analyze
-analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4
-analyzer-config widen-loops=true -verify %s
+
I think it's better to add more expressive tests. Like:
```
struct A {
int x;
A(int x) : x(x) {}
};
void invalid_type_region_access() {
const A = A(10);
for(int i = 0; i < 10; ++i) {}
clang_analyzer_eval(a.x ==10); // expected-warning{{TRUE}}
}
```
I think should use more related names instead of
`loop-widening-invalid-type.cpp`, like `loop-widening-reference-type`.
Comment at: test/Analysis/loop-widening-invalid-type.cpp:8
+
+void invalid_type_region_access() { // expected-no-diagnostics
+ const A = B();
I don't know what the purpose of the test is, is the comment `no-crash` better?
Repository:
rC Clang
https://reviews.llvm.org/D47044
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris updated this revision to Diff 150377.
ormris added a comment.
Use AST matchers to select references for preservation
Repository:
rC Clang
https://reviews.llvm.org/D47044
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/LoopWidening.cpp
test/Analysis/loop-widening-invalid-type.cpp
Index: test/Analysis/loop-widening-invalid-type.cpp
===
--- /dev/null
+++ test/Analysis/loop-widening-invalid-type.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
+
+struct A {
+ ~A() {}
+};
+struct B : public A {};
+
+void invalid_type_region_access() { // expected-no-diagnostics
+ const A = B();
+ for(int i = 0; i < 10; ++i) {}
+}
Index: lib/StaticAnalyzer/Core/LoopWidening.cpp
===
--- lib/StaticAnalyzer/Core/LoopWidening.cpp
+++ lib/StaticAnalyzer/Core/LoopWidening.cpp
@@ -15,9 +15,15 @@
//===--===//
#include "clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h"
+#include "clang/AST/AST.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
+#include "llvm/ADT/SmallSet.h"
using namespace clang;
using namespace ento;
+using namespace clang::ast_matchers;
/// Return the loops condition Stmt or NULL if LoopStmt is not a loop
static const Expr *getLoopCondition(const Stmt *LoopStmt) {
@@ -33,10 +39,27 @@
}
}
+struct Callback : public MatchFinder::MatchCallback {
+ const LocationContext *LCtx;
+ MemRegionManager
+ RegionAndSymbolInvalidationTraits
+ explicit Callback(const LocationContext *LCtx_,
+MemRegionManager _,
+RegionAndSymbolInvalidationTraits _) : LCtx(LCtx_),
+ MRMgr(MRMgr_),
+ ITraits(ITraits_) {}
+ virtual void run(const MatchFinder::MatchResult ) override {
+const VarDecl *VD = Result.Nodes.getNodeAs("match");
+const VarRegion *VarMem = MRMgr.getVarRegion(VD, LCtx);
+ITraits.setTrait(VarMem,
+ RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+ }
+};
+
namespace clang {
namespace ento {
-ProgramStateRef getWidenedLoopState(ProgramStateRef PrevState,
+ProgramStateRef getWidenedLoopState(ProgramStateRef PrevState, ASTContext ,
const LocationContext *LCtx,
unsigned BlockCount, const Stmt *LoopStmt) {
@@ -60,6 +83,12 @@
RegionAndSymbolInvalidationTraits::TK_EntireMemSpace);
}
+ //References should not be invalidated.
+ MatchFinder Finder;
+ Finder.addMatcher(varDecl(hasType(referenceType())).bind("match"), new Callback(LCtx, MRMgr, ITraits));
+ Finder.matchAST(ASTCtx);
+
+
// 'this' pointer is not an lvalue, we should not invalidate it. If the loop
// is located in a method, constructor or destructor, the value of 'this'
// pointer shoule remain unchanged.
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1855,7 +1855,7 @@
// Widen.
const LocationContext *LCtx = Pred->getLocationContext();
ProgramStateRef WidenedState =
-getWidenedLoopState(Pred->getState(), LCtx, BlockCount, Term);
+getWidenedLoopState(Pred->getState(), AMgr.getASTContext(), LCtx, BlockCount, Term);
nodeBuilder.generateNode(WidenedState, Pred);
return;
}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/LoopWidening.h
@@ -26,7 +26,7 @@
///
/// Widen the loop by invalidating anything that might be modified
/// by the loop body in any iteration.
-ProgramStateRef getWidenedLoopState(ProgramStateRef PrevState,
+ProgramStateRef getWidenedLoopState(ProgramStateRef PrevState, ASTContext ,
const LocationContext *LCtx,
unsigned BlockCount, const Stmt *LoopStmt);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris added a comment. Hmm... Thanks for these tips. I'll see what I can find. Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
NoQ added a comment. Hmm, well, i guess it's not going to be a one-liner. You might have to iterate through all live variables in the scope, see if they are references, and add the trait. I think currently there's no way around scanning the current function body (i.e. `LCtx->getDecl()`, where `LCtx` is `Pred->getLocationContext()`) an AST visitor or an AST matcher. Once that's done, you can take `Pred->getState()->getLValue(VD, LCtx)` for every `const VarDecl *` `VD` you find and set the invalidation trait on that. It might be necessary to restrict your search to active variables (in the sense of `LCtx->getAnalysis()->isLive(S, VD)`), where `S` is... dunno, some statement that makes sense here. Probably global/static references should also not be invalidated. It'd take even more effort to find those. I still think it's worth it because widening is indeed at fault, not the common destructor handling code. The assertion you step upon (that the `cast<>` always succeeds) is a valuable assertion that helped us find that bug, we shouldn't suppress it. Loop widening in its current form is an experiment that was never announced to work, and, well, yeah, it has this sort of bugs. There is work started by @szepet in https://reviews.llvm.org/D36690 to turn widening into a whitelist-like thing. Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.
ormris added a comment. This is my first static analyzer fix, so it may take me some time to figure this out. Repository: rC Clang https://reviews.llvm.org/D47044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
