Re: [PATCH] D21506: [analyzer] Block in critical section
zaks.anna added a comment. This checker is now in alpha.unix, because it is new and is in active development. However, alpha checkers are not supported and are not turned on by default, so we should move it into unix package once we think it is ready to be used. Evaluation on a large real codebase (or several) would give us a higher confidence in the checker, so it will be valuable to perform that before we move the package out of alpha. However, clang is probably not a good codebase to test this on because it's not heavily multithreaded. Repository: rL LLVM https://reviews.llvm.org/D21506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24411: [Analyzer] Suppress false positives on the clang static analyzer
zaks.anna added a comment. The thread from cfe-dev is called "Clang Static Analyzer: False Positive Suppression Support": http://clang-developers.42468.n3.nabble.com/Clang-Static-Analyzer-False-Positive-Suppression-Support-tt4053071.html https://reviews.llvm.org/D24411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24759: [RFC][StaticAnalyser] fix unreachable code false positives when block numbers mismatch
zaks.anna added a comment. Sorry, I do not understand the question. What are block numbers? https://reviews.llvm.org/D24759 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24507: Add attribute for return values that shouldn't be cast to bool
zaks.anna added inline comments. Comment at: test/Analysis/ReturnNonBoolTest.c:67 @@ +66,3 @@ + + if (rc < 0) +// error handling How about addressing this as follows: in checkBranchCondition, you check for any comparisons of the tracked value other than comparisons to bool. If you see such a comparison, you assume that the error handling has occurred and remove the symbol from the set of tracked symbols. This will ensure that any code after the cleansing condition (error handling) can cast the return value to bool. The warning will still get triggered if the error handling is **after** the comparison to bool. That could be avoided as well, but the solution would be more complicated. I am thinking something along the lines of tracking all comparisons until the symbol goes out of scope. For each symbol, you'd track it's state (for example, "performedErrorHandling | comparedToBoolAndNoErrorHandling | notSeen"). You can draw the automaton to see what the transitions should be. When the symbol goes out of scope, you'd check if it's state is "comparedToBoolAndNoErrorHandling". Further, we'd need to walk up the path to find the location where we compared the symbol and use that for error reporting. Repository: rL LLVM https://reviews.llvm.org/D24507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22494: [analyzer] Explain why analyzer report is not generated (fix for PR12421).
zaks.anna added a comment. LGTM. Thanks. https://reviews.llvm.org/D22494 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r282011 - [analyzer] Add a checker that detects blocks in critical sections
Author: zaks
Date: Tue Sep 20 15:28:50 2016
New Revision: 282011
URL: http://llvm.org/viewvc/llvm-project?rev=282011&view=rev
Log:
[analyzer] Add a checker that detects blocks in critical sections
This checker should find the calls to blocking functions (for example: sleep,
getc, fgets,read,recv etc.) inside a critical section. When sleep(x) is called
while a mutex is held, other threads cannot lock the same mutex. This might
take some time, leading to bad performance or even deadlock.
Example:
mutex_t m;
void f() {
sleep(1000); // Error: sleep() while m is locked! [f() is called from
foobar() while m is locked]
// do some work
}
void foobar() {
lock(m);
f();
unlock(m);
}
A patch by zdtorok (Zoltán Dániel Török)!
Differential Revision: https://reviews.llvm.org/D21506
Added:
cfe/trunk/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
cfe/trunk/test/Analysis/block-in-critical-section.cpp
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=282011&r1=282010&r2=282011&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Tue Sep 20
15:28:50 2016
@@ -436,6 +436,10 @@ def SimpleStreamChecker : Checker<"Simpl
HelpText<"Check for misuses of stream APIs">,
DescFile<"SimpleStreamChecker.cpp">;
+def BlockInCriticalSectionChecker : Checker<"BlockInCriticalSection">,
+ HelpText<"Check for calls to blocking functions inside a critical section">,
+ DescFile<"BlockInCriticalSectionChecker.cpp">;
+
} // end "alpha.unix"
let ParentPackage = CString in {
Added: cfe/trunk/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp?rev=282011&view=auto
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
(added)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp Tue
Sep 20 15:28:50 2016
@@ -0,0 +1,109 @@
+//===-- BlockInCriticalSectionChecker.cpp ---*- C++
-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Defines a checker for blocks in critical sections. This checker should find
+// the calls to blocking functions (for example: sleep, getc, fgets, read,
+// recv etc.) inside a critical section. When sleep(x) is called while a mutex
+// is held, other threades cannot lock the same mutex. This might take some
+// time, leading to bad performance or even deadlock.
+//
+//===--===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class BlockInCriticalSectionChecker : public Checker {
+
+ CallDescription LockFn, UnlockFn, SleepFn, GetcFn, FgetsFn, ReadFn, RecvFn;
+
+ std::unique_ptr BlockInCritSectionBugType;
+
+ void reportBlockInCritSection(SymbolRef FileDescSym,
+const CallEvent &call,
+CheckerContext &C) const;
+
+public:
+ BlockInCriticalSectionChecker();
+
+ void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+
+ /// Process unlock.
+ /// Process lock.
+ /// Process blocking functions (sleep, getc, fgets, read, recv)
+ void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+
+};
+
+} // end anonymous namespace
+
+REGISTER_TRAIT_WITH_PROGRAMSTATE(MutexCounter, unsigned)
+
+BlockInCriticalSectionChecker::BlockInCriticalSectionChecker()
+: LockFn("lock"), UnlockFn("unlock"), SleepFn("sleep"), GetcFn("getc"),
+ FgetsFn("fgets"), ReadFn("read"), RecvFn("recv") {
+ // Initialize the bug type.
+ BlockInCritSectionBugType.reset(
+ new BugType(this, "Call to blocking function in critical section",
+"Blocking Error"));
+}
+
+void BlockInCriticalSectionChecker::checkPreCall(const CallEvent &Call,
+ CheckerContext &C) const {
+}
+
+void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call,
+
Re: [PATCH] D24792: [analyzer] Fix crash in RetainCountChecker::checkEndFunction
zaks.anna added a comment. > However, the assert here has a reason: we clearly shouldn't be trying to > analyze synthesized bodies as top-level functions. Yes, seems like we should update r264687 so that we use the available body when analyzing as top level. Another possible issue is that we will use the synthesized body if the function name **starts with** "OSAtomicCompareAndSwap" since we do not match the full function name. If the function body is available, there is a higher chance it is implementing something other than the standard compare and swap. We might want to start matching the full names of the functions are are synthesizing. @alexshap is that a problem for your codebase? Repository: rL LLVM https://reviews.llvm.org/D24792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24792: [analyzer] Fix crash in RetainCountChecker::checkEndFunction
zaks.anna added a comment. Can you give a bit more context? Do you see the crash on a redefinition of the OSAtomicCompareAndSwapPtr or one of the other standard functions or do you have another similarly named function that should not be modeled? Repository: rL LLVM https://reviews.llvm.org/D24792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24792: [analyzer] Fix crash in RetainCountChecker::checkEndFunction
zaks.anna added a comment. One approach would be to skip analyzing the functions which we model as top level. - a/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp +++ b/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp @@ -688,6 +688,9 @@ void AnalysisConsumer::ActionExprEngine(Decl *D, bool ObjCGCEnabled, if (!Mgr->getAnalysisDeclContext(D)->getAnalysis()) return; + if (Mgr->getAnalysisDeclContext(D)->isBodyAutosynthesized()) +return; + ExprEngine Eng(*Mgr, ObjCGCEnabled, VisitedCallees, &FunctionSummaries,IMode); The main downside is that we will not be analyzing the bodies of functions that are being modeled at all, so we won't find bugs in them. On the other hand, those definitions should be coming from system headers anyway. Another approach is along the lines of changing the AnalysisDeclContext::getBody() so that it does not choose model over a real body depending on context. It might be less clean and maintainable. Repository: rL LLVM https://reviews.llvm.org/D24792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24792: [analyzer] Fix crash in RetainCountChecker::checkEndFunction
zaks.anna added a comment. Thanks! @alexshap, Do yon have commit access or should we commit on your behalf? Repository: rL LLVM https://reviews.llvm.org/D24792 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22968: [analyzer] A checker for macOS-specific bool-like objects.
zaks.anna added a comment.
Here are more comments. Could you address/answer these and upload the latest
patch that compares NSNumber to other numbers?
Thanks!
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:88
@@ +87,3 @@
+
+auto NSNumberExprM =
+expr(ignoringParenImpCasts(expr(hasType(hasCanonicalType(
Can you test if matching for NSNumber works when they come from ivars,
properties, and method returns, works?
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:117
@@ +116,3 @@
+auto ConversionThroughComparisonM =
+binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ hasEitherOperand(NSNumberExprM),
Can we use any binary operator here without any restrictions?
Comment at: test/Analysis/bool-conversion.cpp:18
@@ +17,3 @@
+#ifdef PEDANTIC
+ if (p) {} // expected-warning{{}}
+ if (!p) {} // expected-warning{{}}
dcoughlin wrote:
> It is generally good to include the diagnostic text in the test for the
> warning. This way we make sure we get the warning we expected.
+1
Are these pedantic because we assume these are comparing the pointer and not
the value? Have you checked that this is a common idiom?
Same comment applies to NSNumber. If it is common practice to compare against
nil..
Comment at: test/Analysis/bool-conversion.cpp:21
@@ +20,3 @@
+ p ? 1 : 2; // expected-warning{{}}
+ (bool)p; // expected-warning{{}}
+#endif
Why is this OK?
Comment at: test/Analysis/bool-conversion.m:2
@@ +1,3 @@
+// RUN: %clang_cc1 -fblocks -w -analyze
-analyzer-checker=alpha.osx.BoolConversion %s -verify
+// RUN: %clang_cc1 -fblocks -w -analyze
-analyzer-checker=alpha.osx.BoolConversion -analyzer-config
alpha.osx.BoolConversion:Pedantic=true -DPEDANTIC %s -verify
+
dcoughlin wrote:
> You should add a test invocation here where -fobjc-arc is set as well. This
> adds a bunch of implicit goop that it would be good to test with.
+ 1!!!
Especially, since we are matching the AST.
Comment at: test/Analysis/bool-conversion.m:10
@@ +9,3 @@
+ if (!p) {} // expected-warning{{}}
+ if (p == YES) {} // expected-warning{{}}
+ if (p == NO) {} // expected-warning{{}}
dcoughlin wrote:
> There is a Sema warning for `p == YES` already, right? ("comparison between
> pointer and integer ('NSNumber *' and 'int')")
These tests seem to be even more relevant to OSBoolean.
https://reviews.llvm.org/D22968
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24278: [analyzer] Extend bug reports with extra notes.
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. I have no further comments. https://reviews.llvm.org/D24278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24916: [analyzer] Extend bug reports with extra notes - CloneChecker
zaks.anna added inline comments.
Comment at: test/Analysis/copypaste/functions.cpp:7
@@ -6,3 +6,3 @@
-int max(int a, int b) { // expected-warning{{Detected code clone.}}
+int max(int a, int b) { // expected-warning{{Clone of this code was detected}}
log();
"was" -> "is"?
Do we use past or present elsewhere?
Comment at: test/Analysis/copypaste/suspicious-clones.cpp:61
@@ -60,3 +60,3 @@
b /= a + b;
- c -= b * a; // expected-warning{{suspicious code clone detected; did you
mean to use 'a'?}}
+ c -= b * a; // expected-warning{{Suspicious code clone detected; did you
mean to use 'a'?}}
return c;
The error message seems too verbose and focused on the implementation rather
than user (ex: "suspicious code clone" and "suggestion is based").
Maybe we could say something like this:
- Did you mean to use 'a'?
- Similar code snippet here
https://reviews.llvm.org/D24916
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24915: [analyzer] Extend bug reports with extra notes - ObjCDeallocChecker
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. LGTM and Devin's comments have been addressed. https://reviews.llvm.org/D24915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24916: [analyzer] Extend bug reports with extra notes - CloneChecker
zaks.anna added inline comments.
Comment at: test/Analysis/copypaste/suspicious-clones.cpp:61
@@ -60,3 +60,3 @@
b /= a + b;
- c -= b * a; // expected-warning{{suspicious code clone detected; did you
mean to use 'a'?}}
+ c -= b * a; // expected-warning{{Suspicious code clone detected; did you
mean to use 'a'?}}
return c;
zaks.anna wrote:
> The error message seems too verbose and focused on the implementation rather
> than user (ex: "suspicious code clone" and "suggestion is based").
>
> Maybe we could say something like this:
>
> - Did you mean to use 'a'?
> - Similar code snippet here
>
>
Better:
Did you mean to use 'a'?
Similar code snippet here uses 'b'
Did you mean to use 'a' instead of 'b'?
Similar code snippet here
https://reviews.llvm.org/D24916
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24916: [analyzer] Extend bug reports with extra notes - CloneChecker
zaks.anna added inline comments.
Comment at: test/Analysis/copypaste/macros.cpp:8
@@ -7,3 +7,3 @@
-int foo(int a) { // expected-warning{{Detected code clone.}}
+int foo(int a) { // expected-warning{{Clones of this code were detected}}
a = a + 1;
- Duplicate code detected
- Similar code here
Comment at: test/Analysis/copypaste/suspicious-clones.cpp:61
@@ -60,3 +60,3 @@
b /= a + b;
- c -= b * a; // expected-warning{{suspicious code clone detected; did you
mean to use 'a'?}}
+ c -= b * a; // expected-warning{{Suspicious code clone detected; did you
mean to use 'a'?}}
return c;
zaks.anna wrote:
> zaks.anna wrote:
> > The error message seems too verbose and focused on the implementation
> > rather than user (ex: "suspicious code clone" and "suggestion is based").
> >
> > Maybe we could say something like this:
> >
> > - Did you mean to use 'a'?
> > - Similar code snippet here
> >
> >
> Better:
>
> Did you mean to use 'a'?
> Similar code snippet here uses 'b'
>
> Did you mean to use 'a' instead of 'b'?
> Similar code snippet here
Another refinement
Potential copy-paste error: did you mean to use 'a' instead of 'b'?
Similar code here
https://reviews.llvm.org/D24916
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24916: [analyzer] Extend bug reports with extra notes - CloneChecker
zaks.anna added inline comments.
Comment at: test/Analysis/copypaste/suspicious-clones.cpp:61
@@ -60,3 +60,3 @@
b /= a + b;
- c -= b * a; // expected-warning{{suspicious code clone detected; did you
mean to use 'a'?}}
+ c -= b * a; // expected-warning{{Suspicious code clone detected; did you
mean to use 'a'?}}
return c;
zaks.anna wrote:
> zaks.anna wrote:
> > zaks.anna wrote:
> > > The error message seems too verbose and focused on the implementation
> > > rather than user (ex: "suspicious code clone" and "suggestion is based").
> > >
> > > Maybe we could say something like this:
> > >
> > > - Did you mean to use 'a'?
> > > - Similar code snippet here
> > >
> > >
> > Better:
> >
> > Did you mean to use 'a'?
> > Similar code snippet here uses 'b'
> >
> > Did you mean to use 'a' instead of 'b'?
> > Similar code snippet here
> Another refinement
>
> Potential copy-paste error: did you mean to use 'a' instead of 'b'?
> Similar code here
Potential copy-paste error; did you really mean to use 'b' here?
Similar code using 'a' here
Similar code using 'c' here
https://reviews.llvm.org/D24916
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24916: [analyzer] Extend bug reports with extra notes - CloneChecker
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D24916 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25092: [analyzer] Add "Assuming..." diagnostic pieces for short-circuit logical operators.
zaks.anna added a comment.
Looks good overall. Very minor nits.
> BugReporterVisitors.cpp:1274
>
> + // In the code below, Term is a CFG terminator, and Cond is a branch
> + // condition expression upon which the decision is made on this terminator.
nit: no comma before "and".
> BugReporterVisitors.cpp:1288
>switch (Term->getStmtClass()) {
> + // FIXME: Come up with something for switch statements?
>default:
It is not clear what the FIXME is for.
> BugReporterVisitors.cpp:1302
> +const auto *BO = cast(Term);
> +if (!BO->isLogicalOp())
> + return nullptr;
Is this expected to ever trigger?
https://reviews.llvm.org/D25092
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D23853: Assert in performTrivialCopy - Bug report and a possible solution
zaks.anna added a comment. Should this revision be closed? https://reviews.llvm.org/D23853 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r283253 - [analyzer] Add PostStmt callback for ArraySubscriptExpr
Author: zaks
Date: Tue Oct 4 15:49:31 2016
New Revision: 283253
URL: http://llvm.org/viewvc/llvm-project?rev=283253&view=rev
Log:
[analyzer] Add PostStmt callback for ArraySubscriptExpr
A patch by Jan Smets!
Differential Revision: https://reviews.llvm.org/D25009
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp?rev=283253&r1=283252&r2=283253&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp Tue Oct 4
15:49:31 2016
@@ -25,7 +25,9 @@ using namespace ento;
namespace {
class AnalysisOrderChecker : public Checker< check::PreStmt,
- check::PostStmt> {
+ check::PostStmt,
+
check::PreStmt,
+
check::PostStmt> {
bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const {
AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions();
return Opts.getBooleanOption("*", false, this) ||
@@ -44,6 +46,16 @@ public:
llvm::errs() << "PostStmt (Kind : " << CE->getCastKindName()
<< ")\n";
}
+
+ void checkPreStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C)
const {
+if (isCallbackEnabled(C, "PreStmtArraySubscriptExpr"))
+ llvm::errs() << "PreStmt\n";
+ }
+
+ void checkPostStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C)
const {
+if (isCallbackEnabled(C, "PostStmtArraySubscriptExpr"))
+ llvm::errs() << "PostStmt\n";
+ }
};
}
Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp?rev=283253&r1=283252&r2=283253&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp Tue Oct 4 15:49:31 2016
@@ -1968,24 +1968,26 @@ void ExprEngine::VisitLvalArraySubscript
const Expr *Base = A->getBase()->IgnoreParens();
const Expr *Idx = A->getIdx()->IgnoreParens();
- ExplodedNodeSet checkerPreStmt;
- getCheckerManager().runCheckersForPreStmt(checkerPreStmt, Pred, A, *this);
+ ExplodedNodeSet CheckerPreStmt;
+ getCheckerManager().runCheckersForPreStmt(CheckerPreStmt, Pred, A, *this);
- StmtNodeBuilder Bldr(checkerPreStmt, Dst, *currBldrCtx);
+ ExplodedNodeSet EvalSet;
+ StmtNodeBuilder Bldr(CheckerPreStmt, EvalSet, *currBldrCtx);
assert(A->isGLValue() ||
(!AMgr.getLangOpts().CPlusPlus &&
A->getType().isCForbiddenLValueType()));
- for (ExplodedNodeSet::iterator it = checkerPreStmt.begin(),
- ei = checkerPreStmt.end(); it != ei; ++it) {
-const LocationContext *LCtx = (*it)->getLocationContext();
-ProgramStateRef state = (*it)->getState();
+ for (auto *Node : CheckerPreStmt) {
+const LocationContext *LCtx = Node->getLocationContext();
+ProgramStateRef state = Node->getState();
SVal V = state->getLValue(A->getType(),
state->getSVal(Idx, LCtx),
state->getSVal(Base, LCtx));
-Bldr.generateNode(A, *it, state->BindExpr(A, LCtx, V), nullptr,
+Bldr.generateNode(A, Node, state->BindExpr(A, LCtx, V), nullptr,
ProgramPoint::PostLValueKind);
}
+
+ getCheckerManager().runCheckersForPostStmt(Dst, EvalSet, A, *this);
}
/// VisitMemberExpr - Transfer function for member expressions.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph
zaks.anna added a comment. Daniel, please, add reviewers to this patch. Repository: rL LLVM https://reviews.llvm.org/D25326 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph
zaks.anna accepted this revision. zaks.anna added a comment. Please, fix the style issues before committing. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:266 +ExplodedNode *Pred, +const ReturnStmt *RS=nullptr) override; Add spaces around '=.' https://reviews.llvm.org/D25326 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25429: [analyzer] Link libStaticAnalyzerCheckers to libASTMatchers.
zaks.anna added a comment. I am in support of this as well. https://reviews.llvm.org/D25429 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25503: [analyzer] Remove superquadratic behaviour from DataflowWorklist
zaks.anna added a comment. Do you have results that show how this effects performance on average code and machine generated code? One concern is that multiset is malloc intensive. See http://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task. Maybe SparseSet/SparseMultiSet would be better? Comment at: lib/Analysis/LiveVariables.cpp:66 return nullptr; - const CFGBlock *b = worklist.pop_back_val(); + const auto I = --worklist.end(); + const CFGBlock *b = *I; '--wroklist.end()' -> 'worklist.rbegin()'? Repository: rL LLVM https://reviews.llvm.org/D25503 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25503: [analyzer] Remove superquadratic behaviour from DataflowWorklist
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. LGTM! I would add info on how much speedup you see in the cryptographic libraries to the commit message. (You could say something like "on a cryptographic library that uses code generation, this patch gives"...) Thanks, Anna. Repository: rL LLVM https://reviews.llvm.org/D25503 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D1805: [scan-build] Whitelist all -fXXXX options.
zaks.anna added a comment. Please, provide more information on why this patch is needed and why the existing processing of the -f flags does not work as expected. Looks like the last modifications to the -f flags were made in r186138. (Please, submit patches with more context: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface) Thank you! https://reviews.llvm.org/D1805 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D1805: [scan-build] Whitelist all -fXXXX options.
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Just realized that this is super old and has probably been fixed by r186138. Closing. https://reviews.llvm.org/D1805 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20811: [analyzer] Model some library functions
zaks.anna added a comment. Ping? Is there something blocking progress here? This functionality is very useful and almost done. Thanks! https://reviews.llvm.org/D20811 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22968: [analyzer] A checker for macOS-specific bool- and number-like objects.
zaks.anna accepted this revision. zaks.anna added inline comments. This revision is now accepted and ready to land. Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:479 + InPackage, + HelpText<"Check for erroneous conversions of number pointers into numbers">, + DescFile<"NumberObjectConversionChecker">; "number pointers" -> "objects representing numbers" https://reviews.llvm.org/D22968 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25606: alpha.core.UnreachableCode - don't warn about unreachable code inside macro
zaks.anna added a comment.
We should pattern match for this specific macro pattern (ex: do{...}while(0) )
instead of suppressing all warnings coming from macros. Maybe we could use the
same heuristic as -Wunreachable-code-return compiler warning?
Repository:
rL LLVM
https://reviews.llvm.org/D25606
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r311182 - [analyzer] Fix modeling of constructors
I approve.Thanks Hans!
Anna
> On Aug 21, 2017, at 1:05 PM, Hans Wennborg wrote:
>
> I'm ok with it if Anna approves.
>
> On Mon, Aug 21, 2017 at 9:06 AM, Artem Dergachev wrote:
>> Hello,
>>
>> Do we have time to merge this change into release 5.0.0? It's an assertion
>> failure fix, which shows up on C++ code involving double-inheritance with
>> empty base classes.
>>
>> Artem.
>>
>>
>> On 8/18/17 9:20 PM, Alexander Shaposhnikov via cfe-commits wrote:
>>>
>>> Author: alexshap
>>> Date: Fri Aug 18 11:20:43 2017
>>> New Revision: 311182
>>>
>>> URL:http://llvm.org/viewvc/llvm-project?rev=311182&view=rev
>>> Log:
>>> [analyzer] Fix modeling of constructors
>>>
>>> This diff fixes analyzer's crash (triggered assert) on the newly added
>>> test case.
>>> The assert being discussed is assert(!B.lookup(R, BindingKey::Direct))
>>> in lib/StaticAnalyzer/Core/RegionStore.cpp, however the root cause is
>>> different.
>>> For classes with empty bases the offsets might be tricky.
>>> For example, let's assume we have
>>> struct S: NonEmptyBase, EmptyBase {
>>> ...
>>> };
>>> In this case Clang applies empty base class optimization and
>>> the offset of EmptyBase will be 0, it can be verified via
>>> clang -cc1 -x c++ -v -fdump-record-layouts main.cpp -emit-llvm -o
>>> /dev/null.
>>> When the analyzer tries to perform zero initialization of EmptyBase
>>> it will hit the assert because that region
>>> has already been "written" by the constructor of NonEmptyBase.
>>>
>>> Test plan:
>>> make check-all
>>>
>>> Differential revision:https://reviews.llvm.org/D36851
>>>
>>> Modified:
>>> cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
>>> cfe/trunk/test/Analysis/ctor.mm
>>>
>>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
>>>
>>> URL:http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=311182&r1=311181&r2=311182&view=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp (original)
>>> +++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Fri Aug 18 11:20:43
>>> 2017
>>> @@ -409,6 +409,19 @@ public: // Part of public interface to c
>>> // BindDefault is only used to initialize a region with a default
>>> value.
>>>StoreRef BindDefault(Store store, const MemRegion *R, SVal V) override
>>> {
>>> +// FIXME: The offsets of empty bases can be tricky because of
>>> +// of the so called "empty base class optimization".
>>> +// If a base class has been optimized out
>>> +// we should not try to create a binding, otherwise we should.
>>> +// Unfortunately, at the moment ASTRecordLayout doesn't expose
>>> +// the actual sizes of the empty bases
>>> +// and trying to infer them from offsets/alignments
>>> +// seems to be error-prone and non-trivial because of the trailing
>>> padding.
>>> +// As a temporary mitigation we don't create bindings for empty
>>> bases.
>>> +if (R->getKind() == MemRegion::CXXBaseObjectRegionKind &&
>>> +cast(R)->getDecl()->isEmpty())
>>> + return StoreRef(store, *this);
>>> +
>>> RegionBindingsRef B = getRegionBindings(store);
>>> assert(!B.lookup(R, BindingKey::Direct));
>>>
>>> Modified: cfe/trunk/test/Analysis/ctor.mm
>>>
>>> URL:http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/ctor.mm?rev=311182&r1=311181&r2=311182&view=diff
>>>
>>> ==
>>> --- cfe/trunk/test/Analysis/ctor.mm (original)
>>> +++ cfe/trunk/test/Analysis/ctor.mm Fri Aug 18 11:20:43 2017
>>> @@ -704,3 +704,20 @@ namespace PR19579 {
>>> };
>>>}
>>> }
>>> +
>>> +namespace NoCrashOnEmptyBaseOptimization {
>>> + struct NonEmptyBase {
>>> +int X;
>>> +explicit NonEmptyBase(int X) : X(X) {}
>>> + };
>>> +
>>> + struct EmptyBase {};
>>> +
>>> + struct S : NonEmptyBase, EmptyBase {
>>> +S() : NonEmptyBase(0), EmptyBase() {}
>>> + };
>>> +
>>> + void testSCtorNoCrash() {
>>> +S s;
>>> + }
>>> +}
>>>
>>>
>>> ___
>>> cfe-commits mailing list
>>> cfe-commits@lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r318567 - Change code owner for Clang Static Analyzer to Devin Coughlin.
Author: zaks Date: Fri Nov 17 15:19:04 2017 New Revision: 318567 URL: http://llvm.org/viewvc/llvm-project?rev=318567&view=rev Log: Change code owner for Clang Static Analyzer to Devin Coughlin. Differential Revision: https://reviews.llvm.org/D39964 Modified: cfe/trunk/CODE_OWNERS.TXT Modified: cfe/trunk/CODE_OWNERS.TXT URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/CODE_OWNERS.TXT?rev=318567&r1=318566&r2=318567&view=diff == --- cfe/trunk/CODE_OWNERS.TXT (original) +++ cfe/trunk/CODE_OWNERS.TXT Fri Nov 17 15:19:04 2017 @@ -25,6 +25,10 @@ N: Eric Christopher E: echri...@gmail.com D: Debug Information, inline assembly +N: Devin Coughlin +E: dcough...@apple.com +D: Clang Static Analyzer + N: Doug Gregor E: dgre...@apple.com D: Emeritus owner @@ -41,10 +45,6 @@ N: Anton Korobeynikov E: an...@korobeynikov.info D: Exception handling, Windows codegen, ARM EABI -N: Anna Zaks -E: ga...@apple.com -D: Clang Static Analyzer - N: John McCall E: rjmcc...@apple.com D: Clang LLVM IR generation ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r289883 - [analyzer] Include type name in Retain Count Checker diagnostics
Author: zaks
Date: Thu Dec 15 16:55:03 2016
New Revision: 289883
URL: http://llvm.org/viewvc/llvm-project?rev=289883&view=rev
Log:
[analyzer] Include type name in Retain Count Checker diagnostics
The more detailed diagnostic will make identifying which object the
diagnostics refer to easier.
Differential Revision: https://reviews.llvm.org/D27740
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
cfe/trunk/test/Analysis/edges-new.mm
cfe/trunk/test/Analysis/inlining/path-notes.m
cfe/trunk/test/Analysis/objc-arc.m
cfe/trunk/test/Analysis/plist-output-alternate.m
cfe/trunk/test/Analysis/retain-release-arc.m
cfe/trunk/test/Analysis/retain-release-path-notes-gc.m
cfe/trunk/test/Analysis/retain-release-path-notes.m
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp?rev=289883&r1=289882&r2=289883&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp Thu Dec 15
16:55:03 2016
@@ -1990,11 +1990,23 @@ PathDiagnosticPiece *CFRefReportVisitor:
}
if (CurrV.getObjKind() == RetEffect::CF) {
-os << " returns a Core Foundation object with a ";
+if (Sym->getType().isNull()) {
+ os << " returns a Core Foundation object with a ";
+} else {
+ os << " returns a Core Foundation object of type "
+ << Sym->getType().getAsString() << " with a ";
+}
}
else {
assert (CurrV.getObjKind() == RetEffect::ObjC);
-os << " returns an Objective-C object with a ";
+QualType T = Sym->getType();
+if (T.isNull() || !isa(T)) {
+ os << " returns an Objective-C object with a ";
+} else {
+ const ObjCObjectPointerType *PT = cast(T);
+ os << " returns an instance of "
+ << PT->getPointeeType().getAsString() << " with a ";
+}
}
if (CurrV.isOwned()) {
Modified: cfe/trunk/test/Analysis/edges-new.mm
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/edges-new.mm?rev=289883&r1=289882&r2=289883&view=diff
==
--- cfe/trunk/test/Analysis/edges-new.mm (original)
+++ cfe/trunk/test/Analysis/edges-new.mm Thu Dec 15 16:55:03 2016
@@ -2438,9 +2438,9 @@ namespace rdar14960554 {
// CHECK-NEXT:
// CHECK-NEXT: depth0
// CHECK-NEXT: extended_message
-// CHECK-NEXT: Call to function 'CFNumberCreate'
returns a Core Foundation object with a +1 retain count
+// CHECK-NEXT: Call to function 'CFNumberCreate'
returns a Core Foundation object of type CFNumberRef with a +1 retain
count
// CHECK-NEXT: message
-// CHECK-NEXT: Call to function 'CFNumberCreate'
returns a Core Foundation object with a +1 retain count
+// CHECK-NEXT: Call to function 'CFNumberCreate'
returns a Core Foundation object of type CFNumberRef with a +1 retain
count
// CHECK-NEXT:
// CHECK-NEXT:
// CHECK-NEXT: kindcontrol
@@ -11355,9 +11355,9 @@ namespace rdar14960554 {
// CHECK-NEXT:
// CHECK-NEXT: depth0
// CHECK-NEXT: extended_message
-// CHECK-NEXT: Method returns an Objective-C object with a +1
retain count
+// CHECK-NEXT: Method returns an instance of RDar10797980 with a
+1 retain count
// CHECK-NEXT: message
-// CHECK-NEXT: Method returns an Objective-C object with a +1
retain count
+// CHECK-NEXT: Method returns an instance of RDar10797980 with a
+1 retain count
// CHECK-NEXT:
// CHECK-NEXT:
// CHECK-NEXT: kindcontrol
@@ -20325,9 +20325,9 @@ namespace rdar14960554 {
// CHECK-NEXT:
// CHECK-NEXT: depth0
// CHECK-NEXT: extended_message
-// CHECK-NEXT: Method returns an Objective-C object with a +1
retain count
+// CHECK-NEXT: Method returns an instance of NSObject with a +1
retain count
// CHECK-NEXT: message
-// CHECK-NEXT: Method returns an Objective-C object with a +1
retain count
+// CHECK-NEXT: Method returns an instance of NSObject with a +1
retain count
// CHECK-NEXT:
// CHECK-NEXT:
// CHECK-NEXT: kindcontrol
Modified: cfe/trunk/test/Analysis/inlining/path-notes.m
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inlining/path-notes.m?rev=289883&r1=289882&r2=289883&view=diff
==
--- cfe/trunk/test/Analysis/inlining/path-notes.m (original)
+++ cfe/trunk/test/Analysis/inlining/path-notes.m Thu Dec 15 16:55:03 2016
@@ -160,7 +160,7 @@ id testAutoreleaseTakesEffectInDispatch(
dispatch_once(&token, ^{});
id x = NSObject alloc] init] autorelease] autorelease];
- // expe
r289885 - [analyzer] Refine the diagnostics in the nullability checker to differentiate between nil and null
Author: zaks
Date: Thu Dec 15 16:55:15 2016
New Revision: 289885
URL: http://llvm.org/viewvc/llvm-project?rev=289885&view=rev
Log:
[analyzer] Refine the diagnostics in the nullability checker to differentiate
between nil and null
This is a big deal for ObjC, where nullability annotations are extensively
used. I've also changed "Null" -> "null" and removed "is" as this is the
pattern that Sema is using.
Differential Revision: https://reviews.llvm.org/D27600
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
cfe/trunk/test/Analysis/nullability-no-arc.mm
cfe/trunk/test/Analysis/nullability.mm
cfe/trunk/test/Analysis/nullability_nullonly.mm
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp?rev=289885&r1=289884&r2=289885&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp Thu Dec 15
16:55:15 2016
@@ -610,9 +610,9 @@ void NullabilityChecker::checkPreStmt(co
SmallString<256> SBuf;
llvm::raw_svector_ostream OS(SBuf);
-OS << "Null is returned from a " << C.getDeclDescription(D) <<
+OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null");
+OS << " returned from a " << C.getDeclDescription(D) <<
" that is expected to return a non-null value";
-
reportBugIfInvariantHolds(OS.str(),
ErrorKind::NilReturnedToNonnull, N, nullptr, C,
RetExpr);
@@ -707,9 +707,11 @@ void NullabilityChecker::checkPreCall(co
ExplodedNode *N = C.generateErrorNode(State);
if (!N)
return;
+
SmallString<256> SBuf;
llvm::raw_svector_ostream OS(SBuf);
- OS << "Null passed to a callee that requires a non-null " << ParamIdx
+ OS << (Param->getType()->isObjCObjectPointerType() ? "nil" : "Null");
+ OS << " passed to a callee that requires a non-null " << ParamIdx
<< llvm::getOrdinalSuffix(ParamIdx) << " parameter";
reportBugIfInvariantHolds(OS.str(), ErrorKind::NilPassedToNonnull, N,
nullptr, C,
@@ -1128,8 +1130,11 @@ void NullabilityChecker::checkBind(SVal
if (ValueExpr)
ValueStmt = ValueExpr;
-reportBugIfInvariantHolds("Null is assigned to a pointer which is "
- "expected to have non-null value",
+SmallString<256> SBuf;
+llvm::raw_svector_ostream OS(SBuf);
+OS << (LocType->isObjCObjectPointerType() ? "nil" : "Null");
+OS << " assigned to a pointer which is expected to have non-null value";
+reportBugIfInvariantHolds(OS.str(),
ErrorKind::NilAssignedToNonnull, N, nullptr, C,
ValueStmt);
return;
Modified: cfe/trunk/test/Analysis/nullability-no-arc.mm
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/nullability-no-arc.mm?rev=289885&r1=289884&r2=289885&view=diff
==
--- cfe/trunk/test/Analysis/nullability-no-arc.mm (original)
+++ cfe/trunk/test/Analysis/nullability-no-arc.mm Thu Dec 15 16:55:15 2016
@@ -17,20 +17,20 @@ NSObject
@interface TestObject : NSObject
@end
-TestObject * _Nonnull returnsNilObjCInstanceIndirectly() {
- TestObject *local = 0;
- return local; // expected-warning {{Null is returned from a function that is
expected to return a non-null value}}
+TestObject *_Nonnull returnsNilObjCInstanceIndirectly() {
+ TestObject *local = nil;
+ return local; // expected-warning {{nil returned from a function that is
expected to return a non-null value}}
}
TestObject * _Nonnull returnsNilObjCInstanceIndirectlyWithSupressingCast() {
- TestObject *local = 0;
+ TestObject *local = nil;
return (TestObject * _Nonnull)local; // no-warning
}
TestObject * _Nonnull returnsNilObjCInstanceDirectly() {
// The first warning is from Sema. The second is from the static analyzer.
return nil; // expected-warning {{null returned from function that requires
a non-null return value}}
- // expected-warning@-1 {{Null is returned from a function that
is expected to return a non-null value}}
+ // expected-warning@-1 {{nil returned from a function that is
expected to return a non-null value}}
}
TestObject * _Nonnull returnsNilObjCInstanceDirectlyWithSuppressingCast() {
@@ -43,7 +43,7 @@ void testObjCNonARCNoInitialization(Test
}
void testObjCNonARCExplicitZeroInitialization() {
- TestObject * _Nonnull explicitlyZeroInitialized = nil; // expected-warning
{{Null is assigned to a pointer which is expected to have non-null value}}
+ TestObject * _Nonnull explicitlyZeroInitialized = nil; // expected-warning
{{nil assigned to a pointer whi
r289884 - [analyzer] Refer to macro names in diagnostics for macros representing a literal
Author: zaks
Date: Thu Dec 15 16:55:11 2016
New Revision: 289884
URL: http://llvm.org/viewvc/llvm-project?rev=289884&view=rev
Log:
[analyzer] Refer to macro names in diagnostics for macros representing a literal
When a macro expending to a literal is used in a comparison, use the macro name
in the diagnostic rather than the literal. This improves readability of path
notes.
Added tests for various macro literals that could occur. Only BOOl, Int, and
NULL tests have changed behavior with this patch.
Differential Revision: https://reviews.llvm.org/D27726
Added:
cfe/trunk/test/Analysis/diagnostics/macros.cpp
cfe/trunk/test/Analysis/diagnostics/macros.m
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
cfe/trunk/test/Analysis/Inputs/system-header-simulator-objc.h
cfe/trunk/test/Analysis/Inputs/system-header-simulator.h
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h?rev=289884&r1=289883&r2=289884&view=diff
==
---
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
(original)
+++
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
Thu Dec 15 16:55:11 2016
@@ -245,6 +245,7 @@ public:
const ExplodedNode *N);
bool patternMatch(const Expr *Ex,
+const Expr *ParentEx,
raw_ostream &Out,
BugReporterContext &BRC,
BugReport &R,
Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=289884&r1=289883&r2=289884&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Thu Dec 15
16:55:11 2016
@@ -15,6 +15,7 @@
#include "clang/AST/Expr.h"
#include "clang/AST/ExprObjC.h"
#include "clang/Analysis/CFGStmtMap.h"
+#include "clang/Lex/Lexer.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
#include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
@@ -1372,7 +1373,9 @@ ConditionBRVisitor::VisitTrueTest(const
return Event;
}
-bool ConditionBRVisitor::patternMatch(const Expr *Ex, raw_ostream &Out,
+bool ConditionBRVisitor::patternMatch(const Expr *Ex,
+ const Expr *ParentEx,
+ raw_ostream &Out,
BugReporterContext &BRC,
BugReport &report,
const ExplodedNode *N,
@@ -1380,6 +1383,47 @@ bool ConditionBRVisitor::patternMatch(co
const Expr *OriginalExpr = Ex;
Ex = Ex->IgnoreParenCasts();
+ // Use heuristics to determine if Ex is a macro expending to a literal and
+ // if so, use the macro's name.
+ SourceLocation LocStart = Ex->getLocStart();
+ SourceLocation LocEnd = Ex->getLocEnd();
+ if (LocStart.isMacroID() && LocEnd.isMacroID() &&
+ (isa(Ex) ||
+ isa(Ex) ||
+ isa(Ex) ||
+ isa(Ex) ||
+ isa(Ex))) {
+
+StringRef StartName = Lexer::getImmediateMacroNameForDiagnostics(LocStart,
+ BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
+StringRef EndName = Lexer::getImmediateMacroNameForDiagnostics(LocEnd,
+ BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
+bool beginAndEndAreTheSameMacro = StartName.equals(EndName);
+
+bool partOfParentMacro = false;
+if (ParentEx->getLocStart().isMacroID()) {
+ StringRef PName = Lexer::getImmediateMacroNameForDiagnostics(
+ParentEx->getLocStart(), BRC.getSourceManager(),
+BRC.getASTContext().getLangOpts());
+ partOfParentMacro = PName.equals(StartName);
+}
+
+if (beginAndEndAreTheSameMacro && !partOfParentMacro ) {
+ // Get the location of the macro name as written by the caller.
+ SourceLocation Loc = LocStart;
+ while (LocStart.isMacroID()) {
+Loc = LocStart;
+LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);
+ }
+ StringRef MacroName = Lexer::getImmediateMacroNameForDiagnostics(
+Loc, BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
+
+ // Return the macro name.
+ Out << MacroName;
+ return false;
+}
+ }
+
if (const DeclRefExpr *DR = dyn_cast(Ex)) {
const bool quotes = isa(DR->getDecl());
if (quotes) {
@@ -1440,10 +1484,10 @@ ConditionBRVisitor::VisitTrueTest(const
S
r289886 - [analyzer] Teach the analyzer that pointers can escape into __cxa_demangle
Author: zaks
Date: Thu Dec 15 16:55:18 2016
New Revision: 289886
URL: http://llvm.org/viewvc/llvm-project?rev=289886&view=rev
Log:
[analyzer] Teach the analyzer that pointers can escape into __cxa_demangle
This fixes a reported false positive in the malloc checker.
Differential Revision: https://reviews.llvm.org/D27599
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
cfe/trunk/test/Analysis/Inputs/system-header-simulator-cxx.h
cfe/trunk/test/Analysis/malloc.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp?rev=289886&r1=289885&r2=289886&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp Thu Dec 15 16:55:18 2016
@@ -382,6 +382,11 @@ bool AnyFunctionCall::argumentsMayEscape
if (II->isStr("funopen"))
return true;
+ // - __cxa_demangle - can reallocate memory and can return the pointer to
+ // the input buffer.
+ if (II->isStr("__cxa_demangle"))
+return true;
+
StringRef FName = II->getName();
// - CoreFoundation functions that end with "NoCopy" can free a passed-in
Modified: cfe/trunk/test/Analysis/Inputs/system-header-simulator-cxx.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/Inputs/system-header-simulator-cxx.h?rev=289886&r1=289885&r2=289886&view=diff
==
--- cfe/trunk/test/Analysis/Inputs/system-header-simulator-cxx.h (original)
+++ cfe/trunk/test/Analysis/Inputs/system-header-simulator-cxx.h Thu Dec 15
16:55:18 2016
@@ -240,3 +240,12 @@ void* operator new (std::size_t size, vo
void* operator new[] (std::size_t size, void* ptr) throw() { return ptr; };
void operator delete (void* ptr, void*) throw() {};
void operator delete[] (void* ptr, void*) throw() {};
+
+namespace __cxxabiv1 {
+extern "C" {
+extern char *__cxa_demangle(const char *mangled_name,
+char *output_buffer,
+size_t *length,
+int *status);
+}}
+namespace abi = __cxxabiv1;
Modified: cfe/trunk/test/Analysis/malloc.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc.cpp?rev=289886&r1=289885&r2=289886&view=diff
==
--- cfe/trunk/test/Analysis/malloc.cpp (original)
+++ cfe/trunk/test/Analysis/malloc.cpp Thu Dec 15 16:55:18 2016
@@ -1,6 +1,8 @@
// RUN: %clang_cc1 -w -analyze
-analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,cplusplus.NewDelete
-analyzer-store=region -verify %s
// RUN: %clang_cc1 -triple i386-unknown-linux-gnu -w -analyze
-analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,cplusplus.NewDelete
-analyzer-store=region -verify %s
+#include "Inputs/system-header-simulator-cxx.h"
+
typedef __typeof(sizeof(int)) size_t;
void *malloc(size_t);
void free(void *);
@@ -125,3 +127,15 @@ namespace PR31226 {
p->m(); // no-crash // no-warning
}
}
+
+// Allow __cxa_demangle to escape.
+char* test_cxa_demangle(const char* sym) {
+ size_t funcnamesize = 256;
+ char* funcname = (char*)malloc(funcnamesize);
+ int status;
+ char* ret = abi::__cxa_demangle(sym, funcname, &funcnamesize, &status);
+ if (status == 0) {
+funcname = ret;
+ }
+ return funcname; // no-warning
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r289887 - [asan][docs] Fix the documentation to use clang++ for the C++ example
Author: zaks Date: Thu Dec 15 16:55:21 2016 New Revision: 289887 URL: http://llvm.org/viewvc/llvm-project?rev=289887&view=rev Log: [asan][docs] Fix the documentation to use clang++ for the C++ example After Darwin has been updated not to link in stdc++ on Darwin this actually started to break. Differential Revision: https://reviews.llvm.org/D27598 Modified: cfe/trunk/docs/AddressSanitizer.rst Modified: cfe/trunk/docs/AddressSanitizer.rst URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/AddressSanitizer.rst?rev=289887&r1=289886&r2=289887&view=diff == --- cfe/trunk/docs/AddressSanitizer.rst (original) +++ cfe/trunk/docs/AddressSanitizer.rst Thu Dec 15 16:55:21 2016 @@ -49,16 +49,16 @@ you may need to disable inlining (just u } # Compile and link -% clang -O1 -g -fsanitize=address -fno-omit-frame-pointer example_UseAfterFree.cc +% clang++ -O1 -g -fsanitize=address -fno-omit-frame-pointer example_UseAfterFree.cc or: .. code-block:: console # Compile -% clang -O1 -g -fsanitize=address -fno-omit-frame-pointer -c example_UseAfterFree.cc +% clang++ -O1 -g -fsanitize=address -fno-omit-frame-pointer -c example_UseAfterFree.cc # Link -% clang -g -fsanitize=address example_UseAfterFree.o +% clang++ -g -fsanitize=address example_UseAfterFree.o If a bug is detected, the program will print an error message to stderr and exit with a non-zero exit code. AddressSanitizer exits on the first detected error. ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22090: [analyzer] Add more FileIDs to PlistDiagnostic map
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thanks! https://reviews.llvm.org/D22090 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23853: Assert in performTrivialCopy - Bug report and a possible solution
zaks.anna added a comment. In https://reviews.llvm.org/D23853#524945, @xazax.hun wrote: > > Also: I think r270511 is unlikely to be the change that caused this -- that > > is a change in LLVM's treatment of DebugInfo, which shouldn't affect the > > analyzer. > > > I think Peter means that, that revision introduced the code that the analyzer > fails to analyze (and not the bug in the analyzer). Can you add a reduced test case? https://reviews.llvm.org/D23853 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24048: [Driver] [Darwin] Add sanitizer libraries even if -nodefaultlibs is passed
zaks.anna added a comment. -nostdlib is often used to build parts of libsystem. It's worth noting that ASan and TSan are not supported for use on libsystem on darwin (and elsewhere?), though some subcomponents of it can be sanitized. I am not sure how this relates to UBSan. The user experience of not passing the liker flag is confusing. One proposed solution is to have the clang driver error out when -fsanitize=address and -nostdlib are used together. I am not sure if the warning is sufficient. The error/warning should say that this combination is unsupported at least on Darwin. Chris, what is the driver behind this? Kuba, what do you think? No tests? https://reviews.llvm.org/D24048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22494: [analyzer] Explain why analyzer report is not generated (fix for PR12421).
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:216 @@ +215,3 @@ +llvm::raw_svector_ostream warning(buf); +warning << "warning: Path diagnostic report is not generated. Current " +<< "output format does not support diagnostics that cross file " ayartsev wrote: > zaks.anna wrote: > > Can/should we be specific about what the user-specified output format is? > It's unable to extract information about user-specified output format from > the "PathDiagnosticConsumer" interface. And this class seem to be too generic > to contain "AnalyzerOptions" member or to have e.g. "pure virtual > getOutputFormatName()". > So the only way I see to get info about output format is to use > "PathDiagnosticConsumer::getName()". > Maybe it makes sense just to add a hint to use "--analyzer-output" driver > option to change output format. However this option is not documented at all > and is not displayed in clang help. What do you think? I think mentioning the option is the best option. What is that option called in scan-build? https://reviews.llvm.org/D22494 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22494: [analyzer] Explain why analyzer report is not generated (fix for PR12421).
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:216 @@ +215,3 @@ +llvm::raw_svector_ostream warning(buf); +warning << "warning: Path diagnostic report is not generated. Current " +<< "output format does not support diagnostics that cross file " ayartsev wrote: > ayartsev wrote: > > zaks.anna wrote: > > > ayartsev wrote: > > > > zaks.anna wrote: > > > > > Can/should we be specific about what the user-specified output format > > > > > is? > > > > It's unable to extract information about user-specified output format > > > > from the "PathDiagnosticConsumer" interface. And this class seem to be > > > > too generic to contain "AnalyzerOptions" member or to have e.g. "pure > > > > virtual getOutputFormatName()". > > > > So the only way I see to get info about output format is to use > > > > "PathDiagnosticConsumer::getName()". > > > > Maybe it makes sense just to add a hint to use "--analyzer-output" > > > > driver option to change output format. However this option is not > > > > documented at all and is not displayed in clang help. What do you think? > > > I think mentioning the option is the best option. What is that option > > > called in scan-build? > > scan-build (both perl and python versions) has two options: "-plist" and > > "-plist-html" that are translated to "-analyzer-output=plist" and > > "-analyzer-output=plist-html" frontend options respectively. > I suggest to document the "--analyzer-output" option and to mention this > option in the warning. That sounds good to me. https://reviews.llvm.org/D22494 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24048: [Driver] [Darwin] Add sanitizer libraries even if -nodefaultlibs is passed
zaks.anna added a comment. I've added kcc as a reviewer to see what his opinion is. The way I see this, is that the sanitizer flags and the -nodefaultlibs and -nostdlib flags are not fully compatible since sanitizers will not work for some users who explicitly pass the "-no*" flags. libcxx happens to work, however, it's just one of the users and it has a much better representation on this list since it's an LLVM project. I do not think this is a blocker for sanitizing libcxx because we could change the build system to explicitly link in the ASan library, correct? In any case, we would need some type of diagnostic in the driver to notify the user about the incompatibility. We just need to decide which default to pick. https://reviews.llvm.org/D24048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1003 @@ +1002,3 @@ +// +ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C, + const CXXNewExpr *NE, I am not sure this code belongs to the malloc checker since it only supports the array bounds checker. Is there a reason it's not part of that checker? https://reviews.llvm.org/D24307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24048: [Driver] [Darwin] Add sanitizer libraries even if -nodefaultlibs is passed
zaks.anna added a comment. > -fsanitize=* as a driver argument *when linking* is an explicit request to > link against the sanitizer runtimes. Sanitizer users pass this option to the clang driver to get the runtime checking. Not all of them understand the implications and immediately realize that extra libraries will be linked in (which might be incompatible with other options they pass) or at least they are not thinking about it when using the feature. So I do not view this as an *explicit request* to link against the extra libraries. (I might be missing the point you are trying to convey by highlighting *when linking*. When this option is passed, we both compile and link differently. I view this as an option to turn on the sanitizers feature as a whole.) We saw several occurrences of people passing this option to clang when building libsystem components and not realizing why there is a problem at link time. This is not a great experience, but happy linking and running into problems at run time would be worse. https://reviews.llvm.org/D24048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24048: [Driver] [Darwin] Add sanitizer libraries even if -nodefaultlibs is passed
zaks.anna added a comment. > I don't see the point of adding another flag to control this when we already > have a perfectly good set of > flags that already do the right thing -- that takes us three levels deep in > flags overriding the behavior of > other flags, and I don't see how it actually helps our users in any way. Going with silently linking the sanitizer runtimes when -nostdlib is passed will cause regressions in user experience. They will start getting inexplicable run time failures instead of build failures. Which solution do you propose to fix this problem? This specific situation comes up often on internal mailing lists, so we should not go for a solution that regresses the behavior on Darwin. https://reviews.llvm.org/D24048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1003
@@ +1002,3 @@
+//
+ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C,
+ const CXXNewExpr *NE,
NoQ wrote:
> dkrupp wrote:
> > xazax.hun wrote:
> > > zaks.anna wrote:
> > > > I am not sure this code belongs to the malloc checker since it only
> > > > supports the array bounds checker. Is there a reason it's not part of
> > > > that checker?
> > > I think it is part of the malloc checker because it already does
> > > something very very similar to malloc, see the MallocMemAux function. So
> > > in fact, for the array bounds checker to work properly, the malloc
> > > checker should be turned on.
> > Extent size is used by ArrayBoundChecker, ArrayBoundCheckerV2 and
> > CStringChecker checkers currently. New expression in case of simple
> > allocations (0 allocation) was already handled in Malloc checker , that's
> > why I implemented it there. But I agree it feels odd that one has to switch
> > on unix.Malloc checker to get the size of new allocated heap regions.
> > Should I move this to ArrayBoundChecker or ArrayBoundCheckerV2?
> 1. Well, in my dreams, this code should be in the silent operator-new
> modelling checker, which would be separate from the stateless operator-new
> bug-finding checker. Then all the checkers that rely on extent would
> automatically load the modelling checker as a dependency.
>
> 2. Yeah, i think many checkers may consider extents, not just array bound;
> other checkers may appear in the future. This info is very useful, which is
> why we have the whole symbol class just for that (however, checker
> communication through constraints on this symbol class wasn't IMHO a good
> idea, because it's harder for the constraint manager to deal with symbols and
> constraints rather than with concrete values).
>
> //Just wanted to speak out, thoughts below might perhaps be more on-topic//
>
> 3. The MallocChecker is not just unix.Malloc, but also cplusplus.NewDelete,
> etc., which makes a lot more sense to leave it here.
>
> 4. Consider another part of MallocChecker's job - modeling the memory spaces
> for symbolic regions (heap vs. unknown). For malloc(), this is done in the
> checker. For operator-new, it is done in the ExprEngine(!), because this is
> part of the language rather than a library function. Perhaps ExprEngine would
> be the proper place for that code as well.
> Well, in my dreams, this code should be in the silent operator-new modelling
> checker, which would be
> separate from the stateless operator-new bug-finding checker. Then all the
> checkers that rely on extent
> would automatically load the modelling checker as a dependency.
I agree. This sounds like the best approach! (Though it's not a must have to
get the patch in.)
> Consider another part of MallocChecker's job - modeling the memory spaces for
> symbolic regions (heap vs.
> unknown). For malloc(), this is done in the checker. For operator-new, it is
> done in the ExprEngine(!), because
> this is part of the language rather than a library function. Perhaps
> ExprEngine would be the proper place for
> that code as well.
Interesting point. Can you clarify the last sentence?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1020
@@ +1019,3 @@
+ } else {
+Size = svalBuilder.makeIntVal(1, true);
+Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs();
Please, be more explicit that this is not a size of allocation (as in not 1
byte)? Maybe call this ElementCount or something like that.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1021
@@ +1020,3 @@
+Size = svalBuilder.makeIntVal(1, true);
+Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs();
+ }
A bit of code repetition from above. Please, add comments explaining why we
need subregion in one case and super region in the other.
Are there test cases for this branch?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1049
@@ +1048,3 @@
+ CharUnits Scaling, SValBuilder &Sb) {
+ return Sb.evalBinOpNN(State, BO_Mul, BaseVal,
+Sb.makeArrayIndex(Scaling.getQuantity()),
This should probably be inline/have different name since it talks about
ArrayIndexType and is not reused elsewhere.
Comment at: test/Analysis/out-of-bounds.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -std=c++11 -Wno-array-bounds -analyze
-analyzer-checker=unix,core,alpha.security.ArrayBoundV2 -verify %s
+
Let's use a more specific file name.
https://reviews.llvm.org/D24307
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24278: [analyzer] Extend bug reports with extra notes.
zaks.anna added a comment.
Thanks!
Looks good overall. Several comments below.
Comment at: lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:160
@@ +159,3 @@
+[](const IntrusiveRefCntPtr &p) {
+ return isa(p.get());
+});
What do you think about calling these PathDiagnosticNotePiece, specifically,
looks like "Extra" does not add anything.
You'll probably need to change quite a few names to remove the "Extra", so up
to you.
Comment at: lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:170
@@ +169,3 @@
+ // Will also make a second pass through those later below,
+ // when the header text is ready.
+ HandlePiece(R, FID, **I, NumExtraPieces, TotalExtraPieces);
Why we need the second path? Please, add a comment as it's not obvious.
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:119
@@ +118,3 @@
+ PE = PD->path.end(); PI != PE; ++PI) {
+if (!isa(PI->get()))
+ continue;
a.sidorin wrote:
> if (isa<...>()) {
> ...
> ...
> }
> ?
@a.sidorin, LLVM coding guidelines prefer early exits
http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code.
Comment at: test/Analysis/copypaste/plist-diagnostics.cpp:4
@@ +3,3 @@
+
+void log();
+
We should call out that this is not working as expected right now. As far as I
understand, this file is a test case for a FIXME, correct?
Comment at: test/Analysis/copypaste/text-diagnostics.cpp:5
@@ +4,3 @@
+
+int max(int a, int b) { // expected-warning{{Detected code clone}} //
expected-note{{Detected code clone}}
+ log();
It's better to use full sentences for the warning messages: "Clone of this code
is detected" (or "Clones of this code are detected" when there are more than
one).
Comment at: test/Analysis/copypaste/text-diagnostics.cpp:12
@@ +11,3 @@
+
+int maxClone(int a, int b) { // expected-note{{Related code clone is here}}
+ log();
I would just say: "Code clone here". When I think about it, I assume the very
first one is not a clone, but the ones we highlight with notes are clones.
https://reviews.llvm.org/D24278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23300: [analyzer] Add "Assuming..." diagnostic pieces for unsupported condition expressions.
zaks.anna added a comment. @NoQ, Let's test in an IDE. Can you send screenshots? https://reviews.llvm.org/D23300 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22968: [analyzer] A checker for macOS-specific bool-like objects.
zaks.anna added a comment.
Let's test it on more real word bugs.
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:11
@@ +10,3 @@
+// This file defines BoolConversionChecker, which checks for a particular
+// common mistake when dealing with NSNumber and OSBoolean objects:
+// When having a pointer P to such object, neither `if (P)' nor `bool X = P'
Should we warn on comparisons of NSNumber to '0'? Comparisons to 'nil' would be
considered a proper pointer comparison, while comparisons to literal '0' would
be considered a faulty comparison of the numbers.
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:49
@@ +48,3 @@
+ virtual void run(const MatchFinder::MatchResult &Result) {
+// This callback only throws reports. All the logic is in the matchers.
+const Stmt *Conv = Result.Nodes.getNodeAs("conv");
"throws" -> "generates"?
https://reviews.llvm.org/D22968
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24484: [analyzer] Fix ExprEngine::VisitMemberExpr
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thanks! Do you have commit access? Repository: rL LLVM https://reviews.llvm.org/D24484 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21506: [analyzer] Block in critical section
zaks.anna added a comment. Do you have commit access or should we commit? Repository: rL LLVM https://reviews.llvm.org/D21506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21506: [analyzer] Block in critical section
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. LGTM! Thanks! Future steps: How do we plan to bring this checker out of alpha? Have you evaluated it on large codebases? Repository: rL LLVM https://reviews.llvm.org/D21506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24507: Add attribute for return values that shouldn't be cast to bool
zaks.anna added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:2055
@@ -2054,1 +2054,3 @@
}
+def WarnImpcastToBoolDocs : Documentation {
+ let Category = DocCatFunction;
You probably need to "propose" the attribute to the clang community. I'd send
an email to the cfe-dev as it might not have enough attention if it's just the
patch.
Comment at: test/ReturnNonBoolTest.c:74
@@ +73,3 @@
+ // no good way to get those from the program state.
+ if (restricted_wrap(2)) // expected-warning{{implicit cast to bool is
dangerous for this value}}
+return;
I do not understand why this is a false positive. In restricted_wrap, r can be
any value. You only return '0' if r is '-1', but it could be '-2' or '100',
which are also not bool and this values would just get returned.
You should be able to query the state to check if a value is a zero or one
using code like this from CStringChecker.cpp:
"
SValBuilder &svalBuilder = C.getSValBuilder();
DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
return state->assume(svalBuilder.evalEQ(state, *val, zero))
"
Repository:
rL LLVM
https://reviews.llvm.org/D24507
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24411: [Analyzer] Suppress false positives on the clang static analyzer
zaks.anna added a comment. It is not clear to me that we've reached a consensus on cfe-dev list that suppressing with comments and printing the checker name is the way to go. https://reviews.llvm.org/D24411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24307: calculate extent size for memory regions allocated by C++ new expression
zaks.anna added a comment. I do not have any more comments; however, let's wait for @NoQ to review this as well. Thanks! https://reviews.llvm.org/D24307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r291869 - [analyzer] Add LocationContext as a parameter to checkRegionChanges
Author: zaks
Date: Thu Jan 12 18:50:57 2017
New Revision: 291869
URL: http://llvm.org/viewvc/llvm-project?rev=291869&view=rev
Log:
[analyzer] Add LocationContext as a parameter to checkRegionChanges
This patch adds LocationContext to checkRegionChanges and removes
wantsRegionChangeUpdate as it was unused.
A patch by Krzysztof Wiśniewski!
Differential Revision: https://reviews.llvm.org/D27090
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h
cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/CheckerManager.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ProgramState.cpp
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h?rev=291869&r1=291868&r2=291869&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h Thu Jan 12 18:50:57
2017
@@ -321,9 +321,11 @@ class RegionChanges {
const InvalidatedSymbols *invalidated,
ArrayRef Explicits,
ArrayRef Regions,
+ const LocationContext *LCtx,
const CallEvent *Call) {
-return ((const CHECKER *)checker)->checkRegionChanges(state, invalidated,
- Explicits, Regions,
Call);
+return ((const CHECKER *) checker)->checkRegionChanges(state, invalidated,
+ Explicits, Regions,
+ LCtx, Call);
}
public:
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h?rev=291869&r1=291868&r2=291869&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h Thu Jan 12
18:50:57 2017
@@ -338,6 +338,7 @@ public:
const InvalidatedSymbols *invalidated,
ArrayRef ExplicitRegions,
ArrayRef Regions,
+ const LocationContext *LCtx,
const CallEvent *Call);
/// \brief Run checkers when pointers escape.
@@ -443,10 +444,11 @@ public:
typedef CheckerFn
CheckLiveSymbolsFunc;
typedef CheckerFn ExplicitRegions,
-ArrayRef Regions,
-const CallEvent *Call)>
+ const InvalidatedSymbols *symbols,
+ ArrayRef
ExplicitRegions,
+ ArrayRef Regions,
+ const LocationContext *LCtx,
+ const CallEvent *Call)>
CheckRegionChangesFunc;
typedef CheckerFnhttp://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h?rev=291869&r1=291868&r2=291869&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
(original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h Thu
Jan 12 18:50:57 2017
@@ -293,6 +293,7 @@ public:
const InvalidatedSymbols *invalidated,
ArrayRef ExplicitRegions,
ArrayRef Regions,
+ const LocationContext *LCtx,
const CallEvent *Call) override;
/// printState - Called by ProgramStateManager to print checker-specific
data.
@@ -522,7 +523,9 @@ protected:
/// Call PointerEscape callback when a value escapes as a result of bind.
ProgramStateRef processPointerEscapedOnBind(ProgramStateRef State,
-
r291866 - [analyzer] Fix false positives in Keychain API checker
Author: zaks
Date: Thu Jan 12 18:50:41 2017
New Revision: 291866
URL: http://llvm.org/viewvc/llvm-project?rev=291866&view=rev
Log:
[analyzer] Fix false positives in Keychain API checker
The checker has several false positives that this patch addresses:
- Do not check if the return status has been compared to error (or no error) at
the time when leaks are reported since the status symbol might no longer be
alive. Instead, pattern match on the assume and stop tracking allocated symbols
on error paths.
- The checker used to report error when an unknown symbol was freed. This could
lead to false positives, let's not repot those. This leads to loss of coverage
in double frees.
- Do not enforce that we should only call free if we are sure that error was
not returned and the pointer is not null. That warning is too noisy and we
received several false positive reports about it. (I removed: "Only call free
if a valid (non-NULL) buffer was returned")
- Use !isDead instead of isLive in leak reporting. Otherwise, we report leaks
for objects we loose track of. This change triggered change #1.
This also adds checker specific dump to the state.
Differential Revision: https://reviews.llvm.org/D28330
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
cfe/trunk/test/Analysis/keychainAPI.m
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp?rev=291866&r1=291865&r2=291866&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp Thu Jan
12 18:50:41 2017
@@ -28,7 +28,8 @@ using namespace ento;
namespace {
class MacOSKeychainAPIChecker : public Checker,
check::PostStmt,
- check::DeadSymbols> {
+ check::DeadSymbols,
+ eval::Assume> {
mutable std::unique_ptr BT;
public:
@@ -57,6 +58,10 @@ public:
void checkPreStmt(const CallExpr *S, CheckerContext &C) const;
void checkPostStmt(const CallExpr *S, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
+ ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond,
+ bool Assumption) const;
+ void printState(raw_ostream &Out, ProgramStateRef State,
+ const char *NL, const char *Sep) const;
private:
typedef std::pair AllocationPair;
@@ -106,19 +111,6 @@ private:
std::unique_ptr generateAllocatedDataNotReleasedReport(
const AllocationPair &AP, ExplodedNode *N, CheckerContext &C) const;
- /// Check if RetSym evaluates to an error value in the current state.
- bool definitelyReturnedError(SymbolRef RetSym,
- ProgramStateRef State,
- SValBuilder &Builder,
- bool noError = false) const;
-
- /// Check if RetSym evaluates to a NoErr value in the current state.
- bool definitelyDidnotReturnError(SymbolRef RetSym,
- ProgramStateRef State,
- SValBuilder &Builder) const {
-return definitelyReturnedError(RetSym, State, Builder, true);
- }
-
/// Mark an AllocationPair interesting for diagnostic reporting.
void markInteresting(BugReport *R, const AllocationPair &AP) const {
R->markInteresting(AP.first);
@@ -221,24 +213,6 @@ static SymbolRef getAsPointeeSymbol(cons
return nullptr;
}
-// When checking for error code, we need to consider the following cases:
-// 1) noErr / [0]
-// 2) someErr / [1, inf]
-// 3) unknown
-// If noError, returns true iff (1).
-// If !noError, returns true iff (2).
-bool MacOSKeychainAPIChecker::definitelyReturnedError(SymbolRef RetSym,
- ProgramStateRef State,
- SValBuilder &Builder,
- bool noError) const {
- DefinedOrUnknownSVal NoErrVal = Builder.makeIntVal(NoErr,
-Builder.getSymbolManager().getType(RetSym));
- DefinedOrUnknownSVal NoErr = Builder.evalEQ(State, NoErrVal,
-
nonloc::SymbolVal(RetSym));
- ProgramStateRef ErrState = State->assume(NoErr, noError);
- return ErrState == State;
-}
-
// Report deallocator mismatch. Remove the region from tracking - reporting a
// missing free error after this one is redundant.
void MacOSKeychainAPIChecker::
@@ -289,27 +263,25 @@ void MacOSKeychainAPIChecker::checkPreSt
const Expr *ArgExpr = CE->getArg(paramIdx);
if (SymbolRef V = getAsPointeeSymbol(ArgExpr, C))
r291868 - [tsan] Do not report errors in __destroy_helper_block_
Author: zaks
Date: Thu Jan 12 18:50:50 2017
New Revision: 291868
URL: http://llvm.org/viewvc/llvm-project?rev=291868&view=rev
Log:
[tsan] Do not report errors in __destroy_helper_block_
There is a synchronization point between the reference count of a block
dropping to zero and it's destruction, which TSan does not observe. Do not
report errors in the compiler-emitted block destroy method and everything
called from it.
This is similar to https://reviews.llvm.org/D25857
Differential Revision: https://reviews.llvm.org/D28387
Modified:
cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
cfe/trunk/test/CodeGen/sanitize-thread-no-checking-at-run-time.m
Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=291868&r1=291867&r2=291868&view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Thu Jan 12 18:50:50 2017
@@ -708,6 +708,11 @@ static bool endsWithReturn(const Decl* F
return false;
}
+static void markAsIgnoreThreadCheckingAtRuntime(llvm::Function *Fn) {
+ Fn->addFnAttr("sanitize_thread_no_checking_at_run_time");
+ Fn->removeFnAttr(llvm::Attribute::SanitizeThread);
+}
+
void CodeGenFunction::StartFunction(GlobalDecl GD,
QualType RetTy,
llvm::Function *Fn,
@@ -751,16 +756,19 @@ void CodeGenFunction::StartFunction(Glob
Fn->addFnAttr(llvm::Attribute::SafeStack);
// Ignore TSan memory acesses from within ObjC/ObjC++ dealloc, initialize,
- // .cxx_destruct and all of their calees at run time.
+ // .cxx_destruct, __destroy_helper_block_ and all of their calees at run
time.
if (SanOpts.has(SanitizerKind::Thread)) {
if (const auto *OMD = dyn_cast_or_null(D)) {
IdentifierInfo *II = OMD->getSelector().getIdentifierInfoForSlot(0);
if (OMD->getMethodFamily() == OMF_dealloc ||
OMD->getMethodFamily() == OMF_initialize ||
(OMD->getSelector().isUnarySelector() &&
II->isStr(".cxx_destruct"))) {
-Fn->addFnAttr("sanitize_thread_no_checking_at_run_time");
-Fn->removeFnAttr(llvm::Attribute::SanitizeThread);
+markAsIgnoreThreadCheckingAtRuntime(Fn);
}
+} else if (const auto *FD = dyn_cast_or_null(D)) {
+ IdentifierInfo *II = FD->getIdentifier();
+ if (II && II->isStr("__destroy_helper_block_"))
+markAsIgnoreThreadCheckingAtRuntime(Fn);
}
}
Modified: cfe/trunk/test/CodeGen/sanitize-thread-no-checking-at-run-time.m
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/sanitize-thread-no-checking-at-run-time.m?rev=291868&r1=291867&r2=291868&view=diff
==
--- cfe/trunk/test/CodeGen/sanitize-thread-no-checking-at-run-time.m (original)
+++ cfe/trunk/test/CodeGen/sanitize-thread-no-checking-at-run-time.m Thu Jan 12
18:50:50 2017
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -emit-llvm -o
- %s | FileCheck -check-prefix=WITHOUT %s
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -emit-llvm -o
- %s -fsanitize=thread | FileCheck -check-prefix=TSAN %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -fblocks
-emit-llvm -o - %s | FileCheck -check-prefix=WITHOUT %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -fblocks
-emit-llvm -o - %s -fsanitize=thread | FileCheck -check-prefix=TSAN %s
+
+// WITHOUT-NOT: "sanitize_thread_no_checking_at_run_time"
__attribute__((objc_root_class))
@interface NSObject
@@ -26,9 +28,14 @@ public:
}
@end
-// WITHOUT-NOT: "sanitize_thread_no_checking_at_run_time"
-
// TSAN: initialize{{.*}}) [[ATTR:#[0-9]+]]
// TSAN: dealloc{{.*}}) [[ATTR:#[0-9]+]]
// TSAN: cxx_destruct{{.*}}) [[ATTR:#[0-9]+]]
+
+void test2(id x) {
+ extern void test2_helper(id (^)(void));
+ test2_helper(^{ return x; });
+// TSAN: define internal void @__destroy_helper_block_(i8*) [[ATTR:#[0-9]+]]
+}
+
// TSAN: attributes [[ATTR]] = { noinline nounwind {{.*}}
"sanitize_thread_no_checking_at_run_time" {{.*}} }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r291867 - [analyzer] Support inlining of '[self classMethod]' and '[[self class] classMethod]'
Author: zaks
Date: Thu Jan 12 18:50:47 2017
New Revision: 291867
URL: http://llvm.org/viewvc/llvm-project?rev=291867&view=rev
Log:
[analyzer] Support inlining of '[self classMethod]' and '[[self class]
classMethod]'
Differential Revision: https://reviews.llvm.org/D28495
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
cfe/trunk/test/Analysis/inlining/InlineObjCClassMethod.m
Modified: cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp?rev=291867&r1=291866&r2=291867&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp Thu Jan 12 18:50:47 2017
@@ -896,6 +896,38 @@ bool ObjCMethodCall::canBeOverridenInSub
llvm_unreachable("The while loop should always terminate.");
}
+static const ObjCMethodDecl *findDefiningRedecl(const ObjCMethodDecl *MD) {
+ if (!MD)
+return MD;
+
+ // Find the redeclaration that defines the method.
+ if (!MD->hasBody()) {
+for (auto I : MD->redecls())
+ if (I->hasBody())
+MD = cast(I);
+ }
+ return MD;
+}
+
+static bool isCallToSelfClass(const ObjCMessageExpr *ME) {
+ const Expr* InstRec = ME->getInstanceReceiver();
+ if (!InstRec)
+return false;
+ const auto *InstRecIg =
dyn_cast(InstRec->IgnoreParenImpCasts());
+
+ // Check that receiver is called 'self'.
+ if (!InstRecIg || !InstRecIg->getFoundDecl() ||
+ !InstRecIg->getFoundDecl()->getName().equals("self"))
+return false;
+
+ // Check that the method name is 'class'.
+ if (ME->getSelector().getNumArgs() != 0 ||
+ !ME->getSelector().getNameForSlot(0).equals("class"))
+return false;
+
+ return true;
+}
+
RuntimeDefinition ObjCMethodCall::getRuntimeDefinition() const {
const ObjCMessageExpr *E = getOriginExpr();
assert(E);
@@ -910,6 +942,7 @@ RuntimeDefinition ObjCMethodCall::getRun
const MemRegion *Receiver = nullptr;
if (!SupersType.isNull()) {
+ // The receiver is guaranteed to be 'super' in this case.
// Super always means the type of immediate predecessor to the method
// where the call occurs.
ReceiverT = cast(SupersType);
@@ -921,7 +954,7 @@ RuntimeDefinition ObjCMethodCall::getRun
DynamicTypeInfo DTI = getDynamicTypeInfo(getState(), Receiver);
QualType DynType = DTI.getType();
CanBeSubClassed = DTI.canBeASubClass();
- ReceiverT = dyn_cast(DynType);
+ ReceiverT = dyn_cast(DynType.getCanonicalType());
if (ReceiverT && CanBeSubClassed)
if (ObjCInterfaceDecl *IDecl = ReceiverT->getInterfaceDecl())
@@ -929,7 +962,32 @@ RuntimeDefinition ObjCMethodCall::getRun
CanBeSubClassed = false;
}
-// Lookup the method implementation.
+// Handle special cases of '[self classMethod]' and
+// '[[self class] classMethod]', which are treated by the compiler as
+// instance (not class) messages. We will statically dispatch to those.
+if (auto *PT = dyn_cast_or_null(ReceiverT)) {
+ // For [self classMethod], return the compiler visible declaration.
+ if (PT->getObjectType()->isObjCClass() &&
+ Receiver == getSelfSVal().getAsRegion())
+return RuntimeDefinition(findDefiningRedecl(E->getMethodDecl()));
+
+ // Similarly, handle [[self class] classMethod].
+ // TODO: We are currently doing a syntactic match for this pattern with
is
+ // limiting as the test cases in
Analysis/inlining/InlineObjCClassMethod.m
+ // shows. A better way would be to associate the meta type with the
symbol
+ // using the dynamic type info tracking and use it here. We can add a new
+ // SVal for ObjC 'Class' values that know what interface declaration they
+ // come from. Then 'self' in a class method would be filled in with
+ // something meaningful in ObjCMethodCall::getReceiverSVal() and we could
+ // do proper dynamic dispatch for class methods just like we do for
+ // instance methods now.
+ if (E->getInstanceReceiver())
+if (const auto *M =
dyn_cast(E->getInstanceReceiver()))
+ if (isCallToSelfClass(M))
+return RuntimeDefinition(findDefiningRedecl(E->getMethodDecl()));
+}
+
+// Lookup the instance method implementation.
if (ReceiverT)
if (ObjCInterfaceDecl *IDecl = ReceiverT->getInterfaceDecl()) {
// Repeatedly calling lookupPrivateMethod() is expensive, especially
Modified: cfe/trunk/test/Analysis/inlining/InlineObjCClassMethod.m
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inlining/InlineObjCClassMethod.m?rev=291867&r1=291866&r2=291867&view=diff
==
--- cfe/trunk/test/Analysis/inlining/InlineObjCClassMethod.m (original)
+++ cfe/trunk/test/Analysis/inlining/InlineObjCClassMethod.m Thu Jan 12
Re: r292800 - [analyzer] Fix memory space of static locals seen from nested blocks.
Yes, ok to merge!
Thank you.
Sent from my iPhone
> On Jan 23, 2017, at 1:50 PM, Hans Wennborg wrote:
>
> Sounds good to me.
>
> Anna, you're the code owner here. Ok to merge this?
>
> Thanks,
> Hans
>
>> On Mon, Jan 23, 2017 at 10:37 AM, Artem Dergachev
>> wrote:
>> Hans,
>>
>> Could we merge this one into the 4.0.0 release branch? It's a recent bugfix
>> for the analyzer.
>>
>> Thanks,
>> Artem.
>>
>>
>>
>>> On 1/23/17 7:57 PM, Artem Dergachev via cfe-commits wrote:
>>>
>>> Author: dergachev
>>> Date: Mon Jan 23 10:57:11 2017
>>> New Revision: 292800
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=292800&view=rev
>>> Log:
>>> [analyzer] Fix memory space of static locals seen from nested blocks.
>>>
>>> When a block within a function accesses a function's static local
>>> variable,
>>> this local is captured by reference rather than copied to the heap.
>>>
>>> Therefore this variable's memory space is known: StaticGlobalSpaceRegion.
>>> Used to be UnknownSpaceRegion, same as for stack locals.
>>>
>>> Fixes a false positive in MacOSXAPIChecker.
>>>
>>> rdar://problem/30105546
>>> Differential revision: https://reviews.llvm.org/D28946
>>>
>>> Modified:
>>> cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
>>> cfe/trunk/test/Analysis/dispatch-once.m
>>>
>>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp?rev=292800&r1=292799&r2=292800&view=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp (original)
>>> +++ cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp Mon Jan 23 10:57:11
>>> 2017
>>> @@ -776,6 +776,22 @@ getStackOrCaptureRegionForDeclContext(co
>>>return (const StackFrameContext *)nullptr;
>>> }
>>> +static CanQualType getBlockPointerType(const BlockDecl *BD, ASTContext
>>> &C) {
>>> + // FIXME: The fallback type here is totally bogus -- though it should
>>> + // never be queried, it will prevent uniquing with the real
>>> + // BlockCodeRegion. Ideally we'd fix the AST so that we always had a
>>> + // signature.
>>> + QualType T;
>>> + if (const TypeSourceInfo *TSI = BD->getSignatureAsWritten())
>>> +T = TSI->getType();
>>> + if (T.isNull())
>>> +T = C.VoidTy;
>>> + if (!T->getAs())
>>> +T = C.getFunctionNoProtoType(T);
>>> + T = C.getBlockPointerType(T);
>>> + return C.getCanonicalType(T);
>>> +}
>>> +
>>> const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
>>> const LocationContext
>>> *LC) {
>>>const MemRegion *sReg = nullptr;
>>> @@ -803,7 +819,7 @@ const VarRegion* MemRegionManager::getVa
>>> sReg = getGlobalsRegion();
>>> }
>>> - // Finally handle static locals.
>>> + // Finally handle locals.
>>>} else {
>>> // FIXME: Once we implement scope handling, we will need to properly
>>> lookup
>>> // 'D' to the proper LocationContext.
>>> @@ -816,9 +832,22 @@ const VarRegion* MemRegionManager::getVa
>>>const StackFrameContext *STC = V.get();
>>> -if (!STC)
>>> - sReg = getUnknownRegion();
>>> -else {
>>> +if (!STC) {
>>> + if (D->isStaticLocal()) {
>>> +const CodeTextRegion *fReg = nullptr;
>>> +if (const auto *ND = dyn_cast(DC))
>>> + fReg = getFunctionCodeRegion(ND);
>>> +else if (const auto *BD = dyn_cast(DC))
>>> + fReg = getBlockCodeRegion(BD, getBlockPointerType(BD,
>>> getContext()),
>>> +LC->getAnalysisDeclContext());
>>> +assert(fReg && "Unable to determine code region for a static
>>> local!");
>>> +sReg = getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
>>> fReg);
>>> + } else {
>>> +// We're looking at a block-captured local variable, which may be
>>> either
>>> +// still local, or already moved to the heap. So we're not sure.
>>> +sReg = getUnknownRegion();
>>> + }
>>> +} else {
>>>if (D->hasLocalStorage()) {
>>> sReg = isa(D) || isa(D)
>>> ? static_cast>> MemRegion*>(getStackArgumentsRegion(STC))
>>> @@ -831,22 +860,9 @@ const VarRegion* MemRegionManager::getVa
>>>sReg =
>>> getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
>>>
>>> getFunctionCodeRegion(cast(STCD)));
>>> else if (const BlockDecl *BD = dyn_cast(STCD)) {
>>> - // FIXME: The fallback type here is totally bogus -- though it
>>> should
>>> - // never be queried, it will prevent uniquing with the real
>>> - // BlockCodeRegion. Ideally we'd fix the AST so that we always
>>> had a
>>> - // signature.
>>> - QualType T;
>>> - if (const TypeSourceInfo *TSI = BD->getSignatureAsWritten())
>>> -T = TSI->getType();
>>> - if (T.isNull())
>>> -T = getContext().VoidTy;
>
Re: r293043 - [analyzer] Fix MacOSXAPIChecker fp with static locals seen from nested blocks.
Fine with merging. Thank you!
Anna.
> On Feb 1, 2017, at 11:00 AM, Hans Wennborg wrote:
>
> If Anna is Ok with it, I'm fine with merging.
>
> Thanks,
> Hans
>
> On Wed, Feb 1, 2017 at 10:29 AM, Artem Dergachev wrote:
>> Hans,
>>
>> This is a fixed and tested version of the previously-merged-and-reverted
>> r292800, do we still have time to land this into 4.0?
>>
>> Thanks,
>> Artem.
>>
>>
>> On 1/25/17 1:21 PM, Artem Dergachev via cfe-commits wrote:
>>>
>>> Author: dergachev
>>> Date: Wed Jan 25 04:21:45 2017
>>> New Revision: 293043
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=293043&view=rev
>>> Log:
>>> [analyzer] Fix MacOSXAPIChecker fp with static locals seen from nested
>>> blocks.
>>>
>>> This is an attempt to avoid new false positives caused by the reverted
>>> r292800,
>>> however the scope of the fix is significantly reduced - some variables are
>>> still
>>> in incorrect memory spaces.
>>>
>>> Relevant test cases added.
>>>
>>> rdar://problem/30105546
>>> rdar://problem/30156693
>>> Differential revision: https://reviews.llvm.org/D28946
>>>
>>> Added:
>>> cfe/trunk/test/Analysis/null-deref-static.m
>>> Modified:
>>> cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
>>> cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
>>> cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
>>> cfe/trunk/test/Analysis/dispatch-once.m
>>>
>>> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp?rev=293043&r1=293042&r2=293043&view=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp (original)
>>> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp Wed Jan 25
>>> 04:21:45 2017
>>> @@ -94,11 +94,18 @@ void MacOSXAPIChecker::CheckDispatchOnce
>>>bool SuggestStatic = false;
>>>os << "Call to '" << FName << "' uses";
>>>if (const VarRegion *VR = dyn_cast(RB)) {
>>> +const VarDecl *VD = VR->getDecl();
>>> +// FIXME: These should have correct memory space and thus should be
>>> filtered
>>> +// out earlier. This branch only fires when we're looking from a
>>> block,
>>> +// which we analyze as a top-level declaration, onto a static local
>>> +// in a function that contains the block.
>>> +if (VD->isStaticLocal())
>>> + return;
>>> // We filtered out globals earlier, so it must be a local variable
>>> // or a block variable which is under UnknownSpaceRegion.
>>> if (VR != R)
>>>os << " memory within";
>>> -if (VR->getDecl()->hasAttr())
>>> +if (VD->hasAttr())
>>>os << " the block variable '";
>>> else
>>>os << " the local variable '";
>>>
>>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp?rev=293043&r1=293042&r2=293043&view=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp (original)
>>> +++ cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp Wed Jan 25 04:21:45
>>> 2017
>>> @@ -816,9 +816,11 @@ const VarRegion* MemRegionManager::getVa
>>>const StackFrameContext *STC = V.get();
>>> -if (!STC)
>>> +if (!STC) {
>>> + // FIXME: Assign a more sensible memory space to static locals
>>> + // we see from within blocks that we analyze as top-level
>>> declarations.
>>>sReg = getUnknownRegion();
>>> -else {
>>> +} else {
>>>if (D->hasLocalStorage()) {
>>> sReg = isa(D) || isa(D)
>>> ? static_cast>> MemRegion*>(getStackArgumentsRegion(STC))
>>>
>>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=293043&r1=293042&r2=293043&view=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp (original)
>>> +++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Wed Jan 25 04:21:45
>>> 2017
>>> @@ -1849,6 +1849,8 @@ SVal RegionStoreManager::getBindingForVa
>>>// Function-scoped static variables are default-initialized to 0;
>>> if they
>>> // have an initializer, it would have been processed by now.
>>> +// FIXME: This is only true when we're starting analysis from main().
>>> +// We're losing a lot of coverage here.
>>> if (isa(MS))
>>>return svalBuilder.makeZeroVal(T);
>>>
>>> Modified: cfe/trunk/test/Analysis/dispatch-once.m
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/dispatch-once.m?rev=293043&r1=293042&r2=293043&view=diff
>>>
>>> ==
>>> --- cf
Re: [PATCH] D29303: In VirtualCallChecker, handle indirect calls
Thank you! On Friday, February 24, 2017, Hans Wennborg wrote: > Yes, this looks very straight-forward. Merged in r296154. > > On Fri, Feb 24, 2017 at 4:29 AM, Sam McCall via cfe-commits > > wrote: > > Thanks Anna, I'm new to the release process here. > > > > Hans: this is a simple fix for a null-dereference in the static analyzer. > > Does it make sense to cherrypick? > > > > On Sat, Feb 18, 2017 at 2:46 AM, Anna Zaks via Phabricator > > > wrote: > >> > >> zaks.anna added a comment. > >> > >> Has this been cherry-picked into the clang 4.0 release branch? If not, > we > >> should definitely do that! > >> > >> > >> Repository: > >> rL LLVM > >> > >> https://reviews.llvm.org/D29303 > >> > >> > >> > > > > > > ___ > > cfe-commits mailing list > > cfe-commits@lists.llvm.org > > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r297324 - [analyzer] Add bug visitor for taint checker.
Author: zaks
Date: Wed Mar 8 18:01:07 2017
New Revision: 297324
URL: http://llvm.org/viewvc/llvm-project?rev=297324&view=rev
Log:
[analyzer] Add bug visitor for taint checker.
Add a bug visitor to the taint checker to make it easy to distinguish where
the tainted value originated. This is especially useful when the original
taint source is obscured by complex data flow.
A patch by Vlad Tsyrklevich!
Differential Revision: https://reviews.llvm.org/D30289
Added:
cfe/trunk/test/Analysis/taint-diagnostic-visitor.c
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp?rev=297324&r1=297323&r2=297324&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp Wed Mar 8
18:01:07 2017
@@ -101,6 +101,22 @@ private:
bool generateReportIfTainted(const Expr *E, const char Msg[],
CheckerContext &C) const;
+ /// The bug visitor prints a diagnostic message at the location where a given
+ /// variable was tainted.
+ class TaintBugVisitor
+ : public BugReporterVisitorImpl {
+ private:
+const SVal V;
+
+ public:
+TaintBugVisitor(const SVal V) : V(V) {}
+void Profile(llvm::FoldingSetNodeID &ID) const override { ID.Add(V); }
+
+std::shared_ptr VisitNode(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext &BRC,
+ BugReport &BR) override;
+ };
typedef SmallVector ArgVector;
@@ -194,6 +210,28 @@ const char GenericTaintChecker::MsgTaint
/// points to data, which should be tainted on return.
REGISTER_SET_WITH_PROGRAMSTATE(TaintArgsOnPostVisit, unsigned)
+std::shared_ptr
+GenericTaintChecker::TaintBugVisitor::VisitNode(const ExplodedNode *N,
+const ExplodedNode *PrevN, BugReporterContext &BRC, BugReport &BR) {
+
+ // Find the ExplodedNode where the taint was first introduced
+ if (!N->getState()->isTainted(V) || PrevN->getState()->isTainted(V))
+return nullptr;
+
+ const Stmt *S = PathDiagnosticLocation::getStmt(N);
+ if (!S)
+return nullptr;
+
+ const LocationContext *NCtx = N->getLocationContext();
+ PathDiagnosticLocation L =
+ PathDiagnosticLocation::createBegin(S, BRC.getSourceManager(), NCtx);
+ if (!L.isValid() || !L.asLocation().isValid())
+return nullptr;
+
+ return std::make_shared(
+ L, "Taint originated here");
+}
+
GenericTaintChecker::TaintPropagationRule
GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule(
const FunctionDecl *FDecl,
@@ -635,8 +673,13 @@ bool GenericTaintChecker::generateReport
// Check for taint.
ProgramStateRef State = C.getState();
- if (!State->isTainted(getPointedToSymbol(C, E)) &&
- !State->isTainted(E, C.getLocationContext()))
+ const SymbolRef PointedToSym = getPointedToSymbol(C, E);
+ SVal TaintedSVal;
+ if (State->isTainted(PointedToSym))
+TaintedSVal = nonloc::SymbolVal(PointedToSym);
+ else if (State->isTainted(E, C.getLocationContext()))
+TaintedSVal = C.getSVal(E);
+ else
return false;
// Generate diagnostic.
@@ -644,6 +687,7 @@ bool GenericTaintChecker::generateReport
initBugType();
auto report = llvm::make_unique(*BT, Msg, N);
report->addRange(E->getSourceRange());
+report->addVisitor(llvm::make_unique(TaintedSVal));
C.emitReport(std::move(report));
return true;
}
Added: cfe/trunk/test/Analysis/taint-diagnostic-visitor.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/taint-diagnostic-visitor.c?rev=297324&view=auto
==
--- cfe/trunk/test/Analysis/taint-diagnostic-visitor.c (added)
+++ cfe/trunk/test/Analysis/taint-diagnostic-visitor.c Wed Mar 8 18:01:07 2017
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core
-analyzer-output=text -verify %s
+
+// This file is for testing enhanced diagnostics produced by the
GenericTaintChecker
+
+int scanf(const char *restrict format, ...);
+int system(const char *command);
+
+void taintDiagnostic()
+{
+ char buf[128];
+ scanf("%s", buf); // expected-note {{Taint originated here}}
+ system(buf); // expected-warning {{Untrusted data is passed to a system
call}} // expected-note {{Untrusted data is passed to a system call
(CERT/STR02-C. Sanitize data passed to complex subsystems)}}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo
r297325 - [analyzer] Improve usability of ExprInspectionChecker
Author: zaks
Date: Wed Mar 8 18:01:10 2017
New Revision: 297325
URL: http://llvm.org/viewvc/llvm-project?rev=297325&view=rev
Log:
[analyzer] Improve usability of ExprInspectionChecker
Some of the magic functions take arguments of arbitrary type. However,
for semantic correctness, the compiler still requires a declaration
of these functions with the correct type. Since C does not have
argument-type-overloaded function, this made those functions hard to
use in C code. Improve this situation by allowing arbitrary suffixes
in the affected magic functions' names, thus allowing the user to
create different declarations for different types.
A patch by Keno Fischer!
Differential Revision: https://reviews.llvm.org/D30589
Added:
cfe/trunk/test/Analysis/explain-svals.c
Modified:
cfe/trunk/docs/analyzer/DebugChecks.rst
cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
Modified: cfe/trunk/docs/analyzer/DebugChecks.rst
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/analyzer/DebugChecks.rst?rev=297325&r1=297324&r2=297325&view=diff
==
--- cfe/trunk/docs/analyzer/DebugChecks.rst (original)
+++ cfe/trunk/docs/analyzer/DebugChecks.rst Wed Mar 8 18:01:10 2017
@@ -178,15 +178,21 @@ ExprInspection checks
This function explains the value of its argument in a human-readable manner
in the warning message. You can make as many overrides of its prototype
in the test code as necessary to explain various integral, pointer,
- or even record-type values.
+ or even record-type values. To simplify usage in C code (where overloading
+ the function declaration is not allowed), you may append an arbitrary suffix
+ to the function name, without affecting functionality.
Example usage::
void clang_analyzer_explain(int);
void clang_analyzer_explain(void *);
+// Useful in C code
+void clang_analyzer_explain_int(int);
+
void foo(int param, void *ptr) {
clang_analyzer_explain(param); // expected-warning{{argument 'param'}}
+ clang_analyzer_explain_int(param); // expected-warning{{argument
'param'}}
if (!ptr)
clang_analyzer_explain(ptr); // expected-warning{{memory address '0'}}
}
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp?rev=297325&r1=297324&r2=297325&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp Wed Mar 8
18:01:10 2017
@@ -72,8 +72,8 @@ bool ExprInspectionChecker::evalCall(con
&ExprInspectionChecker::analyzerWarnIfReached)
.Case("clang_analyzer_warnOnDeadSymbol",
&ExprInspectionChecker::analyzerWarnOnDeadSymbol)
-.Case("clang_analyzer_explain", &ExprInspectionChecker::analyzerExplain)
-.Case("clang_analyzer_dump", &ExprInspectionChecker::analyzerDump)
+.StartsWith("clang_analyzer_explain",
&ExprInspectionChecker::analyzerExplain)
+.StartsWith("clang_analyzer_dump", &ExprInspectionChecker::analyzerDump)
.Case("clang_analyzer_getExtent",
&ExprInspectionChecker::analyzerGetExtent)
.Case("clang_analyzer_printState",
&ExprInspectionChecker::analyzerPrintState)
Added: cfe/trunk/test/Analysis/explain-svals.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/explain-svals.c?rev=297325&view=auto
==
--- cfe/trunk/test/Analysis/explain-svals.c (added)
+++ cfe/trunk/test/Analysis/explain-svals.c Wed Mar 8 18:01:10 2017
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze
-analyzer-checker=core.builtin,debug.ExprInspection,unix.cstring -verify %s
+
+struct S {
+ int z;
+};
+
+void clang_analyzer_explain_int(int);
+void clang_analyzer_explain_voidp(void *);
+void clang_analyzer_explain_S(struct S);
+
+int glob;
+
+void test_1(int param, void *ptr) {
+ clang_analyzer_explain_voidp(&glob); // expected-warning-re^pointer to
global variable 'glob'$
+ clang_analyzer_explain_int(param); // expected-warning-re^argument
'param'$
+ clang_analyzer_explain_voidp(ptr); // expected-warning-re^argument
'ptr'$
+ if (param == 42)
+clang_analyzer_explain_int(param); // expected-warning-re^signed
32-bit integer '42'$
+}
+
+void test_2(struct S s) {
+ clang_analyzer_explain_S(s); //expected-warning-re^lazily frozen
compound value of parameter 's'$
+ clang_analyzer_explain_voidp(&s); // expected-warning-re^pointer to
parameter 's'$
+ clang_analyzer_explain_int(s.z); // expected-warning-re^initial value
of field 'z' of parameter 's'$
+}
___
cfe-commits ma
r297323 - [analyzer] Teach the MallocChecker about about Glib API
Author: zaks
Date: Wed Mar 8 18:01:01 2017
New Revision: 297323
URL: http://llvm.org/viewvc/llvm-project?rev=297323&view=rev
Log:
[analyzer] Teach the MallocChecker about about Glib API
A patch by Leslie Zhai!
Differential Revision: https://reviews.llvm.org/D28348
Added:
cfe/trunk/test/Analysis/gmalloc.c
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=297323&r1=297322&r2=297323&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Wed Mar 8 18:01:01
2017
@@ -174,7 +174,10 @@ public:
II_valloc(nullptr), II_reallocf(nullptr), II_strndup(nullptr),
II_strdup(nullptr), II_win_strdup(nullptr), II_kmalloc(nullptr),
II_if_nameindex(nullptr), II_if_freenameindex(nullptr),
-II_wcsdup(nullptr), II_win_wcsdup(nullptr) {}
+II_wcsdup(nullptr), II_win_wcsdup(nullptr), II_g_malloc(nullptr),
+II_g_malloc0(nullptr), II_g_realloc(nullptr),
II_g_try_malloc(nullptr),
+II_g_try_malloc0(nullptr), II_g_try_realloc(nullptr),
+II_g_free(nullptr), II_g_memdup(nullptr) {}
/// In pessimistic mode, the checker assumes that it does not know which
/// functions might free the memory.
@@ -236,7 +239,9 @@ private:
*II_realloc, *II_calloc, *II_valloc, *II_reallocf,
*II_strndup, *II_strdup, *II_win_strdup, *II_kmalloc,
*II_if_nameindex, *II_if_freenameindex, *II_wcsdup,
- *II_win_wcsdup;
+ *II_win_wcsdup, *II_g_malloc, *II_g_malloc0,
+ *II_g_realloc, *II_g_try_malloc, *II_g_try_malloc0,
+ *II_g_try_realloc, *II_g_free, *II_g_memdup;
mutable Optional KernelZeroFlagVal;
void initIdentifierInfo(ASTContext &C) const;
@@ -554,6 +559,16 @@ void MallocChecker::initIdentifierInfo(A
II_win_strdup = &Ctx.Idents.get("_strdup");
II_win_wcsdup = &Ctx.Idents.get("_wcsdup");
II_win_alloca = &Ctx.Idents.get("_alloca");
+
+ // Glib
+ II_g_malloc = &Ctx.Idents.get("g_malloc");
+ II_g_malloc0 = &Ctx.Idents.get("g_malloc0");
+ II_g_realloc = &Ctx.Idents.get("g_realloc");
+ II_g_try_malloc = &Ctx.Idents.get("g_try_malloc");
+ II_g_try_malloc0 = &Ctx.Idents.get("g_try_malloc0");
+ II_g_try_realloc = &Ctx.Idents.get("g_try_realloc");
+ II_g_free = &Ctx.Idents.get("g_free");
+ II_g_memdup = &Ctx.Idents.get("g_memdup");
}
bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext &C) const
{
@@ -589,7 +604,8 @@ bool MallocChecker::isCMemFunction(const
initIdentifierInfo(C);
if (Family == AF_Malloc && CheckFree) {
- if (FunI == II_free || FunI == II_realloc || FunI == II_reallocf)
+ if (FunI == II_free || FunI == II_realloc || FunI == II_reallocf ||
+ FunI == II_g_free)
return true;
}
@@ -597,7 +613,11 @@ bool MallocChecker::isCMemFunction(const
if (FunI == II_malloc || FunI == II_realloc || FunI == II_reallocf ||
FunI == II_calloc || FunI == II_valloc || FunI == II_strdup ||
FunI == II_win_strdup || FunI == II_strndup || FunI == II_wcsdup ||
- FunI == II_win_wcsdup || FunI == II_kmalloc)
+ FunI == II_win_wcsdup || FunI == II_kmalloc ||
+ FunI == II_g_malloc || FunI == II_g_malloc0 ||
+ FunI == II_g_realloc || FunI == II_g_try_malloc ||
+ FunI == II_g_try_malloc0 || FunI == II_g_try_realloc ||
+ FunI == II_g_memdup)
return true;
}
@@ -762,7 +782,7 @@ void MallocChecker::checkPostStmt(const
initIdentifierInfo(C.getASTContext());
IdentifierInfo *FunI = FD->getIdentifier();
-if (FunI == II_malloc) {
+if (FunI == II_malloc || FunI == II_g_malloc || FunI == II_g_try_malloc) {
if (CE->getNumArgs() < 1)
return;
if (CE->getNumArgs() < 3) {
@@ -791,7 +811,8 @@ void MallocChecker::checkPostStmt(const
return;
State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
State = ProcessZeroAllocation(C, CE, 0, State);
-} else if (FunI == II_realloc) {
+} else if (FunI == II_realloc || FunI == II_g_realloc ||
+ FunI == II_g_try_realloc) {
State = ReallocMem(C, CE, false, State);
State = ProcessZeroAllocation(C, CE, 1, State);
} else if (FunI == II_reallocf) {
@@ -801,7 +822,7 @@ void MallocChecker::checkPostStmt(const
State = CallocMem(C, CE, State);
State = ProcessZeroAllocation(C, CE, 0, State);
State = ProcessZeroAllocation(C, CE, 1, State);
-} else if (FunI == II_free) {
+} else if (FunI == II_free || FunI == II_g_free) {
State = Fre
r297326 - [analyzer] Extend taint propagation and checking to support LazyCompoundVal
Author: zaks
Date: Wed Mar 8 18:01:16 2017
New Revision: 297326
URL: http://llvm.org/viewvc/llvm-project?rev=297326&view=rev
Log:
[analyzer] Extend taint propagation and checking to support LazyCompoundVal
A patch by Vlad Tsyrklevich!
Differential Revision: https://reviews.llvm.org/D28445
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
cfe/trunk/test/Analysis/taint-generic.c
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h?rev=297326&r1=297325&r2=297326&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h Wed Mar
8 18:01:16 2017
@@ -59,6 +59,30 @@ public:
/// \return The value bound to the location \c loc.
virtual SVal getBinding(Store store, Loc loc, QualType T = QualType()) = 0;
+ /// Return the default value bound to a region in a given store. The default
+ /// binding is the value of sub-regions that were not initialized separately
+ /// from their base region. For example, if the structure is zero-initialized
+ /// upon construction, this method retrieves the concrete zero value, even if
+ /// some or all fields were later overwritten manually. Default binding may
be
+ /// an unknown, undefined, concrete, or symbolic value.
+ /// \param[in] store The store in which to make the lookup.
+ /// \param[in] R The region to find the default binding for.
+ /// \return The default value bound to the region in the store, if a default
+ /// binding exists.
+ virtual Optional getDefaultBinding(Store store, const MemRegion *R) =
0;
+
+ /// Return the default value bound to a LazyCompoundVal. The default binding
+ /// is used to represent the value of any fields or elements within the
+ /// structure represented by the LazyCompoundVal which were not initialized
+ /// explicitly separately from the whole structure. Default binding may be an
+ /// unknown, undefined, concrete, or symbolic value.
+ /// \param[in] lcv The lazy compound value.
+ /// \return The default value bound to the LazyCompoundVal \c lcv, if a
+ /// default binding exists.
+ Optional getDefaultBinding(nonloc::LazyCompoundVal lcv) {
+return getDefaultBinding(lcv.getStore(), lcv.getRegion());
+ }
+
/// Return a state with the specified value bound to the given location.
/// \param[in] store The analysis state.
/// \param[in] loc The symbolic memory location.
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp?rev=297326&r1=297325&r2=297326&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp Wed Mar 8
18:01:16 2017
@@ -65,6 +65,18 @@ private:
/// and thus, is tainted.
static bool isStdin(const Expr *E, CheckerContext &C);
+ /// This is called from getPointedToSymbol() to resolve symbol references for
+ /// the region underlying a LazyCompoundVal. This is the default binding
+ /// for the LCV, which could be a conjured symbol from a function call that
+ /// initialized the region. It only returns the conjured symbol if the LCV
+ /// covers the entire region, e.g. we avoid false positives by not returning
+ /// a default bindingc for an entire struct if the symbol for only a single
+ /// field or element within it is requested.
+ // TODO: Return an appropriate symbol for sub-fields/elements of an LCV so
+ // that they are also appropriately tainted.
+ static SymbolRef getLCVSymbol(CheckerContext &C,
+nonloc::LazyCompoundVal &LCV);
+
/// \brief Given a pointer argument, get the symbol of the value it contains
/// (points to).
static SymbolRef getPointedToSymbol(CheckerContext &C, const Expr *Arg);
@@ -461,6 +473,27 @@ bool GenericTaintChecker::checkPre(const
return false;
}
+SymbolRef GenericTaintChecker::getLCVSymbol(CheckerContext &C,
+nonloc::LazyCompoundVal &LCV) {
+ StoreManager &StoreMgr = C.getStoreManager();
+
+ // getLCVSymbol() is reached in a PostStmt so we can always expect a default
+ // binding to exist if one is present.
+ if (Optional binding = StoreMgr.getDefaultBinding(LCV)) {
+SymbolRef Sym = binding->getAsSymbol();
+if (!Sym)
+ return nullptr;
+
+// If the LCV covers an entire base region return the default conjured
symbol.
+if (LCV.getRegion() == LCV.getRegion()->getBaseRegion())
r297429 - [analyzer] Turn suppress-c++-stdlib on by default
Author: zaks
Date: Thu Mar 9 18:33:19 2017
New Revision: 297429
URL: http://llvm.org/viewvc/llvm-project?rev=297429&view=rev
Log:
[analyzer] Turn suppress-c++-stdlib on by default
We have several reports of false positives coming from libc++. For example,
there are reports of false positives in std::regex, std::wcout, and also
a bunch of issues are reported in https://reviews.llvm.org/D30593. In many
cases, the analyzer trips over the complex libc++ code invariants. Let's turn
off the reports coming from these headers until we can re-evalate the support.
We can turn this back on once we individually suppress all known false
positives and perform deeper evaluation on large codebases that use libc++.
We'd also need to commit to doing these evaluations regularly as libc++
headers change.
Differential Revision: https://reviews.llvm.org/D30798
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
cfe/trunk/test/Analysis/diagnostics/explicit-suppression.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp?rev=297429&r1=297428&r2=297429&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp Thu Mar 9 18:33:19
2017
@@ -230,7 +230,7 @@ bool AnalyzerOptions::shouldSuppressInli
bool AnalyzerOptions::shouldSuppressFromCXXStandardLibrary() {
return getBooleanOption(SuppressFromCXXStandardLibrary,
"suppress-c++-stdlib",
- /* Default = */ false);
+ /* Default = */ true);
}
bool AnalyzerOptions::shouldReportIssuesInMainSourceFile() {
Modified: cfe/trunk/test/Analysis/diagnostics/explicit-suppression.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/diagnostics/explicit-suppression.cpp?rev=297429&r1=297428&r2=297429&view=diff
==
--- cfe/trunk/test/Analysis/diagnostics/explicit-suppression.cpp (original)
+++ cfe/trunk/test/Analysis/diagnostics/explicit-suppression.cpp Thu Mar 9
18:33:19 2017
@@ -1,5 +1,6 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection
-analyzer-config suppress-c++-stdlib=false -verify %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection
-analyzer-config suppress-c++-stdlib=true -DSUPPRESSED=1 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection
-DSUPPRESSED=1 -verify %s
#ifdef SUPPRESSED
// expected-no-diagnostics
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26588: Add LocationContext to members of check::RegionChanges
zaks.anna added a comment.
Hi and welcome to the project!
This patch definitely looks quite complex for a first contribution, so great
job at digging through the analyzer internals!
One higher level comment I have is that you should try and split patches
whenever possible. For example, in the description, you mention that the patch
contains 2 changes:
- extending RegionChanges interface with LocationContext
- adding an easy way to obtain arguments' SVals from StackFrameCtx
Since they are independent changes, please submit them as separate patches.
This simplifies the job of the reviewers and anyone who will ever look at this
patch in commit history or on phabricator. Also, this ensures that each change
has sufficient test coverage. The LLVM Dev policy has a great explanation of
the reasoning behind this:
http://llvm.org/docs/DeveloperPolicy.html#incremental-development.
How do you plan on testing these changes? The main 2 methods are unit tests and
checker regression tests. Since no checker uses these maybe we should only
commit them once such checker is added...
I've only started looking at the first change and would like to understand the
motivation behind it a bit more.
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:325
+ const CallEvent *Call,
+ const LocationContext *LCtx) {
+return ((const CHECKER *) checker)->checkRegionChanges(state, invalidated,
LocationContext can be obtained by calling CallEvent::getLocationContext(). I
do not think that adding another parameter here buys us much. Am I missing
something?
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:333
+ ProgramStateRef state,
+ const LocationContext *LCtx) {
+return ((const CHECKER *) checker)->wantsRegionChangeUpdate(state, LCtx);
What is the scenario in which you need the context here? The idea is that the
checker would be interested in region changes if it is already tracking
information in the state. For that check, the information in the state is
sufficient. What is your scenario?
Also, For any checker API change, we need to update the
CheckerDocumentation.cpp.
https://reviews.llvm.org/D26588
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26588: Add LocationContext to members of check::RegionChanges
zaks.anna added inline comments.
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:325
+ const CallEvent *Call,
+ const LocationContext *LCtx) {
+return ((const CHECKER *) checker)->checkRegionChanges(state, invalidated,
k-wisniewski wrote:
> NoQ wrote:
> > zaks.anna wrote:
> > > LocationContext can be obtained by calling
> > > CallEvent::getLocationContext(). I do not think that adding another
> > > parameter here buys us much. Am I missing something?
> > CallEvent* is optional here - it's there only for invalidations through
> > calls.
> How about the situation when this callback is triggered by something other
> than a call, like variable assignment? I've tried using that location context
> and had lots of segfaults as in such cases it appeared to be null. Do you
> have some info that it should be non-null in such a case?
Ok, makes sense. Have you looked into providing a checker context there? How
much more difficult would that be? If that is too difficult, adding
LocationContext is good as well.
If Call is optional and LocationContext is not, should the Call parameter be
last.
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:333
+ ProgramStateRef state,
+ const LocationContext *LCtx) {
+return ((const CHECKER *) checker)->wantsRegionChangeUpdate(state, LCtx);
k-wisniewski wrote:
> NoQ wrote:
> > zaks.anna wrote:
> > > What is the scenario in which you need the context here? The idea is that
> > > the checker would be interested in region changes if it is already
> > > tracking information in the state. For that check, the information in the
> > > state is sufficient. What is your scenario?
> > >
> > > Also, For any checker API change, we need to update the
> > > CheckerDocumentation.cpp.
> > Environment, which is a significant portion of the state, is essentially
> > unaccessible without a location context.
> >
> > Call stack is also an important bit of information to any checker that does
> > something special to support/exploit the inlining IPA - there are many
> > checkers that scan the call stack, but so far none of them needed to
> > subscribe to checkRegionChanges; their possible use case may look like "we
> > want an update only if a certain condition was met within the current stack
> > frame".
> >
> > For the recursion checker, i think, the point is "if current frame is
> > spoiled, we no longer want updates" (which should probably be fixed in the
> > checker patch: even though the callback is broken, it'd take one less
> > action to fix it if we decide to).
> Well I've added it to be consistent with checkRegionChanges, but AFAIK this
> callback is no longer necessary and there was a discussion about getting rid
> of it altogether. Maybe it's a good moment ot purge it?
I don't recall where that discussion has ended. If the callback is not used,
I'd be fine with removing it. (That would need to be a separate patch.)
https://reviews.llvm.org/D26588
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26759: Remove unused check::RegionChanges::wantsRegionChangeUpdate callback
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Looks great! https://reviews.llvm.org/D26759 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26759: Remove unused check::RegionChanges::wantsRegionChangeUpdate callback
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Core/CheckerManager.cpp:535 + ExplicitRegions, Regions, + Call, LCtx); } Looks like the other patch leaked in here: you have LCtx. https://reviews.llvm.org/D26759 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r287175 - [analyzer] Remove unused check::RegionChanges::wantsRegionChangeUpdate callback
Author: zaks
Date: Wed Nov 16 16:59:01 2016
New Revision: 287175
URL: http://llvm.org/viewvc/llvm-project?rev=287175&view=rev
Log:
[analyzer] Remove unused check::RegionChanges::wantsRegionChangeUpdate callback
Remove the check::RegionChanges::wantsRegionChangeUpdate callback as it is no
longer used (since checkPointerEscape has been added).
A patch by Krzysztof Wiśniewski!
Differential Revision: https://reviews.llvm.org/D26759
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h
cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/CheckerManager.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h?rev=287175&r1=287174&r2=287175&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h Wed Nov 16 16:59:01
2016
@@ -325,20 +325,13 @@ class RegionChanges {
return ((const CHECKER *)checker)->checkRegionChanges(state, invalidated,
Explicits, Regions,
Call);
}
- template
- static bool _wantsRegionChangeUpdate(void *checker,
- ProgramStateRef state) {
-return ((const CHECKER *)checker)->wantsRegionChangeUpdate(state);
- }
public:
template
static void _register(CHECKER *checker, CheckerManager &mgr) {
mgr._registerForRegionChanges(
CheckerManager::CheckRegionChangesFunc(checker,
- _checkRegionChanges),
- CheckerManager::WantsRegionChangeUpdateFunc(checker,
-
_wantsRegionChangeUpdate));
+
_checkRegionChanges));
}
};
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h?rev=287175&r1=287174&r2=287175&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h Wed Nov 16
16:59:01 2016
@@ -322,9 +322,6 @@ public:
ExprEngine &Eng,
ProgramPoint::Kind K);
- /// \brief True if at least one checker wants to check region changes.
- bool wantsRegionChangeUpdate(ProgramStateRef state);
-
/// \brief Run checkers for region changes.
///
/// This corresponds to the check::RegionChanges callback.
@@ -452,8 +449,6 @@ public:
const CallEvent *Call)>
CheckRegionChangesFunc;
- typedef CheckerFn WantsRegionChangeUpdateFunc;
-
typedef CheckerFn DeadSymbolsCheckers;
- struct RegionChangesCheckerInfo {
-CheckRegionChangesFunc CheckFn;
-WantsRegionChangeUpdateFunc WantUpdateFn;
- };
- std::vector RegionChangesCheckers;
+ std::vector RegionChangesCheckers;
std::vector PointerEscapeCheckers;
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h?rev=287175&r1=287174&r2=287175&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
(original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h Wed
Nov 16 16:59:01 2016
@@ -286,10 +286,6 @@ public:
ProgramStateRef processAssume(ProgramStateRef state, SVal cond,
bool assumption) override;
- /// wantsRegionChangeUpdate - Called by ProgramStateManager to determine if a
- /// region change should trigger a processRegionChanges update.
- bool wantsRegionChangeUpdate(ProgramStateRef state) override;
-
/// processRegionChanges - Called by ProgramStateManager whenever a change
is made
/// to the store. Used to update checkers that track region values.
ProgramStateRef
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h?rev=287175&r1=287174&r2=287175&view=diff
==
--- cfe/trunk/include/clang/Sta
[PATCH] D26773: [analyzer] Refactor recursive symbol reachability check to use symbol_iterator
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thank you for the cleanup!!! For bonus points, please add comments to the class APIs:) https://reviews.llvm.org/D26773 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:423
+
+void IteratorPastEndChecker::handleComparison(CheckerContext &C,
+ const SVal &LVal,
baloghadamsoftware wrote:
> zaks.anna wrote:
> > baloghadamsoftware wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > baloghadamsoftware wrote:
> > > > > > NoQ wrote:
> > > > > > > a.sidorin wrote:
> > > > > > > > What will happen if we write something like this:
> > > > > > > > ```
> > > > > > > > bool Eq1 = it1 == it2;
> > > > > > > > bool Eq2 = it3 == it4;
> > > > > > > > if (Eq1) {...}?
> > > > > > > > ```
> > > > > > > >
> > > > > > > > As I understand, we'll assume the second condition instead of
> > > > > > > > first.
> > > > > > > Had a look. So the problem is, we obtain the result of the
> > > > > > > comparison as a symbol, from which it is too hard to recover the
> > > > > > > operands in order to move iterator position data from one value
> > > > > > > to another.
> > > > > > >
> > > > > > > Normally we obtain a simple SymbolConjured for the return value
> > > > > > > of the `operator==()` call (or, similarly, `operator!=()`). For
> > > > > > > plain-value iterators (eg. `typedef T *iterator`) we might be
> > > > > > > obtaining an actual binary symbolic expression, but even then
> > > > > > > it's not quite clear how to obtain operands (the structure of the
> > > > > > > expression might have changed due to algebraic simplifications).
> > > > > > > Additionally, LHS and RHS aren't necessarily symbols (they might
> > > > > > > be semi-concrete), so composing symbolic expressions from them in
> > > > > > > general is problematic with our symbol hierarchy, which is rarely
> > > > > > > a problem for numbers but for structural symbols it'd be a mess.
> > > > > > >
> > > > > > > For now i suggest, instead of storing only the last LHS and RHS,
> > > > > > > to save a map from symbols (which are results of comparisons) to
> > > > > > > (LHS value, RHS value) pairs. This map should, apart from the
> > > > > > > obvious, be cleaned up whenever one of the iterators in the pair
> > > > > > > gets mutated (incremented or decremented). This should take care
> > > > > > > of the problem Alexey points out, and should work with
> > > > > > > semi-concrete stuff.
> > > > > > >
> > > > > > > For the future i suggest to let users construct their own symbols
> > > > > > > and symbolic expressions more easily. In fact, if only we had all
> > > > > > > iterators as regions, we should have probably used SymbolMetadata
> > > > > > > for this purpose: it's easy to both recover the parent region
> > > > > > > from it and use it in symbolic expressions. We could also
> > > > > > > deprecate the confusing structural symbols (provide default-bound
> > > > > > > lazy compound values for conjured structures instead), and then
> > > > > > > it'd be possible to transition to SymbolMetadata entirely.
> > > > > > Thank you for the suggestion. I am not sure if I fully understand
> > > > > > you. If I create a map where the key is the resulting symbol of the
> > > > > > comparison, it will not work because evalAssume is called for the
> > > > > > innermost comparison. So if the body of operator== (or operator!=)
> > > > > > is inlined, then I get a binary symbolic expression in evalAssume,
> > > > > > not the SymbolConjured. This binary Symbolic expression is a
> > > > > > comparison of the internals of the iterators, e.g. the internal
> > > > > > pointer. So the key will not match any LHS and RHS value pair in
> > > > > > the map. I also thought on such solution earlier but I dismissed it
> > > > > > because of this.
> > > > > Well, even if the body of the comparison operator is inlined,
> > > > > PreStmt/PostStmt callbacks should still work, and it doesn't really
> > > > > matter if there's a `SymbolConjured` or not, we can still add the
> > > > > symbolic expression to our map as a key.
> > > > >
> > > > > Essentially, you ignore whatever happens in the iterator's
> > > > > operator==() when it's inlined (including any evalAssume events),
> > > > > then in PostStmt of operator==() you map the return-value symbol of
> > > > > the operator to the operator's arguments (operands), then whenever an
> > > > > assumption is being made against the return-value symbol, you carry
> > > > > over this assumption to the operands. I think it shouldn't really
> > > > > matter if the operator call was inlined.
> > > > >
> > > > > The only unexpected thing that may happen due to inlining is if the
> > > > > inlined operator returns a concrete value (plain true or plain false)
> > > > > instead of the symbol, but in this case what we need to do is to just
> > > > > carry over the assumption to the operands instantly.
> > > > Maybe if I evaluate the operator==() call for iterators using
> > > > evalCall()?
[PATCH] D26588: Add LocationContext to members of check::RegionChanges
zaks.anna added inline comments.
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:325
+ const CallEvent *Call,
+ const LocationContext *LCtx) {
+return ((const CHECKER *) checker)->checkRegionChanges(state, invalidated,
NoQ wrote:
> zaks.anna wrote:
> > k-wisniewski wrote:
> > > NoQ wrote:
> > > > zaks.anna wrote:
> > > > > LocationContext can be obtained by calling
> > > > > CallEvent::getLocationContext(). I do not think that adding another
> > > > > parameter here buys us much. Am I missing something?
> > > > CallEvent* is optional here - it's there only for invalidations through
> > > > calls.
> > > How about the situation when this callback is triggered by something
> > > other than a call, like variable assignment? I've tried using that
> > > location context and had lots of segfaults as in such cases it appeared
> > > to be null. Do you have some info that it should be non-null in such a
> > > case?
> > Ok, makes sense. Have you looked into providing a checker context there?
> > How much more difficult would that be? If that is too difficult, adding
> > LocationContext is good as well.
> >
> > If Call is optional and LocationContext is not, should the Call parameter
> > be last.
> If we add a CheckerContext, then we may end up calling callbacks that split
> states within callbacks that split states, and that'd be quite a mess to
> support.
>
> Right now a checker may trigger `checkRegionChanges()` within its callback by
> manipulating the Store, which already leads to callbacks within callbacks,
> but that's bearable as long as you can't add transitions within the inner
> callbacks.
This argument by itself does not seem to be preventing us from providing
CheckerContext. For example, we could disallow splitting states (ex: by setting
some flag within the CheckerContext) but still provide most of the convenience
APIs.
The advantage of providing CheckerContext is that it is a class that provides
access to different analyzer APIs that the checker writer might want to access.
It is also familiar and somewhat expected as it is available in most other
cases. It might be difficult to pipe enough information to construct it... I
suspect that is the reason we do not provide it in this case.
I am OK with the patch as is but it would be good to explore if we could extend
this API by providing the full CheckerContext.
https://reviews.llvm.org/D26588
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25606: alpha.core.UnreachableCode - don't warn about unreachable code inside macro
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. LGTM. Thank you! Repository: rL LLVM https://reviews.llvm.org/D25606 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25663: [analyzer] Update alpha and potential checker documentation, esp. alpha.valist
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thank you for the cleanup! Anna. https://reviews.llvm.org/D25663 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25731: [analyzer] NumberObjectConversion: Support CFNumberRef.
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:72
assert(Conv);
- const Expr *Osboolean = Result.Nodes.getNodeAs("osboolean");
- const Expr *Nsnumber = Result.Nodes.getNodeAs("nsnumber");
- bool IsObjC = (bool)Nsnumber;
- const Expr *Obj = IsObjC ? Nsnumber : Osboolean;
+ const Expr *CObject = Result.Nodes.getNodeAs("c_object");
+ const Expr *CppObject = Result.Nodes.getNodeAs("cpp_object");
I'd keep the names more specific. By reading this code alone it looks like you
are just checking for **any** ObjC object.
Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:111
+ QualType ObjT = (IsCpp || IsObjC)
+ ? Obj->getType().getCanonicalType().getUnqualifiedType()
+ : Obj->getType();
Why do we need a case split here? Would calling
getCanonicalType().getUnqualifiedType() result in a wrong result for ObjC?
Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:129
+} else if (IsCpp) {
OS << "; please compare the pointer to NULL or nullptr instead "
"to suppress this warning";
Let's just recommend `nullptr` for C++, but allow both `NULL` and `nullptr`.
Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:152
- auto OSBooleanExprM =
+ auto SuspiciousCExprM =
+ expr(ignoringParenImpCasts(
Please, use more a expressive name. I'd prefer a super long name if it's more
expressive.
Comment at: test/Analysis/number-object-conversion.c:14
+ if (p) {} // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
+ if (!p) {} // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
+ p ? 1 : 2; // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
How about:
"Converting 'CFNumberRef' pointer to a plain boolean value; instead, compare
the pointer to NULL or compare to the encapsulated scalar value"
- I've added "pointer".
- I would remove "for branching". Does it add anything?
- Also, we should remove "please" as it makes the warning text longer.
https://reviews.llvm.org/D25731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25596: alpha.core.Conversion - Fix false positive for 'U32 += S16; ' expression, that is not unsafe
zaks.anna added a comment. Looks like you've also added handling of Xor, Or , Div, and Rem. Should there be tests for those? Repository: rL LLVM https://reviews.llvm.org/D25596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:111 + QualType ObjT = (IsCpp || IsObjC) + ? Obj->getType().getCanonicalType().getUnqualifiedType() + : Obj->getType(); NoQ wrote: > zaks.anna wrote: > > Why do we need a case split here? Would calling > > getCanonicalType().getUnqualifiedType() result in a wrong result for ObjC? > It'd result in a wrong result for `CFNumberRef`, which is a typedef we > shouldn't remove (turning this into `struct __CFNumber *` would be ugly). > > I'd probably rather avoid the case split by removing the > `.getCanonicalType()` part, because rarely anybody makes typedefs for > `NSNumber*` or `OSBoolean*` etc (such as in tests with the word "sugared"). > > Or i could descend down to the necessary typedef for C objects, discarding > all user's typedefs but not library typedefs. But that'd be even more > complicated and probably not very useful for such minor matter. > It'd result in a wrong result for CFNumberRef, which is a typedef we > shouldn't > remove (turning this into struct __CFNumber * would be ugly). I see. Displaying "CFNumberRef" is definitely the right thing to do! Please, add a comment to explain this. https://reviews.llvm.org/D25731 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25857: [tsan] Introduce a function attribute to disable TSan checking at run time
zaks.anna created this revision.
zaks.anna added reviewers: kcc, kubabrecka, dvyukov.
zaks.anna added a subscriber: cfe-commits.
This introduces a function annotation that disables TSan checking for the
function at run time. The benefit over __attribute__((no_sanitize("thread")))
is that the accesses within the callees will also be suppressed.
The motivation for this attribute is a guarantee given by the objective C
language that the calls to the reference count decrement and object
deallocation will be synchronized. To model this properly, we would need to
intercept all ref count decrement calls (which are very common in ObjC due to
use of ARC) and also every single message send. Instead, we propose to just
ignore all accesses made from within dealloc at run time. The main downside is
that this still does not introduce any synchronization, which means we might
still report false positives if the code that relies on this synchronization is
not executed from within dealloc. However, we have not seen this in practice so
far and think these cases will be very rare.
(This problem is similar in nature to https://reviews.llvm.org/D21609;
unfortunately, the same solution does not apply here.)
https://reviews.llvm.org/D25857
Files:
lib/CodeGen/CodeGenFunction.cpp
test/CodeGen/sanitize-thread-attr-dealloc.m
Index: test/CodeGen/sanitize-thread-attr-dealloc.m
===
--- /dev/null
+++ test/CodeGen/sanitize-thread-attr-dealloc.m
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -emit-llvm -o - %s | FileCheck
-check-prefix=WITHOUT %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=thread | FileCheck -check-prefix=TSAN %s
+
+__attribute__((objc_root_class))
+@interface NSObject
+- (void)dealloc;
+@end
+
+@interface MyObject : NSObject
+- (void) dealloc;
+@end
+
+@implementation MyObject
+- (void)dealloc {
+ [super dealloc];
+}
+@end
+
+// WITHOUT-NOT: "sanitize_thread_no_checking_at_run_time"
+
+// TSAN: dealloc{{.*}}) [[ATTR:#[0-9]+]]
+// TSAN: attributes [[ATTR]] = { nounwind sanitize_thread {{.*}}
"sanitize_thread_no_checking_at_run_time" {{.*}} }
+
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -714,6 +714,13 @@
if (SanOpts.has(SanitizerKind::SafeStack))
Fn->addFnAttr(llvm::Attribute::SafeStack);
+ // Ignore TSan memory acesses from within dealloc and all of its calees at
+ // run time.
+ if (SanOpts.has(SanitizerKind::Thread))
+if (const auto *M = dyn_cast_or_null(D))
+ if (M->getMethodFamily() == OMF_dealloc)
+Fn->addFnAttr("sanitize_thread_no_checking_at_run_time");
+
// Apply xray attributes to the function (as a string, for now)
if (D && ShouldXRayInstrumentFunction()) {
if (const auto *XRayAttr = D->getAttr()) {
Index: test/CodeGen/sanitize-thread-attr-dealloc.m
===
--- /dev/null
+++ test/CodeGen/sanitize-thread-attr-dealloc.m
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -emit-llvm -o - %s | FileCheck -check-prefix=WITHOUT %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -emit-llvm -o - %s -fsanitize=thread | FileCheck -check-prefix=TSAN %s
+
+__attribute__((objc_root_class))
+@interface NSObject
+- (void)dealloc;
+@end
+
+@interface MyObject : NSObject
+- (void) dealloc;
+@end
+
+@implementation MyObject
+- (void)dealloc {
+ [super dealloc];
+}
+@end
+
+// WITHOUT-NOT: "sanitize_thread_no_checking_at_run_time"
+
+// TSAN: dealloc{{.*}}) [[ATTR:#[0-9]+]]
+// TSAN: attributes [[ATTR]] = { nounwind sanitize_thread {{.*}} "sanitize_thread_no_checking_at_run_time" {{.*}} }
+
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -714,6 +714,13 @@
if (SanOpts.has(SanitizerKind::SafeStack))
Fn->addFnAttr(llvm::Attribute::SafeStack);
+ // Ignore TSan memory acesses from within dealloc and all of its calees at
+ // run time.
+ if (SanOpts.has(SanitizerKind::Thread))
+if (const auto *M = dyn_cast_or_null(D))
+ if (M->getMethodFamily() == OMF_dealloc)
+Fn->addFnAttr("sanitize_thread_no_checking_at_run_time");
+
// Apply xray attributes to the function (as a string, for now)
if (D && ShouldXRayInstrumentFunction()) {
if (const auto *XRayAttr = D->getAttr()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
zaks.anna added inline comments.
Comment at: test/Analysis/number-object-conversion.c:14
+ if (p) {} // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
+ if (!p) {} // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
+ p ? 1 : 2; // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
NoQ wrote:
> zaks.anna wrote:
> > How about:
> > "Converting 'CFNumberRef' pointer to a plain boolean value; instead,
> > compare the pointer to NULL or compare to the encapsulated scalar value"
> >
> > - I've added "pointer".
> > - I would remove "for branching". Does it add anything?
> > - Also, we should remove "please" as it makes the warning text longer.
> >
> > I would remove "for branching". Does it add anything?
>
> Because there's otherwise no obvious boolean value around, i wanted to point
> out what exactly is going on.
>
> > or compare to the encapsulated scalar value
>
> They don't necessarily compare the value. Maybe "take"?
>
> > I've added "pointer".
>
> Not sure it's worth keeping for other objects ("`'NSNumber *' pointer`"
> sounds like a pointer to a pointer).
>> or compare to the encapsulated scalar value
> They don't necessarily compare the value. Maybe "take"?
OSBoolean or CFNumber:
[Comparing|Converting] a pointer value of type '[CFNumberRef|NSNumber *]' to a
scalar [boolean|integer] value; instead, either compare the pointer to
[NULL|nullptr|nil] or [call [APIName] | compare the result of calling [API
Name]].
NSNumber or OSNumber:
Converting a pointer value of type '[NSNumber *]' to a scalar [boolean|integer]
value; instead, either compare the pointer to [NULL|nullptr|nil] or call a
method on '[NSNumber *]' to get the scalar value.
Comparing a pointer value of type '[NSNumber *]' to a scalar [boolean|integer]
value; instead, either compare the pointer to [NULL|nullptr|nil] or call a
method on '[NSNumber *]' to get the scalar value.
Comment at: test/Analysis/number-object-conversion.c:23
+#endif
+ if (p > 0) {} // expected-warning{{Converting 'CFNumberRef' pointer to a
plain integer value; pointer value is being used instead}}
+ int x = p; // expected-warning{{Converting 'CFNumberRef' pointer to a plain
integer value; pointer value is being used instead}}
Comparing a pointer value of type '[CFNumberRef|NSNumber *]' to a primitive
[boolean|integer] value; did you mean to compare the result of calling [API
Name].
Comment at: test/Analysis/number-object-conversion.cpp:32
+ p ? 1 : 2; // expected-warning{{Converting 'const class OSBoolean *' pointer
to a branch condition; instead, compare the pointer to nullptr or take the
encapsulated scalar value}}
+ (bool)p; // expected-warning{{Converting 'const class OSBoolean *' pointer
to a plain bool value; instead, compare the pointer to nullptr or take the
encapsulated scalar value}}
+#else
Converting a pointer value of type '[CFNumberRef|NSNumber *]' to a primitive
[boolean|integer] value; did you mean to call [APIName].
Comment at: test/Analysis/number-object-conversion.cpp:41
+ x = p; // expected-warning{{Converting 'const class OSBoolean *' pointer to
a plain bool value; pointer value is being used instead}}
+ takes_bool(p); // expected-warning{{Converting 'const class OSBoolean *'
pointer to a plain bool value; pointer value is being used instead}}
takes_bool(x); // no-warning
Same as cast.
https://reviews.llvm.org/D25731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25876: [analyzer] Report CFNumberGetValue API misuse
zaks.anna created this revision.
zaks.anna added reviewers: dcoughlin, NoQ.
zaks.anna added subscribers: cfe-commits, rgov.
This patch contains 2 improvements to the CFNumber checker:
- Checking of CFNumberGetValue misuse.
- Treating all CFNumber API misuse errors as non-fatal. (Previously we treated
errors that could cause uninitialized memory as syncs and the truncation errors
as non-fatal.)
This implements a subset of functionality from https://reviews.llvm.org/D17954.
https://reviews.llvm.org/D25876
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
test/Analysis/CFNumber.c
Index: test/Analysis/CFNumber.c
===
--- test/Analysis/CFNumber.c
+++ test/Analysis/CFNumber.c
@@ -13,21 +13,35 @@
kCFNumberMaxType = 16 };
typedef CFIndex CFNumberType;
typedef const struct __CFNumber * CFNumberRef;
+typedef unsigned char Boolean;
extern CFNumberRef CFNumberCreate(CFAllocatorRef allocator, CFNumberType theType, const void *valuePtr);
+Boolean CFNumberGetValue(CFNumberRef number, CFNumberType theType, void *valuePtr);
-CFNumberRef f1(unsigned char x) {
+__attribute__((cf_returns_retained)) CFNumberRef f1(unsigned char x) {
return CFNumberCreate(0, kCFNumberSInt16Type, &x); // expected-warning{{An 8 bit integer is used to initialize a CFNumber object that represents a 16 bit integer. 8 bits of the CFNumber value will be garbage}}
}
__attribute__((cf_returns_retained)) CFNumberRef f2(unsigned short x) {
- return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16 bit integer is used to initialize a CFNumber object that represents an 8 bit integer. 8 bits of the input integer will be lost}}
+ return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16 bit integer is used to initialize a CFNumber object that represents an 8 bit integer. 8 bits of the integer value will be lost}}
}
// test that the attribute overrides the naming convention.
__attribute__((cf_returns_not_retained)) CFNumberRef CreateNum(unsigned char x) {
return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{leak}}
}
-CFNumberRef f3(unsigned i) {
+__attribute__((cf_returns_retained)) CFNumberRef f3(unsigned i) {
return CFNumberCreate(0, kCFNumberLongType, &i); // expected-warning{{A 32 bit integer is used to initialize a CFNumber object that represents a 64 bit integer}}
}
+
+unsigned char getValueTest1(CFNumberRef x) {
+ unsigned char scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt16Type, &scalar); // expected-warning{{A CFNumber object that represents a 16 bit integer is used to initialize an 8 bit integer. 8 bits of the CFNumber value will be lost}}
+ return scalar;
+}
+
+unsigned char getValueTest2(CFNumberRef x) {
+ unsigned short scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt8Type, &scalar); // expected-warning{{A CFNumber object that represents an 8 bit integer is used to initialize a 16 bit integer. 8 bits of the integer value will be garbage}}
+ return scalar;
+}
Index: lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -336,15 +336,15 @@
}
//===--===//
-// Error reporting.
+// Checking for mismatched types passed to CFNumberCreate/CFNumberGetValue.
//===--===//
namespace {
-class CFNumberCreateChecker : public Checker< check::PreStmt > {
+class CFNumberChecker : public Checker< check::PreStmt > {
mutable std::unique_ptr BT;
- mutable IdentifierInfo* II;
+ mutable IdentifierInfo *ICreate, *IGetValue;
public:
- CFNumberCreateChecker() : II(nullptr) {}
+ CFNumberChecker() : ICreate(nullptr), IGetValue(nullptr) {}
void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
@@ -425,18 +425,20 @@
}
#endif
-void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE,
+void CFNumberChecker::checkPreStmt(const CallExpr *CE,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
const FunctionDecl *FD = C.getCalleeDecl(CE);
if (!FD)
return;
ASTContext &Ctx = C.getASTContext();
- if (!II)
-II = &Ctx.Idents.get("CFNumberCreate");
-
- if (FD->getIdentifier() != II || CE->getNumArgs() != 3)
+ if (!ICreate) {
+ICreate = &Ctx.Idents.get("CFNumberCreate");
+IGetValue = &Ctx.Idents.get("CFNumberGetValue");
+ }
+ if (!(FD->getIdentifier() == ICreate || FD->getIdentifier() == IGetValue) ||
+ CE->getNumArgs() != 3)
return;
// Get the value of the "theType" argument.
@@ -450,13 +452,13 @@
return;
uint64_t NumberKind = V->getValue().getLimitedValue();
- Option
[PATCH] D25876: [analyzer] Report CFNumberGetValue API misuse
zaks.anna updated this revision to Diff 75488.
zaks.anna added a comment.
Address comments from Devin.
https://reviews.llvm.org/D25876
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
test/Analysis/CFNumber.c
Index: test/Analysis/CFNumber.c
===
--- test/Analysis/CFNumber.c
+++ test/Analysis/CFNumber.c
@@ -13,21 +13,35 @@
kCFNumberMaxType = 16 };
typedef CFIndex CFNumberType;
typedef const struct __CFNumber * CFNumberRef;
+typedef unsigned char Boolean;
extern CFNumberRef CFNumberCreate(CFAllocatorRef allocator, CFNumberType theType, const void *valuePtr);
+Boolean CFNumberGetValue(CFNumberRef number, CFNumberType theType, void *valuePtr);
-CFNumberRef f1(unsigned char x) {
- return CFNumberCreate(0, kCFNumberSInt16Type, &x); // expected-warning{{An 8 bit integer is used to initialize a CFNumber object that represents a 16 bit integer. 8 bits of the CFNumber value will be garbage}}
+__attribute__((cf_returns_retained)) CFNumberRef f1(unsigned char x) {
+ return CFNumberCreate(0, kCFNumberSInt16Type, &x); // expected-warning{{An 8-bit integer is used to initialize a CFNumber object that represents a 16-bit integer; 8 bits of the CFNumber value will be garbage}}
}
__attribute__((cf_returns_retained)) CFNumberRef f2(unsigned short x) {
- return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16 bit integer is used to initialize a CFNumber object that represents an 8 bit integer. 8 bits of the input integer will be lost}}
+ return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{A 16-bit integer is used to initialize a CFNumber object that represents an 8-bit integer; 8 bits of the integer value will be lost}}
}
// test that the attribute overrides the naming convention.
__attribute__((cf_returns_not_retained)) CFNumberRef CreateNum(unsigned char x) {
return CFNumberCreate(0, kCFNumberSInt8Type, &x); // expected-warning{{leak}}
}
-CFNumberRef f3(unsigned i) {
- return CFNumberCreate(0, kCFNumberLongType, &i); // expected-warning{{A 32 bit integer is used to initialize a CFNumber object that represents a 64 bit integer}}
+__attribute__((cf_returns_retained)) CFNumberRef f3(unsigned i) {
+ return CFNumberCreate(0, kCFNumberLongType, &i); // expected-warning{{A 32-bit integer is used to initialize a CFNumber object that represents a 64-bit integer}}
+}
+
+unsigned char getValueTest1(CFNumberRef x) {
+ unsigned char scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt16Type, &scalar); // expected-warning{{A CFNumber object that represents a 16-bit integer is used to initialize an 8-bit integer; 8 bits of the CFNumber value will overwrite adjacent storage}}
+ return scalar;
+}
+
+unsigned char getValueTest2(CFNumberRef x) {
+ unsigned short scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt8Type, &scalar); // expected-warning{{A CFNumber object that represents an 8-bit integer is used to initialize a 16-bit integer; 8 bits of the integer value will be garbage}}
+ return scalar;
}
Index: lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
===
--- lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
+++ lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
@@ -336,15 +336,15 @@
}
//===--===//
-// Error reporting.
+// Checking for mismatched types passed to CFNumberCreate/CFNumberGetValue.
//===--===//
namespace {
-class CFNumberCreateChecker : public Checker< check::PreStmt > {
+class CFNumberChecker : public Checker< check::PreStmt > {
mutable std::unique_ptr BT;
- mutable IdentifierInfo* II;
+ mutable IdentifierInfo *ICreate, *IGetValue;
public:
- CFNumberCreateChecker() : II(nullptr) {}
+ CFNumberChecker() : ICreate(nullptr), IGetValue(nullptr) {}
void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
@@ -425,18 +425,20 @@
}
#endif
-void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE,
+void CFNumberChecker::checkPreStmt(const CallExpr *CE,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
const FunctionDecl *FD = C.getCalleeDecl(CE);
if (!FD)
return;
ASTContext &Ctx = C.getASTContext();
- if (!II)
-II = &Ctx.Idents.get("CFNumberCreate");
-
- if (FD->getIdentifier() != II || CE->getNumArgs() != 3)
+ if (!ICreate) {
+ICreate = &Ctx.Idents.get("CFNumberCreate");
+IGetValue = &Ctx.Idents.get("CFNumberGetValue");
+ }
+ if (!(FD->getIdentifier() == ICreate || FD->getIdentifier() == IGetValue) ||
+ CE->getNumArgs() != 3)
return;
// Get the value of the "theType" argument.
@@ -450,13 +452,13 @@
return;
uint64_t NumberKind = V->getValue().getLimit
[PATCH] D25909: [analyzer] MacOSXApiChecker: Disallow dispatch_once predicates on heap and in ivars.
zaks.anna added a comment. Looks good overall! https://reviews.llvm.org/D25909 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
zaks.anna accepted this revision.
zaks.anna added a comment.
This revision is now accepted and ready to land.
Minor nit below.
Thanks for iterating so much on this!
Anna.
Comment at: test/Analysis/number-object-conversion.cpp:46
+#ifdef PEDANTIC
+ if (p) {} // expected-warning{{Converting a pointer value of type 'class
OSNumber *' to a primitive boolean value; instead, either compare the pointer
to nullptr or call a method on 'class OSNumber *' to get the scalar value}}
+ if (!p) {} // expected-warning{{Converting a pointer value of type 'class
OSNumber *' to a primitive boolean value; instead, either compare the pointer
to nullptr or call a method on 'class OSNumber *' to get the scalar value}}
Minor nit: Here, we want to use scalar instead of primitive so that both parts
of the sentence are consistent. Devin did not like "scalar", but that's what
it is called in the API docs, so I think should use that word in this specific
case. (We can keep 'primitive' for the error messages that do not have "to get
the ... value")
Sorry, this is a bit confusing.
https://reviews.llvm.org/D25731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25876: [analyzer] Report CFNumberGetValue API misuse
zaks.anna added inline comments.
Comment at: test/Analysis/CFNumber.c:39
+ unsigned char scalar = 0;
+ CFNumberGetValue(x, kCFNumberSInt16Type, &scalar); // expected-warning{{A
CFNumber object that represents a 16-bit integer is used to initialize an 8-bit
integer; 8 bits of the CFNumber value will overwrite adjacent storage}}
+ return scalar;
NoQ wrote:
> We're not sure from this code if the `CFNumber` object `x` actually
> represents a 16-bit integer, or somebody just misplaced the
> `kCFNumberSInt16Type` thing. I think the warning message could be made more
> precise in this sence, but i'm not good at coming up with warning messages.
>
> Hmm, there could actually be a separate check for detecting inconsistent type
> specifiers used for accessing the same CFNumber object.
I see your point. Looks like we'd need to modify both first part of the
sentence and the second to address this concern. We could do something like "A
CFNumber object treated as if it represents a 16-bit integer is used to
initialize an 8-bit integer; 8 bits of the CFNumber value or the adjacent
storage will overwrite adjacent storage of the integer".
Though this is more correct, I do not think it's worth the new language
complexity. Also, the warning message is already quite long.
https://reviews.llvm.org/D25876
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25985: [analyzer] Export coverage information from the analyzer.
zaks.anna added a comment.
Please, add multi-file tests and tests where a line is covered more than once.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:262
+
+static void dumpCoverageInfo(llvm::SmallVectorImpl &Path,
+ SourceManager &SM) {
Can this be a debug checker?
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:274
+if (Invalid)
+ continue;
+std::ofstream OutFile(FilePath.c_str());
Would it be better to break if the buffer is invalid?
Should this be hoisted out of the loop?
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:277
+if (!OutFile) {
+ llvm::errs() << FilePath << " Fuck!\n";
+ continue;
Please, come up with constructive error messages.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:281
+llvm::raw_os_ostream Out(OutFile);
+Out << "-:0:Source:" << FE->getName() << '\n';
+Out << "-:0:Runs:1\n";
What does '-' mean in this case? Why is it needed?
Comment at: test/Analysis/record-coverage.cpp.expected:2
+// CHECK: -:4:int main() {
+// CHECK-NEXT: -:5: int i = 2;
+// CHECK-NEXT: 1:6: ++i;
Does '-' mean not covered? If so, why the first 2 statements are not covered?
https://reviews.llvm.org/D25985
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25985: [analyzer] Export coverage information from the analyzer.
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:274 +if (Invalid) + continue; +std::ofstream OutFile(FilePath.c_str()); zaks.anna wrote: > Would it be better to break if the buffer is invalid? > Should this be hoisted out of the loop? I see why this cannot be hoisted. https://reviews.llvm.org/D25985 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r285253 - [analyzer] Report CFNumberGetValue API misuse
Author: zaks
Date: Wed Oct 26 17:51:47 2016
New Revision: 285253
URL: http://llvm.org/viewvc/llvm-project?rev=285253&view=rev
Log:
[analyzer] Report CFNumberGetValue API misuse
This patch contains 2 improvements to the CFNumber checker:
- Checking of CFNumberGetValue misuse.
- Treating all CFNumber API misuse errors as non-fatal. (Previously we treated
errors that could cause uninitialized memory as syncs and the truncation errors
as non-fatal.)
This implements a subset of functionality from https://reviews.llvm.org/D17954.
Differential Revision: https://reviews.llvm.org/D25876
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
cfe/trunk/test/Analysis/CFNumber.c
Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=285253&r1=285252&r2=285253&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Wed Oct 26
17:51:47 2016
@@ -586,8 +586,8 @@ def DirectIvarAssignmentForAnnotatedFunc
let ParentPackage = CoreFoundation in {
-def CFNumberCreateChecker : Checker<"CFNumber">,
- HelpText<"Check for proper uses of CFNumberCreate">,
+def CFNumberChecker : Checker<"CFNumber">,
+ HelpText<"Check for proper uses of CFNumber APIs">,
DescFile<"BasicObjCFoundationChecks.cpp">;
def CFRetainReleaseChecker : Checker<"CFRetainRelease">,
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp?rev=285253&r1=285252&r2=285253&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
(original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp Wed Oct
26 17:51:47 2016
@@ -336,15 +336,15 @@ void NilArgChecker::checkPostStmt(const
}
//===--===//
-// Error reporting.
+// Checking for mismatched types passed to CFNumberCreate/CFNumberGetValue.
//===--===//
namespace {
-class CFNumberCreateChecker : public Checker< check::PreStmt > {
+class CFNumberChecker : public Checker< check::PreStmt > {
mutable std::unique_ptr BT;
- mutable IdentifierInfo* II;
+ mutable IdentifierInfo *ICreate, *IGetValue;
public:
- CFNumberCreateChecker() : II(nullptr) {}
+ CFNumberChecker() : ICreate(nullptr), IGetValue(nullptr) {}
void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
@@ -425,7 +425,7 @@ static const char* GetCFNumberTypeStr(ui
}
#endif
-void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE,
+void CFNumberChecker::checkPreStmt(const CallExpr *CE,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
const FunctionDecl *FD = C.getCalleeDecl(CE);
@@ -433,10 +433,12 @@ void CFNumberCreateChecker::checkPreStmt
return;
ASTContext &Ctx = C.getASTContext();
- if (!II)
-II = &Ctx.Idents.get("CFNumberCreate");
-
- if (FD->getIdentifier() != II || CE->getNumArgs() != 3)
+ if (!ICreate) {
+ICreate = &Ctx.Idents.get("CFNumberCreate");
+IGetValue = &Ctx.Idents.get("CFNumberGetValue");
+ }
+ if (!(FD->getIdentifier() == ICreate || FD->getIdentifier() == IGetValue) ||
+ CE->getNumArgs() != 3)
return;
// Get the value of the "theType" argument.
@@ -450,13 +452,13 @@ void CFNumberCreateChecker::checkPreStmt
return;
uint64_t NumberKind = V->getValue().getLimitedValue();
- Optional OptTargetSize = GetCFNumberSize(Ctx, NumberKind);
+ Optional OptCFNumberSize = GetCFNumberSize(Ctx, NumberKind);
// FIXME: In some cases we can emit an error.
- if (!OptTargetSize)
+ if (!OptCFNumberSize)
return;
- uint64_t TargetSize = *OptTargetSize;
+ uint64_t CFNumberSize = *OptCFNumberSize;
// Look at the value of the integer being passed by reference. Essentially
// we want to catch cases where the value passed in is not equal to the
@@ -481,39 +483,44 @@ void CFNumberCreateChecker::checkPreStmt
if (!T->isIntegralOrEnumerationType())
return;
- uint64_t SourceSize = Ctx.getTypeSize(T);
+ uint64_t PrimitiveTypeSize = Ctx.getTypeSize(T);
- // CHECK: is SourceSize == TargetSize
- if (SourceSize == TargetSize)
+ if (PrimitiveTypeSize == CFNumberSize)
return;
- // Generate an error. Only generate a sink error node
- // if 'SourceSize < TargetSize'; otherwise generate a non-fatal error node.
- //
// FIXME: We can actually create an abstract "CFNumber" object that has
// the b
[PATCH] D25909: [analyzer] MacOSXApiChecker: Disallow dispatch_once predicates on heap and in ivars.
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Looks great! Please, commit. https://reviews.llvm.org/D25909 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.
zaks.anna added a comment.
>> Actually, I always test first on real code, and it seemed to be inlined. But
>> now, even if I
>> removed the pragma it was not inlined.
Looks like this patch is interfering with this inlining suppression. We had
many false positives without it. Mainly, the analyzer would not understand the
invariants of the container data structures.
`ExprEngine::defaultEvalCall` calls `mayInlineCallKind `which contains this:
`// Conditionally control the inlining of methods on objects that look
// like C++ containers.
if (!Opts.mayInlineCXXContainerMethods())
if (!Ctx.getSourceManager().isInMainFile(FD->getLocation()))
if (isContainerMethod(Ctx, FD))
return false;`
Comment at: test/Analysis/iterator-past-end.cpp:3
+
+template struct __iterator {
+ typedef __iterator iterator;
NoQ wrote:
> We should probably separate this into an #include-able header in
> `test/Analysis/Inputs/`.
>
> Also, there's always a bit of concern that it wasn't copy-pasted from a
> standard library implementation with an incompatible license such as (L)GPL.
> Which often happens when you do your best to emulate the normal way of
> defining things as closely as possible.
We often do forward declare in the implementation file as it is done here. We
mainly use the Inputs directory to simulate system headers.
https://reviews.llvm.org/D25660
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r285349 - [docs] Update the TSan and MSan docs to refer to the new no_sanitize attribute
Author: zaks
Date: Thu Oct 27 16:38:44 2016
New Revision: 285349
URL: http://llvm.org/viewvc/llvm-project?rev=285349&view=rev
Log:
[docs] Update the TSan and MSan docs to refer to the new no_sanitize attribute
TSan and MSan were the only remaining sanitizers referring to the deprecated
attribute for issue suppression. Update the docs to recommend
__attribute__((no_sanitize("..."))) instead.
Differential Revision: https://reviews.llvm.org/D25885
Modified:
cfe/trunk/docs/MemorySanitizer.rst
cfe/trunk/docs/ThreadSanitizer.rst
Modified: cfe/trunk/docs/MemorySanitizer.rst
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/MemorySanitizer.rst?rev=285349&r1=285348&r2=285349&view=diff
==
--- cfe/trunk/docs/MemorySanitizer.rst (original)
+++ cfe/trunk/docs/MemorySanitizer.rst Thu Oct 27 16:38:44 2016
@@ -76,14 +76,14 @@ whether MemorySanitizer is enabled. :ref
# endif
#endif
-``__attribute__((no_sanitize_memory))``
+``__attribute__((no_sanitize("memory")))``
---
Some code should not be checked by MemorySanitizer. One may use the function
-attribute `no_sanitize_memory` to disable uninitialized checks in a particular
-function. MemorySanitizer may still instrument such functions to avoid false
-positives. This attribute may not be supported by other compilers, so we
-suggest to use it together with ``__has_feature(memory_sanitizer)``.
+attribute ``no_sanitize("memory")`` to disable uninitialized checks in a
+particular function. MemorySanitizer may still instrument such functions to
+avoid false positives. This attribute may not be supported by other compilers,
+so we suggest to use it together with ``__has_feature(memory_sanitizer)``.
Blacklist
-
Modified: cfe/trunk/docs/ThreadSanitizer.rst
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ThreadSanitizer.rst?rev=285349&r1=285348&r2=285349&view=diff
==
--- cfe/trunk/docs/ThreadSanitizer.rst (original)
+++ cfe/trunk/docs/ThreadSanitizer.rst Thu Oct 27 16:38:44 2016
@@ -83,11 +83,11 @@ this purpose.
# endif
#endif
-``__attribute__((no_sanitize_thread))``
+``__attribute__((no_sanitize("thread")))``
---
Some code should not be instrumented by ThreadSanitizer. One may use the
-function attribute `no_sanitize_thread` to disable instrumentation of plain
+function attribute ``no_sanitize("thread")`` to disable instrumentation of
plain
(non-atomic) loads/stores in a particular function. ThreadSanitizer still
instruments such functions to avoid false positives and provide meaningful
stack
traces. This attribute may not be supported by other compilers, so we suggest
@@ -99,9 +99,9 @@ Blacklist
ThreadSanitizer supports ``src`` and ``fun`` entity types in
:doc:`SanitizerSpecialCaseList`, that can be used to suppress data race reports
in the specified source files or functions. Unlike functions marked with
-`no_sanitize_thread` attribute, blacklisted functions are not instrumented at
-all. This can lead to false positives due to missed synchronization via atomic
-operations and missed stack frames in reports.
+``no_sanitize("thread")`` attribute, blacklisted functions are not instrumented
+at all. This can lead to false positives due to missed synchronization via
+atomic operations and missed stack frames in reports.
Limitations
---
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25940: [analyzer] LibraryFunctions: Fix errors due to different integral types and typedefs on different architectures.
zaks.anna added a comment. How about this imperfect solution that will work quite well in practice? For the ssize_t case, where type size cannot be used, we check the function name, # of arguments , and check that the functions are coming from the system header. https://reviews.llvm.org/D25940 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25857: [tsan][clang] Introduce a function attribute to disable TSan checking at run time
zaks.anna added a comment. Thanks for the review! I'll submit the updated patches soon. https://reviews.llvm.org/D25857 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25857: [tsan][clang] Introduce a function attribute to disable TSan checking at run time
zaks.anna updated this revision to Diff 76643.
zaks.anna added a comment.
Addressed the review comments.
I also added ObjC +initialize method to the list because TSan does not observe
the guaranteed synchronization between +initialize and initial object accesses.
https://reviews.llvm.org/D25857
Files:
lib/CodeGen/CodeGenFunction.cpp
test/CodeGen/sanitize-thread-no-checking-at-run-time.m
Index: test/CodeGen/sanitize-thread-no-checking-at-run-time.m
===
--- /dev/null
+++ test/CodeGen/sanitize-thread-no-checking-at-run-time.m
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -emit-llvm -o
- %s | FileCheck -check-prefix=WITHOUT %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -emit-llvm -o
- %s -fsanitize=thread | FileCheck -check-prefix=TSAN %s
+
+__attribute__((objc_root_class))
+@interface NSObject
+- (void)dealloc;
+@end
+
+class NeedCleanup {
+public:
+ ~NeedCleanup() __attribute__((no_sanitize("thread"))) {}
+};
+
+@interface MyObject : NSObject {
+ NeedCleanup v;
+};
++ (void) initialize;
+- (void) dealloc;
+@end
+
+@implementation MyObject
++ (void)initialize {
+}
+- (void)dealloc {
+ [super dealloc];
+}
+@end
+
+// WITHOUT-NOT: "sanitize_thread_no_checking_at_run_time"
+
+// TSAN: initialize{{.*}}) [[ATTR:#[0-9]+]]
+// TSAN: dealloc{{.*}}) [[ATTR:#[0-9]+]]
+// TSAN: cxx_destruct{{.*}}) [[ATTR:#[0-9]+]]
+// TSAN: attributes [[ATTR]] = { nounwind {{.*}}
"sanitize_thread_no_checking_at_run_time" {{.*}} }
+// TSAN-NOT: sanitize_thread
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -731,6 +731,20 @@
if (SanOpts.has(SanitizerKind::SafeStack))
Fn->addFnAttr(llvm::Attribute::SafeStack);
+ // Ignore TSan memory acesses from within ObjC/ObjC++ dealloc, initialize,
+ // .cxx_destruct and all of their calees at run time.
+ if (SanOpts.has(SanitizerKind::Thread)) {
+if (const auto *OMD = dyn_cast_or_null(D)) {
+ IdentifierInfo *II = OMD->getSelector().getIdentifierInfoForSlot(0);
+ if (OMD->getMethodFamily() == OMF_dealloc ||
+ OMD->getMethodFamily() == OMF_initialize ||
+ (OMD->getSelector().isUnarySelector() &&
II->isStr(".cxx_destruct"))) {
+Fn->addFnAttr("sanitize_thread_no_checking_at_run_time");
+Fn->removeFnAttr(llvm::Attribute::SanitizeThread);
+ }
+}
+ }
+
// Apply xray attributes to the function (as a string, for now)
if (D && ShouldXRayInstrumentFunction()) {
if (const auto *XRayAttr = D->getAttr()) {
Index: test/CodeGen/sanitize-thread-no-checking-at-run-time.m
===
--- /dev/null
+++ test/CodeGen/sanitize-thread-no-checking-at-run-time.m
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -emit-llvm -o - %s | FileCheck -check-prefix=WITHOUT %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -emit-llvm -o - %s -fsanitize=thread | FileCheck -check-prefix=TSAN %s
+
+__attribute__((objc_root_class))
+@interface NSObject
+- (void)dealloc;
+@end
+
+class NeedCleanup {
+public:
+ ~NeedCleanup() __attribute__((no_sanitize("thread"))) {}
+};
+
+@interface MyObject : NSObject {
+ NeedCleanup v;
+};
++ (void) initialize;
+- (void) dealloc;
+@end
+
+@implementation MyObject
++ (void)initialize {
+}
+- (void)dealloc {
+ [super dealloc];
+}
+@end
+
+// WITHOUT-NOT: "sanitize_thread_no_checking_at_run_time"
+
+// TSAN: initialize{{.*}}) [[ATTR:#[0-9]+]]
+// TSAN: dealloc{{.*}}) [[ATTR:#[0-9]+]]
+// TSAN: cxx_destruct{{.*}}) [[ATTR:#[0-9]+]]
+// TSAN: attributes [[ATTR]] = { nounwind {{.*}} "sanitize_thread_no_checking_at_run_time" {{.*}} }
+// TSAN-NOT: sanitize_thread
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -731,6 +731,20 @@
if (SanOpts.has(SanitizerKind::SafeStack))
Fn->addFnAttr(llvm::Attribute::SafeStack);
+ // Ignore TSan memory acesses from within ObjC/ObjC++ dealloc, initialize,
+ // .cxx_destruct and all of their calees at run time.
+ if (SanOpts.has(SanitizerKind::Thread)) {
+if (const auto *OMD = dyn_cast_or_null(D)) {
+ IdentifierInfo *II = OMD->getSelector().getIdentifierInfoForSlot(0);
+ if (OMD->getMethodFamily() == OMF_dealloc ||
+ OMD->getMethodFamily() == OMF_initialize ||
+ (OMD->getSelector().isUnarySelector() && II->isStr(".cxx_destruct"))) {
+Fn->addFnAttr("sanitize_thread_no_checking_at_run_time");
+Fn->removeFnAttr(llvm::Attribute::SanitizeThread);
+ }
+}
+ }
+
// Apply xray attributes to the function (as a string, for now)
if (D && ShouldXRayInstrumentFunction()) {
if (const auto *XRayAttr = D->getAttr()) {
_
[PATCH] D25940: [analyzer] LibraryFunctions: Fix errors due to different integral types and typedefs on different architectures.
zaks.anna added a comment. Please, explain what variants are for in comments. https://reviews.llvm.org/D25940 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25940: [analyzer] LibraryFunctions: Fix errors due to different integral types and typedefs on different architectures.
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. LGTM other than the missing explanation in comments. https://reviews.llvm.org/D25940 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
