[PATCH] D50363: [analyzer] pr37204: Take signedness into account in BasicValueFactory::getTruthValue().
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, baloghadamsoftware,
eraman.
I don't see why BasicValueFactory::getTruthValue(bool, QualType) always returns
an unsigned `APSInt` even if the `QualType` is signed, so i fixed it.
This was causing a couple of crashes, including pr37204.
Repository:
rC Clang
https://reviews.llvm.org/D50363
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
test/Analysis/casts.c
test/Analysis/std-c-library-functions-inlined.c
Index: test/Analysis/std-c-library-functions-inlined.c
===
--- /dev/null
+++ test/Analysis/std-c-library-functions-inlined.c
@@ -0,0 +1,17 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.StdCLibraryFunctions -verify
%s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux
-analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux
-analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux
-analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux
-analyzer-checker=unix.StdCLibraryFunctions -verify %s
+
+// This test tests crashes that occur when standard functions are available
+// for inlining.
+
+// expected-no-diagnostics
+
+int isdigit(int _) { return !0; }
+void test_redefined_isdigit(int x) {
+ int (*func)(int) = isdigit;
+ for (; func(x);) // no-crash
+;
+}
Index: test/Analysis/casts.c
===
--- test/Analysis/casts.c
+++ test/Analysis/casts.c
@@ -182,3 +182,9 @@
c += 1;
}
}
+
+void testSwitchWithSizeofs() {
+ switch (sizeof(char) == 1) { // expected-warning{{switch condition has
boolean value}}
+ case sizeof(char):; // no-crash
+ }
+}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -211,7 +211,8 @@
}
const llvm::APSInt &getTruthValue(bool b, QualType T) {
-return getValue(b ? 1 : 0, Ctx.getIntWidth(T), true);
+return T, getValue(b ? 1 : 0, Ctx.getIntWidth(T),
+ T->isUnsignedIntegerOrEnumerationType());
}
const llvm::APSInt &getTruthValue(bool b) {
Index: test/Analysis/std-c-library-functions-inlined.c
===
--- /dev/null
+++ test/Analysis/std-c-library-functions-inlined.c
@@ -0,0 +1,17 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
+
+// This test tests crashes that occur when standard functions are available
+// for inlining.
+
+// expected-no-diagnostics
+
+int isdigit(int _) { return !0; }
+void test_redefined_isdigit(int x) {
+ int (*func)(int) = isdigit;
+ for (; func(x);) // no-crash
+;
+}
Index: test/Analysis/casts.c
===
--- test/Analysis/casts.c
+++ test/Analysis/casts.c
@@ -182,3 +182,9 @@
c += 1;
}
}
+
+void testSwitchWithSizeofs() {
+ switch (sizeof(char) == 1) { // expected-warning{{switch condition has boolean value}}
+ case sizeof(char):; // no-crash
+ }
+}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -211,7 +211,8 @@
}
const llvm::APSInt &getTruthValue(bool b, QualType T) {
-return getValue(b ? 1 : 0, Ctx.getIntWidth(T), true);
+return T, getValue(b ? 1 : 0, Ctx.getIntWidth(T),
+ T->isUnsignedIntegerOrEnumerationType());
}
const llvm::APSInt &getTruthValue(bool b) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50211: [analyzer] Fix displayed checker name for InnerPointerChecker
NoQ added a comment. Welcome to the club! https://reviews.llvm.org/D50211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50363: [analyzer] pr37204: Take signedness into account in BasicValueFactory::getTruthValue().
NoQ updated this revision to Diff 159435.
NoQ added a comment.
Whoops, how did this `T, ` thing get in there.
https://reviews.llvm.org/D50363
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
test/Analysis/casts.c
test/Analysis/std-c-library-functions-inlined.c
Index: test/Analysis/std-c-library-functions-inlined.c
===
--- /dev/null
+++ test/Analysis/std-c-library-functions-inlined.c
@@ -0,0 +1,17 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.StdCLibraryFunctions -verify
%s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux
-analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux
-analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux
-analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux
-analyzer-checker=unix.StdCLibraryFunctions -verify %s
+
+// This test tests crashes that occur when standard functions are available
+// for inlining.
+
+// expected-no-diagnostics
+
+int isdigit(int _) { return !0; }
+void test_redefined_isdigit(int x) {
+ int (*func)(int) = isdigit;
+ for (; func(x);) // no-crash
+;
+}
Index: test/Analysis/casts.c
===
--- test/Analysis/casts.c
+++ test/Analysis/casts.c
@@ -182,3 +182,9 @@
c += 1;
}
}
+
+void testSwitchWithSizeofs() {
+ switch (sizeof(char) == 1) { // expected-warning{{switch condition has
boolean value}}
+ case sizeof(char):; // no-crash
+ }
+}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -211,7 +211,8 @@
}
const llvm::APSInt &getTruthValue(bool b, QualType T) {
-return getValue(b ? 1 : 0, Ctx.getIntWidth(T), true);
+return getValue(b ? 1 : 0, Ctx.getIntWidth(T),
+T->isUnsignedIntegerOrEnumerationType());
}
const llvm::APSInt &getTruthValue(bool b) {
Index: test/Analysis/std-c-library-functions-inlined.c
===
--- /dev/null
+++ test/Analysis/std-c-library-functions-inlined.c
@@ -0,0 +1,17 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
+
+// This test tests crashes that occur when standard functions are available
+// for inlining.
+
+// expected-no-diagnostics
+
+int isdigit(int _) { return !0; }
+void test_redefined_isdigit(int x) {
+ int (*func)(int) = isdigit;
+ for (; func(x);) // no-crash
+;
+}
Index: test/Analysis/casts.c
===
--- test/Analysis/casts.c
+++ test/Analysis/casts.c
@@ -182,3 +182,9 @@
c += 1;
}
}
+
+void testSwitchWithSizeofs() {
+ switch (sizeof(char) == 1) { // expected-warning{{switch condition has boolean value}}
+ case sizeof(char):; // no-crash
+ }
+}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -211,7 +211,8 @@
}
const llvm::APSInt &getTruthValue(bool b, QualType T) {
-return getValue(b ? 1 : 0, Ctx.getIntWidth(T), true);
+return getValue(b ? 1 : 0, Ctx.getIntWidth(T),
+T->isUnsignedIntegerOrEnumerationType());
}
const llvm::APSInt &getTruthValue(bool b) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50382: [analyzer] Fix a typo in `RegionStore.txt`.
NoQ accepted this revision. NoQ added a comment. Yay somebody reads those. Repository: rC Clang https://reviews.llvm.org/D50382 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49199: [analyzer][UninitializedObjectChecker] Pointer/reference objects are dereferenced according to dynamic type
NoQ accepted this revision. NoQ added a comment. This looks roughly correct, but at the same time none of the tests actually exercise the dynamic type propagation. In these tests all the necessary information is obtained from the structure of the MemRegion (directly or via the initial `StripCasts`), not from the dynamic type map that is an additional layer of metadata over the program state. The actual test would assume, as an example, chasing undefined values through a symbolic pointer produced by `operator new()` - which is a symbolic void pointer, but it points to a well-defined type of object. Because we skip symbolic pointers for now, i guess you cannot really write such tests. But at the same time chasing through //heap// symbolic pointers (i.e., pointers in the heap //memory space//) should be safe (so safe that they shouldn't really have been implemented as symbolic pointers in the first place). https://reviews.llvm.org/D49199 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Ok, let's commit this and see how to fix it later. I still think it's more important to come up with clear rules of who is responsible for initializing fields than making sure our warnings are properly grouped together. > ideally we wouldn't like to have a warning for an object (`t`) and a separate > warning for it's field (`t.bptr.x`) I don't quite understand this. How would the first warning look like? What would it warn about? Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:669-671 + Optional CurrentObject = getObjectVal(Ctor, Context); + if (!CurrentObject) +return false; All uses of `getObjectVal` so far are followed by retrieving the parent region from the `LazyCompoundVal`. Why do you need to obtain the `LazyCompoundVal` in the first place, i.e. do the second `getSVal` "for '*this'" in `getObjectVal`? Why not just operate over the this-region on the current Store? I think there isn't even a guarantee that these two regions are the same. Like, in this case they probably will be the same, but we shouldn't rely on that. https://reviews.llvm.org/D48436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50408: [analyzer] Avoid querying this-pointers for static-methods.
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yup, fair enough. https://reviews.llvm.org/D50408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50487: [CFG] [analyzer] Find argument constructors in CXXTemporaryObjectExprs.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, baloghadamsoftware.
`CXXTemporaryObjectExpr` is a sub-class of `CXXConstructExpr` that represents a
construct-expression written as `T(Arg1, ..., ArgN)`, where `N` is not equal to
1. If `N` is equal to 1, such expression is parsed as a construction conversion
from type of `Arg1` to type `T` instead. Not every temporary object is
represented by `CXXTemporaryObjectExpr` (for instance, implicit temporary
copies aren't), and `CXXTemporaryObjectExpr` doesn't necessarily represent a
temporary object (constructor written as a `CXXTemporaryObjectExpr` may
construct a pretty much arbitrary object if copy elision kicks in).
If any of `Arg1`, ..., `ArgN` requires construction, give it a construction
context of an argument. For now it only works for `CXXConstructExpr`s that
aren't `CXXTemporaryObjectExpr`s. I forgot to add it when i was adding stubs
for `CXXConstructExpr` argument constructors in
https://reviews.llvm.org/D48249. I thought i had a test for it
(`passArgumentIntoAnotherConstructor()`), but in fact in this test `N` was
equal to 1, so it wasn't really testing a `CXXTemporaryObjectExpr`.
Repository:
rC Clang
https://reviews.llvm.org/D50487
Files:
lib/Analysis/CFG.cpp
test/Analysis/cfg-rich-constructors.cpp
Index: test/Analysis/cfg-rich-constructors.cpp
===
--- test/Analysis/cfg-rich-constructors.cpp
+++ test/Analysis/cfg-rich-constructors.cpp
@@ -820,6 +820,7 @@
class E {
public:
E(D d);
+ E(D d1, D d2);
};
void useC(C c);
@@ -939,6 +940,38 @@
void passArgumentIntoAnotherConstructor() {
E e = E(D());
}
+
+
+// CHECK: void passTwoArgumentsIntoAnotherConstructor()
+// CXX11-ELIDE: 1: argument_constructors::D() (CXXConstructExpr,
[B1.2], [B1.4], [B1.5], class argument_constructors::D)
+// CXX11-NOELIDE: 1: argument_constructors::D() (CXXConstructExpr,
[B1.2], [B1.4], class argument_constructors::D)
+// CXX11-NEXT: 2: [B1.1] (BindTemporary)
+// CXX11-NEXT: 3: [B1.2] (ImplicitCastExpr, NoOp, const class
argument_constructors::D)
+// CXX11-NEXT: 4: [B1.3]
+// CXX11-NEXT: 5: [B1.4] (CXXConstructExpr, [B1.6], [B1.13]+0, class
argument_constructors::D)
+// CXX11-NEXT: 6: [B1.5] (BindTemporary)
+// CXX11-ELIDE-NEXT: 7: argument_constructors::D() (CXXConstructExpr,
[B1.8], [B1.10], [B1.11], class argument_constructors::D)
+// CXX11-NOELIDE-NEXT: 7: argument_constructors::D() (CXXConstructExpr,
[B1.8], [B1.10], class argument_constructors::D)
+// CXX11-NEXT: 8: [B1.7] (BindTemporary)
+// CXX11-NEXT: 9: [B1.8] (ImplicitCastExpr, NoOp, const class
argument_constructors::D)
+// CXX11-NEXT:10: [B1.9]
+// CXX11-NEXT:11: [B1.10] (CXXConstructExpr, [B1.12], [B1.13]+1, class
argument_constructors::D)
+// CXX11-NEXT:12: [B1.11] (BindTemporary)
+// CXX11-NEXT:13: argument_constructors::E([B1.6], [B1.12])
(CXXConstructExpr, class argument_constructors::E)
+// CXX11-NEXT:14: ~argument_constructors::D() (Temporary object destructor)
+// CXX11-NEXT:15: ~argument_constructors::D() (Temporary object destructor)
+// CXX11-NEXT:16: ~argument_constructors::D() (Temporary object destructor)
+// CXX11-NEXT:17: ~argument_constructors::D() (Temporary object destructor)
+// CXX17: 1: argument_constructors::D() (CXXConstructExpr, [B1.2],
[B1.5]+0, class argument_constructors::D)
+// CXX17-NEXT: 2: [B1.1] (BindTemporary)
+// CXX17-NEXT: 3: argument_constructors::D() (CXXConstructExpr, [B1.4],
[B1.5]+1, class argument_constructors::D)
+// CXX17-NEXT: 4: [B1.3] (BindTemporary)
+// CXX17-NEXT: 5: argument_constructors::E([B1.2], [B1.4])
(CXXConstructExpr, class argument_constructors::E)
+// CXX17-NEXT: 6: ~argument_constructors::D() (Temporary object destructor)
+// CXX17-NEXT: 7: ~argument_constructors::D() (Temporary object destructor)
+void passTwoArgumentsIntoAnotherConstructor() {
+ E(D(), D());
+}
} // end namespace argument_constructors
namespace copy_elision_with_extra_arguments {
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -4352,6 +4352,11 @@
CFGBlock *CFGBuilder::VisitCXXTemporaryObjectExpr(CXXTemporaryObjectExpr *C,
AddStmtChoice asc) {
+ // If the constructor takes objects as arguments by value, we need to
properly
+ // construct these objects. Construction contexts we find here aren't for the
+ // constructor C, they're for its arguments only.
+ findConstructionContextsForArguments(C);
+
autoCreateBlock();
appendConstructor(Block, C);
return VisitChildren(C);
Index: test/Analysis/cfg-rich-constructors.cpp
==
[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like keys
NoQ added a comment.
Hmm, this might make a good `optin.*` checker. Also i see that this is a pure
AST-based checker; did you consider putting this into `clang-tidy`? Like, you
shouldn't if you're not using `clang-tidy` yourself (it's a bit messy) but this
is an option. Also we're actively using `ASTMatchers` in Analyzer checkers
anyway these days because they're just shorter and easier to read and generally
cool.
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:91-100
+ const QualType IterTy = CE->getArg(0)->getType();
+ const RecordDecl *RD =
+IterTy->getUnqualifiedDesugaredType()->getAsCXXRecordDecl();
+
+ if (RD->field_empty())
+return;
+
This heuristic ("the argument of `std::sort` is //a class whose first field is
of pointer type//") is quite wonky, do you have a plan on how to make it more
precise?
Repository:
rC Clang
https://reviews.llvm.org/D50488
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like keys
NoQ added a comment. > I'm only a beginner, but here are some things that caught my eye. I really > like the idea! :) Thanks, i appreciate help with reviews greatly. Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:93 + const RecordDecl *RD = +IterTy->getUnqualifiedDesugaredType()->getAsCXXRecordDecl(); + Szelethus wrote: > Is there a reason for not directly acquiring the record declaration? I suspect it's about typedefs. We usually use `getCanonicalType()` for this purpose. Comment at: test/Analysis/ptr-sort.cpp:1 +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.nondeterminism.PointerSorting %s -analyzer-output=text -verify + Szelethus wrote: > Always run the core checkers too. > `-analyzer-checker=core,alpha.nondeterminism.PointerSorting` This is a good discipline, but it's not very useful in case of AST-based checkers because they don't interact at all with each other. Repository: rC Clang https://reviews.llvm.org/D50488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49570: [analyzer] Improve warning messages and notes of InnerPointerChecker
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. I think this is way easier to understand :) https://reviews.llvm.org/D49570 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50594: [analyzer] [NFC] Introduce separate targets for testing the analyzer: check-clang-analyzer and check-clang-analyzer-z3
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yup, i hope it'll be comfy now. https://reviews.llvm.org/D50594 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32747: [Analyzer] Iterator Checker - Part 3: Invalidation check, first for (copy) assignments
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Herald added a subscriber: Szelethus. Looks great, let's land this! https://reviews.llvm.org/D32747 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32845: [Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function parameters
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Herald added a subscriber: Szelethus. Looks good. I guess we may have to tone down the heuristic about "all template functions" if we see it fail. @a.sidorin and @whisperity have some valid minor comments. https://reviews.llvm.org/D32845 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like keys
NoQ added a comment. I guess one of the things the analyzer could find with path-sensitive analysis is direct comparison of non-aliasing pointers. Not only this is non-deterministic, but there's a related problem that comparison for equality would always yield false and is therefore useless. However, there will be many false negatives because most of the time it's hard to figure out if pointers alias by looking at a small part of the program (a call stack of at most 3-4 functions in the middle of nowhere), as the analyzer does. Repository: rC Clang https://reviews.llvm.org/D50488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32859: [Analyzer] Iterator Checker - Part 5: Move Assignment of Containers
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Herald added a subscriber: Szelethus. I think this patch is in good shape. In https://reviews.llvm.org/D32859#1187551, @baloghadamsoftware wrote: > I do not see which lines exactly you commented This was about the `replaceSymbol()` sub. You can traverse history using the History tab, or if there's still a greyed out comment you can push the |<< button on it to jump to the respective historical diff. In any case, i think in current form `replaceSymbol()` is much easier to understand, but still deserves a short comment, maybe a small example, maybe a better name (the word "rebase" comes to mind). https://reviews.llvm.org/D32859 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32906: [Analyzer] Iterator Checker - Part 10: Support for iterators passed as parameter
NoQ requested changes to this revision. NoQ added a comment. This revision now requires changes to proceed. Herald added subscribers: Szelethus, mikhail.ramalho. Let's see if this is still necessary after https://reviews.llvm.org/D49443. Iterators will be constructed directly into the argument region, so i suspect that the manual copy is no longer necessary. https://reviews.llvm.org/D32906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Herald added a subscriber: Szelethus. This looks great, thanks, this is exactly how i imagined it! Repository: rC Clang https://reviews.llvm.org/D48027 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49722: [CStringSyntaxChecker] Check strlcat sizeof check
NoQ added inline comments.
Comment at: cfe/trunk/test/Analysis/cstring-syntax.c:45
+ strlcpy(dest, "012345678", sizeof(dest));
+ strlcat(dest, "910", sizeof(dest)); // expected-warning {{The third argument
allows to potentially copy more bytes than it should. Replace with the value
- strlen(dest) - 1 or lower}}
+ strlcpy(dest, "0123456789", sizeof(dest));
There seem to be two spaces around ``.
Repository:
rL LLVM
https://reviews.llvm.org/D49722
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50824: [CFG] [analyzer] pr37769: Disable argument construction contexts for variadic functions.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, Szelethus, mikhail.ramalho,
baloghadamsoftware.
The analyzer doesn't need them and they seem to have pretty weird AST from time
to time, so let's just skip them for now.
Fixes the assertion failure in https://bugs.llvm.org/show_bug.cgi?id=38427.
Repository:
rC Clang
https://reviews.llvm.org/D50824
Files:
lib/Analysis/CFG.cpp
test/Analysis/cfg-rich-constructors.cpp
Index: test/Analysis/cfg-rich-constructors.cpp
===
--- test/Analysis/cfg-rich-constructors.cpp
+++ test/Analysis/cfg-rich-constructors.cpp
@@ -1028,3 +1028,18 @@
C(1) + C(2);
}
} // namespace operators
+
+namespace variadic_function_arguments {
+class C {
+ public:
+ C(int);
+};
+
+int variadic(...);
+
+// This code is quite exotic, so let's not test the CFG for it,
+// but only make sure we don't crash.
+void testCrashOnVariadicArgument() {
+ C c(variadic(0 ? c : 0)); // no-crash
+}
+} // namespace variadic_function_arguments
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -2421,8 +2421,6 @@
if (!boundType.isNull()) calleeType = boundType;
}
- findConstructionContextsForArguments(C);
-
// If this is a call to a no-return function, this stops the block here.
bool NoReturn = getFunctionExtInfo(*calleeType).getNoReturn();
@@ -2439,6 +2437,13 @@
bool OmitArguments = false;
if (FunctionDecl *FD = C->getDirectCallee()) {
+// TODO: Support construction contexts for variadic function arguments.
+// These are a bit problematic and not very useful because passing
+// C++ objects as C-style variadic arguments doesn't work in general
+// (see [expr.call]).
+if (!FD->isVariadic())
+ findConstructionContextsForArguments(C);
+
if (FD->isNoReturn() || C->isBuiltinAssumeFalse(*Context))
NoReturn = true;
if (FD->hasAttr())
Index: test/Analysis/cfg-rich-constructors.cpp
===
--- test/Analysis/cfg-rich-constructors.cpp
+++ test/Analysis/cfg-rich-constructors.cpp
@@ -1028,3 +1028,18 @@
C(1) + C(2);
}
} // namespace operators
+
+namespace variadic_function_arguments {
+class C {
+ public:
+ C(int);
+};
+
+int variadic(...);
+
+// This code is quite exotic, so let's not test the CFG for it,
+// but only make sure we don't crash.
+void testCrashOnVariadicArgument() {
+ C c(variadic(0 ? c : 0)); // no-crash
+}
+} // namespace variadic_function_arguments
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -2421,8 +2421,6 @@
if (!boundType.isNull()) calleeType = boundType;
}
- findConstructionContextsForArguments(C);
-
// If this is a call to a no-return function, this stops the block here.
bool NoReturn = getFunctionExtInfo(*calleeType).getNoReturn();
@@ -2439,6 +2437,13 @@
bool OmitArguments = false;
if (FunctionDecl *FD = C->getDirectCallee()) {
+// TODO: Support construction contexts for variadic function arguments.
+// These are a bit problematic and not very useful because passing
+// C++ objects as C-style variadic arguments doesn't work in general
+// (see [expr.call]).
+if (!FD->isVariadic())
+ findConstructionContextsForArguments(C);
+
if (FD->isNoReturn() || C->isBuiltinAssumeFalse(*Context))
NoReturn = true;
if (FD->hasAttr())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50855: [analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, Szelethus, mikhail.ramalho,
baloghadamsoftware.
Despite the effort to eliminate the need for
`skipRValueSubobjectAdjustments()`, we still encounter ASTs that require it
from time to time, for example in https://bugs.llvm.org/show_bug.cgi?id=37578
One way of properly modeling such expressions would be to include the
member-expression "adjustment" into the //construction context// of the
temporary, but that's not something i'm planning to do, because such ASTs are
rare and seem to be only becoming more rare over time, so for now i'm just
fixing the old code.
The root cause of the problem in this example is that while evaluating the
`MemberExpr` in
`-MemberExpr 0x7fd5ef0035b8 'a::(anonymous struct at
test.cpp:2:3)' . 0x7fd5ee06a068
`-CXXTemporaryObjectExpr 0x7fd5ef001f70 'a' 'void ()
noexcept' zeroing
there's no way for `createTemporaryRegionIfNeeded()` to communicate the newly
created temporary region through the Environment (as it usually does), because
all expressions so far have been prvalues.
The current code works around that problem by binding the region to the
`CXXTemporaryObjectExpr`, which is of course a bad thing to do because we
should not bind `Loc`s to prvalue expressions, and it leads to a crash when
eventually this bad binding propagates to the Store and the Store is unable to
load it.
The solution is to bind the correct [lazy compound] value to the
`CXXTemporaryObjectExpr` and then communicate the region to the caller directly
via an out-parameter.
Repository:
rC Clang
https://reviews.llvm.org/D50855
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/temporaries.cpp
Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -1152,3 +1152,23 @@
// and the non-definition decl should be found by direct lookup.
void T::foo(C) {}
} // namespace argument_virtual_decl_lookup
+
+namespace union_indirect_field_crash {
+union U {
+ struct {
+int x;
+ };
+};
+
+template class C {
+public:
+ void foo() const {
+(void)(true ? U().x : 0);
+ }
+};
+
+void test() {
+ C c;
+ c.foo();
+}
+} // namespace union_indirect_field_crash
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -283,11 +283,10 @@
return state;
}
-ProgramStateRef
-ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
- const LocationContext *LC,
- const Expr *InitWithAdjustments,
- const Expr *Result) {
+ProgramStateRef ExprEngine::createTemporaryRegionIfNeeded(
+ProgramStateRef State, const LocationContext *LC,
+const Expr *InitWithAdjustments, const Expr *Result,
+const SubRegion **OutRegionWithAdjustments) {
// FIXME: This function is a hack that works around the quirky AST
// we're often having with respect to C++ temporaries. If only we modelled
// the actual execution order of statements properly in the CFG,
@@ -297,8 +296,11 @@
if (!Result) {
// If we don't have an explicit result expression, we're in "if needed"
// mode. Only create a region if the current value is a NonLoc.
-if (!InitValWithAdjustments.getAs())
+if (!InitValWithAdjustments.getAs()) {
+ if (OutRegionWithAdjustments)
+*OutRegionWithAdjustments = nullptr;
return State;
+}
Result = InitWithAdjustments;
} else {
// We need to create a region no matter what. For sanity, make sure we don't
@@ -418,11 +420,16 @@
// The result expression would now point to the correct sub-region of the
// newly created temporary region. Do this last in order to getSVal of Init
// correctly in case (Result == Init).
- State = State->BindExpr(Result, LC, Reg);
+ if (Result->isGLValue())
+State = State->BindExpr(Result, LC, Reg);
+ else
+State = State->BindExpr(Result, LC, InitValWithAdjustments);
// Notify checkers once for two bindLoc()s.
State = processRegionChange(State, TR, LC);
+ if (OutRegionWithAdjustments)
+*OutRegionWithAdjustments = cast(Reg.getAsRegion());
return State;
}
@@ -2533,8 +2540,11 @@
}
// Handle regular struct fields / member variables.
- state = createTemporaryRegionIfNeeded(state, LCtx, BaseExpr);
- SVal baseExprVal = state->getSVal(BaseExpr, LCtx);
+ const SubRegion *MR;
+ state =
+ createTemporaryRegionIfNeeded(state, LCtx, BaseExpr, nullptr, &MR);
+ SVal baseExprVal =
+ MR ? loc::MemRegionVal(MR) : state->
[PATCH] D50855: [analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
NoQ updated this revision to Diff 161073.
NoQ added a comment.
Add comments for optional arguments.
The reason why i chose an out-parameter rather than returning a std::pair is
because most callers //presumably// don't need this feature.
https://reviews.llvm.org/D50855
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/temporaries.cpp
Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -1152,3 +1152,23 @@
// and the non-definition decl should be found by direct lookup.
void T::foo(C) {}
} // namespace argument_virtual_decl_lookup
+
+namespace union_indirect_field_crash {
+union U {
+ struct {
+int x;
+ };
+};
+
+template class C {
+public:
+ void foo() const {
+(void)(true ? U().x : 0);
+ }
+};
+
+void test() {
+ C c;
+ c.foo();
+}
+} // namespace union_indirect_field_crash
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -283,11 +283,10 @@
return state;
}
-ProgramStateRef
-ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
- const LocationContext *LC,
- const Expr *InitWithAdjustments,
- const Expr *Result) {
+ProgramStateRef ExprEngine::createTemporaryRegionIfNeeded(
+ProgramStateRef State, const LocationContext *LC,
+const Expr *InitWithAdjustments, const Expr *Result,
+const SubRegion **OutRegionWithAdjustments) {
// FIXME: This function is a hack that works around the quirky AST
// we're often having with respect to C++ temporaries. If only we modelled
// the actual execution order of statements properly in the CFG,
@@ -297,8 +296,11 @@
if (!Result) {
// If we don't have an explicit result expression, we're in "if needed"
// mode. Only create a region if the current value is a NonLoc.
-if (!InitValWithAdjustments.getAs())
+if (!InitValWithAdjustments.getAs()) {
+ if (OutRegionWithAdjustments)
+*OutRegionWithAdjustments = nullptr;
return State;
+}
Result = InitWithAdjustments;
} else {
// We need to create a region no matter what. For sanity, make sure we don't
@@ -418,11 +420,16 @@
// The result expression would now point to the correct sub-region of the
// newly created temporary region. Do this last in order to getSVal of Init
// correctly in case (Result == Init).
- State = State->BindExpr(Result, LC, Reg);
+ if (Result->isGLValue())
+State = State->BindExpr(Result, LC, Reg);
+ else
+State = State->BindExpr(Result, LC, InitValWithAdjustments);
// Notify checkers once for two bindLoc()s.
State = processRegionChange(State, TR, LC);
+ if (OutRegionWithAdjustments)
+*OutRegionWithAdjustments = cast(Reg.getAsRegion());
return State;
}
@@ -2533,8 +2540,12 @@
}
// Handle regular struct fields / member variables.
- state = createTemporaryRegionIfNeeded(state, LCtx, BaseExpr);
- SVal baseExprVal = state->getSVal(BaseExpr, LCtx);
+ const SubRegion *MR;
+ state = createTemporaryRegionIfNeeded(state, LCtx, BaseExpr,
+/*Result=*/nullptr,
+/*OutRegionWithAdjustments=*/&MR);
+ SVal baseExprVal =
+ MR ? loc::MemRegionVal(MR) : state->getSVal(BaseExpr, LCtx);
const auto *field = cast(Member);
SVal L = state->getLValue(field, baseExprVal);
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -734,10 +734,14 @@
///
/// If \p Result is provided, the new region will be bound to this expression
/// instead of \p InitWithAdjustments.
- ProgramStateRef createTemporaryRegionIfNeeded(ProgramStateRef State,
-const LocationContext *LC,
-const Expr *InitWithAdjustments,
-const Expr *Result = nullptr);
+ ///
+ /// Returns the temporary region with adjustments into the optional
+ /// OutRegionWithAdjustments out-parameter if a new region was indeed needed,
+ /// otherwise sets it to nullptr.
+ ProgramStateRef createTemporaryRegionIfNeeded(
+ ProgramStateRef State, const LocationContext *LC,
+ const Expr *InitWithAdjustments, const Expr *Result = nullptr,
+ const SubRegion **OutRegionWithAdjustments = nullptr);
/// Returns a region representing the first element
[PATCH] D50855: [analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
NoQ added a comment. > which is of course a bad thing to do because we should not bind `Loc`s to > `prvalue` expressions ... of non-pointer type. On the other hand, binding `NonLoc`s to glvalue expressions is always a bad thing to do; but, for the reference, adding this as an assertion currently crashes 92 tests. https://reviews.llvm.org/D50855 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50855: [analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
NoQ added a comment. ...and adding the aforementioned assertion for prvalues increases the number of crashes on tests to 196. It seems that the analyzer assigns values of improper loc-less very often, but then immediately overwrites them :/ I wonder how much performance could be gained by fixing these bugs. https://reviews.llvm.org/D50855 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50866: [analyzer] CFRetainReleaseChecker: Avoid checking C++ methods with the same name.
NoQ created this revision.
NoQ added reviewers: dcoughlin, george.karpenkov.
Herald added subscribers: cfe-commits, Szelethus, mikhail.ramalho, a.sidorin,
szepet, baloghadamsoftware, xazax.hun.
CFRetainReleaseChecker is a tiny checker that verifies that arguments of
CoreFoundation retain/release functions are non-null. The checker accidentally
checks all functions with the respective name, not just the actual `CFRetain`
etc, which has caused a crash.
Fix it and modernize the checker to use CallEvent/CallDescription APIs to avoid
further mistakes.
rdar://problem/42433152
Repository:
rC Clang
https://reviews.llvm.org/D50866
Files:
lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
test/Analysis/retain-release.mm
Index: test/Analysis/retain-release.mm
===
--- test/Analysis/retain-release.mm
+++ test/Analysis/retain-release.mm
@@ -470,3 +470,18 @@
void rdar33832412() {
void* x = IOBSDNameMatching(); // no-warning
}
+
+
+namespace member_CFRetains {
+class Foo {
+public:
+ void CFRetain(const Foo &) {}
+ void CFRetain(int) {}
+};
+
+void bar() {
+ Foo foo;
+ foo.CFRetain(foo); // no-warning
+ foo.CFRetain(0); // no-warning
+}
+}
Index: lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -36,6 +36,7 @@
using namespace clang;
using namespace ento;
+using namespace llvm;
namespace {
class APIMisuse : public BugType {
@@ -531,93 +532,59 @@
//===--===//
namespace {
-class CFRetainReleaseChecker : public Checker< check::PreStmt > {
- mutable std::unique_ptr BT;
- mutable IdentifierInfo *Retain, *Release, *MakeCollectable, *Autorelease;
+class CFRetainReleaseChecker : public Checker {
+ mutable APIMisuse BT{this, "null passed to CF memory management function"};
+ CallDescription CFRetain{"CFRetain", 1},
+ CFRelease{"CFRelease", 1},
+ CFMakeCollectable{"CFMakeCollectable", 1},
+ CFAutorelease{"CFAutorelease", 1};
public:
- CFRetainReleaseChecker()
- : Retain(nullptr), Release(nullptr), MakeCollectable(nullptr),
-Autorelease(nullptr) {}
- void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
};
} // end anonymous namespace
-void CFRetainReleaseChecker::checkPreStmt(const CallExpr *CE,
+void CFRetainReleaseChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
- // If the CallExpr doesn't have exactly 1 argument just give up checking.
- if (CE->getNumArgs() != 1)
+ // TODO: Make this check part of CallDescription.
+ if (!Call.isGlobalCFunction())
return;
- ProgramStateRef state = C.getState();
- const FunctionDecl *FD = C.getCalleeDecl(CE);
- if (!FD)
-return;
-
- if (!BT) {
-ASTContext &Ctx = C.getASTContext();
-Retain = &Ctx.Idents.get("CFRetain");
-Release = &Ctx.Idents.get("CFRelease");
-MakeCollectable = &Ctx.Idents.get("CFMakeCollectable");
-Autorelease = &Ctx.Idents.get("CFAutorelease");
-BT.reset(new APIMisuse(
-this, "null passed to CF memory management function"));
- }
-
// Check if we called CFRetain/CFRelease/CFMakeCollectable/CFAutorelease.
- const IdentifierInfo *FuncII = FD->getIdentifier();
- if (!(FuncII == Retain || FuncII == Release || FuncII == MakeCollectable ||
-FuncII == Autorelease))
+ if (!(Call.isCalled(CFRetain) || Call.isCalled(CFRelease) ||
+Call.isCalled(CFMakeCollectable) || Call.isCalled(CFAutorelease)))
return;
- // FIXME: The rest of this just checks that the argument is non-null.
- // It should probably be refactored and combined with NonNullParamChecker.
-
// Get the argument's value.
- const Expr *Arg = CE->getArg(0);
- SVal ArgVal = C.getSVal(Arg);
+ SVal ArgVal = Call.getArgSVal(0);
Optional DefArgVal = ArgVal.getAs();
if (!DefArgVal)
return;
- // Get a NULL value.
- SValBuilder &svalBuilder = C.getSValBuilder();
- DefinedSVal zero =
- svalBuilder.makeZeroVal(Arg->getType()).castAs();
-
- // Make an expression asserting that they're equal.
- DefinedOrUnknownSVal ArgIsNull = svalBuilder.evalEQ(state, zero, *DefArgVal);
-
- // Are they equal?
- ProgramStateRef stateTrue, stateFalse;
- std::tie(stateTrue, stateFalse) = state->assume(ArgIsNull);
+ // Is it null?
+ ProgramStateRef state = C.getState();
+ ProgramStateRef stateNonNull, stateNull;
+ std::tie(stateNonNull, stateNull) = state->assume(*DefArgVal);
- if (stateTrue && !stateFalse) {
-ExplodedNode *N = C.generateErrorNode(stateTrue);
+ if (!stateNonNull) {
+ExplodedNode *N = C.generateErrorNode(stateNull);
if
[PATCH] D32902: [Analyzer] Iterator Checker - Part 7: Support for push and pop operations
NoQ added inline comments.
Herald added subscribers: Szelethus, mikhail.ramalho.
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1477-1530
+bool isPushBackCall(const FunctionDecl *Func) {
+ const auto *IdInfo = Func->getIdentifier();
+ if (!IdInfo)
+return false;
+ if (Func->getNumParams() != 1)
+return false;
+ return IdInfo->getName() == "push_back";
I guess we should think if we want to use `CallDescription` for these when
D48027 lands.
https://reviews.llvm.org/D32902
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50747: [analyzer] Drop support for GC mode in RetainCountChecker
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yay. Repository: rC Clang https://reviews.llvm.org/D50747 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
NoQ added inline comments.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:187-191
// If FR is a pointer pointing to a non-primitive type.
if (Optional RecordV =
DerefdV.getAs()) {
const TypedValueRegion *R = RecordV->getRegion();
This looks like one more situation where we dereference a location to get a
value and then struggle to get back to the location that we've dereferenced by
looking at the value. Can we just use `V`?
Comment at: test/Analysis/cxx-uninitialized-object-inheritance.cpp:787
// TODO: we'd expect the note: {{uninitialized field 'this->x'}}
int x; // no-note
};
Szelethus wrote:
> The checker should be able to catch this one -- for some reason it is
> regarded as an unknown region. Odd, as the test case right after this one
> works perfectly.
There's a variety of problems we have with empty base classes, might be one of
those, and they are usually easy to fix because, well, yes it's a special case,
but it's also an extremely simple case.
I encourage you to open up the Exploded Graph and study it carefully to see
what and where goes wrong (not for this revision).
Repository:
rC Clang
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50904: [analyzer][UninitializedObjectChecker] Added documentation to the checker list
NoQ accepted this revision. NoQ added inline comments. This revision is now accepted and ready to land. Comment at: www/analyzer/alpha_checks.html:334-336 +The checker regards inherited fields as direct fields, so one +will recieve warnings for uninitialized inherited data members +as well. Szelethus wrote: > This statement has been debated for a while, so I'm all for discussing it, as > I feel I've gained a lot more knowledge about this subject since it was last > mentioned. The documentation should still reflect the //actual// state of things, right? https://reviews.llvm.org/D50904 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50905: [analyzer][UninitializedObjectChecker][WIP] Explicit namespace resolution for inherited data members
NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp:114-115
+ virtual void printNode(llvm::raw_ostream &Out) const override {
+Out << StringRef(BaseClassT.getAsString()).ltrim("struct ").ltrim("class ")
+<< "::";
+ }
Maybe just `BaseClassT->getAsCXXRecordDecl()->getName()`?
Repository:
rC Clang
https://reviews.llvm.org/D50905
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50509: [analyzer][UninitializedObjectChecker] Refactoring p6.: Move dereferencing to a function
NoQ added inline comments.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:223
+ // int*).
+ while (auto Tmp = V.getAs()) {
+// We can't reason about symbolic regions, assume its initialized.
Hmm, i still have concerns about things like `int *x = (int *)&x;`. Why not
just check the type to terminate the loop? Type hierarchy is guaranteed to be
finite.
https://reviews.llvm.org/D50509
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50509: [analyzer][UninitializedObjectChecker] Refactoring p6.: Move dereferencing to a function
NoQ accepted this revision. NoQ added a comment. Which shouldn't prevent us from moving code around. https://reviews.llvm.org/D50509 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50509: [analyzer][UninitializedObjectChecker] Refactoring p6.: Move dereferencing to a function
NoQ added inline comments.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:223
+ // int*).
+ while (auto Tmp = V.getAs()) {
+// We can't reason about symbolic regions, assume its initialized.
Szelethus wrote:
> NoQ wrote:
> > Hmm, i still have concerns about things like `int *x = (int *)&x;`. Why not
> > just check the type to terminate the loop? Type hierarchy is guaranteed to
> > be finite.
> There actually is a testcase for that -- it would create a
> nonloc::LocAsInteger, not a loc::MemRegionVal.
>
> I'll add a TODO to revisit this loop condition (again :) ).
Ok, let's try with one more asterisk:
```
1 void test() {
2 int **x = (int **)&x;
3 int *y = *x;
4 int z = *y;
5 }
```
Here's what i get in the Store:
```
(x,0,direct) : &element{x,0 S64b,int *}
(y,0,direct) : &element{x,0 S64b,int *}
(z,0,direct) : &element{x,0 S64b,int *}
```
https://reviews.llvm.org/D50509
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50866: [analyzer] CFRetainReleaseChecker: Avoid checking C++ methods with the same name.
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp:537
+ mutable APIMisuse BT{this, "null passed to CF memory management function"};
+ CallDescription CFRetain{"CFRetain", 1},
+ CFRelease{"CFRelease", 1},
george.karpenkov wrote:
> I personally would prefer being less fancy, and avoiding the comma operator,
> but I suppose it's a matter of style.
This isn't comma operator, just initializer list.
Alternatives are:
- `CallDescription CFRetain = {"CFRetain", 1}` (longer but looks the same)
- `CallDescription CFRetain = CallDescription("CFRetain", 1), ...` (longer and
duplicates type)
- `CallDescription CFRetain;` `CFRetainReleaseChecker(): CFRetain("CFRetain",
1)` (longer and duplicates variable name)
Comment at: lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp:567
+ ProgramStateRef stateNonNull, stateNull;
+ std::tie(stateNonNull, stateNull) = state->assume(*DefArgVal);
george.karpenkov wrote:
> There's also DefArgVal
?
Repository:
rC Clang
https://reviews.llvm.org/D50866
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49811: [analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor
NoQ added a comment. Herald added a subscriber: Szelethus. In https://reviews.llvm.org/D49811#1175927, @NoQ wrote: > Devin has recently pointed out that we might have as well reordered CFG > elements to have return statement kick in after automatic destructors, so > that callbacks were called in a different order. I don't see any problems > with that solution, but let's stick to the current solution for now, because > who knows. In https://bugs.llvm.org/show_bug.cgi?id=11645#c9 Ted wrote: > If we treat an occurrence of "return" in the CFG is meaning "bind an > expression result for the return value" and not as a transfer of control then > it is is fine for the destructors to appear after the "return". From this > view, the transfer back to the caller is when we hit the Exit block. Another possible solution is to check in the destructor if the newly dangling pointer is already bound to the return statement. Repository: rC Clang https://reviews.llvm.org/D49811 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51057: [analyzer][UninitializedObjectChecker] Fixed dereferencing
NoQ added a comment.
Yup, this looks great and that's exactly how i imagined it.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h:261-265
+inline bool isLocType(const QualType &T) {
+ return T->isAnyPointerType() || T->isReferenceType() ||
+ T->isBlockPointerType();
+}
+
We have a fancy static `Loc::isLocType()`.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:126-127
if (V.isUndef()) {
+assert(!FR->getDecl()->getType()->isReferenceType() &&
+ "References must be initialized!");
return addFieldToUninits(
Good catch.
It might still be possible to initialize a reference with an already-undefined
pointer if core checkers are turned off, but we don't support turning them off,
so i guess it's fine.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:177
- if (isPrimitiveUninit(V)) {
+ const SVal &PointeeV = State->getSVal(loc::MemRegionVal(R));
+ if (isPrimitiveUninit(PointeeV)) {
Just `SVal`. And you should be able to pass `R` directly into `getSVal`.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:210
assert(V.getAs() && "V must be loc::MemRegionVal!");
+ auto Tmp = V.getAs();
+
We usually just do `.getAsRegion()`.
And then later `dyn_cast` it.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:212-216
+ // We can't reason about symbolic regions, assume its initialized.
+ // Note that this also avoids a potential infinite recursion, because
+ // constructors for list-like classes are checked without being called, and
+ // the Static Analyzer will construct a symbolic region for Node *next; or
+ // similar code snippets.
Ok, i have a new concern that i didn't think about before.
There's nothing special about symbolic regions. You can make a linked list
entirely of concrete regions:
```
struct List {
List *next;
List(): next(this) {}
};
void foo() {
List list;
}
```
In this case the finite-ness of the type system won't save us.
I guess we could also memoize base regions that we visit :/ This is guaranteed
to terminate because all of our concrete regions are based either on AST nodes
(eg. global variables) or on certain events that happened previously during
analysis (eg. local variables or temporaries, as they depend on the stack frame
which must have been previously entered). I don't immediately see a better
solution.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:223
+ const auto *R = Tmp->getRegionAs();
+ assert(R);
+
We might have eliminated symbolic regions but we may still have concrete
function pointers (which are `TypedRegion`s that aren't `TypedValueRegion`s,
it's pretty weird), and i guess we might even encounter an `AllocaRegion`
(which is completely untyped). I guess we should not try to dereference them
either.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:240-244
+if (Tmp->getRegion()->getSymbolicBase())
return None;
-}
-V = State->getSVal(*Tmp, DynT);
+DynT = DynT->getPointeeType();
+R = Tmp->getRegionAs();
This code seems to be duplicated with the "0th iteration" before the loop. I
guess you can put everything into the loop.
Repository:
rC Clang
https://reviews.llvm.org/D51057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees
NoQ added inline comments.
Comment at:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:187-191
// If FR is a pointer pointing to a non-primitive type.
if (Optional RecordV =
DerefdV.getAs()) {
const TypedValueRegion *R = RecordV->getRegion();
Szelethus wrote:
> NoQ wrote:
> > This looks like one more situation where we dereference a location to get a
> > value and then struggle to get back to the location that we've dereferenced
> > by looking at the value. Can we just use `V`?
> I've struggled with derefencing for months now -- I'm afraid I just don't
> really get what you'd like to see here.
>
> Here's what I attempted to implement:
> I'd like to obtain the pointee's region of a `Loc` region, even if it has to
> be casted to another type, like through void pointers and
> `nonloc::LocAsInteger`, and continue analysis on said region as usual.
>
> The trickiest part I can't seem to get right is the acquisition of the
> pointee region. For the problem this patch attempts to solve, even though
> `DynT` correctly says that the dynamic type is `DynTDerived2 *`, `DerefdV`
> contains a region for `DynTBase`.
>
> I uploaded a new patch, D51057, which hopefully settles derefence related
> issues. Please note that it **does not **replace this diff, as the acquired
> region is still of type `DynTBase`.
>
> I find understanding these intricate details of the analyzer very difficult,
> as I found very little documentation about how this works, which often left
> me guessing what the proper way to do this is. Can you recommend some
> literature for me on this field?
> Can you recommend some literature for me on this field?
This is pretty specific to our analyzer. `SVal`/`SymExpr`/`MemRegion` hierarchy
is tightly coupled to implementation details of the `RegionStore`, which is our
memory model. There's a paper on it [1]. We have some in-tree documentation of
the `RegionStore` [2] (other docs there are also interesting to read). And
there's my old workbook [3]. And i guess that's it.
[1] Xu, Zhongxing & Kremenek, Ted & Zhang, Jian. (2010). A Memory Model for
Static Analysis of C Programs. 535-548. 10.1007/978-3-642-16558-0_44.
[2]
https://github.com/llvm-mirror/clang/blob/master/docs/analyzer/RegionStore.txt
[3]
https://github.com/haoNoQ/clang-analyzer-guide/releases/download/v0.1/clang-analyzer-guide-v0.1.pdf
Repository:
rC Clang
https://reviews.llvm.org/D50892
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:419
+ case CK_LValueBitCast:
+ case CK_FixedPointCast: {
state =
a.sidorin wrote:
> Should we consider this construction as unsupported rather than supported as
> a normal cast?
Uhm, this code seems to be held together by magic. We squeeze all sorts of
casts (eg., float casts) into a subroutine that deals with casts of //lvalues//
(!?) Fortunately, it dissolves into `SValBuilder::evalCast()` pretty quickly,
so we don't really get punished for that. So it's not this patch's fault but
our technical debt. I guess this change on its own doesn't make things worse,
so i'm ok with it.
Repository:
rC Clang
https://reviews.llvm.org/D50616
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.
NoQ added a comment.
In https://reviews.llvm.org/D48027#1207721, @rnkovacs wrote:
> I guess it is highly unlikely for someone to write namespaces like that, and
> if they do, I think they deserve to have them treated as a `std::string`.
Yup. On the other hand, if we specify our method as `{"basic_string", "c_str"}`
and they make a `namespace basic_string { const char *c_str(); }`, we are
pretty likely to //crash// when we try to obtain this-value for the call that
isn't a method. This is still not a big deal because it's still highly
unlikely, but we've seen crash reports of this sort, and the easier our spec
is, the more likely it becomes that somebody has a different thing with the
same name. For example, if we want to model iterators and we specify
`{"begin"}` without specifying the base class (so that all sorts of containers
were covered), we still want to specify that we're looking for a method call
and not for a global function call.
So i believe that one of the important remaining problems with
`CallDescription` is to teach it to discriminate between global functions and
methods. We can do it in a number of ways:
1. Make a special sub-class for CallDescription depending on the sort of the
function (too verbose),
2. Tag all items in the list as "record" vs. "namespace" (too many tags),
3. Dunno, tons of other boring and verbose approaches,
4. Change our PreCall/PostCall callbacks to callback templates that let allow
the user subscribe to specific sub-classes of `CallEvent` similarly to how he
can subscribe to different kinds of statements in
PreStmt/PostStmt, and then the user doesn't need to write any
code to check it dynamically.
Repository:
rC Clang
https://reviews.llvm.org/D48027
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46187: [Analyzer] getRegisteredCheckers(): handle debug checkers too.
NoQ added a comment. In https://reviews.llvm.org/D46187#1089087, @lebedev.ri wrote: > > I believe we could also benefit from a method of extracting the analyzer's > > `clang -cc1` run-line from clang-tidy. This would allow arbitrary debugging > > over a single analyzer invocation. > > I'm not really following, sorry. In https://reviews.llvm.org/D46187#1091309, @alexfh wrote: > In https://reviews.llvm.org/D46187#1088722, @NoQ wrote: > > > I believe we could also benefit from a method of extracting the analyzer's > > `clang -cc1` run-line from clang-tidy. > > > It should be possible now using `clang-tidy -extra-arg=-v some/file.cpp` (or > `clang-check -extra-arg=-v ...`). Nice! So essentially @lebedev.ri could do `clang-check -analyze -extra-arg=-v ../llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp`, then copy the printed `clang Invocation:` and append `-analyzer-checker debug.ViewCallGraph` to it. Or maybe even something as horrible as clang-check -analyze -extra-arg=-Xclang -extra-arg=-analyzer-checker -extra-arg=-Xclang -extra-arg=debug.ViewCallGraph ../llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp And it sounds like the roughly right level of verbosity for discriminating between a user-facing feature and a definitely-not-user-facing feature. I don't mind having a separate flag specifically for enabling debug checkers, but when it comes to actual debugging of the analyzer, debug checkers are usually not sufficient. Playing with other flags and `-analyzer-config` options is very often necessary, so if i was to debug the analyzer from clang-tidy, most of the time i'd have preferred to do the `-v` trick. Repository: rC Clang https://reviews.llvm.org/D46187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.
NoQ added a comment.
Thank you! Looks good overall.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1013-1014
+
+ // Get the offset and the base region to figure out whether the offset of
+ // buffer is 0.
+ RegionOffset Offset = MR->getAsOffset();
Please say something here (or above) about why do we want our offset to be 0:
> We're about to model memset by producing a "default binding" in the Store.
> Our current implementation - RegionStore - doesn't support default bindings
> that don't cover the whole base region.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1037
+
+if (StateWholeReg && !StateNotWholeReg && CharVal.isZeroConstant()) {
+ // If the 'memset()' acts on the whole region of destination buffer and
I think we should use `StateNonNullChar` (that's computed below) instead of
`CharVal.isZeroConstant()` because the Environment doesn't provide a guarantee
that all symbols within it are collapsed to constants where applicable.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1055
+std::tie(StateNullChar, StateNonNullChar) =
+assumeZero(C, State, CharVal, Ctx.UnsignedCharTy);
+
I think this should use `IntTy` here. Because that's the type of the `memset`'s
argument, and that's what `assumeZero()` expects.
Comment at: test/Analysis/string.c:1412
+ clang_analyzer_eval(strlen(str) >= 10); // expected-warning{{TRUE}}
+ // FIXME: This shoule be TRUE.
+ clang_analyzer_eval(str[1] == '0'); // expected-warning{{UNKNOWN}}
Typo: `should` :)
Repository:
rC Clang
https://reviews.llvm.org/D44934
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45177: CStringChecker, check strlcpy/strlcat
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Thanks! I'll commit again. https://reviews.llvm.org/D45177 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46823: [analyzer] const init: handle non-explicit cases more accurately
NoQ added a comment.
Yay thanks!
I think some cornercases would need to be dealt with.
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1650
+
+// If there is a list, but no init, it must be zero.
+if (i >= InitList->getNumInits())
NoQ wrote:
> Would this work correctly if the element is not of an integral or enumeration
> type? I think this needs an explicit check.
What if we have an out-of-bounds access to a variable-length array? I don't
think it'd yield zero.
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1650-1652
+// If there is a list, but no init, it must be zero.
+if (i >= InitList->getNumInits())
+ return svalBuilder.makeZeroVal(R->getElementType());
Would this work correctly if the element is not of an integral or enumeration
type? I think this needs an explicit check.
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1733
+ } else {
+return svalBuilder.makeZeroVal(Ty);
+ }
Same: would this work correctly if the field is not of an integral or
enumeration type?
Comment at: test/Analysis/initialization.c:3
+
+void clang_analyzer_dump(int);
We try to avoid using `dump()` on tests because it makes tests test the dump
syntax, which isn't the point.
For checking constants, it's easier to do something like
`clang_analyzer_eval(parr[i] == 2); // expected-warning{{TRUE}}`.
For finding undefined values, you can enable `core.uninitialized` checkers and
receive warnings when the argument of `clang_analyzer_eval` is an uninitialized
value. Or just increment the value.
Repository:
rC Clang
https://reviews.llvm.org/D46823
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46902: [analyzer] Make plist-html multi-file.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, baloghadamsoftware.
Plist diagnostics support multi-file reports since forever. HTML diagnostics
support multi-file reports since
https://reviews.llvm.org/D30406/https://reviews.llvm.org/rC309968.
plist-html is the diagnostic output mode that produces both html and plist
files. It's mostly useful for testing the analyzer itself because plist output
is helpful for comparing results produced by different versions of the analyzer
via the `utils/analyzer/CmpRuns.py` and html output allows you to immediately
have a look at the changed reports.
Previously plist-html output produced multi-file HTML reports but only
single-file Plist reports.
Change plist-html output to produce multi-file Plist reports as well.
I don't think we should care about backwards compatibility here because not
only the old mode made no sense but also it doesn't make sense to use
plist-html in any user-facing UI anyway.
Repository:
rC Clang
https://reviews.llvm.org/D46902
Files:
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
test/Analysis/diagnostics/plist-multi-file.c
test/Analysis/diagnostics/plist-multi-file.h
Index: test/Analysis/diagnostics/plist-multi-file.h
===
--- /dev/null
+++ test/Analysis/diagnostics/plist-multi-file.h
@@ -0,0 +1,3 @@
+void foo(int *ptr) {
+ *ptr = 1; // expected-warning{{Dereference of null pointer (loaded from variable 'ptr')}}
+}
Index: test/Analysis/diagnostics/plist-multi-file.c
===
--- /dev/null
+++ test/Analysis/diagnostics/plist-multi-file.c
@@ -0,0 +1,205 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=plist-html -o %t.plist -verify %s
+// RUN: FileCheck --input-file=%t.plist %s
+
+#include "plist-multi-file.h"
+
+void bar() {
+ foo(0);
+}
+
+// CHECK: diagnostics
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: path
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: kindevent
+// CHECK-NEXT: location
+// CHECK-NEXT:
+// CHECK-NEXT: line7
+// CHECK-NEXT: col7
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT: ranges
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line7
+// CHECK-NEXT: col7
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line7
+// CHECK-NEXT: col7
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: depth0
+// CHECK-NEXT: extended_message
+// CHECK-NEXT: Passing null pointer value via 1st parameter 'ptr'
+// CHECK-NEXT: message
+// CHECK-NEXT: Passing null pointer value via 1st parameter 'ptr'
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: kindevent
+// CHECK-NEXT: location
+// CHECK-NEXT:
+// CHECK-NEXT: line7
+// CHECK-NEXT: col3
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT: ranges
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line7
+// CHECK-NEXT: col3
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line7
+// CHECK-NEXT: col8
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: depth0
+// CHECK-NEXT: extended_message
+// CHECK-NEXT: Calling 'foo'
+// CHECK-NEXT: message
+// CHECK-NEXT: Calling 'foo'
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: kindevent
+// CHECK-NEXT: location
+// CHECK-NEXT:
+// CHECK-NEXT: line1
+// CHECK-NEXT: col1
+// CHECK-NEXT: file1
+// CHECK-NEXT:
+// CHECK-NEXT: depth1
+// CHECK-NEXT: extended_message
+// CHECK-NEXT: Entered call from 'bar'
+// CHECK-NEXT: message
+// CHECK-NEXT: Entered call from 'bar'
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: kindcontrol
+// CHECK-NEXT: edges
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:start
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line1
+// CHECK-NEXT: col1
+// CHECK-NEXT: file1
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line1
+// CHECK-NEXT: col4
+// CHECK-NEXT: file1
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:end
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line2
+// CHECK-NEXT: col3
+// CHECK-NEXT: file1
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line2
+// CHECK-NEXT: col3
+// CHECK-NEXT: file1
+// CHECK-NEXT:
+// CHECK-NEXT:
+// C
[PATCH] D46902: [analyzer] Make plist-html multi-file.
NoQ added inline comments.
Comment at: test/Analysis/diagnostics/plist-multi-file.c:202
+// CHECK-NEXT:
+// CHECK-NEXT: report-{{([0-9a-f]{6})}}.html
+// CHECK-NEXT:
Yay we have regular expressions.
`()` are needed so that not to confuse `}``}}` with `}}``}`.
Repository:
rC Clang
https://reviews.llvm.org/D46902
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Thanks! This looks great now. Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1055 +std::tie(StateNullChar, StateNonNullChar) = +assumeZero(C, State, CharVal, Ctx.UnsignedCharTy); + MTC wrote: > NoQ wrote: > > I think this should use `IntTy` here. Because that's the type of the > > `memset`'s argument, and that's what `assumeZero()` expects. > I confirmed again that `memset()` will convert the value to `unsigned char` > first, see http://en.cppreference.com/w/c/string/byte/memset. > > In the next update, I will `evalCast(value, UnsignedCharTy, IntTy)` first, > therefore, I will continue to use `UnsignedCharTy`. Aha, yup, it does convert to `unsigned char`, but `assumeZero()` doesn't. The new code looks correct. Repository: rC Clang https://reviews.llvm.org/D44934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46891: [StaticAnalyzer] Added a getLValue method to ProgramState for bases
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Looks good, thanks! Yeah, seems that these were accidentally omitted. You may want to add the `const CXXRecordDecl *` variant as well. Repository: rC Clang https://reviews.llvm.org/D46891 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46891: [StaticAnalyzer] Added a getLValue method to ProgramState for bases
NoQ added inline comments.
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:732
+ const SubRegion *Super) const {
+ const auto Base = BaseSpec.getType()->getAsCXXRecordDecl();
+ return loc::MemRegionVal(
I think you wanted to say `const auto *` here. It's not really important that
the local variable is const, but that the pointer points to const.
Repository:
rC Clang
https://reviews.llvm.org/D46891
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45177: CStringChecker, check strlcpy/strlcat
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1560-1566
// If the size is known to be zero, we're done.
if (StateZeroSize && !StateNonZeroSize) {
StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, DstVal);
C.addTransition(StateZeroSize);
return;
}
One more cornercase where the return value needs to be corrected. It'd be great
to de-duplicate this code to avoid similar problems in the future.
Test case:
```
int foo(char *dst, const char *src) {
return strlcpy(dst, src, 0); // no-crash
}
```
Repository:
rC Clang
https://reviews.llvm.org/D45177
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: Ensure that we only visit a destructor for a reference if type information is available.
NoQ added a comment. Mmm, i think loop widening simply shouldn't invalidate references (though it should invalidate objects bound to them). Simply because you can't really reassign a reference. Could we mark them as "preserve contents", like in https://reviews.llvm.org/D45491? 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] D47007: [Sanitizer] CStringChecker fix for strlcpy when no bytes are copied to the dest buffer
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
I've been thinking if we could de-duplicate this whole set of branches that
computes the return value so that we didn't have to fix every bug twice. Maybe
move it to an auxiliary function.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1795
// copied element, or a pointer to the start of the destination buffer.
Result = (returnEnd ? UnknownVal() : DstVal);
} else {
Do we need to consider `returnEnd` on the short path as well?
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1869-1875
if (returnPtr) {
// If this is a stpcpy-style copy, but we were unable to check for a buffer
// overflow, we still need a result. Conjure a return value.
if (returnEnd && Result.isUnknown()) {
Result = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
}
}
Do we need to do that on the short path as well?
Repository:
rC Clang
https://reviews.llvm.org/D47007
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47044: Ensure that we only visit a destructor for a reference if type information is available.
NoQ added a comment. Hopefully. 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] D44238: [CFG] Fix automatic destructors when a member is bound to a reference.
NoQ updated this revision to Diff 147635.
NoQ added a comment.
Switch to `skipRValueSubobjectAdjustments`. Yup, that's much better.
Add FIXME tests for lifetime extension through non-references that are still
not working - that correspond to a nearby FIXME in the code. I'm not planning
to fix them immediately, but it's nice to know what else isn't working in this
realm.
https://reviews.llvm.org/D44238
Files:
lib/Analysis/CFG.cpp
test/Analysis/auto-obj-dtors-cfg-output.cpp
Index: test/Analysis/auto-obj-dtors-cfg-output.cpp
===
--- test/Analysis/auto-obj-dtors-cfg-output.cpp
+++ test/Analysis/auto-obj-dtors-cfg-output.cpp
@@ -1,7 +1,11 @@
-// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=false %s > %t 2>&1
-// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,WARNINGS %s
-// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=true %s > %t 2>&1
-// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,ANALYZER %s
+// RUN: %clang_analyze_cc1 -std=c++98 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=false %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX98,WARNINGS,CXX98-WARNINGS %s
+// RUN: %clang_analyze_cc1 -std=c++98 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=true %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX98,ANALYZER,CXX98-ANALYZER %s
+// RUN: %clang_analyze_cc1 -std=c++11 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=false %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX11,WARNINGS,CXX11-WARNINGS %s
+// RUN: %clang_analyze_cc1 -std=c++11 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=true %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX11,ANALYZER,CXX11-ANALYZER %s
// This file tests how we construct two different flavors of the Clang CFG -
// the CFG used by the Sema analysis-based warnings and the CFG used by the
@@ -14,6 +18,8 @@
class A {
public:
+ int x;
+
// CHECK: [B1 (ENTRY)]
// CHECK-NEXT: Succs (1): B0
// CHECK: [B0 (EXIT)]
@@ -70,6 +76,287 @@
// CHECK: [B2 (ENTRY)]
// CHECK-NEXT: Succs (1): B1
// CHECK: [B1]
+// WARNINGS-NEXT: 1: A() (CXXConstructExpr, class A)
+// CXX98-ANALYZER-NEXT: 1: A() (CXXConstructExpr, [B1.2], class A)
+// CXX11-ANALYZER-NEXT: 1: A() (CXXConstructExpr, [B1.2], [B1.3], class A)
+// CHECK-NEXT: 2: [B1.1] (BindTemporary)
+// CXX98-NEXT: 3: [B1.2].x
+// CXX98-NEXT: 4: [B1.3]
+// CXX98-NEXT: 5: const int &x = A().x;
+// CXX98-NEXT: 6: [B1.5].~A() (Implicit destructor)
+// CXX11-NEXT: 3: [B1.2]
+// CXX11-NEXT: 4: [B1.3].x
+// CXX11-NEXT: 5: [B1.4] (ImplicitCastExpr, NoOp, const int)
+// CXX11-NEXT: 6: const int &x = A().x;
+// CXX11-NEXT: 7: [B1.6].~A() (Implicit destructor)
+// CHECK-NEXT: Preds (1): B2
+// CHECK-NEXT: Succs (1): B0
+// CHECK: [B0 (EXIT)]
+// CHECK-NEXT: Preds (1): B1
+void test_const_ref_to_field() {
+ const int &x = A().x;
+}
+
+// CHECK:[B2 (ENTRY)]
+// CHECK-NEXT: Succs (1): B1
+// CHECK:[B1]
+// WARNINGS-NEXT: 1: A() (CXXConstructExpr, class A)
+// CXX98-ANALYZER-NEXT: 1: A() (CXXConstructExpr, [B1.2], class A)
+// CXX11-ANALYZER-NEXT: 1: A() (CXXConstructExpr, [B1.2], [B1.3], class A)
+// CHECK-NEXT: 2: [B1.1] (BindTemporary)
+// CXX98-NEXT: 3: A::x
+// CXX98-NEXT: 4: &[B1.3]
+// CXX98-NEXT: 5: [B1.2] .* [B1.4]
+// CXX98-NEXT: 6: [B1.5]
+// CXX98-NEXT: 7: const int &x = A() .* &A::x;
+// CXX98-NEXT: 8: [B1.7].~A() (Implicit destructor)
+// CXX11-NEXT: 3: [B1.2]
+// CXX11-NEXT: 4: A::x
+// CXX11-NEXT: 5: &[B1.4]
+// CXX11-NEXT: 6: [B1.3] .* [B1.5]
+// CXX11-NEXT: 7: [B1.6] (ImplicitCastExpr, NoOp, const int)
+// CXX11-NEXT: 8: const int &x = A() .* &A::x;
+// CXX11-NEXT: 9: [B1.8].~A() (Implicit destructor)
+// CHECK-NEXT: Preds (1): B2
+// CHECK-NEXT: Succs (1): B0
+// CHECK:[B0 (EXIT)]
+// CHECK-NEXT: Preds (1): B1
+void test_pointer_to_member() {
+ const int &x = A().*&A::x;
+}
+
+// FIXME: There should be automatic destructors at the end of scope.
+// CHECK:[B2 (ENTRY)]
+// CHECK-NEXT: Succs (1): B1
+// CHECK:[B1]
+// WARNINGS-NEXT: 1: A() (CXXConstructExpr, class A)
+// ANALYZER-NEXT: 1: A() (CXXConstructExpr, [B1.2], [B1.4], class A)
+// CHECK-NEXT: 2: [B1.1] (BindTemporary)
+// CHECK-NEXT: 3: [B1.2] (ImplicitCastExpr, NoOp, const class A)
+// CHECK-NEXT: 4: [B1.3]
+// CHECK-NEXT: 5: {[B1.4]}
+// CHECK-NEXT: 6: B b = {A()};
+// WARNINGS-NEXT: 7: A() (CXXConstructExpr,
[PATCH] D47007: [analyzer] CStringChecker fix for strlcpy when no bytes are copied to the dest buffer
NoQ added a comment. @devnexen I think you should request commit access. You're already an active contributor :) Repository: rC Clang https://reviews.llvm.org/D47007 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44239: [analyzer] Re-enable constructor inlining when lifetime extension through fields occurs.
NoQ updated this revision to Diff 147637.
NoQ added a comment.
Herald added a subscriber: baloghadamsoftware.
Rebase.
https://reviews.llvm.org/D44239
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/lifetime-extension.cpp
Index: test/Analysis/lifetime-extension.cpp
===
--- test/Analysis/lifetime-extension.cpp
+++ test/Analysis/lifetime-extension.cpp
@@ -46,10 +46,18 @@
const int &y = A().j[1]; // no-crash
const int &z = (A().j[1], A().j[0]); // no-crash
- // FIXME: All of these should be TRUE, but constructors aren't inlined.
- clang_analyzer_eval(x == 1); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(y == 3); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(z == 2); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(x == 1);
+ clang_analyzer_eval(y == 3);
+ clang_analyzer_eval(z == 2);
+#ifdef TEMPORARIES
+ // expected-warning@-4{{TRUE}}
+ // expected-warning@-4{{TRUE}}
+ // expected-warning@-4{{TRUE}}
+#else
+ // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-8{{UNKNOWN}}
+#endif
}
} // end namespace pr19539_crash_on_destroying_an_integer
@@ -144,8 +152,12 @@
{
const bool &x = C(true, &after, &before).x; // no-crash
}
- // FIXME: Should be TRUE. Should not warn about garbage value.
- clang_analyzer_eval(after == before); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(after == before);
+#ifdef TEMPORARIES
+ // expected-warning@-2{{TRUE}}
+#else
+ // expected-warning@-4{{UNKNOWN}}
+#endif
}
struct A { // A is an aggregate.
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -695,12 +695,7 @@
if (CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion)
return CIP_DisallowedOnce;
- // If the temporary is lifetime-extended by binding a smaller object
- // within it to a reference, automatic destructors don't work properly.
- if (CallOpts.IsTemporaryLifetimeExtendedViaSubobject)
-return CIP_DisallowedOnce;
-
- // If the temporary is lifetime-extended by binding it to a reference-typ
+ // If the temporary is lifetime-extended by binding it to a reference-type
// field within an aggregate, automatic destructors don't work properly.
if (CallOpts.IsTemporaryLifetimeExtendedViaAggregate)
return CIP_DisallowedOnce;
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -182,22 +182,13 @@
const auto *TOCC = cast(CC);
if (const auto *MTE = TOCC->getMaterializedTemporaryExpr()) {
if (const ValueDecl *VD = MTE->getExtendingDecl()) {
- // Pattern-match various forms of lifetime extension that aren't
- // currently supported by the CFG.
- // FIXME: Is there a better way to retrieve this information from
- // the MaterializeTemporaryExpr?
assert(MTE->getStorageDuration() != SD_FullExpression);
- if (VD->getType()->isReferenceType()) {
-assert(VD->getType()->isReferenceType());
-if (VD->getType()->getPointeeType().getCanonicalType() !=
-MTE->GetTemporaryExpr()->getType().getCanonicalType()) {
- // We're lifetime-extended via our field. Automatic destructors
- // aren't quite working in this case.
- CallOpts.IsTemporaryLifetimeExtendedViaSubobject = true;
-}
- } else {
+ if (!VD->getType()->isReferenceType()) {
// We're lifetime-extended by a surrounding aggregate.
-// Automatic destructors aren't quite working in this case.
+// Automatic destructors aren't quite working in this case
+// on the CFG side. We should warn the caller about that.
+// FIXME: Is there a better way to retrieve this information from
+// the MaterializeTemporaryExpr?
CallOpts.IsTemporaryLifetimeExtendedViaAggregate = true;
}
}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -106,11 +106,6 @@
bool IsTemporaryCtorOrDtor = false;
/// This call is a constructor for a temporary that is lifetime-extended
-/// by binding a smaller object within it to a reference, for example
-/// 'const int &x = C().x;'.
-bool Is
[PATCH] D47135: [analyzer][WIP] A checker for dangling string pointers in C++
NoQ added a comment.
This looks great, i think we should make a single super simple mock test and
commit this.
@MTC, i really appreciate your help!
Comment at: lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp:59
+ QualType RegType = TypedR->getValueType();
+ if (RegType.getAsString() != "std::string")
+return;
MTC wrote:
> A little tip, there are other string types besides `std::string`, like
> `std::wstring`, which can be added in the future. See [[
> http://en.cppreference.com/w/cpp/string/basic_string | basic_string ]].
Yup, the current check should work in many cases, but it should be better to
compare the class name to `basic_string`.
I'm also worried about various sorts of `std::__1::string`s (an inline
namespace in libc++).
Comment at: lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp:64
+
+ if (Call.isCalled(CStrFn)) {
+SymbolRef RawPtr = Call.getReturnValue().getAsSymbol();
It has long been planned to extend `isCalled` to C++ methods, i.e. something
like this:
```
DanglingStringPointerChecker() : CStrFn("std::string::c_str") {}
...
void DanglingStringPointerChecker::checkPostCall(const CallEvent &Call,
CheckerContext &C) const {
// Skip the class check.
if (Call.isCalled(CStrFn)) {
...
}
...
}
```
Still looking for volunteers^^
Comment at: lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp:71
+
+ if (dyn_cast(ICall)) {
+if (State->contains(TypedR)) {
MTC wrote:
> `CXXDestructorCall::classof(const CallEvent *CA)` can also be used here.
`isa`.
Comment at: lib/StaticAnalyzer/Checkers/RegionState.h:18
+
+namespace region_state {
+
MTC wrote:
> I'm not sure whether `region_state` follows the naming conventions of LLVM
> coding standards. Can someone answer this question?
I think it does, cf. `namespace ast_matchers`.
The name should be more specific though, a lot of checkers track "region
states". Maybe "allocation_state"?
Repository:
rC Clang
https://reviews.llvm.org/D47135
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47155: [analyzer] Reduce simplifySVal complexity threshold further.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, baloghadamsoftware.
Reported by Mikael Holmén on
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20180416/225349.html -
a combination of https://reviews.llvm.org/rC329780 and
https://reviews.llvm.org/rC300178 caused a performance regression that seemed
to be a hang on the attached test case.
Reducing even further from 20 to 10 gives a ~15% further speed boost on the
test, but i don't think it's worth it because such code is not common and
therefore accuracy is more important.
Repository:
rC Clang
https://reviews.llvm.org/D47155
Files:
lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
test/Analysis/hangs.c
Index: test/Analysis/hangs.c
===
--- /dev/null
+++ test/Analysis/hangs.c
@@ -0,0 +1,16 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker core -verify %s
+
+// expected-no-diagnostics
+
+// Stuff that used to hang.
+
+int g();
+
+int f(int y) {
+ return y + g();
+}
+
+int produce_a_very_large_symbol(int x) {
+ return f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(
+ f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(x;
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1274,7 +1274,7 @@
SVal VisitNonLocSymbolVal(nonloc::SymbolVal V) {
// Simplification is much more costly than computing complexity.
// For high complexity, it may be not worth it.
- if (V.getSymbol()->computeComplexity() > 100)
+ if (V.getSymbol()->computeComplexity() > 20)
return V;
return Visit(V.getSymbol());
}
Index: test/Analysis/hangs.c
===
--- /dev/null
+++ test/Analysis/hangs.c
@@ -0,0 +1,16 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker core -verify %s
+
+// expected-no-diagnostics
+
+// Stuff that used to hang.
+
+int g();
+
+int f(int y) {
+ return y + g();
+}
+
+int produce_a_very_large_symbol(int x) {
+ return f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(
+ f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(x;
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1274,7 +1274,7 @@
SVal VisitNonLocSymbolVal(nonloc::SymbolVal V) {
// Simplification is much more costly than computing complexity.
// For high complexity, it may be not worth it.
- if (V.getSymbol()->computeComplexity() > 100)
+ if (V.getSymbol()->computeComplexity() > 20)
return V;
return Visit(V.getSymbol());
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47155: [analyzer] Reduce simplifySVal complexity threshold further.
NoQ added a reviewer: mikhail.ramalho. NoQ added a subscriber: mikhail.ramalho. NoQ added a comment. @mikhail.ramalho Does it solve your problems with ffmpeg as well? :) Repository: rC Clang https://reviews.llvm.org/D47155 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47135: [analyzer][WIP] A checker for dangling string pointers in C++
NoQ added a comment. We'll have to track `string_view` ourselves, not relying on `MallocChecker`. So we only need an `AF_` for the pointer case. `DanglingInternalBufferChecker` and `AF_InternalBuffer` sound great to me. Repository: rC Clang https://reviews.llvm.org/D47135 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47303: [analyzer] NFC: Merge object construction related state traits into one.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, baloghadamsoftware.
As noticed in http://lists.llvm.org/pipermail/cfe-dev/2018-May/058055.html we
need a path-sensitive program state trait that for, essentially, every
constructed object maintains the region of that object until the statement that
consumes the object is encountered.
We already have such trait for new-expressions and bind/materialize temporary
expressions, which are three separate traits. Because we plan to add more, it
doesn't make sense to maintain that many traits that do the same thing in
different circumstances, so i guess it's better to merge them into a single
multi-purpose trait. "Multi-purpose" is definitely not the top buzzword in
programming, but here i believe it's worth it because the underlying reasoning
for needing these traits and needing them to work in a particular manner is the
same. We need them because our constructor expressions are turned inside out,
and we need a better connection between "inside" and "out" in both directions.
One of these directions is handled by the ConstructionContext; the other is
path-sensitive.
No functional change intended here; this is a refactoring.
Repository:
rC Clang
https://reviews.llvm.org/D47303
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -288,8 +288,8 @@
AllocV, CNE->getType(),
getContext().getPointerType(getContext().VoidTy));
- state =
- setCXXNewAllocatorValue(state, CNE, calleeCtx->getParent(), AllocV);
+ state = addObjectUnderConstruction(state, CNE, calleeCtx->getParent(),
+ AllocV);
}
}
@@ -354,8 +354,9 @@
/*WasInlined=*/true);
for (auto I : DstPostPostCallCallback) {
getCheckerManager().runCheckersForNewAllocator(
-CNE, getCXXNewAllocatorValue(I->getState(), CNE,
- calleeCtx->getParent()),
+CNE,
+*getObjectUnderConstruction(I->getState(), CNE,
+calleeCtx->getParent()),
DstPostCall, I, *this,
/*WasInlined=*/true);
}
@@ -588,8 +589,8 @@
// Conjure a temporary if the function returns an object by value.
MemRegionManager &MRMgr = svalBuilder.getRegionManager();
const CXXTempObjectRegion *TR = MRMgr.getCXXTempObjectRegion(E, LCtx);
-State = addAllNecessaryTemporaryInfo(State, RTC->getConstructionContext(),
- LCtx, TR);
+State = markStatementsCorrespondingToConstructedObject(
+State, RTC->getConstructionContext(), LCtx, loc::MemRegionVal(TR));
// Invalidate the region so that it didn't look uninitialized. Don't notify
// the checkers.
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -111,11 +111,10 @@
}
-const MemRegion *
-ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
- ExplodedNode *Pred,
- const ConstructionContext *CC,
- EvalCallOptions &CallOpts) {
+SVal ExprEngine::getLocationForConstructedObject(const CXXConstructExpr *CE,
+ ExplodedNode *Pred,
+ const ConstructionContext *CC,
+ EvalCallOptions &CallOpts) {
const LocationContext *LCtx = Pred->getLocationContext();
ProgramStateRef State = Pred->getState();
MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
@@ -130,9 +129,8 @@
const auto *Var = cast(DS->getSingleDecl());
SVal LValue = State->getLValue(Var, LCtx);
QualType Ty = Var->getType();
- LValue =
- makeZeroElementRegion(State, LValue, Ty, CallOpts.IsArrayCtorOrDtor);
- return LValue.getAsRegion();
+ return makeZeroElementRegion(State, LValue, Ty,
+ CallOpts.IsArrayCtorOrDtor);
}
case ConstructionContext::SimpleConstructorInitializerKind: {
const auto *ICC = cast(CC);
@@ -156,25 +154,26 @@
QualType Ty = Field->getType();
FieldVal = makeZeroElementRegion(State, FieldVal, Ty,
[PATCH] D47304: [analyzer] NFC: Merge the functions for obtaining constructed object location and storing this location for later use.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, baloghadamsoftware.
`getLocationForConstructedObject()` looks at the construction context and
figures out what region should represent the object.
`markStatementsCorrespondingToConstructedObject()` looks at the construction
context and figures out what statements will need to retrieve that region
directly later.
These functions are coupled and code is duplicated between them. They should be
merged. The resulting function is large, so it'd probably later need to be
split in a different manner (i.e. by construction context kinds). It'll also
soon become recursive as we add better support for copy elision at return
sites. I really hope we don't end up coding any sort of
ConstructionContextVisitor.
No functional change intended here; this is a refactoring pass.
Repository:
rC Clang
https://reviews.llvm.org/D47304
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -587,18 +587,20 @@
unsigned Count = currBldrCtx->blockCount();
if (auto RTC = getCurrentCFGElement().getAs()) {
// Conjure a temporary if the function returns an object by value.
-MemRegionManager &MRMgr = svalBuilder.getRegionManager();
-const CXXTempObjectRegion *TR = MRMgr.getCXXTempObjectRegion(E, LCtx);
-State = markStatementsCorrespondingToConstructedObject(
-State, RTC->getConstructionContext(), LCtx, loc::MemRegionVal(TR));
-
+SVal Target;
+assert(RTC->getStmt() == Call.getOriginExpr());
+EvalCallOptions CallOpts; // FIXME: We won't really need those.
+std::tie(State, Target) =
+prepareForObjectConstruction(Call.getOriginExpr(), State, LCtx,
+ RTC->getConstructionContext(), CallOpts);
+assert(Target.getAsRegion());
// Invalidate the region so that it didn't look uninitialized. Don't notify
// the checkers.
-State = State->invalidateRegions(TR, E, Count, LCtx,
+State = State->invalidateRegions(Target.getAsRegion(), E, Count, LCtx,
/* CausedByPointerEscape=*/false, nullptr,
&Call, nullptr);
-R = State->getSVal(TR, E->getType());
+R = State->getSVal(Target.castAs(), E->getType());
} else {
// Conjure a symbol if the return value is unknown.
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -110,13 +110,9 @@
return LValue;
}
-
-SVal ExprEngine::getLocationForConstructedObject(const CXXConstructExpr *CE,
- ExplodedNode *Pred,
- const ConstructionContext *CC,
- EvalCallOptions &CallOpts) {
- const LocationContext *LCtx = Pred->getLocationContext();
- ProgramStateRef State = Pred->getState();
+std::pair ExprEngine::prepareForObjectConstruction(
+const Expr *E, ProgramStateRef State, const LocationContext *LCtx,
+const ConstructionContext *CC, EvalCallOptions &CallOpts) {
MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
// See if we're constructing an existing region by looking at the
@@ -129,8 +125,9 @@
const auto *Var = cast(DS->getSingleDecl());
SVal LValue = State->getLValue(Var, LCtx);
QualType Ty = Var->getType();
- return makeZeroElementRegion(State, LValue, Ty,
- CallOpts.IsArrayCtorOrDtor);
+ return std::make_pair(
+ State,
+ makeZeroElementRegion(State, LValue, Ty, CallOpts.IsArrayCtorOrDtor));
}
case ConstructionContext::SimpleConstructorInitializerKind: {
const auto *ICC = cast(CC);
@@ -154,7 +151,7 @@
QualType Ty = Field->getType();
FieldVal = makeZeroElementRegion(State, FieldVal, Ty,
CallOpts.IsArrayCtorOrDtor);
- return FieldVal;
+ return std::make_pair(State, FieldVal);
}
case ConstructionContext::NewAllocatedObjectKind: {
if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
@@ -167,65 +164,97 @@
// TODO: In fact, we need to call the constructor for every
// allocated element, not just the first one!
CallOpts.IsArrayCtorOrDtor = true;
-return loc::MemRegionVal(getStoreManager().GetElementZeroRegi
[PATCH] D47305: [analyzer] pr37270: Fix binding constructed object to DeclStmt when ConstructionContext is already lost.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, baloghadamsoftware.
As explained in http://lists.llvm.org/pipermail/cfe-dev/2018-May/058055.html,
`findDirectConstructorForCurrentCFGElement()` was not working correctly in the
current example and erroneously made us think that we've constructed into a
dummy temporary region rather than into the variable. Instead, it was proposed
to track it in the program state every time we are performing construction
correctly.
Additionally this information will be used to maintain liveness of the
variable's Store bindings, because previously an incorrect Environment binding
of the target region to the construct-expression was used for that purpose.
Such binding is incorrect because the constructor is a prvalue of an object
type and should therefore have a NonLoc representing its symbolic value.
Therefore the hack implemented by `isTemporaryPRValue()` can be safely removed.
`findDirectConstructorForCurrentCFGElement()` cannot be removed yet because it
is also used for constructor member initializers for the same purpose. It
doesn't seem to introduce bugs though.
Repository:
rC Clang
https://reviews.llvm.org/D47305
Files:
lib/StaticAnalyzer/Core/ExprEngineC.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/cxx17-mandatory-elision.cpp
Index: test/Analysis/cxx17-mandatory-elision.cpp
===
--- test/Analysis/cxx17-mandatory-elision.cpp
+++ test/Analysis/cxx17-mandatory-elision.cpp
@@ -3,6 +3,26 @@
void clang_analyzer_eval(bool);
+namespace variable_functional_cast_crash {
+
+struct A {
+ A(int) {}
+};
+
+void foo() {
+ A a = A(0);
+}
+
+struct B {
+ A a;
+ B(): a(A(0)) {}
+};
+
+} // namespace variable_functional_cast_crash
+
+
+namespace address_vector_tests {
+
template struct AddressVector {
T *buf[10];
int len;
@@ -56,3 +76,5 @@
clang_analyzer_eval(v.buf[4] == &c); // expected-warning{{TRUE}}
#endif
}
+
+} // namespace address_vector_tests
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -193,23 +193,6 @@
return RuntimeCallee->getCanonicalDecl() != StaticDecl->getCanonicalDecl();
}
-/// Returns true if the CXXConstructExpr \p E was intended to construct a
-/// prvalue for the region in \p V.
-///
-/// Note that we can't just test for rvalue vs. glvalue because
-/// CXXConstructExprs embedded in DeclStmts and initializers are considered
-/// rvalues by the AST, and the analyzer would like to treat them as lvalues.
-static bool isTemporaryPRValue(const CXXConstructExpr *E, SVal V) {
- if (E->isGLValue())
-return false;
-
- const MemRegion *MR = V.getAsRegion();
- if (!MR)
-return false;
-
- return isa(MR);
-}
-
/// The call exit is simulated with a sequence of nodes, which occur between
/// CallExitBegin and CallExitEnd. The following operations occur between the
/// two program points:
@@ -269,11 +252,7 @@
loc::MemRegionVal This =
svalBuilder.getCXXThis(CCE->getConstructor()->getParent(), calleeCtx);
SVal ThisV = state->getSVal(This);
-
- // If the constructed object is a temporary prvalue, get its bindings.
- if (isTemporaryPRValue(CCE, ThisV))
-ThisV = state->getSVal(ThisV.castAs());
-
+ ThisV = state->getSVal(ThisV.castAs());
state = state->BindExpr(CCE, callerCtx, ThisV);
}
@@ -574,11 +553,7 @@
}
} else if (const CXXConstructorCall *C = dyn_cast(&Call)){
SVal ThisV = C->getCXXThisVal();
-
-// If the constructed object is a temporary prvalue, get its bindings.
-if (isTemporaryPRValue(cast(E), ThisV))
- ThisV = State->getSVal(ThisV.castAs());
-
+ThisV = State->getSVal(ThisV.castAs());
return State->BindExpr(E, LCtx, ThisV);
}
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -125,9 +125,11 @@
const auto *Var = cast(DS->getSingleDecl());
SVal LValue = State->getLValue(Var, LCtx);
QualType Ty = Var->getType();
- return std::make_pair(
- State,
- makeZeroElementRegion(State, LValue, Ty, CallOpts.IsArrayCtorOrDtor));
+ LValue =
+ makeZeroElementRegion(State, LValue, Ty, CallOpts.IsArrayCtorOrDtor);
+ State =
+ addObjectUnderConstruction(State, DSCC->getDeclStmt(), LCtx, LValue);
+ return std::make_pair(State, LValue);
}
case ConstructionContext::SimpleConstructorInitializerKind: {
const auto *ICC = cast(CC);
Index: lib/StaticAnalyzer/Cor
[PATCH] D47350: [analyzer] Track class member initializer constructors path-sensitively within their construction context.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, baloghadamsoftware.
Same as https://reviews.llvm.org/D47305 but for `CXXCtorInitializer`-based
constructors, based on the discussion in
http://lists.llvm.org/pipermail/cfe-dev/2018-May/058055.html
Because these don't suffer from liveness bugs (member variables are born much
earlier than their constructors are called and live much longer than stack
locals), this is mostly a refactoring pass. It has minor observable side
effects, but it will have way more visible effects when we enable C++17
construction contexts because finding the direct constructor would be much
harder.
Currently the observable effect is that we can preserve direct bindings to the
object more often and we need to fall back to binding the lazy compound value
of the initializer expression less often. Direct bindings are modeled better by
the store. In the provided test case the default binding produced by
trivial-copying `s` to `t.s` would overwrite the existing default binding to
`t`. But if we instead preserve direct bindings, only bindings that correspond
to `t.s` will be overwritten and the binding for `t.w` will remain untouched.
This only works for C++17 because in C++11 the member variable is still modeled
as a trivial copy from the temporary object, because there semantically *is* a
temporary object, while in C++17 it is elided.
Repository:
rC Clang
https://reviews.llvm.org/D47350
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/cxx17-mandatory-elision.cpp
Index: test/Analysis/cxx17-mandatory-elision.cpp
===
--- test/Analysis/cxx17-mandatory-elision.cpp
+++ test/Analysis/cxx17-mandatory-elision.cpp
@@ -21,6 +21,36 @@
} // namespace variable_functional_cast_crash
+namespace ctor_initializer {
+
+struct S {
+ int x, y, z;
+};
+
+struct T {
+ S s;
+ int w;
+ T(int w): s(), w(w) {}
+};
+
+class C {
+ T t;
+public:
+ C() : t(T(4)) {
+S s = {1, 2, 3};
+t.s = s;
+clang_analyzer_eval(t.w == 4);
+#if __cplusplus >= 201703L
+// expected-warning@-2{{TRUE}}
+#else
+// expected-warning@-4{{UNKNOWN}}
+#endif
+ }
+};
+
+} // namespace ctor_initializer
+
+
namespace address_vector_tests {
template struct AddressVector {
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -153,6 +153,7 @@
QualType Ty = Field->getType();
FieldVal = makeZeroElementRegion(State, FieldVal, Ty,
CallOpts.IsArrayCtorOrDtor);
+ State = addObjectUnderConstruction(State, Init, LCtx, FieldVal);
return std::make_pair(State, FieldVal);
}
case ConstructionContext::NewAllocatedObjectKind: {
@@ -272,35 +273,6 @@
State, loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx)));
}
-const CXXConstructExpr *
-ExprEngine::findDirectConstructorForCurrentCFGElement() {
- // Go backward in the CFG to see if the previous element (ignoring
- // destructors) was a CXXConstructExpr. If so, that constructor
- // was constructed directly into an existing region.
- // This process is essentially the inverse of that performed in
- // findElementDirectlyInitializedByCurrentConstructor().
- if (currStmtIdx == 0)
-return nullptr;
-
- const CFGBlock *B = getBuilderContext().getBlock();
-
- unsigned int PreviousStmtIdx = currStmtIdx - 1;
- CFGElement Previous = (*B)[PreviousStmtIdx];
-
- while (Previous.getAs() && PreviousStmtIdx > 0) {
---PreviousStmtIdx;
-Previous = (*B)[PreviousStmtIdx];
- }
-
- if (Optional PrevStmtElem = Previous.getAs()) {
-if (auto *CtorExpr = dyn_cast(PrevStmtElem->getStmt())) {
- return CtorExpr;
-}
- }
-
- return nullptr;
-}
-
void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
ExplodedNode *Pred,
ExplodedNodeSet &destNodes) {
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -100,7 +100,68 @@
// Keeps track of whether objects corresponding to the statement have been
// fully constructed.
-typedef std::pair ConstructedObjectKey;
+
+/// ConstructedObjectKey is used for being able to find the path-sensitive
+/// memory region of a freshly constructed object while modeling the AST node
+/// that syntactically represents the object that is being constructed.
+/// Semantics of such nodes may sometimes require access to the region that's
+/// not otherwise
[PATCH] D47350: [analyzer] Track class member initializer constructors path-sensitively within their construction context.
NoQ updated this revision to Diff 148502.
NoQ added a comment.
Add a forgotten FIXME to the test.
https://reviews.llvm.org/D47350
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/cxx17-mandatory-elision.cpp
Index: test/Analysis/cxx17-mandatory-elision.cpp
===
--- test/Analysis/cxx17-mandatory-elision.cpp
+++ test/Analysis/cxx17-mandatory-elision.cpp
@@ -21,6 +21,37 @@
} // namespace variable_functional_cast_crash
+namespace ctor_initializer {
+
+struct S {
+ int x, y, z;
+};
+
+struct T {
+ S s;
+ int w;
+ T(int w): s(), w(w) {}
+};
+
+class C {
+ T t;
+public:
+ C() : t(T(4)) {
+S s = {1, 2, 3};
+t.s = s;
+// FIXME: Should be TRUE in C++11 as well.
+clang_analyzer_eval(t.w == 4);
+#if __cplusplus >= 201703L
+// expected-warning@-2{{TRUE}}
+#else
+// expected-warning@-4{{UNKNOWN}}
+#endif
+ }
+};
+
+} // namespace ctor_initializer
+
+
namespace address_vector_tests {
template struct AddressVector {
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -153,6 +153,7 @@
QualType Ty = Field->getType();
FieldVal = makeZeroElementRegion(State, FieldVal, Ty,
CallOpts.IsArrayCtorOrDtor);
+ State = addObjectUnderConstruction(State, Init, LCtx, FieldVal);
return std::make_pair(State, FieldVal);
}
case ConstructionContext::NewAllocatedObjectKind: {
@@ -272,35 +273,6 @@
State, loc::MemRegionVal(MRMgr.getCXXTempObjectRegion(E, LCtx)));
}
-const CXXConstructExpr *
-ExprEngine::findDirectConstructorForCurrentCFGElement() {
- // Go backward in the CFG to see if the previous element (ignoring
- // destructors) was a CXXConstructExpr. If so, that constructor
- // was constructed directly into an existing region.
- // This process is essentially the inverse of that performed in
- // findElementDirectlyInitializedByCurrentConstructor().
- if (currStmtIdx == 0)
-return nullptr;
-
- const CFGBlock *B = getBuilderContext().getBlock();
-
- unsigned int PreviousStmtIdx = currStmtIdx - 1;
- CFGElement Previous = (*B)[PreviousStmtIdx];
-
- while (Previous.getAs() && PreviousStmtIdx > 0) {
---PreviousStmtIdx;
-Previous = (*B)[PreviousStmtIdx];
- }
-
- if (Optional PrevStmtElem = Previous.getAs()) {
-if (auto *CtorExpr = dyn_cast(PrevStmtElem->getStmt())) {
- return CtorExpr;
-}
- }
-
- return nullptr;
-}
-
void ExprEngine::VisitCXXConstructExpr(const CXXConstructExpr *CE,
ExplodedNode *Pred,
ExplodedNodeSet &destNodes) {
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -100,7 +100,68 @@
// Keeps track of whether objects corresponding to the statement have been
// fully constructed.
-typedef std::pair ConstructedObjectKey;
+
+/// ConstructedObjectKey is used for being able to find the path-sensitive
+/// memory region of a freshly constructed object while modeling the AST node
+/// that syntactically represents the object that is being constructed.
+/// Semantics of such nodes may sometimes require access to the region that's
+/// not otherwise present in the program state, or to the very fact that
+/// the construction context was present and contained references to these
+/// AST nodes.
+class ConstructedObjectKey {
+ llvm::PointerUnion P;
+ const LocationContext *LC;
+
+ const void *getAnyASTNodePtr() const {
+if (const Stmt *S = getStmt())
+ return S;
+else
+ return getCXXCtorInitializer();
+ }
+
+public:
+ ConstructedObjectKey(
+ llvm::PointerUnion P,
+ const LocationContext *LC)
+ : P(P), LC(LC) {
+// This is the full list of statements that require additional actions when
+// encountered. This list may be expanded when new actions are implemented.
+assert(getCXXCtorInitializer() || isa(getStmt()) ||
+ isa(getStmt()) || isa(getStmt()) ||
+ isa(getStmt()));
+ }
+
+ const Stmt *getStmt() const {
+return P.dyn_cast();
+ }
+ const CXXCtorInitializer *getCXXCtorInitializer() const {
+return P.dyn_cast();
+ }
+ const LocationContext *getLocationContext() const {
+return LC;
+ }
+
+ void print(llvm::raw_ostream &OS, PrinterHelper *Helper, PrintingPolicy &PP) {
+OS << '(' << LC << ',' << getAnyASTNodePtr() << ") ";
+if (const Stmt *S = P.dyn_cast()) {
+ S->printPretty(OS, Helper, PP);
+} else {
+ const CXXCtorInitializer *I = P.get();
+ OS << I->getAnyMember()->getNameAsString();
+
[PATCH] D47351: [analyzer] Re-enable C++17-specific variable and member variable construction contexts.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, baloghadamsoftware.
The recent refactoring allows us to easily support C++17 "copy-elided"
constructor syntax for variables and constructor-initializers. Similar
constructors for return values are still to be supported.
In particular, the change of using path-sensitive state traits made our
liveness problems go away. The `elision_on_ternary_op_branches` test was
failing without that.
https://reviews.llvm.org/D47350 allowed us to support the C++17 destructor in
`testCtorInitializer()`.
Repository:
rC Clang
https://reviews.llvm.org/D47351
Files:
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/cxx17-mandatory-elision.cpp
Index: test/Analysis/cxx17-mandatory-elision.cpp
===
--- test/Analysis/cxx17-mandatory-elision.cpp
+++ test/Analysis/cxx17-mandatory-elision.cpp
@@ -49,9 +49,64 @@
}
};
+
+struct A {
+ int x;
+ A(): x(0) {}
+ ~A() {}
+};
+
+struct B {
+ A a;
+ B() : a(A()) {}
+};
+
+void foo() {
+ B b;
+ clang_analyzer_eval(b.a.x == 0); // expected-warning{{TRUE}}
+}
+
} // namespace ctor_initializer
+namespace elision_on_ternary_op_branches {
+class C1 {
+ int x;
+public:
+ C1(int x): x(x) {}
+ int getX() const { return x; }
+ ~C1();
+};
+
+class C2 {
+ int x;
+ int y;
+public:
+ C2(int x, int y): x(x), y(y) {}
+ int getX() const { return x; }
+ int getY() const { return y; }
+ ~C2();
+};
+
+void foo(int coin) {
+ C1 c1 = coin ? C1(1) : C1(2);
+ if (coin) {
+clang_analyzer_eval(c1.getX() == 1); // expected-warning{{TRUE}}
+ } else {
+clang_analyzer_eval(c1.getX() == 2); // expected-warning{{TRUE}}
+ }
+ C2 c2 = coin ? C2(3, 4) : C2(5, 6);
+ if (coin) {
+clang_analyzer_eval(c2.getX() == 3); // expected-warning{{TRUE}}
+clang_analyzer_eval(c2.getY() == 4); // expected-warning{{TRUE}}
+ } else {
+clang_analyzer_eval(c2.getX() == 5); // expected-warning{{TRUE}}
+clang_analyzer_eval(c2.getY() == 6); // expected-warning{{TRUE}}
+ }
+}
+} // namespace elision_on_ternary_op_branches
+
+
namespace address_vector_tests {
template struct AddressVector {
@@ -108,4 +163,68 @@
#endif
}
+class ClassWithDestructor {
+ AddressVector &v;
+
+public:
+ ClassWithDestructor(AddressVector &v) : v(v) {
+v.push(this);
+ }
+
+ ClassWithDestructor(ClassWithDestructor &&c) : v(c.v) { v.push(this); }
+ ClassWithDestructor(const ClassWithDestructor &c) : v(c.v) {
+v.push(this);
+ }
+
+ ~ClassWithDestructor() { v.push(this); }
+};
+
+void testVariable() {
+ AddressVector v;
+ {
+ClassWithDestructor c = ClassWithDestructor(v);
+ }
+#if __cplusplus >= 201703L
+ // 0. Construct the variable.
+ // 1. Destroy the variable.
+ clang_analyzer_eval(v.len == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == v.buf[1]); // expected-warning{{TRUE}}
+#else
+ // 0. Construct the temporary.
+ // 1. Construct the variable.
+ // 2. Destroy the temporary.
+ // 3. Destroy the variable.
+ clang_analyzer_eval(v.len == 4); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == v.buf[2]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[1] == v.buf[3]); // expected-warning{{TRUE}}
+#endif
+}
+
+struct TestCtorInitializer {
+ ClassWithDestructor c;
+ TestCtorInitializer(AddressVector &v)
+: c(ClassWithDestructor(v)) {}
+};
+
+void testCtorInitializer() {
+ AddressVector v;
+ {
+TestCtorInitializer t(v);
+ }
+#if __cplusplus >= 201703L
+ // 0. Construct the member variable.
+ // 1. Destroy the member variable.
+ clang_analyzer_eval(v.len == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == v.buf[1]); // expected-warning{{TRUE}}
+#else
+ // 0. Construct the temporary.
+ // 1. Construct the member variable.
+ // 2. Destroy the temporary.
+ // 3. Destroy the member variable.
+ clang_analyzer_eval(v.len == 4); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == v.buf[2]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[1] == v.buf[3]); // expected-warning{{TRUE}}
+#endif
+}
+
} // namespace address_vector_tests
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -119,8 +119,9 @@
// current construction context.
if (CC) {
switch (CC->getKind()) {
+case ConstructionContext::CXX17ElidedCopyVariableKind:
case ConstructionContext::SimpleVariableKind: {
- const auto *DSCC = cast(CC);
+ const auto *DSCC = cast(CC);
const auto *DS = DSCC->getDeclStmt();
const auto *Var = cast(DS->getSingleDecl());
SVal LValue = State->getLValue(Var, LCtx);
@@ -131,6 +132,7 @@
addObjectUnderConstruction(State, DSCC->getDeclStmt(), LCtx, LValue);
[PATCH] D46823: [analyzer] const init: handle non-explicit cases more accurately
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
Looks good!
Do you have commit access? I think you should get commit access.
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1650
+
+// If there is a list, but no init, it must be zero.
+if (i >= InitList->getNumInits())
r.stahl wrote:
> r.stahl wrote:
> > NoQ wrote:
> > > NoQ wrote:
> > > > Would this work correctly if the element is not of an integral or
> > > > enumeration type? I think this needs an explicit check.
> > > What if we have an out-of-bounds access to a variable-length array? I
> > > don't think it'd yield zero.
> > I'm getting "variable-sized object may not be initialized", so this case
> > should not be possible.
> I'm having a hard time reproducing this either.
>
>
> ```
> struct S {
> int a = 3;
> };
> S const sarr[2] = {};
> void definit() {
> int i = 1;
> clang_analyzer_dump(sarr[i].a); // expected-warning{{test}}
> }
> ```
>
> results in a symbolic value, because makeZeroVal returns an empty SVal list
> for arrays, records, vectors and complex types. Otherwise it just returns
> UnknownVal (for example for not-yet-implemented floats).
>
> Can you think of a case where this would be an issue?
Yup, sounds reasonable.
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1650-1652
+// If there is a list, but no init, it must be zero.
+if (i >= InitList->getNumInits())
+ return svalBuilder.makeZeroVal(R->getElementType());
NoQ wrote:
> r.stahl wrote:
> > r.stahl wrote:
> > > NoQ wrote:
> > > > NoQ wrote:
> > > > > Would this work correctly if the element is not of an integral or
> > > > > enumeration type? I think this needs an explicit check.
> > > > What if we have an out-of-bounds access to a variable-length array? I
> > > > don't think it'd yield zero.
> > > I'm getting "variable-sized object may not be initialized", so this case
> > > should not be possible.
> > I'm having a hard time reproducing this either.
> >
> >
> > ```
> > struct S {
> > int a = 3;
> > };
> > S const sarr[2] = {};
> > void definit() {
> > int i = 1;
> > clang_analyzer_dump(sarr[i].a); // expected-warning{{test}}
> > }
> > ```
> >
> > results in a symbolic value, because makeZeroVal returns an empty SVal list
> > for arrays, records, vectors and complex types. Otherwise it just returns
> > UnknownVal (for example for not-yet-implemented floats).
> >
> > Can you think of a case where this would be an issue?
> Yup, sounds reasonable.
Had a look. This indeed looks fine, but for a completely different reason. In
fact structs don't appear in `getBindingForElement()` because they all go
through `getBindingForStruct()` instead. Hopefully this does take care of other
cornercases.
So your test case doesn't even trigger your code to be executed, neither would
`S s = sarr[i]; clang_analyzer_dump(s.a);`.
Still, as far as i understand, `sarr` here should be zero-initialized, so i
think the symbolic value can be improved upon, so we might as well add this as
a FIXME test.
https://reviews.llvm.org/D46823
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call
NoQ added a comment. Ok! Putting any remaining work into follow-up patches would be great indeed. Let's land this into `alpha.cplusplus` for now because i still haven't made up my mind for how to organize the proposed `bugprone` category (sorry!). Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:223-225 + ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState()); + if (!Node) +return; Szelethus wrote: > NoQ wrote: > > I suspect that a fatal error is better here. We don't want the user to > > receive duplicate report from other checkers that catch uninitialized > > values; just one report is enough. > I think that would be a bad idea. For example, this checker shouts so loudly > while checking the LLVM project, it would practically halt the analysis of > the code, reducing the coverage, which means that checkers other then uninit > value checkers would "suffer" from it. > > However, I also think that having multiple uninit reports for the same object > might be good, especially with this checker, as it would be very easy to see > where the problem originated from. > > What do you think? Well, i guess that's the reason to not use the checker on LLVM. Regardless of fatal/nonfatal warnings, enabling this checker on LLVM regularly would be a bad idea because it's unlikely that anybody will be able to fix all the false positives to make it usable. And for other projects that don't demonstrate many false positives, this shouldn't be a problem. In order to indicate where the problem originates from, we have our bug reporter visitors that try their best to add such info directly to the report. In particular, @george.karpenkov's recent `NoStoreFuncVisitor` highlights functions in which a variable was //not// initialized but was probably expected to be. Not sure if it highlights constructors in its current shape, but that's definitely a better way to give this piece of information to the user because it doesn't make the user look for a different report to understand the current report. Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260 +Report->addNote(NoteBuf, +PathDiagnosticLocation::create(FieldChain.getEndOfChain(), + Context.getSourceManager())); Szelethus wrote: > NoQ wrote: > > NoQ wrote: > > > Szelethus wrote: > > > > NoQ wrote: > > > > > Aha, ok, got it, so you're putting the warning at the end of the > > > > > constructor and squeezing all uninitialized fields into a single > > > > > warning by presenting them as notes. > > > > > > > > > > Because for now notes aren't supported everywhere (and aren't always > > > > > supported really well), i guess it'd be necessary to at least provide > > > > > an option to make note-less reports before the checker is enabled by > > > > > default everywhere. In this case notes contain critical pieces of > > > > > information and as such cannot be really discarded. > > > > > > > > > > I'm not sure that notes are providing a better user experience here > > > > > than simply saying that "field x.y->z is never initialized". With > > > > > notes, the user will have to scroll around (at least in scan-build) > > > > > to find which field is uninitialized. > > > > > > > > > > Notes will probably also not decrease the number of reports too much > > > > > because in most cases there will be just one uninitialized field. > > > > > Though i agree that when there is more than one report, the user will > > > > > be likely to deal with all fields at once rather than separately. > > > > > > > > > > So it's a bit wonky. > > > > > > > > > > We might want to enable one mode or another in the checker depending > > > > > on the analyzer output flag. Probably in the driver > > > > > (`RenderAnalyzerOptions()`). > > > > > > > > > > It'd be a good idea to land the checker into alpha first and fix this > > > > > in a follow-up patch because this review is already overweight. > > > > > [...]i guess it'd be necessary to at least provide an option to make > > > > > note-less reports before the checker is enabled by default > > > > > everywhere. In this case notes contain critical pieces of information > > > > > and as such cannot be really discarded. > > > > This can already be achieved with `-analyzer-config > > > > notes-as-events=true`. There no good reason however not to make an > > > > additional flag that would emit warnings instead of notes. > > > > > I'm not sure that notes are providing a better user experience here > > > > > than simply saying that "field x.y->z is never initialized". With > > > > > notes, the user will have to scroll around (at least in scan-build) > > > > > to find which field is uninitialized. > > > > This checker had a previous implementation that emitted a warning for > > > > each uninitialized fiel
[PATCH] D35110: [Analyzer] Constraint Manager Negates Difference
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:265-276 + const llvm::APSInt &from = i->From(), &to = i->To(); + const llvm::APSInt &newFrom = (to.isMinSignedValue() ? + BV.getMaxValue(to) : + (to.isMaxSignedValue() ? + BV.getMinValue(to) : + BV.getValue(- to))); + const llvm::APSInt &newTo = (from.isMinSignedValue() ? baloghadamsoftware wrote: > NoQ wrote: > > baloghadamsoftware wrote: > > > NoQ wrote: > > > > Hmm, wait a minute, is this actually correct? > > > > > > > > For the range [-2³¹, -2³¹ + 1] over a 32-bit integer, the negated range > > > > will be [-2³¹, -2³¹] U [2³¹ - 1, 2³¹ - 1]. > > > > > > > > So there must be a place in the code where we take one range and add > > > > two ranges. > > > The two ends of the range of the type usually stands for +/- infinity. If > > > we add the minimum of the type when negating a negative range, then we > > > lose the whole point of this transformation. > > > > > > Example: If `A - B < 0`, then the range of `A - B` is `[-2³¹, -1]`, If we > > > negate this, and keep the `-2³¹` range end, then we get `[-2³¹, -2³¹]U[1, > > > 2³¹-1]`. However, this does not mean `B - A > 0`. If we make assumption > > > about this, we get two states instead of one, in the true state `A - B`'s > > > range is `[1, 2³¹-1]` and in the false state it is `[-2³¹, -2³¹]`. This > > > is surely not what we want. > > Well, we can't turn math into something we want, it is what it is. > > > > Iterator-related symbols are all planned to be within range [-2²⁹, -2²⁹], > > right? So if we subtract one such symbol from another, it's going to be in > > range [-2³⁰, 2³⁰]. Can we currently infer that? Or maybe we should make the > > iterator checker to enforce that separately? Because this range doesn't > > include -2³¹, so we won't have any problems with doing negation correctly. > > > > So as usual i propose to get this code mathematically correct and then see > > if we can ensure correct behavior by enforcing reasonable constraints on > > our symbols. > I agree that the code should do mathematically correct things, but what I > argue here is what math here means. Computer science is based on math, but it > is somewhat different because of finite ranges and overflows. So I initially > regarded the minimal and maximal values as infinity. Maybe that is not > correct. However, I am sure that negating `-2³¹` should never be `-2³¹`. This > is mathematically incorrect, and renders the whole calculation useless, since > the union of a positive range and a negative range is unsuitable for any > reasoning. I see two options here: > > 1. Remove the extension when negating a range which ends at the maximal value > of the type. So the negated range begins at the minimal value plus one. > However, cut the range which begins at the minimal value of the type by one. > So the negated range ends at the maximal value, as in the current version in > the patch. > > 2. Remove the extension as in 1. and disable the whole negation if we have > the range begins at the minimal value. > > Iterator checkers are of course not affected because of the max/4 constraints. > However, I am sure that negating `-2³¹` should never be `-2³¹`. This is > mathematically incorrect, and renders the whole calculation useless, since > the union of a positive range and a negative range is unsuitable for any > reasoning. Well, that's how computers already work. And that's how all sorts of abstract algebra work as well, so this is totally mathematically correct. We promise to support the [[ https://en.wikipedia.org/wiki/Two's_complement | two's complement ]] semantics in the analyzer when it comes to signed integer overflows. Even though it's technically UB, most implementations follow this semantics and a lot of real-world applications explicitly rely on that. Also we cannot simply drop values from our constraint ranges in the general case because the values we drop might be the only valid values, and the assumption that at least one non-dropped value can definitely be taken is generally incorrect. Finding cornercases like this one is one of the strong sides of any static analysis: it is in fact our job to make the user aware of it if he doesn't understand overflow rules. If it cannot be said that the variable on a certain path is non-negative because it might as well be -2³¹, we should totally explore this possibility. If for a certain checker it brings no benefit because such value would be unlikely in certain circumstances, that checker is free to cut off the respective paths, but the core should perform operations precisely. I don't think we have much room for personal preferences here. https://reviews.llvm.org/D35110 ___
[PATCH] D47155: [analyzer] Reduce simplifySVal complexity threshold further.
NoQ updated this revision to Diff 148681.
NoQ added a comment.
Optimize `simplifySVal()` instead of reducing the threshold.
I'll address the memoization separately.
https://reviews.llvm.org/D47155
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
test/Analysis/hangs.c
Index: test/Analysis/hangs.c
===
--- /dev/null
+++ test/Analysis/hangs.c
@@ -0,0 +1,30 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker core -verify %s
+
+// expected-no-diagnostics
+
+// Stuff that used to hang.
+
+int g();
+
+int f(int y) {
+ return y + g();
+}
+
+int produce_a_very_large_symbol(int x) {
+ return f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(
+ f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(x;
+}
+
+void produce_an_exponentially_exploding_symbol(int x, int y) {
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1222,6 +1222,10 @@
ProgramStateRef State;
SValBuilder &SVB;
+static bool isUnchanged(SymbolRef Sym, SVal Val) {
+ return Sym == Val.getAsSymbol();
+}
+
public:
Simplifier(ProgramStateRef State)
: State(State), SVB(State->getStateManager().getSValBuilder()) {}
@@ -1231,15 +1235,16 @@
SVB.getKnownValue(State, nonloc::SymbolVal(S)))
return Loc::isLocType(S->getType()) ? (SVal)SVB.makeIntLocVal(*I)
: (SVal)SVB.makeIntVal(*I);
- return Loc::isLocType(S->getType()) ? (SVal)SVB.makeLoc(S)
- : nonloc::SymbolVal(S);
+ return SVB.makeSymbolVal(S);
}
// TODO: Support SymbolCast. Support IntSymExpr when/if we actually
// start producing them.
SVal VisitSymIntExpr(const SymIntExpr *S) {
SVal LHS = Visit(S->getLHS());
+ if (isUnchanged(S->getLHS(), LHS))
+return SVB.makeSymbolVal(S);
SVal RHS;
// By looking at the APSInt in the right-hand side of S, we cannot
// figure out if it should be treated as a Loc or as a NonLoc.
@@ -1264,6 +1269,8 @@
SVal VisitSymSymExpr(const SymSymExpr *S) {
SVal LHS = Visit(S->getLHS());
SVal RHS = Visit(S->getRHS());
+ if (isUnchanged(S->getLHS(), LHS) && isUnchanged(S->getRHS(), RHS))
+return SVB.makeSymbolVal(S);
return SVB.evalBinOp(State, S->getOpcode(), LHS, RHS, S->getType());
}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -367,6 +367,15 @@
return loc::ConcreteInt(BasicVals.getValue(integer));
}
+ /// Make an SVal that represents the given symbol. This follows the
convention
+ /// of representing Loc-type symbols (symbolic pointers and references)
+ /// as Loc values wrapping the symbol rather than as plain symbol values.
+ SVal makeSymbolVal(SymbolRef Sym) {
+if (Loc::isLocType(Sym->getType()))
+ return makeLoc(Sym);
+return nonloc::SymbolVal(Sym);
+ }
+
/// Return a memory region for the 'this' object reference.
loc::MemRegionVal getCXXThis(const CXXMethodDecl *D,
const StackFrameContext *SFC);
Index: test/Analysis/hangs.c
===
--- /dev/null
+++ test/Analysis/hangs.c
@@ -0,0 +1,30 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker core -verify %s
+
+// expected-no-diagnostics
+
+// Stuff that used to hang.
+
+int g();
+
+int f(int y) {
+ return y + g();
+}
+
+int produce_a_very_large_symbol(int x) {
+ return f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(
+ f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(x;
+}
+
+void produce_an_exponentially_exploding_symbol(int x, int y) {
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1222,6 +1222,10 @@
ProgramStateRef State;
SValBuilder &SVB;
+static bool isUnc
[PATCH] D47155: [analyzer] Reduce simplifySVal complexity threshold further.
NoQ added a comment.
I only essentially did one optimization - introduce a short path that returns
the original value if visiting its sub-values changed nothing, which is a
relatively common case. The reason it works though is that `evalBinOp()` will
be called later to combine the sub-values, which may in turn call
`simplifySVal()` again, which is clearly unwanted.
Comment at: test/Analysis/hangs.c:18-30
+void produce_an_exponentially_exploding_symbol(int x, int y) {
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
This currently finishes in 1 second on my machine. This test, unlike the
original test, is easy to experiment with.
https://reviews.llvm.org/D47155
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47155: [analyzer] Reduce simplifySVal complexity threshold further.
NoQ updated this revision to Diff 148684.
NoQ added a comment.
Add an explicit brute-force protection against re-entering `simplifySVal()`.
Remove the threshold completely.
https://reviews.llvm.org/D47155
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
test/Analysis/hangs.c
Index: test/Analysis/hangs.c
===
--- /dev/null
+++ test/Analysis/hangs.c
@@ -0,0 +1,30 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker core -verify %s
+
+// expected-no-diagnostics
+
+// Stuff that used to hang.
+
+int g();
+
+int f(int y) {
+ return y + g();
+}
+
+int produce_a_very_large_symbol(int x) {
+ return f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(
+ f(f(f(f(f(f(f(f(f(f(f(f(f(f(f(x;
+}
+
+void produce_an_exponentially_exploding_symbol(int x, int y) {
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+ x += y; y += x + g();
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1222,6 +1222,10 @@
ProgramStateRef State;
SValBuilder &SVB;
+static bool isUnchanged(SymbolRef Sym, SVal Val) {
+ return Sym == Val.getAsSymbol();
+}
+
public:
Simplifier(ProgramStateRef State)
: State(State), SVB(State->getStateManager().getSValBuilder()) {}
@@ -1231,15 +1235,16 @@
SVB.getKnownValue(State, nonloc::SymbolVal(S)))
return Loc::isLocType(S->getType()) ? (SVal)SVB.makeIntLocVal(*I)
: (SVal)SVB.makeIntVal(*I);
- return Loc::isLocType(S->getType()) ? (SVal)SVB.makeLoc(S)
- : nonloc::SymbolVal(S);
+ return SVB.makeSymbolVal(S);
}
// TODO: Support SymbolCast. Support IntSymExpr when/if we actually
// start producing them.
SVal VisitSymIntExpr(const SymIntExpr *S) {
SVal LHS = Visit(S->getLHS());
+ if (isUnchanged(S->getLHS(), LHS))
+return SVB.makeSymbolVal(S);
SVal RHS;
// By looking at the APSInt in the right-hand side of S, we cannot
// figure out if it should be treated as a Loc or as a NonLoc.
@@ -1264,6 +1269,8 @@
SVal VisitSymSymExpr(const SymSymExpr *S) {
SVal LHS = Visit(S->getLHS());
SVal RHS = Visit(S->getRHS());
+ if (isUnchanged(S->getLHS(), LHS) && isUnchanged(S->getRHS(), RHS))
+return SVB.makeSymbolVal(S);
return SVB.evalBinOp(State, S->getOpcode(), LHS, RHS, S->getType());
}
@@ -1274,13 +1281,20 @@
SVal VisitNonLocSymbolVal(nonloc::SymbolVal V) {
// Simplification is much more costly than computing complexity.
// For high complexity, it may be not worth it.
- if (V.getSymbol()->computeComplexity() > 100)
-return V;
return Visit(V.getSymbol());
}
SVal VisitSVal(SVal V) { return V; }
};
- return Simplifier(State).Visit(V);
+ // A crude way of preventing this function from calling itself from evalBinOp.
+ static bool isReentering = false;
+ if (isReentering)
+return V;
+
+ isReentering = true;
+ SVal SimplifiedV = Simplifier(State).Visit(V);
+ isReentering = false;
+
+ return SimplifiedV;
}
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -367,6 +367,15 @@
return loc::ConcreteInt(BasicVals.getValue(integer));
}
+ /// Make an SVal that represents the given symbol. This follows the convention
+ /// of representing Loc-type symbols (symbolic pointers and references)
+ /// as Loc values wrapping the symbol rather than as plain symbol values.
+ SVal makeSymbolVal(SymbolRef Sym) {
+if (Loc::isLocType(Sym->getType()))
+ return makeLoc(Sym);
+return nonloc::SymbolVal(Sym);
+ }
+
/// Return a memory region for the 'this' object reference.
loc::MemRegionVal getCXXThis(const CXXMethodDecl *D,
const StackFrameContext *SFC);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47303: [analyzer] NFC: Merge object construction related state traits into one.
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:377
+ if (!State->contains(Key)) {
+return State->set(Key, V);
}
george.karpenkov wrote:
> nitpick: most C++ containers eliminate the need for two lookups by allowing
> the `get` method to return a reference. I'm not sure whether this can be done
> here though.
It's likely to be harder than usual because one doesn't simply obtain a
non-const reference to an object from an immutable map.
That's said, it's quite unimportant what do we do if the object is already
there.
https://reviews.llvm.org/D47303
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47303: [analyzer] NFC: Merge object construction related state traits into one.
NoQ updated this revision to Diff 148692.
NoQ marked 3 inline comments as done.
NoQ added a comment.
Fix review comments.
https://reviews.llvm.org/D47303
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -288,8 +288,8 @@
AllocV, CNE->getType(),
getContext().getPointerType(getContext().VoidTy));
- state =
- setCXXNewAllocatorValue(state, CNE, calleeCtx->getParent(), AllocV);
+ state = addObjectUnderConstruction(state, CNE, calleeCtx->getParent(),
+ AllocV);
}
}
@@ -354,8 +354,9 @@
/*WasInlined=*/true);
for (auto I : DstPostPostCallCallback) {
getCheckerManager().runCheckersForNewAllocator(
-CNE, getCXXNewAllocatorValue(I->getState(), CNE,
- calleeCtx->getParent()),
+CNE,
+*getObjectUnderConstruction(I->getState(), CNE,
+calleeCtx->getParent()),
DstPostCall, I, *this,
/*WasInlined=*/true);
}
@@ -588,8 +589,8 @@
// Conjure a temporary if the function returns an object by value.
MemRegionManager &MRMgr = svalBuilder.getRegionManager();
const CXXTempObjectRegion *TR = MRMgr.getCXXTempObjectRegion(E, LCtx);
-State = addAllNecessaryTemporaryInfo(State, RTC->getConstructionContext(),
- LCtx, TR);
+State = markStatementsCorrespondingToConstructedObject(
+State, RTC->getConstructionContext(), LCtx, loc::MemRegionVal(TR));
// Invalidate the region so that it didn't look uninitialized. Don't notify
// the checkers.
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -111,11 +111,10 @@
}
-const MemRegion *
-ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
- ExplodedNode *Pred,
- const ConstructionContext *CC,
- EvalCallOptions &CallOpts) {
+SVal ExprEngine::getLocationForConstructedObject(const CXXConstructExpr *CE,
+ ExplodedNode *Pred,
+ const ConstructionContext *CC,
+ EvalCallOptions &CallOpts) {
const LocationContext *LCtx = Pred->getLocationContext();
ProgramStateRef State = Pred->getState();
MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
@@ -130,9 +129,8 @@
const auto *Var = cast(DS->getSingleDecl());
SVal LValue = State->getLValue(Var, LCtx);
QualType Ty = Var->getType();
- LValue =
- makeZeroElementRegion(State, LValue, Ty, CallOpts.IsArrayCtorOrDtor);
- return LValue.getAsRegion();
+ return makeZeroElementRegion(State, LValue, Ty,
+ CallOpts.IsArrayCtorOrDtor);
}
case ConstructionContext::SimpleConstructorInitializerKind: {
const auto *ICC = cast(CC);
@@ -156,25 +154,26 @@
QualType Ty = Field->getType();
FieldVal = makeZeroElementRegion(State, FieldVal, Ty,
CallOpts.IsArrayCtorOrDtor);
- return FieldVal.getAsRegion();
+ return FieldVal;
}
case ConstructionContext::NewAllocatedObjectKind: {
if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
const auto *NECC = cast(CC);
const auto *NE = NECC->getCXXNewExpr();
-// TODO: Detect when the allocator returns a null pointer.
-// Constructor shall not be called in this case.
-if (const SubRegion *MR = dyn_cast_or_null(
-getCXXNewAllocatorValue(State, NE, LCtx).getAsRegion())) {
+SVal V = *getObjectUnderConstruction(State, NE, LCtx);
+if (const SubRegion *MR =
+dyn_cast_or_null(V.getAsRegion())) {
if (NE->isArray()) {
// TODO: In fact, we need to call the constructor for every
// allocated element, not just the first one!
CallOpts.IsArrayCtorOrDtor = true;
-return getStoreManager().GetElementZeroRegion(
-MR, NE->getType()->getPointeeType());
+return loc::MemRegionVal(getStoreManager().GetElementZeroRegion(
+MR, NE->getType()->getPointeeType()));
[PATCH] D41881: [analyzer] Flag bcmp, bcopy and bzero as obsolete
NoQ added a comment. Ouch, this one really got out of hand. Sorry. Repository: rC Clang https://reviews.llvm.org/D41881 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47402: [analyzer] Improve simplifySVal performance further.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, baloghadamsoftware.
Memoize `SValBuilder::simplifySVal()` so that it didn't try to re-simplify the
same symbolic expression in the same program state.
Speeds up the analysis by ~25% on the artificial test in
`test/Analysis/hangs.c`.
Repository:
rC Clang
https://reviews.llvm.org/D47402
Files:
lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1222,6 +1222,12 @@
ProgramStateRef State;
SValBuilder &SVB;
+// Cache results for the lifetime of the Simplifier. Results change every
+// time new constraints are added to the program state, which is the whole
+// point of simplifying, and for that very reason it's pointless to
maintain
+// the same cache for the duration of the whole analysis.
+llvm::DenseMap Cached;
+
static bool isUnchanged(SymbolRef Sym, SVal Val) {
return Sym == Val.getAsSymbol();
}
@@ -1242,9 +1248,16 @@
// start producing them.
SVal VisitSymIntExpr(const SymIntExpr *S) {
+ auto I = Cached.find(S);
+ if (I != Cached.end())
+return I->second;
+
SVal LHS = Visit(S->getLHS());
- if (isUnchanged(S->getLHS(), LHS))
-return SVB.makeSymbolVal(S);
+ if (isUnchanged(S->getLHS(), LHS)) {
+SVal V = SVB.makeSymbolVal(S);
+Cached[S] = V;
+return V;
+ }
SVal RHS;
// By looking at the APSInt in the right-hand side of S, we cannot
// figure out if it should be treated as a Loc or as a NonLoc.
@@ -1263,15 +1276,27 @@
} else {
RHS = SVB.makeIntVal(S->getRHS());
}
- return SVB.evalBinOp(State, S->getOpcode(), LHS, RHS, S->getType());
+
+ SVal V = SVB.evalBinOp(State, S->getOpcode(), LHS, RHS, S->getType());
+ Cached[S] = V;
+ return V;
}
SVal VisitSymSymExpr(const SymSymExpr *S) {
+ auto I = Cached.find(S);
+ if (I != Cached.end())
+return I->second;
+
SVal LHS = Visit(S->getLHS());
SVal RHS = Visit(S->getRHS());
- if (isUnchanged(S->getLHS(), LHS) && isUnchanged(S->getRHS(), RHS))
-return SVB.makeSymbolVal(S);
- return SVB.evalBinOp(State, S->getOpcode(), LHS, RHS, S->getType());
+ if (isUnchanged(S->getLHS(), LHS) && isUnchanged(S->getRHS(), RHS)) {
+SVal V = SVB.makeSymbolVal(S);
+Cached[S] = V;
+return V;
+ }
+ SVal V = SVB.evalBinOp(State, S->getOpcode(), LHS, RHS, S->getType());
+ Cached[S] = V;
+ return V;
}
SVal VisitSymExpr(SymbolRef S) { return nonloc::SymbolVal(S); }
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1222,6 +1222,12 @@
ProgramStateRef State;
SValBuilder &SVB;
+// Cache results for the lifetime of the Simplifier. Results change every
+// time new constraints are added to the program state, which is the whole
+// point of simplifying, and for that very reason it's pointless to maintain
+// the same cache for the duration of the whole analysis.
+llvm::DenseMap Cached;
+
static bool isUnchanged(SymbolRef Sym, SVal Val) {
return Sym == Val.getAsSymbol();
}
@@ -1242,9 +1248,16 @@
// start producing them.
SVal VisitSymIntExpr(const SymIntExpr *S) {
+ auto I = Cached.find(S);
+ if (I != Cached.end())
+return I->second;
+
SVal LHS = Visit(S->getLHS());
- if (isUnchanged(S->getLHS(), LHS))
-return SVB.makeSymbolVal(S);
+ if (isUnchanged(S->getLHS(), LHS)) {
+SVal V = SVB.makeSymbolVal(S);
+Cached[S] = V;
+return V;
+ }
SVal RHS;
// By looking at the APSInt in the right-hand side of S, we cannot
// figure out if it should be treated as a Loc or as a NonLoc.
@@ -1263,15 +1276,27 @@
} else {
RHS = SVB.makeIntVal(S->getRHS());
}
- return SVB.evalBinOp(State, S->getOpcode(), LHS, RHS, S->getType());
+
+ SVal V = SVB.evalBinOp(State, S->getOpcode(), LHS, RHS, S->getType());
+ Cached[S] = V;
+ return V;
}
SVal VisitSymSymExpr(const SymSymExpr *S) {
+ auto I = Cached.find(S);
+ if (I != Cached.end())
+return I->second;
+
SVal LHS = Visit(S->getLHS());
SVal RHS = Visit(S->getRHS());
- if (isUnchanged(S->getLHS(), LHS) && isUnchanged(S->getRHS(), RHS))
-return SVB.makeSymbolVal(S);
- return SVB.evalBinOp(State, S->getOpcode(), LHS, RHS, S->getType());
+ if (isUnchanged(S->getL
[PATCH] D47402: [analyzer] Improve simplifySVal performance further.
NoQ added a comment. The remaining slowness is in `removeDead()`. It's going to be fun to optimize. Some parts of it are already memoized (eg. the live set in `SymbolReaper`). Memory usage on the artificial test seems stable. Repository: rC Clang https://reviews.llvm.org/D47402 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47405: [analyzer] Re-enable C++17-specific return value constructors.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, baloghadamsoftware.
The refactoring conducted in https://reviews.llvm.org/D47304 made it easy for
the analyzer to find the target region for the constructor across multiple
stack frames. We ascend to the parent stack frame recursively until we find a
construction context that doesn't represent yet another return value of a
function. This is the semantics of copy elision in return statements that
return the object by value: they return it directly to a memory region (eg., a
variable) that may be located a few (indefinitely many) stack frames above the
statement. In particular, this is how return statements work in C++17 where
copy elision is mandatory. This commit enables this feature for the AST of
C++17, but extra work is necessary to perform copy elision in the AST produced
by older language standard modes.
Repository:
rC Clang
https://reviews.llvm.org/D47405
Files:
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
test/Analysis/cxx17-mandatory-elision.cpp
Index: test/Analysis/cxx17-mandatory-elision.cpp
===
--- test/Analysis/cxx17-mandatory-elision.cpp
+++ test/Analysis/cxx17-mandatory-elision.cpp
@@ -150,9 +150,8 @@
ClassWithoutDestructor c = make3(v);
#if __cplusplus >= 201703L
- // FIXME: Both should be TRUE.
clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
- clang_analyzer_eval(v.buf[0] == &c); // expected-warning{{FALSE}}
+ clang_analyzer_eval(v.buf[0] == &c); // expected-warning{{TRUE}}
#else
clang_analyzer_eval(v.len == 5); // expected-warning{{TRUE}}
clang_analyzer_eval(v.buf[0] != v.buf[1]); // expected-warning{{TRUE}}
@@ -183,6 +182,13 @@
AddressVector v;
{
ClassWithDestructor c = ClassWithDestructor(v);
+// Check if the last destructor is an automatic destructor.
+// A temporary destructor would have fired by now.
+#if __cplusplus >= 201703L
+clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
+#else
+clang_analyzer_eval(v.len == 3); // expected-warning{{TRUE}}
+#endif
}
#if __cplusplus >= 201703L
// 0. Construct the variable.
@@ -210,6 +216,13 @@
AddressVector v;
{
TestCtorInitializer t(v);
+// Check if the last destructor is an automatic destructor.
+// A temporary destructor would have fired by now.
+#if __cplusplus >= 201703L
+clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
+#else
+clang_analyzer_eval(v.len == 3); // expected-warning{{TRUE}}
+#endif
}
#if __cplusplus >= 201703L
// 0. Construct the member variable.
@@ -227,4 +240,53 @@
#endif
}
+
+ClassWithDestructor make1(AddressVector &v) {
+ return ClassWithDestructor(v);
+}
+ClassWithDestructor make2(AddressVector &v) {
+ return make1(v);
+}
+ClassWithDestructor make3(AddressVector &v) {
+ return make2(v);
+}
+
+void testMultipleReturnsWithDestructors() {
+ AddressVector v;
+ {
+ClassWithDestructor c = make3(v);
+// Check if the last destructor is an automatic destructor.
+// A temporary destructor would have fired by now.
+#if __cplusplus >= 201703L
+clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
+#else
+clang_analyzer_eval(v.len == 9); // expected-warning{{TRUE}}
+#endif
+ }
+
+#if __cplusplus >= 201703L
+ // 0. Construct the variable. Yes, constructor in make1() constructs
+ //the variable 'c'.
+ // 1. Destroy the variable.
+ clang_analyzer_eval(v.len == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == v.buf[1]); // expected-warning{{TRUE}}
+#else
+ // 0. Construct the temporary in make1().
+ // 1. Construct the temporary in make2().
+ // 2. Destroy the temporary in make1().
+ // 3. Construct the temporary in make3().
+ // 4. Destroy the temporary in make2().
+ // 5. Construct the temporary here.
+ // 6. Destroy the temporary in make3().
+ // 7. Construct the variable.
+ // 8. Destroy the temporary here.
+ // 9. Destroy the variable.
+ clang_analyzer_eval(v.len == 10); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[0] == v.buf[2]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[1] == v.buf[4]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[3] == v.buf[6]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[5] == v.buf[8]); // expected-warning{{TRUE}}
+ clang_analyzer_eval(v.buf[7] == v.buf[9]); // expected-warning{{TRUE}}
+#endif
+}
} // namespace address_vector_tests
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -180,7 +180,8 @@
}
break;
}
-case ConstructionContext::SimpleReturnedValueKind: {
+case ConstructionContext::SimpleReturnedValueKind:
+case ConstructionContext::C
[PATCH] D47405: [analyzer] Re-enable C++17-specific return value constructors.
NoQ added inline comments.
Comment at: test/Analysis/cxx17-mandatory-elision.cpp:185-191
+// Check if the last destructor is an automatic destructor.
+// A temporary destructor would have fired by now.
+#if __cplusplus >= 201703L
+clang_analyzer_eval(v.len == 1); // expected-warning{{TRUE}}
+#else
+clang_analyzer_eval(v.len == 3); // expected-warning{{TRUE}}
+#endif
This sounded like a good thing to check, so i added such checks in a few more
places.
Repository:
rC Clang
https://reviews.llvm.org/D47405
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47135: [analyzer] A checker for dangling internal buffer pointers in C++
NoQ accepted this revision.
NoQ added a comment.
Looks great, thanks! I think you should add a check for UnknownVal and commit.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:65
+ if (Call.isCalled(CStrFn)) {
+SymbolRef RawPtr = Call.getReturnValue().getAsSymbol();
+State = State->set(TypedR, RawPtr);
xazax.hun wrote:
> xazax.hun wrote:
> > I wonder if we can always get a symbol.
> > I can think of two cases when the call above could fail:
> > * Non-standard implementation that does not return a pointer
> > * The analyzer able to inline stuff and the returned value is a constant (a
> > specific address that is shared between all empty strings in some
> > implementation?)
> >
> > Even though I do find any of the above likely. @NoQ what do you think? Does
> > this worth a check?
> Sorry for the spam. Unfortunately, it is not possible to edit the comments.
> I do *not* find any of the above likely.
We should almost always check if any value is an `UnknownVal`. The engine
simply doesn't give any guarantees: it can give up any time and fall back to
`UnknownVal`.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:73
+if (State->contains(TypedR)) {
+ const SymbolRef *StrBufferPtr = State->get(TypedR);
+ const Expr *Origin = Call.getOriginExpr();
xazax.hun wrote:
> xazax.hun wrote:
> > What if no symbol is associated with the region? Won't this return null
> > that we dereference later on?
> Oh, never mind this one, I did not notice the `contains` call above.
The interesting part here is what happens if no expression is associated with
the call. For instance, if the call is an automatic destructor at the end of
scope. I hope it works, but i'm not sure how Origin is used.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1661
+ case AF_CXXNewArray:
+ case AF_InternalBuffer: {
if (IsALeakCheck) {
rnkovacs wrote:
> Is tying this new family to NewDeleteChecker reasonable? I did it because it
> was NewDelete that warned naturally before creating the new
> `AllocationFamily`. Should I introduce a new checker kind in MallocChecker
> for this purpose instead?
Uhm, this code is weird.
I think you can add an "external" ("associated", "plugin") CheckKind that
always warns.
https://reviews.llvm.org/D47135
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47417: [analyzer] Add missing state transition in IteratorChecker
NoQ added a comment. Nice catch! Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:399 + + C.addTransition(State); } MTC wrote: > I have two questions may need @NoQ or @xazax.hun who is more familiar with > the analyzer engine help to answer. > > - `State` may not change at all, do we need a check here like [[ > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp#L227 > | if (state != originalState) ]] > - A more basic problem is that do we need `originalState = State` trick. It > seems that `addTransitionImpl()` has a check about same state transition, see > [[ > https://github.com/llvm-mirror/clang/blob/master/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h#L339 > | addTransitionImp() ]]. > > Thanks in advance! > > > > It seems that `addTransitionImpl()` has a check about same state transition, > see `addTransitionImp()`. Yep, you pretty much answered your question. The check in the checker code is unnecessary. Repository: rC Clang https://reviews.llvm.org/D47417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47416: [analyzer] Clean up the program state map of DanglingInternalBufferChecker
NoQ added a comment.
Yep, great stuff!
I guess, please add a comment on incomplete destructor support in the CFG
and/or analyzer core that may cause the region to be never cleaned up. Or
perform an extra cleanup pass - not sure if it's worth it, but it should be
easy and safe.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:94
+ RawPtrMapTy RPM = State->get();
+ for (const auto Region : RPM) {
+if (SymReaper.isDead(Region.second))
It's kinda weird that something that's not a `const MemRegion *` is called
"Region".
I'd rather use a neutral term like "Item".
Repository:
rC Clang
https://reviews.llvm.org/D47416
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47451: [analyzer] Remove the redundant check about same state transition in `ArrayBoundCheckerV2.cpp`.
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Thx! Repository: rC Clang https://reviews.llvm.org/D47451 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35110: [Analyzer] Constraint Manager Negates Difference
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:191
+}
+if (from.isMinSignedValue()) {
+ F.add(newRanges, Range(BV.getMinValue(from), BV.getMinValue(from)));
We'll also need to merge the two adjacent segments if the original range had
both a [MinSingedValue, MinSignedValue] and a [X, MaxSignedValue]:
{F6287707}
Because our immutable sets are sorted, you can conduct the check for the first
and the last segment separately.
I think this code needs comments because even though it's short it's pretty
hard to get right.
Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:192
+if (from.isMinSignedValue()) {
+ F.add(newRanges, Range(BV.getMinValue(from), BV.getMinValue(from)));
+}
Return value of `add` seems to be accidentally discarded here.
I guess i'll look into adding an `__attribute__((warn_unused_result))` to these
functions, because it's a super common bug.
https://reviews.llvm.org/D35110
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35110: [Analyzer] Constraint Manager Negates Difference
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:192
+if (from.isMinSignedValue()) {
+ F.add(newRanges, Range(BV.getMinValue(from), BV.getMinValue(from)));
+}
NoQ wrote:
> Return value of `add` seems to be accidentally discarded here.
>
> I guess i'll look into adding an `__attribute__((warn_unused_result))` to
> these functions, because it's a super common bug.
Also tests would have saved us.
https://reviews.llvm.org/D35110
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47499: [analyzer] Annotate program state update methods with LLVM_NODISCARD.
NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, rnkovacs, baloghadamsoftware. Herald added a subscriber: cfe-commits. A follow-up to https://reviews.llvm.org/D47496. This would warn the developer on the very common bug that consists in writing, eg., `State->set(Key, Value)` instead of `State = State->set(Key, Value)`. Because in the first snippet the updated `State` object is discarded while the original object remains unchanged (because it's, well, immutable), which isn't what you ever want to do. For now no such bugs were found in the analyzer, but i made these mistakes myself knowingly and i see many such bugs during code review. Repository: rC Clang https://reviews.llvm.org/D47499 Files: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h Index: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h === --- include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h +++ include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h @@ -177,38 +177,38 @@ /// /// This returns a new state with the added constraint on \p cond. /// If no new state is feasible, NULL is returned. - ProgramStateRef assume(DefinedOrUnknownSVal cond, bool assumption) const; + LLVM_NODISCARD ProgramStateRef assume(DefinedOrUnknownSVal cond, +bool assumption) const; /// Assumes both "true" and "false" for \p cond, and returns both /// corresponding states (respectively). /// /// This is more efficient than calling assume() twice. Note that one (but not /// both) of the returned states may be NULL. - std::pair + LLVM_NODISCARD std::pair assume(DefinedOrUnknownSVal cond) const; - ProgramStateRef assumeInBound(DefinedOrUnknownSVal idx, - DefinedOrUnknownSVal upperBound, - bool assumption, - QualType IndexType = QualType()) const; + LLVM_NODISCARD ProgramStateRef + assumeInBound(DefinedOrUnknownSVal idx, DefinedOrUnknownSVal upperBound, +bool assumption, QualType IndexType = QualType()) const; /// Assumes that the value of \p Val is bounded with [\p From; \p To] /// (if \p assumption is "true") or it is fully out of this range /// (if \p assumption is "false"). /// /// This returns a new state with the added constraint on \p cond. /// If no new state is feasible, NULL is returned. - ProgramStateRef assumeInclusiveRange(DefinedOrUnknownSVal Val, - const llvm::APSInt &From, - const llvm::APSInt &To, - bool assumption) const; + LLVM_NODISCARD ProgramStateRef assumeInclusiveRange(DefinedOrUnknownSVal Val, + const llvm::APSInt &From, + const llvm::APSInt &To, + bool assumption) const; /// Assumes given range both "true" and "false" for \p Val, and returns both /// corresponding states (respectively). /// /// This is more efficient than calling assume() twice. Note that one (but not /// both) of the returned states may be NULL. - std::pair + LLVM_NODISCARD std::pair assumeInclusiveRange(DefinedOrUnknownSVal Val, const llvm::APSInt &From, const llvm::APSInt &To) const; @@ -232,30 +232,32 @@ /// Create a new state by binding the value 'V' to the statement 'S' in the /// state's environment. - ProgramStateRef BindExpr(const Stmt *S, const LocationContext *LCtx, - SVal V, bool Invalidate = true) const; + LLVM_NODISCARD ProgramStateRef BindExpr(const Stmt *S, + const LocationContext *LCtx, SVal V, + bool Invalidate = true) const; - ProgramStateRef bindLoc(Loc location, - SVal V, - const LocationContext *LCtx, - bool notifyChanges = true) const; + LLVM_NODISCARD ProgramStateRef bindLoc(Loc location, SVal V, + const LocationContext *LCtx, + bool notifyChanges = true) const; - ProgramStateRef bindLoc(SVal location, SVal V, const LocationContext *LCtx) const; + LLVM_NODISCARD ProgramStateRef bindLoc(SVal location, SVal V, + const LocationContext *LCtx) const; /// Initializes the region of memory represented by \p loc with an initial /// value. Once initialized, all values loaded from any sub-regions of that /// region will be equal to \p V, unless overwritten later by the program. /// This method should not be used on regions that are alrea
[PATCH] D45517: [analyzer] False positive refutation with Z3
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:1292 + Constraints = + Z3Expr::fromBinOp(Constraints, BO_LOr, SymRange, IsSignedTy); +} george.karpenkov wrote: > I'm very confused as to why are we doing disjunctions here. I think this corresponds to RangeSet being a union of Ranges. https://reviews.llvm.org/D45517 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37023: [analyzer] Fix bugreporter::getDerefExpr() again.
NoQ updated this revision to Diff 116542.
NoQ added a comment.
@dcoughlin: You're right, my reasoning and understanding was not correct, and
your explanation is much more clear. My code still makes sense to me though, so
i updated the comments to match. And moved the unusual logic for the
lvalue-to-rvalue cast unwrap to the bottom of the function.
Essentially, `getDerefExpr()` tries its best to explain where the null pointer
comes from, on the syntactic level, by unpacking expressions that wrap the
expression from which the pointer "originates", where "originates" is
understood in a vague manner defined only by what the callers of this utility
function expect to see. This leaves us with a few kinds of expressions that we
need to unwrap, and we shouldn't touch other expressions, leaving
pattern-matching over them to the caller.
There are other facilities that work on non-syntactic level, like your example
where we might jump from function expression to return statement within the
callee if the callee was inlined, or how `getNilReceiver()` function unwraps
`ObjCMessageExpr` to the `self` pointer *iff* it `self` was `nil` during
symbolic execution (which implies that the whole message expression is `nil`).
So that's right - lvalue-to-rvalue casts are not "the whole point", but merely
an example of an expression kind that we do not have a right to unwrap. In
fact, the callers still expect us to unwrap it once, but immediately stop
afterwards. This inconsistency should probably be fixed, but `getDerefExpr` is
used by checkers, and current code in checkers seems clean and concise enough.
I had a look at other cast kinds in `OperationKinds.def`, and all of them
seemed as if they're worth unwrapping, apart from, of course,
`CK_LValueToRValue`.
@xazax.hun: So, i guess, if the cast is earlier than we'd expect, then we'd
just fail to unwrap something. So we won't find where the pointer comes from,
and give up prematurely on our pattern-matching, while looking at roughly the
same expression/value, plus-minus offsets. It sounds better than the case when
the cast is //later// than we'd expect, where we may accidentally unwrap wrong
stuff and start tracking a completely unrelated value, so i hope it shouldn't
be a problem. Thanks for sharing the concern - the AST is huge and very few
people possess really deep understanding of it, so any curious findings about
it are really nice to share.
https://reviews.llvm.org/D37023
Files:
lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
test/Analysis/null-deref-path-notes.m
Index: test/Analysis/null-deref-path-notes.m
===
--- test/Analysis/null-deref-path-notes.m
+++ test/Analysis/null-deref-path-notes.m
@@ -50,6 +50,23 @@
*p = 1; // expected-warning{{Dereference of null pointer}} expected-note{{Dereference of null pointer}}
}
+@interface WithArrayPtr
+- (void) useArray;
+@end
+
+@implementation WithArrayPtr {
+@public int *p;
+}
+- (void)useArray {
+ p[1] = 2; // expected-warning{{Array access (via ivar 'p') results in a null pointer dereference}}
+// expected-note@-1{{Array access (via ivar 'p') results in a null pointer dereference}}
+}
+@end
+
+void testWithArrayPtr(WithArrayPtr *w) {
+ w->p = 0; // expected-note{{Null pointer value stored to 'p'}}
+ [w useArray]; // expected-note{{Calling 'useArray'}}
+}
// CHECK: diagnostics
// CHECK-NEXT:
@@ -801,4 +818,227 @@
// CHECK-NEXT:file0
// CHECK-NEXT:
// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:path
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: kindevent
+// CHECK-NEXT: location
+// CHECK-NEXT:
+// CHECK-NEXT: line67
+// CHECK-NEXT: col3
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT: ranges
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line67
+// CHECK-NEXT: col3
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line67
+// CHECK-NEXT: col10
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: depth0
+// CHECK-NEXT: extended_message
+// CHECK-NEXT: Null pointer value stored to 'p'
+// CHECK-NEXT: message
+// CHECK-NEXT: Null pointer value stored to 'p'
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: kindcontrol
+// CHECK-NEXT: edges
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: start
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:line67
+// CHECK-NEXT:col3
+// CHECK-NEXT:file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:line67
+// CHECK-NEXT:col3
+// CHECK-NEXT:file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: end
+//
[PATCH] D37023: [analyzer] Fix bugreporter::getDerefExpr() again.
NoQ updated this revision to Diff 116673.
NoQ added a comment.
Add no-crash test cases from https://bugs.llvm.org/show_bug.cgi?id=34373 and
https://bugs.llvm.org/show_bug.cgi?id=34731 .
https://reviews.llvm.org/D37023
Files:
lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
test/Analysis/null-deref-path-notes.c
test/Analysis/null-deref-path-notes.cpp
test/Analysis/null-deref-path-notes.m
Index: test/Analysis/null-deref-path-notes.m
===
--- test/Analysis/null-deref-path-notes.m
+++ test/Analysis/null-deref-path-notes.m
@@ -50,6 +50,23 @@
*p = 1; // expected-warning{{Dereference of null pointer}} expected-note{{Dereference of null pointer}}
}
+@interface WithArrayPtr
+- (void) useArray;
+@end
+
+@implementation WithArrayPtr {
+@public int *p;
+}
+- (void)useArray {
+ p[1] = 2; // expected-warning{{Array access (via ivar 'p') results in a null pointer dereference}}
+// expected-note@-1{{Array access (via ivar 'p') results in a null pointer dereference}}
+}
+@end
+
+void testWithArrayPtr(WithArrayPtr *w) {
+ w->p = 0; // expected-note{{Null pointer value stored to 'p'}}
+ [w useArray]; // expected-note{{Calling 'useArray'}}
+}
// CHECK: diagnostics
// CHECK-NEXT:
@@ -801,4 +818,227 @@
// CHECK-NEXT:file0
// CHECK-NEXT:
// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:path
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: kindevent
+// CHECK-NEXT: location
+// CHECK-NEXT:
+// CHECK-NEXT: line67
+// CHECK-NEXT: col3
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT: ranges
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line67
+// CHECK-NEXT: col3
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line67
+// CHECK-NEXT: col10
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: depth0
+// CHECK-NEXT: extended_message
+// CHECK-NEXT: Null pointer value stored to 'p'
+// CHECK-NEXT: message
+// CHECK-NEXT: Null pointer value stored to 'p'
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: kindcontrol
+// CHECK-NEXT: edges
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: start
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:line67
+// CHECK-NEXT:col3
+// CHECK-NEXT:file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:line67
+// CHECK-NEXT:col3
+// CHECK-NEXT:file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: end
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:line68
+// CHECK-NEXT:col3
+// CHECK-NEXT:file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:line68
+// CHECK-NEXT:col3
+// CHECK-NEXT:file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: kindevent
+// CHECK-NEXT: location
+// CHECK-NEXT:
+// CHECK-NEXT: line68
+// CHECK-NEXT: col3
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT: ranges
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line68
+// CHECK-NEXT: col3
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: line68
+// CHECK-NEXT: col14
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: depth0
+// CHECK-NEXT: extended_message
+// CHECK-NEXT: Calling 'useArray'
+// CHECK-NEXT: message
+// CHECK-NEXT: Calling 'useArray'
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: kindevent
+// CHECK-NEXT: location
+// CHECK-NEXT:
+// CHECK-NEXT: line60
+// CHECK-NEXT: col1
+// CHECK-NEXT: file0
+// CHECK-NEXT:
+// CHECK-NEXT: depth1
+// CHECK-NEXT: extended_message
+// CHECK-NEXT: Entered call from 'testWithArrayPtr'
+// CHECK-NEXT: message
+// CHECK-NEXT: Entered call from 'testWithArrayPtr'
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: kindcontrol
+// CHECK-NEXT: edges
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: start
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:line60
+// CHECK-NEXT:col1
+// CHECK-NEXT:file0
+// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT:line60
+// CHECK-NEXT:col1
+// CHECK-NEXT:file0
+// CHECK-NEXT:
+// CHECK-NEX
[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.
NoQ created this revision.
In `ProgramState::getSVal(Loc, QualType)`, the `QualType` parameter, which
represents the type of the expected value, is optional. If it is not supplied,
it is auto-detected by looking at the memory region in `Loc`. For typed-value
regions such autodetection is trivial. For other kinds of regions (most
importantly, for symbolic regions) it apparently never worked correctly: it
detected the location type (pointer type), not the value type in this location
(pointee type).
Our tests contain numerous cases when such autodetection was working
incorrectly, but for existing tests it never mattered. There are three notable
places where the type was regularly auto-detected incorrectly:
1. `ExprEngine::performTrivialCopy()`. Trivial copy from symbolic references
never worked - test case attached.
2. `CallAndMessageChecker::uninitRefOrPointer()`. Here the code only cares if
the value is `Undefined` or not, so autodetected type didn't matter.
3. `GTestChecker::modelAssertionResultBoolConstructor()`. This is how the issue
was found in https://bugs.llvm.org/show_bug.cgi?id=34305 - test case attached.
I added a few more sanity checks - we'd now also fail with an assertion if we
are unable to autodetect the pointer type, warning the author of the checker to
pass the type explicitly.
It is sometimes indeed handy to put a void-pointer-typed `Loc` into
`getSVal(Loc)`, as demonstrated in the `exercise-ps.c`'s `f2()` test through
`CallAndMessageChecker` (case '2.' above). I handled this on the API side in
order to simplify checker API, implicitly turning `void` into `char`, though i
don't mind modifying `CallAndMessageChecker` to pass `CharTy` explicitly
instead.
https://reviews.llvm.org/D38358
Files:
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/ctor.mm
test/Analysis/gtest.cpp
Index: test/Analysis/gtest.cpp
===
--- test/Analysis/gtest.cpp
+++ test/Analysis/gtest.cpp
@@ -151,3 +151,9 @@
ASSERT_TRUE(false);
clang_analyzer_warnIfReached(); // no-warning
}
+
+void testAssertSymbolic(const bool &b) {
+ ASSERT_TRUE(b);
+
+ clang_analyzer_eval(b); // expected-warning{{UNKNOWN}}
+}
Index: test/Analysis/ctor.mm
===
--- test/Analysis/ctor.mm
+++ test/Analysis/ctor.mm
@@ -199,7 +199,7 @@
Inner p;
};
- void testPOD() {
+ void testPOD(const POD &pp) {
POD p;
p.x = 1;
POD p2 = p; // no-warning
@@ -210,6 +210,15 @@
// Use rvalues as well.
clang_analyzer_eval(POD(p3).x == 1); // expected-warning{{TRUE}}
+// Copy from symbolic references correctly.
+POD p4 = pp;
+// Make sure that p4.x contains a symbol after copy.
+if (p4.x > 0)
+ clang_analyzer_eval(p4.x > 0); // expected-warning{{TRUE}}
+// FIXME: Element region gets in the way, so these aren't the same symbols
+// as they should be.
+clang_analyzer_eval(pp.x == p4.x); // expected-warning{{UNKNOWN}}
+
PODWrapper w;
w.p.y = 1;
PODWrapper w2 = w; // no-warning
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1393,17 +1393,20 @@
return UnknownVal();
}
- if (isa(MR) ||
- isa(MR) ||
- isa(MR)) {
+ if (!isa(MR)) {
if (T.isNull()) {
- if (const TypedRegion *TR = dyn_cast(MR))
-T = TR->getLocationType();
- else {
-const SymbolicRegion *SR = cast(MR);
-T = SR->getSymbol()->getType();
+ if (const TypedRegion *TR = dyn_cast(MR)) {
+T = TR->getLocationType()->getPointeeType();
+ } else if (const SymbolicRegion *SR = dyn_cast(MR)) {
+T = SR->getSymbol()->getType()->getPointeeType();
+if (T->isVoidType()) {
+ // When trying to dereference a void pointer, read the first byte.
+ T = Ctx.CharTy;
+}
}
}
+assert(!T.isNull() && "Unable to auto-detect binding type!");
+assert(!T->isVoidType() && "Attempted to retrieve a void value!");
MR = GetElementZeroRegion(cast(MR), T);
}
Index: test/Analysis/gtest.cpp
===
--- test/Analysis/gtest.cpp
+++ test/Analysis/gtest.cpp
@@ -151,3 +151,9 @@
ASSERT_TRUE(false);
clang_analyzer_warnIfReached(); // no-warning
}
+
+void testAssertSymbolic(const bool &b) {
+ ASSERT_TRUE(b);
+
+ clang_analyzer_eval(b); // expected-warning{{UNKNOWN}}
+}
Index: test/Analysis/ctor.mm
===
--- test/Analysis/ctor.mm
+++ test/Analysis/ctor.mm
@@ -199,7 +199,7 @@
Inner p;
};
- void testPOD() {
+ void testPOD(const POD &pp) {
POD p;
p.x = 1;
POD p2 = p; // no-warning
@@ -210,6 +210,15 @@
// Use rvalues as well.
clang_analyzer_eval(POD(
[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.
NoQ updated this revision to Diff 116986.
NoQ added a comment.
Add a forgotten comment.
https://reviews.llvm.org/D38358
Files:
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/ctor.mm
test/Analysis/gtest.cpp
Index: test/Analysis/gtest.cpp
===
--- test/Analysis/gtest.cpp
+++ test/Analysis/gtest.cpp
@@ -151,3 +151,10 @@
ASSERT_TRUE(false);
clang_analyzer_warnIfReached(); // no-warning
}
+
+void testAssertSymbolic(const bool &b) {
+ ASSERT_TRUE(b);
+
+ // FIXME: Our solver doesn't handle this well yet.
+ clang_analyzer_eval(b); // expected-warning{{UNKNOWN}}
+}
Index: test/Analysis/ctor.mm
===
--- test/Analysis/ctor.mm
+++ test/Analysis/ctor.mm
@@ -199,7 +199,7 @@
Inner p;
};
- void testPOD() {
+ void testPOD(const POD &pp) {
POD p;
p.x = 1;
POD p2 = p; // no-warning
@@ -210,6 +210,15 @@
// Use rvalues as well.
clang_analyzer_eval(POD(p3).x == 1); // expected-warning{{TRUE}}
+// Copy from symbolic references correctly.
+POD p4 = pp;
+// Make sure that p4.x contains a symbol after copy.
+if (p4.x > 0)
+ clang_analyzer_eval(p4.x > 0); // expected-warning{{TRUE}}
+// FIXME: Element region gets in the way, so these aren't the same symbols
+// as they should be.
+clang_analyzer_eval(pp.x == p4.x); // expected-warning{{UNKNOWN}}
+
PODWrapper w;
w.p.y = 1;
PODWrapper w2 = w; // no-warning
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1393,17 +1393,20 @@
return UnknownVal();
}
- if (isa(MR) ||
- isa(MR) ||
- isa(MR)) {
+ if (!isa(MR)) {
if (T.isNull()) {
- if (const TypedRegion *TR = dyn_cast(MR))
-T = TR->getLocationType();
- else {
-const SymbolicRegion *SR = cast(MR);
-T = SR->getSymbol()->getType();
+ if (const TypedRegion *TR = dyn_cast(MR)) {
+T = TR->getLocationType()->getPointeeType();
+ } else if (const SymbolicRegion *SR = dyn_cast(MR)) {
+T = SR->getSymbol()->getType()->getPointeeType();
+if (T->isVoidType()) {
+ // When trying to dereference a void pointer, read the first byte.
+ T = Ctx.CharTy;
+}
}
}
+assert(!T.isNull() && "Unable to auto-detect binding type!");
+assert(!T->isVoidType() && "Attempted to retrieve a void value!");
MR = GetElementZeroRegion(cast(MR), T);
}
Index: test/Analysis/gtest.cpp
===
--- test/Analysis/gtest.cpp
+++ test/Analysis/gtest.cpp
@@ -151,3 +151,10 @@
ASSERT_TRUE(false);
clang_analyzer_warnIfReached(); // no-warning
}
+
+void testAssertSymbolic(const bool &b) {
+ ASSERT_TRUE(b);
+
+ // FIXME: Our solver doesn't handle this well yet.
+ clang_analyzer_eval(b); // expected-warning{{UNKNOWN}}
+}
Index: test/Analysis/ctor.mm
===
--- test/Analysis/ctor.mm
+++ test/Analysis/ctor.mm
@@ -199,7 +199,7 @@
Inner p;
};
- void testPOD() {
+ void testPOD(const POD &pp) {
POD p;
p.x = 1;
POD p2 = p; // no-warning
@@ -210,6 +210,15 @@
// Use rvalues as well.
clang_analyzer_eval(POD(p3).x == 1); // expected-warning{{TRUE}}
+// Copy from symbolic references correctly.
+POD p4 = pp;
+// Make sure that p4.x contains a symbol after copy.
+if (p4.x > 0)
+ clang_analyzer_eval(p4.x > 0); // expected-warning{{TRUE}}
+// FIXME: Element region gets in the way, so these aren't the same symbols
+// as they should be.
+clang_analyzer_eval(pp.x == p4.x); // expected-warning{{UNKNOWN}}
+
PODWrapper w;
w.p.y = 1;
PODWrapper w2 = w; // no-warning
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1393,17 +1393,20 @@
return UnknownVal();
}
- if (isa(MR) ||
- isa(MR) ||
- isa(MR)) {
+ if (!isa(MR)) {
if (T.isNull()) {
- if (const TypedRegion *TR = dyn_cast(MR))
-T = TR->getLocationType();
- else {
-const SymbolicRegion *SR = cast(MR);
-T = SR->getSymbol()->getType();
+ if (const TypedRegion *TR = dyn_cast(MR)) {
+T = TR->getLocationType()->getPointeeType();
+ } else if (const SymbolicRegion *SR = dyn_cast(MR)) {
+T = SR->getSymbol()->getType()->getPointeeType();
+if (T->isVoidType()) {
+ // When trying to dereference a void pointer, read the first byte.
+ T = Ctx.CharTy;
+}
}
}
+assert(!T.isNull() && "Unable to auto-detect binding type!");
+assert(!T->is
[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.
NoQ updated this revision to Diff 116992.
NoQ added a subscriber: alexfh.
NoQ added a comment.
Add @alexfh's small reproducer test case. It was so small i never noticed it
until now!
https://reviews.llvm.org/D38358
Files:
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/ctor.mm
test/Analysis/gtest.cpp
Index: test/Analysis/gtest.cpp
===
--- test/Analysis/gtest.cpp
+++ test/Analysis/gtest.cpp
@@ -151,3 +151,17 @@
ASSERT_TRUE(false);
clang_analyzer_warnIfReached(); // no-warning
}
+
+void testAssertSymbolicPtr(const bool *b) {
+ ASSERT_TRUE(*b); // no-crash
+
+ // FIXME: Our solver doesn't handle this well yet.
+ clang_analyzer_eval(*b); // expected-warning{{UNKNOWN}}
+}
+
+void testAssertSymbolicRef(const bool &b) {
+ ASSERT_TRUE(b); // no-crash
+
+ // FIXME: Our solver doesn't handle this well yet.
+ clang_analyzer_eval(b); // expected-warning{{UNKNOWN}}
+}
Index: test/Analysis/ctor.mm
===
--- test/Analysis/ctor.mm
+++ test/Analysis/ctor.mm
@@ -199,7 +199,7 @@
Inner p;
};
- void testPOD() {
+ void testPOD(const POD &pp) {
POD p;
p.x = 1;
POD p2 = p; // no-warning
@@ -210,6 +210,15 @@
// Use rvalues as well.
clang_analyzer_eval(POD(p3).x == 1); // expected-warning{{TRUE}}
+// Copy from symbolic references correctly.
+POD p4 = pp;
+// Make sure that p4.x contains a symbol after copy.
+if (p4.x > 0)
+ clang_analyzer_eval(p4.x > 0); // expected-warning{{TRUE}}
+// FIXME: Element region gets in the way, so these aren't the same symbols
+// as they should be.
+clang_analyzer_eval(pp.x == p4.x); // expected-warning{{UNKNOWN}}
+
PODWrapper w;
w.p.y = 1;
PODWrapper w2 = w; // no-warning
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1393,17 +1393,20 @@
return UnknownVal();
}
- if (isa(MR) ||
- isa(MR) ||
- isa(MR)) {
+ if (!isa(MR)) {
if (T.isNull()) {
- if (const TypedRegion *TR = dyn_cast(MR))
-T = TR->getLocationType();
- else {
-const SymbolicRegion *SR = cast(MR);
-T = SR->getSymbol()->getType();
+ if (const TypedRegion *TR = dyn_cast(MR)) {
+T = TR->getLocationType()->getPointeeType();
+ } else if (const SymbolicRegion *SR = dyn_cast(MR)) {
+T = SR->getSymbol()->getType()->getPointeeType();
+if (T->isVoidType()) {
+ // When trying to dereference a void pointer, read the first byte.
+ T = Ctx.CharTy;
+}
}
}
+assert(!T.isNull() && "Unable to auto-detect binding type!");
+assert(!T->isVoidType() && "Attempted to retrieve a void value!");
MR = GetElementZeroRegion(cast(MR), T);
}
Index: test/Analysis/gtest.cpp
===
--- test/Analysis/gtest.cpp
+++ test/Analysis/gtest.cpp
@@ -151,3 +151,17 @@
ASSERT_TRUE(false);
clang_analyzer_warnIfReached(); // no-warning
}
+
+void testAssertSymbolicPtr(const bool *b) {
+ ASSERT_TRUE(*b); // no-crash
+
+ // FIXME: Our solver doesn't handle this well yet.
+ clang_analyzer_eval(*b); // expected-warning{{UNKNOWN}}
+}
+
+void testAssertSymbolicRef(const bool &b) {
+ ASSERT_TRUE(b); // no-crash
+
+ // FIXME: Our solver doesn't handle this well yet.
+ clang_analyzer_eval(b); // expected-warning{{UNKNOWN}}
+}
Index: test/Analysis/ctor.mm
===
--- test/Analysis/ctor.mm
+++ test/Analysis/ctor.mm
@@ -199,7 +199,7 @@
Inner p;
};
- void testPOD() {
+ void testPOD(const POD &pp) {
POD p;
p.x = 1;
POD p2 = p; // no-warning
@@ -210,6 +210,15 @@
// Use rvalues as well.
clang_analyzer_eval(POD(p3).x == 1); // expected-warning{{TRUE}}
+// Copy from symbolic references correctly.
+POD p4 = pp;
+// Make sure that p4.x contains a symbol after copy.
+if (p4.x > 0)
+ clang_analyzer_eval(p4.x > 0); // expected-warning{{TRUE}}
+// FIXME: Element region gets in the way, so these aren't the same symbols
+// as they should be.
+clang_analyzer_eval(pp.x == p4.x); // expected-warning{{UNKNOWN}}
+
PODWrapper w;
w.p.y = 1;
PODWrapper w2 = w; // no-warning
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1393,17 +1393,20 @@
return UnknownVal();
}
- if (isa(MR) ||
- isa(MR) ||
- isa(MR)) {
+ if (!isa(MR)) {
if (T.isNull()) {
- if (const TypedRegion *TR = dyn_cast(MR))
-T = TR->getLocationType();
- else {
-const SymbolicRegion *SR = cast(
[PATCH] D35216: [analyzer] Escape symbols when creating std::initializer_list.
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1127
+// only consist of ObjC objects, and escapes of ObjC objects
+// aren't so important (eg., retain count checker ignores them).
+if (isa(Ex) ||
dcoughlin wrote:
> Note that we do have other ObjC checkers that rely on escaping of ObjC
> objects, such as the ObjCLoopChecker and ObjCDeallocChecker. I think having
> the TODO is great, but I'd like you to remove the the bit about "escapes of
> ObjC objects aren't so important".
Hmm, should have double-checked. Will fix.
Comment at: test/Analysis/objc-boxing.m:66
+ BoxableStruct bs;
+ bs.str = strdup("dynamic string"); // The duped string shall be owned by val.
+ NSValue *val = @(bs); // no-warning
dcoughlin wrote:
> In this case the duped string is not owned by `val`. NSValue doesn't take
> ownership of the string, so this *will* leak and we should warn about it.
I mean, the pointer to the raw string is stored inside the `NSValue`, and can
be used or freed from there. The caller can free this string by looking into
the `val`, even though `val` itself won't release the pointer (i guess i messed
up the comment again). From MallocChecker's perspective, this is an escape and
no-warning. If we free the string in this function, it'd most likely cause
use-after-free in the caller.
I tested that the string is indeed not strduped during boxing:
**`$ cat test.m`**
```
#import
typedef struct __attribute__((objc_boxable)) {
const char *str;
} BoxableStruct;
int main() {
BoxableStruct bs;
bs.str = strdup("dynamic string");
NSLog(@"%p\n", bs.str);
NSValue *val = @(bs);
BoxableStruct bs2;
[val getValue:&bs2];
NSLog(@"%p\n", bs2.str);
return 0;
}
```
**`$ clang test.m -framework Foundation`**
**`$ ./a.out`**
```
2017-10-02 17:56:00.004 a.out[17933:1083757] 0x7ffd23407380
2017-10-02 17:56:00.004 a.out[17933:1083757] 0x7ffd23407380
```
So it's possible to retrieve the exact same pointer from the boxed value. So if
`val` is returned to the caller, like in the test, it shouldn't be freed.
If the `NSValue` itself dies and never escapes, then of course it's a leak, but
in order to see that we'd need to model contents of `NSValue`.
https://reviews.llvm.org/D35216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.
NoQ updated this revision to Diff 117360.
NoQ added a comment.
Yeah, nice catch. So we need to either always tell the checkers to specify
their `CharTy` when they are dealing with void pointers, or to do our
substitution consistently, not only for `SymbolicRegion` but also for
`AllocaRegion` (did that in this diff).
https://reviews.llvm.org/D38358
Files:
lib/StaticAnalyzer/Core/RegionStore.cpp
test/Analysis/ctor.mm
test/Analysis/exercise-ps.c
test/Analysis/gtest.cpp
Index: test/Analysis/gtest.cpp
===
--- test/Analysis/gtest.cpp
+++ test/Analysis/gtest.cpp
@@ -151,3 +151,17 @@
ASSERT_TRUE(false);
clang_analyzer_warnIfReached(); // no-warning
}
+
+void testAssertSymbolicPtr(const bool *b) {
+ ASSERT_TRUE(*b); // no-crash
+
+ // FIXME: Our solver doesn't handle this well yet.
+ clang_analyzer_eval(*b); // expected-warning{{UNKNOWN}}
+}
+
+void testAssertSymbolicRef(const bool &b) {
+ ASSERT_TRUE(b); // no-crash
+
+ // FIXME: Our solver doesn't handle this well yet.
+ clang_analyzer_eval(b); // expected-warning{{UNKNOWN}}
+}
Index: test/Analysis/exercise-ps.c
===
--- test/Analysis/exercise-ps.c
+++ test/Analysis/exercise-ps.c
@@ -21,3 +21,11 @@
memcpy((&x[1]), (buf), 1); // expected-warning{{implicitly declaring library
function 'memcpy' with type 'void *(void *, const void *}} \
// expected-note{{include the header or explicitly provide a
declaration for 'memcpy'}}
}
+
+// AllocaRegion is untyped. Void pointer isn't of much help either. Before
+// realizing that the value is undefined, we need to somehow figure out
+// what type of value do we expect.
+void f3(void *dest) {
+ void *src = __builtin_alloca(5);
+ memcpy(dest, src, 1); // expected-warning{{2nd function call argument is a
pointer to uninitialized value}}
+}
Index: test/Analysis/ctor.mm
===
--- test/Analysis/ctor.mm
+++ test/Analysis/ctor.mm
@@ -199,7 +199,7 @@
Inner p;
};
- void testPOD() {
+ void testPOD(const POD &pp) {
POD p;
p.x = 1;
POD p2 = p; // no-warning
@@ -210,6 +210,15 @@
// Use rvalues as well.
clang_analyzer_eval(POD(p3).x == 1); // expected-warning{{TRUE}}
+// Copy from symbolic references correctly.
+POD p4 = pp;
+// Make sure that p4.x contains a symbol after copy.
+if (p4.x > 0)
+ clang_analyzer_eval(p4.x > 0); // expected-warning{{TRUE}}
+// FIXME: Element region gets in the way, so these aren't the same symbols
+// as they should be.
+clang_analyzer_eval(pp.x == p4.x); // expected-warning{{UNKNOWN}}
+
PODWrapper w;
w.p.y = 1;
PODWrapper w2 = w; // no-warning
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1393,16 +1393,19 @@
return UnknownVal();
}
- if (isa(MR) ||
- isa(MR) ||
- isa(MR)) {
+ if (!isa(MR)) {
if (T.isNull()) {
if (const TypedRegion *TR = dyn_cast(MR))
-T = TR->getLocationType();
- else {
-const SymbolicRegion *SR = cast(MR);
-T = SR->getSymbol()->getType();
- }
+T = TR->getLocationType()->getPointeeType();
+ else if (const SymbolicRegion *SR = dyn_cast(MR))
+T = SR->getSymbol()->getType()->getPointeeType();
+ else if (isa(MR))
+T = Ctx.VoidTy;
+}
+assert(!T.isNull() && "Unable to auto-detect binding type!");
+if (T->isVoidType()) {
+ // When trying to dereference a void pointer, read the first byte.
+ T = Ctx.CharTy;
}
MR = GetElementZeroRegion(cast(MR), T);
}
Index: test/Analysis/gtest.cpp
===
--- test/Analysis/gtest.cpp
+++ test/Analysis/gtest.cpp
@@ -151,3 +151,17 @@
ASSERT_TRUE(false);
clang_analyzer_warnIfReached(); // no-warning
}
+
+void testAssertSymbolicPtr(const bool *b) {
+ ASSERT_TRUE(*b); // no-crash
+
+ // FIXME: Our solver doesn't handle this well yet.
+ clang_analyzer_eval(*b); // expected-warning{{UNKNOWN}}
+}
+
+void testAssertSymbolicRef(const bool &b) {
+ ASSERT_TRUE(b); // no-crash
+
+ // FIXME: Our solver doesn't handle this well yet.
+ clang_analyzer_eval(b); // expected-warning{{UNKNOWN}}
+}
Index: test/Analysis/exercise-ps.c
===
--- test/Analysis/exercise-ps.c
+++ test/Analysis/exercise-ps.c
@@ -21,3 +21,11 @@
memcpy((&x[1]), (buf), 1); // expected-warning{{implicitly declaring library function 'memcpy' with type 'void *(void *, const void *}} \
// expected-note{{include the header or explicitly provide a declaration for 'memcpy'}}
}
+
+// AllocaRegion is untyped. Void pointer isn't of much help either. Before
+// realizing that the v
[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.
NoQ added a comment.
Whoops forgot to submit inline comments.
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1404
+ // When trying to dereference a void pointer, read the first byte.
+ T = Ctx.CharTy;
+}
dcoughlin wrote:
> Nit: It seems a bit odd to read the first byte here since (unless I'm
> misunderstanding) this would never be triggered by actual C semantics, only
> by a checker. Did you consider just returning UnknownVal() in this case?
`UnknownVal` seems to be quite similar to assertion failure: in both cases we'd
have to force the checker to specify `CharTy` explicitly. But assertions are
better because the author of the checker would instantly know that he needs to
specify the type, instead of never noticing the problem, or spending a lot of
time in the debugger trying to understand why he gets an `UnknownVal`.
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1408
}
+assert(!T.isNull() && "Unable to auto-detect binding type!");
+assert(!T->isVoidType() && "Attempted to retrieve a void value!");
dcoughlin wrote:
> I think you missed handling the AllocaRegion case from the old version in
> your new version. This means the assert will fire on the following when
> core.alpha is enabled:
> ```
> void foo(void *dest) {
> void *src = __builtin_alloca(5);
> memcpy(dest, src, 1);
> }
> ```
Nice one!
Always been that way though. In the old code, if `AllocaRegion` is supplied and
no type is explicitly specified, `cast(MR)` would fail. Also
`ElementRegion` is created in both old and new code if the type was specified.
https://reviews.llvm.org/D38358
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1404 + // When trying to dereference a void pointer, read the first byte. + T = Ctx.CharTy; +} dcoughlin wrote: > NoQ wrote: > > dcoughlin wrote: > > > Nit: It seems a bit odd to read the first byte here since (unless I'm > > > misunderstanding) this would never be triggered by actual C semantics, > > > only by a checker. Did you consider just returning UnknownVal() in this > > > case? > > `UnknownVal` seems to be quite similar to assertion failure: in both cases > > we'd have to force the checker to specify `CharTy` explicitly. But > > assertions are better because the author of the checker would instantly > > know that he needs to specify the type, instead of never noticing the > > problem, or spending a lot of time in the debugger trying to understand why > > he gets an `UnknownVal`. > I think having an assertion with a helpful message indicating how the > checker author could fix the problem would be great! But you're not > triggering an assertion failure here since you're changing T to be CharTy. > Instead, you're papering over the problem by making up a fake value that > doesn't correspond to the program semantics. If you're going to paper over > the the problem and return a value, I think it is far preferable to use > UnknownVal. This would signal "we don't know what is going here" rather than > just making up a value that is likely wrong. > If you're going to paper over the the problem and return a value, I think it > is far preferable to use UnknownVal. If we return an UnknownVal, the user would never figure out what's wrong by looking at the value, i.e. what limitation of the analyzer he's stepping on (in fact he isn't, he's just using the APIs incorrectly, while we know very well what's going on). Additionally, returning the first byte makes API simpler in cases the type actually doesn't matter (which is why it's not provided), unlike returning UnknownVal. This makes me think that returning UnknownVal is worse than both returning first byte and stepping on assertion. However, yeah, if we try to model an API that manipulates objects of well-defined types through void pointers, returning bytes might cause issues when the user doesn't realize he needs to specify his types, especially when RegionStore would often ignore the type and only take the offset, unless it has no bindings and needs to return region value or derived symbol (so the user may easily fail to test this case). I guess i'd follow up with a patch to remove the defensive behavior and bring back the assertion and modify checkers to provide types when necessary, and later see if it's worth it to remove auto-detection completely. Repository: rL LLVM https://reviews.llvm.org/D38358 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35216: [analyzer] Escape symbols when creating std::initializer_list.
NoQ marked 3 inline comments as done. NoQ added a comment. In https://reviews.llvm.org/D35216#886212, @rsmith wrote: > This is precisely how the rest of the compiler handles > `CXXStdInitializerListExpr` Wow. Cool. I'd see what I can do. Yeah, it seems that this is a great case for us to pattern-match the implementations as well (the problems are still there for other STL stuff). Do you think this patch should be blocked in favor of complete modeling? Or i can land that and follow up with complete modeling later; it'd be a larger, but not much to discuss indeed. Or i can try always "inlining" initializer_list constructor and methods during analysis, if all methods are available in the header in all implementations. https://reviews.llvm.org/D35216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35216: [analyzer] Escape symbols when creating std::initializer_list.
NoQ updated this revision to Diff 117699.
NoQ added a comment.
Herald added a subscriber: szepet.
Escape into array and dictionary literals, add relevant tests. Fix the null
statement check.
https://reviews.llvm.org/D35216
Files:
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/initializer.cpp
test/Analysis/objc-boxing.m
test/Analysis/objc-for.m
Index: test/Analysis/objc-for.m
===
--- test/Analysis/objc-for.m
+++ test/Analysis/objc-for.m
@@ -1,6 +1,7 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,osx.cocoa.Loops,debug.ExprInspection -verify %s
void clang_analyzer_eval(int);
+void clang_analyzer_warnIfReached();
#define nil ((id)0)
@@ -20,11 +21,13 @@
@interface NSArray : NSObject
- (NSUInteger)count;
- (NSEnumerator *)objectEnumerator;
++ (NSArray *)arrayWithObjects:(const id [])objects count:(NSUInteger)count;
@end
@interface NSDictionary : NSObject
- (NSUInteger)count;
- (id)objectForKey:(id)key;
++ (id)dictionaryWithObjects:(const id [])objects forKeys:(const id /* */ [])keys count:(NSUInteger)count;
@end
@interface NSDictionary (SomeCategory)
@@ -324,3 +327,19 @@
for (id key in array)
clang_analyzer_eval(0); // expected-warning{{FALSE}}
}
+
+NSArray *globalArray;
+NSDictionary *globalDictionary;
+void boxedArrayEscape(NSMutableArray *array) {
+ if ([array count])
+return;
+ globalArray = @[array];
+ for (id key in array)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+
+ if ([array count])
+return;
+ globalDictionary = @{ @"array" : array };
+ for (id key in array)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
Index: test/Analysis/objc-boxing.m
===
--- test/Analysis/objc-boxing.m
+++ test/Analysis/objc-boxing.m
@@ -5,6 +5,16 @@
typedef signed char BOOL;
typedef long NSInteger;
typedef unsigned long NSUInteger;
+
+@protocol NSObject
+@end
+@interface NSObject {}
+@end
+@protocol NSCopying
+@end
+@protocol NSCoding
+@end
+
@interface NSString @end
@interface NSString (NSStringExtensionMethods)
+ (id)stringWithUTF8String:(const char *)nullTerminatedCString;
@@ -28,7 +38,15 @@
+ (NSNumber *)numberWithUnsignedInteger:(NSUInteger)value ;
@end
+@interface NSValue : NSObject
+- (void)getValue:(void *)value;
++ (NSValue *)valueWithBytes:(const void *)value
+ objCType:(const char *)type;
+@end
+typedef typeof(sizeof(int)) size_t;
+extern void *malloc(size_t);
+extern void free(void *);
extern char *strdup(const char *str);
id constant_string() {
@@ -39,6 +57,23 @@
return @(strdup("boxed dynamic string")); // expected-warning{{Potential memory leak}}
}
+typedef struct __attribute__((objc_boxable)) {
+ const char *str;
+} BoxableStruct;
+
+id leak_within_boxed_struct() {
+ BoxableStruct bs;
+ bs.str = strdup("dynamic string"); // The duped string shall be owned by val.
+ NSValue *val = @(bs); // no-warning
+ return val;
+}
+
+id leak_of_boxed_struct() {
+ BoxableStruct *bs = malloc(sizeof(BoxableStruct)); // The pointer stored in bs isn't owned by val.
+ NSValue *val = @(*bs); // expected-warning{{Potential leak of memory pointed to by 'bs'}}
+ return val;
+}
+
id const_char_pointer(int *x) {
if (x)
return @(3);
Index: test/Analysis/initializer.cpp
===
--- test/Analysis/initializer.cpp
+++ test/Analysis/initializer.cpp
@@ -1,7 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-config c++-inlining=constructors -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,cplusplus.NewDeleteLeaks,debug.ExprInspection -analyzer-config c++-inlining=constructors -std=c++11 -verify %s
void clang_analyzer_eval(bool);
+#include "Inputs/system-header-simulator-cxx.h"
+
class A {
int x;
public:
@@ -204,3 +206,17 @@
const char(&f)[2];
};
}
+
+namespace CXX_initializer_lists {
+struct C {
+ C(std::initializer_list list);
+};
+void foo() {
+ C empty{}; // no-crash
+
+ // Do not warn that 'x' leaks. It might have been deleted by
+ // the destructor of 'c'.
+ int *x = new int;
+ C c{x}; // no-warning
+}
+}
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -827,6 +827,21 @@
}
}
+namespace {
+class CollectReachableSymbolsCallback final : public SymbolVisitor {
+ InvalidatedSymbols Symbols;
+
+public:
+ explicit CollectReachableSymbolsCallback(ProgramStateRef State) {}
+ const InvalidatedSymbols &getSymbols() const { return Symbols; }
+
+ bool VisitSymbol(SymbolRef Sym) override {
+Symbols.insert(Sym);
+return true;
+ }
+};
+} // end anonymous namespace
+
void ExprEngine::Visit(const Stmt *S, Explode
[PATCH] D35216: [analyzer] Escape symbols when creating std::initializer_list.
NoQ added inline comments.
Comment at: test/Analysis/objc-for.m:328
for (id key in array)
clang_analyzer_eval(0); // expected-warning{{FALSE}}
}
I guess these old tests should be replaced with `warnIfReached()`.
https://reviews.llvm.org/D35216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35216: [analyzer] Escape symbols when creating std::initializer_list.
NoQ added a comment.
In https://reviews.llvm.org/D35216#886212, @rsmith wrote:
> This is precisely how the rest of the compiler handles
> `CXXStdInitializerListExpr`
Wow. Cool. I'd see what I can do. Yeah, it seems that this is a great case for
us to pattern-match the implementations as well (the problems are still there
for other STL stuff).
Do you think this patch should be blocked? Or i can land that and follow up
with complete modeling later; it'd be larger.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1132
+ for (auto Child : Ex->children()) {
+if (!Child)
+ continue;
dcoughlin wrote:
> Is this 'if' needed?
Not sure, wanted to be defensive. It seems that `CXXStdInitializerList` always
contains a single child (`InitListExpr`) so it's irrelevant if the list is
empty, while boxed expressions contain no children when empty, and hanging
commas are ignored. Replaced with an assertion just in case.
https://reviews.llvm.org/D35216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
