Re: [PATCH] D12726: [analyzer] A fix for symbolic element region index lifetime.

2015-10-12 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 37094. NoQ marked an inline comment as done. NoQ added a comment. > What are the implications in each case? Uhm, sorry, right, I guess i have to admit i don't quite understand what exactly is going on, so it'd take more time for me to conduct a deeper audit of

Re: [PATCH] D12726: [analyzer] A fix for symbolic element region index lifetime.

2015-10-13 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 37267. NoQ added a comment. A, another mis-submit. Sorry for the noise. http://reviews.llvm.org/D12726 Files: docs/analyzer/DebugChecks.rst include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h

Re: [PATCH] D5104: [analyzer] [proposal] Fix for PR20653

2015-10-13 Thread Artem Dergachev via cfe-commits
NoQ added a subscriber: NoQ. NoQ added a comment. While investigating http://reviews.llvm.org/D12726 , i accidentally came up with a way to test this patch; with the extension of `ExprInspectionChecker` in the aforementioned review, which allows testing SymbolReaper directly, the following

Re: [PATCH] D12726: [analyzer] A fix for symbolic element region index lifetime.

2015-10-13 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 37263. NoQ added a comment. Hmm, i think i'm ready to explain most of the stuff. - First of all, the piece of code in `EnvironmentManager::removeDeadBindings()` i mentioned above is truly useless; everything would be marked as live anyway on the next line. -

Re: [PATCH] D12726: [analyzer] A fix for symbolic element region index lifetime.

2015-10-13 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 37266. NoQ added a comment. Whoops accidentally left out a dead code line in tests. http://reviews.llvm.org/D12726 Files: docs/analyzer/DebugChecks.rst include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h

Re: [PATCH] D12726: [analyzer] A fix for symbolic element region index lifetime.

2015-10-09 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 36946. NoQ marked 4 inline comments as done. NoQ added a comment. Thanks for the review! Fixed all comments. I don't notice any significant performance hit with this patch (+/- 2% on the whole Android codebase runs). I guess i won't break this into separate

Re: [PATCH] D12725: [analyzer] A fix for substraction of an integer from a pointer.

2015-09-10 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Thanks, fixed :) http://reviews.llvm.org/D12725 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12725: [analyzer] A fix for substraction of an integer from a pointer.

2015-09-10 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 34423. NoQ added a comment. Make the tests easier to understand. http://reviews.llvm.org/D12725 Files: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp test/Analysis/ptr-arith.c Index: test/Analysis/ptr-arith.c

Re: [PATCH] D12725: [analyzer] A fix for substraction of an integer from a pointer.

2015-09-09 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 34320. http://reviews.llvm.org/D12725 Files: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp test/Analysis/ptr-arith.c Index: test/Analysis/ptr-arith.c === --- test/Analysis/ptr-arith.c +++

[PATCH] D12726: [analyzer] A fix for symbolic element region index lifetime.

2015-09-09 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, krememek. NoQ added subscribers: cfe-commits, dergachev.a. In Clang Static Analyzer, when the symbol is referenced by an index value of an element region, it does not prevent garbage collection (reaping) of that symbol. Hence, if the

Re: [PATCH] D12726: [analyzer] A fix for symbolic element region index lifetime.

2015-09-18 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 35067. NoQ added a comment. Thanks for the quick reply, sorry for the delay! Was afk for a couple of days. Yeah, right, in fact i didn't even fix the issue for store keys at all; only for store values and environment values. It also seems much harder to test

Re: [PATCH] D12725: [analyzer] A fix for substraction of an integer from a pointer.

2015-09-18 Thread Artem Dergachev via cfe-commits
NoQ added a comment. I've got no commit access yet, sorry, that's my first patch here actually :) http://reviews.llvm.org/D12725 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12726: [analyzer] A fix for symbolic element region index lifetime.

2015-12-07 Thread Artem Dergachev via cfe-commits
NoQ added a comment. In http://reviews.llvm.org/D12726#303122, @zaks.anna wrote: > > So the real question is whether (or rather how) the Store should maintain > > correct region liveness information > > > after completing its internal garbage collection pass, because there are, > > in fact,

[PATCH] D15448: [analyzer] SVal Visitor.

2015-12-11 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin, xazax.hun. NoQ added a subscriber: cfe-commits. It seems that in several places in the code Clang Static Analyzer tries to recursively traverse the `SVal` hierarchy, so i made a visitor for `SVal`, `SymExpr`, and `MemRegion`

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

2015-12-17 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Hmm, just noticed the related work on casts in http://reviews.llvm.org/D12901, which seems to be directly related to my hand-waving above. It might accidentally be useful for reducing FPs of this checker as well. http://reviews.llvm.org/D13126

Re: [PATCH] D15448: [analyzer] SVal Visitor.

2015-12-16 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Good point, will try to make a .def file. There's a tiny inconsistency with `SVal` naming that would most likely need to be fixed in this approach: nonloc::SymbolVal => SymbolValKind loc::MemRegionVal => MemRegionKind // no "Val"! Hmm, maybe make a .def file for

r255236 - [analyzer] Fix symbolic element index lifetime.

2015-12-10 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Thu Dec 10 03:28:06 2015 New Revision: 255236 URL: http://llvm.org/viewvc/llvm-project?rev=255236=rev Log: [analyzer] Fix symbolic element index lifetime. SymbolReaper was destroying the symbol too early when it was referenced only from an index SVal of a live

Re: [PATCH] D15448: [analyzer] SVal Visitor.

2016-01-06 Thread Artem Dergachev via cfe-commits
NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/Regions.def:31 @@ +30,3 @@ +// is both instantiated and derived from. +// Additionally, its kind is not its name with "Kind" suffix, +// unlike all other regions. zaks.anna

Re: [PATCH] D15448: [analyzer] SVal Visitor.

2015-12-28 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 43683. NoQ marked an inline comment as done. NoQ added a comment. An attempt on the .def-files. The next step would probably be the `VisitChildren()` thing, and I'll see if it allows to refactor and simplify some code. __ Forgot to answer: I guess there

Re: [PATCH] D15007: [analyzer] Improve modelling of nullptr_t in the analyzer. Fix PR25414.

2015-11-26 Thread Artem Dergachev via cfe-commits
NoQ added a subscriber: NoQ. NoQ added a comment. Wow, useful stuff! There's a little problem with the `shouldNotCrash()` test: the first warning on `invokeF()` on line 107 generates a sink, and the rest of the function never gets executed. It's probably a good idea to split it into three

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

2015-11-25 Thread Artem Dergachev via cfe-commits
NoQ added a subscriber: NoQ. NoQ added a comment. In http://reviews.llvm.org/D13126#291763, @danielmarjamaki wrote: > I have problems with the "default" handling of expressions. > > I want to warn about loss of precision for such code: > > unsigned int x = 256; > unsigned char c; > c = x;

Re: [Static Analyzer] New checker hook: checkInitialState

2015-11-30 Thread Artem Dergachev via cfe-commits
Hmm. I once thought about creating a 'checkBeginAnalysis()' callback to match 'checkEndAnalysis()'; this one's more powerful, and matches 'checkEndFunction()' in a similar manner. At a glance, I wonder if it's worth it to provide a CheckerContext inside this callback and then handle

Re: [PATCH] D15090: [Static Analyzer] New checker hook: checkInitialState

2015-12-01 Thread Artem Dergachev via cfe-commits
NoQ added a subscriber: NoQ. NoQ added a comment. Yeah, that's what i had in mind. Additionally, `Decl` can be obtained as `Context.getStackFrame().getDecl()` (and in fact the `getStackFrame()` thing itself is of interest as well), so there's no need to pass it as an extra argument. On the

Re: [PATCH] D15448: [analyzer] SVal Visitor.

2016-01-13 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 44734. NoQ added a comment. Rebase on top of http://reviews.llvm.org/D12901 - support `SymbolCast` in the explainer, as it finally appears in the wild. http://reviews.llvm.org/D15448 Files: docs/analyzer/DebugChecks.rst

Re: [PATCH] D16062: [analyzer] Rename kind-enumeration values of SVal, SymExpr, MemRegion classes, for consistency.

2016-01-12 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 44628. NoQ marked 2 inline comments as done. NoQ added a comment. Good point! Removed the friend-class directive here, and also in `NonStaticGlobalSpaceRegion`, which is also abstract. Agreed and renamed text regions to code regions.

Re: [PATCH] D20863: [analyzer] Fix for the strdup family in unix.malloc checker

2016-06-06 Thread Artem Dergachev via cfe-commits
NoQ added a comment. This patch is doing a very good thing - modeling contents of the copied string as a lazy compound value of the original string. For that, i guess it's worth adding some tests (ExprInspection-based(?)) that show that, say, first few characters of the copied string are same

Re: [PATCH] D20811: [analyzer] Model some library functions

2016-06-02 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Yeah, good point, the "Std" part should definitely appear in the name, not sure about the "C" thing, as we could probably expand this checker to model some simple C++ functions as well (and then we'd make a separate checker section to move from unix. to cplusplus. or

[PATCH] D20811: [analyzer] Model some library functions

2016-05-31 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin. NoQ added a subscriber: cfe-commits. Herald added a subscriber: aemerson. I've put together a simple checker that throws no warnings, but models some library functions, which has already helped us to suppress some false

Re: [PATCH] D21506: [analyzer] Block in critical section

2016-06-20 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Useful stuff! When I see your approach with `mutexCount`, the following test case comes to mind: std::mutex m; void foo() { // Suppose this function is always called // with the mutex 'm' locked. m.unlock(); // Here probably some useful work is done.

[PATCH] D16062: [analyzer] Rename kind-enumeration values of SVal, SymExpr, MemRegion classes, for consistency.

2016-01-11 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin. NoQ added a subscriber: cfe-commits. Based on discussion in D15448. - For every sub-class `C`, its kind in the relevant enumeration is `CKind` (or `C##Kind` in preprocessor-ish terms), eg: `MemRegionKind` ->

Re: [PATCH] D15448: [analyzer] SVal Visitor.

2016-01-11 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 44475. NoQ marked 5 inline comments as done. NoQ added a comment. Renamed the kinds for consistency (review http://reviews.llvm.org/D16062), this diff is updated to use the new naming convention. The 'kind' column gets removed from the def-files.

r257893 - [analyzer] Provide .def-files and visitors for SVal/SymExpr/MemRegion, v2.

2016-01-15 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Fri Jan 15 09:22:05 2016 New Revision: 257893 URL: http://llvm.org/viewvc/llvm-project?rev=257893=rev Log: [analyzer] Provide .def-files and visitors for SVal/SymExpr/MemRegion, v2. Provide separate visitor templates for the three hierarchies, and also the

Re: [PATCH] D15448: [analyzer] SVal Visitor.

2016-01-15 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Managed to reproduce the build error with `-fmodules` on my machine. Committed the updated patch as r257893, the buildbot seems happy. I hope this review is actually closed now :) http://reviews.llvm.org/D15448 ___ cfe-commits

[PATCH] D16178: [analyzer] A quick fix on top of D12901/r257464

2016-01-14 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added reviewers: pgousseau, zaks.anna, dcoughlin, xazax.hun. NoQ added a subscriber: cfe-commits. Sorry for being a bit slow, i should have had a look at the review earlier; i noticed some stuff after the recent patch by Pierre Gousseau was committed. 1. There's

r258039 - [analyzer] Fix an off-by-one in evalIntegralCast()

2016-01-18 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Mon Jan 18 04:17:16 2016 New Revision: 258039 URL: http://llvm.org/viewvc/llvm-project?rev=258039=rev Log: [analyzer] Fix an off-by-one in evalIntegralCast() Make sure that we do not add SymbolCast at the very boundary of the range in which the cast would not certainly

r259345 - [analyzer] Use a wider integer type for an array index.

2016-02-01 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Mon Feb 1 03:29:17 2016 New Revision: 259345 URL: http://llvm.org/viewvc/llvm-project?rev=259345=rev Log: [analyzer] Use a wider integer type for an array index. Avoids unexpected overflows while performing pointer arithmetics in 64-bit code. Moreover, neither

Re: [PATCH] D16317: [Analyzer] Fix for PR23790: bind real value returned from strcmp when modelling strcmp.

2016-01-19 Thread Artem Dergachev via cfe-commits
NoQ added a subscriber: NoQ. NoQ added a comment. Hmm. If we want to catch bugs resulting from alternative `strcmp()` implementations, then probably a test case that demonstrates the improvement would be worth it, eg.: int x = strcmp("foo", "bar")); if (x == 1 || x == -1)

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

2016-03-23 Thread Artem Dergachev via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:84 @@ +83,3 @@ +// Can E value be greater or equal than Val? +static bool canBeGreaterEqual(CheckerContext , const Expr *E, + unsigned long long Val) {

[PATCH] D19057: [analyzer] Let TK_PreserveContents span across the whole base region.

2016-04-13 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin. NoQ added a subscriber: cfe-commits. Essentially, if `s` is a structure, and `foo(const void *)` is evaluated conservatively, then `foo()` does not invalidate `s`, but `foo(&(s.x))` invalidates the whole `s`, because the

r266292 - [ASTImporter] Implement some expression-related AST node import.

2016-04-14 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Thu Apr 14 06:51:27 2016 New Revision: 266292 URL: http://llvm.org/viewvc/llvm-project?rev=266292=rev Log: [ASTImporter] Implement some expression-related AST node import. Introduce ASTImporter unit test framework. Fix a memory leak introduced in cf8ccff5: an array is

Re: r276782 - [analyzer] Add basic capabilities to detect source code clones.

2016-07-26 Thread Artem Dergachev via cfe-commits
Dergachev via cfe-commits <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote: Author: dergachev Date: Tue Jul 26 13:13:12 2016 New Revision: 276782 URL: http://llvm.org/viewvc/llvm-project?rev=276782=rev Log: [analyzer] Add basic capabiliti

Re: [PATCH] D20795: Added basic capabilities to detect source code clones.

2016-07-26 Thread Artem Dergachev via cfe-commits
NoQ accepted this revision. NoQ added a reviewer: NoQ. NoQ added a comment. Great! Will commit. https://reviews.llvm.org/D20795 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r276791 - [analyzer] Hotfix for build failure due to declaration shadowing in r276782.

2016-07-26 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Tue Jul 26 14:05:22 2016 New Revision: 276791 URL: http://llvm.org/viewvc/llvm-project?rev=276791=rev Log: [analyzer] Hotfix for build failure due to declaration shadowing in r276782. CloneDetector member variable is shadowing the class with the same name, which causes

r276782 - [analyzer] Add basic capabilities to detect source code clones.

2016-07-26 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Tue Jul 26 13:13:12 2016 New Revision: 276782 URL: http://llvm.org/viewvc/llvm-project?rev=276782=rev Log: [analyzer] Add basic capabilities to detect source code clones. This patch adds the CloneDetector class which allows searching source code for clones. For every

Re: [PATCH] D22419: [CFG] Fix crash in thread sanitizer.

2016-07-26 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Thanks for working on this! While i probably won't be of much help (no expert in temporaries yet), i notice that this code piece (with `getReferenceInitTemporaryType()`) seems to be duplicated at least three times in this file, with slight variations and FIXMEs. So i

Re: [PATCH] D20795: Added basic capabilities to detect source code clones.

2016-07-25 Thread Artem Dergachev via cfe-commits
NoQ added a comment. The idea with strings as keys is curious! I've got a few more minor comments :) Comment at: lib/Analysis/CloneDetection.cpp:104 @@ +103,3 @@ +// Storage for the signatures of the direct child statements. This is only +// needed if the current

Re: [PATCH] D20811: [analyzer] Model some library functions

2016-07-25 Thread Artem Dergachev via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:192 @@ +191,3 @@ + }; + + // The map of all functions supported by the checker. It is initialized Even though there are some doxygen-style comments in the

Re: [PATCH] D22856: [analyzer] Change -analyze-function to accept qualified names.

2016-07-27 Thread Artem Dergachev via cfe-commits
NoQ added a comment. > I think it would be better for fully-qualified Objective-C methods to be > specified with their Objective-C-style names. For example: "-[Test1 > myMethodWithY:withX:]". Uhm, need to know more about those. So i just print "`-[`", then fully-qualified class name, then

Re: [PATCH] D22862: [analyzer] Fix for PR15623: eliminate unwanted ProgramState checker data propagation.

2016-07-28 Thread Artem Dergachev via cfe-commits
NoQ added inline comments. Comment at: test/Analysis/misc-ps-region-store.m:332 @@ -330,3 +331,3 @@ if (p < q) { // If we reach here, 'p' cannot be null. If 'p' is null, then 'n' must // be '0', meaning that this branch is not feasible. ayartsev

[PATCH] D22874: [analyzer] Fixes to the checker developer manual.

2016-07-27 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added a reviewer: zaks.anna. NoQ added subscribers: dcoughlin, xazax.hun, a.sidorin, cfe-commits. 1. Fix the explanation of how to run tests after migration from autotools to cmake. 2. Expand the "debugging" section with more interesting stuff, as accidentally

Re: [PATCH] D20811: [analyzer] Model some library functions

2016-07-28 Thread Artem Dergachev via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:509 @@ +508,3 @@ + //} + // } + //} dcoughlin wrote: > I disagree about compactness being valuable here. I think it is more > important to intrinsically

r276557 - [analyzer] Pring LocationContext in ExplodedGraph dumps.

2016-07-24 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Sun Jul 24 03:15:58 2016 New Revision: 276557 URL: http://llvm.org/viewvc/llvm-project?rev=276557=rev Log: [analyzer] Pring LocationContext in ExplodedGraph dumps. Remove some FIXMEs in the surrounding code, which have been addressed long time ago by introducing

Re: [PATCH] D20795: Added basic capabilities to detect source code clones.

2016-07-22 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Regarding the cache stack - it feels easier for me to allocate a separate stack for each statement, and put the stack on stack (!) rather than having it global. This way it'd be automatically cleaned for you when VisitStmt() exits, and you'd be able to address child cache

r277029 - [analyzer] Update the web manual for checker developers.

2016-07-28 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Thu Jul 28 15:13:14 2016 New Revision: 277029 URL: http://llvm.org/viewvc/llvm-project?rev=277029=rev Log: [analyzer] Update the web manual for checker developers. Fix the explanation of how to run tests after migration from autotools to cmake. Significantly expand the

[PATCH] D22969: [analyzer] Fix execution permissions for scan-build-py.

2016-07-29 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin, rizsotto.mailinglist. NoQ added a subscriber: cfe-commits. It seems that the executable scripts are not marked as executable in the repository, so i had to `chmod +x` them before using. Because the old scan-build is `+x`, i

Re: [PATCH] D20811: [analyzer] Model some library functions

2016-07-30 Thread Artem Dergachev via cfe-commits
NoQ added a comment. > Is it really a problem if the checker comments are part of the Doxygen > documentation? Of course not :) I've been mostly thinking about the benefits of the anonymous namespace itself (cleaner global scope, no name collisions, but even these benefits are extremely

r277338 - [analyzer] Fix execution permissions for the scan-build-py scripts.

2016-08-01 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Mon Aug 1 05:55:59 2016 New Revision: 277338 URL: http://llvm.org/viewvc/llvm-project?rev=277338=rev Log: [analyzer] Fix execution permissions for the scan-build-py scripts. Differential Revision: https://reviews.llvm.org/D22969 Modified:

Re: [PATCH] D22514: CloneDetection now respects statement specific data when searching for clones.

2016-08-01 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Congrats on the fantastic hack that nobody else noticed! I've a feeling we cannot add tests for that, because any test would quickly break if we change the hash, though demonstrating that we fixed the problem would still be cool (eg. take two unsupported expressions of

Re: [PATCH] D22982: [CloneDetector] No longer reporting clones that don't have a common referenced variable pattern.

2016-08-01 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Looks good, minor comments :) Comment at: lib/Analysis/CloneDetection.cpp:99 @@ +98,3 @@ + /// Every item in this list is unique. + std::vector Variables; + I've a feeling this is essentially a `map`, because that harmonizes with our

Re: [PATCH] D22982: [CloneDetector] No longer reporting clones that don't have a common referenced variable pattern.

2016-08-01 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Uhm. I've a feeling that now i realize how it should have been written from the start. That's a pretty bad feeling to have. I wish we had a //series of passes//. Before the first pass, all statements are considered equivalent. After the first pass, they're split into

Re: [PATCH] D20795: Added basic capabilities to detect source code clones.

2016-07-26 Thread Artem Dergachev via cfe-commits
NoQ added inline comments. Comment at: lib/Analysis/CloneDetection.cpp:148 @@ +147,3 @@ + +// FIXME: This function has quadratic runtime right now. Check if skipping +// this function for too long CompoundStmts is an option. I've a feeling this comment

Re: [PATCH] D20811: [analyzer] Model some library functions

2016-07-25 Thread Artem Dergachev via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:191 @@ +190,3 @@ +// impossible to verify this precisely, but at least +// this check avoids potential crashes. +bool matchesCall(const CallExpr *CE) const;

Re: [PATCH] D20811: [analyzer] Model some library functions

2016-07-25 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 65369. NoQ marked 4 inline comments as done. https://reviews.llvm.org/D20811 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/CMakeLists.txt lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

[PATCH] D22856: [analyzer] Change -analyze-function to accept qualified names.

2016-07-27 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added a reviewer: zaks.anna. NoQ added subscribers: dcoughlin, xazax.hun, a.sidorin, cfe-commits. The `-analyze-function` option is very useful when debugging test failures, especially when printing or viewing exploded graphs - with this option you no longer need

Re: [PATCH] D20811: [analyzer] Model some library functions

2016-07-27 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Answering myself: Hmm, so the only reason why MPI checker class appears in doxygen (http://clang.llvm.org/doxygen/classclang_1_1ento_1_1mpi_1_1MPIChecker.html) is because this class is not in anonymous namespace (as far as i understand, they needed to be multi-file for

Re: [PATCH] D22374: [analyzer] Copy and move constructors - ExprEngine extended for "almost trivial" copy and move constructors

2016-07-27 Thread Artem Dergachev via cfe-commits
NoQ added a comment. I see what's going on. `performTrivialCopy()` already calls `evalBind()`, which in turn calls `runCheckersForBind()`, so no effort is needed there. However, the bind event itself is now different - instead of a separate event for every bind, you're having only one event

r278238 - [analyzer] Fix a crash in CloneDetector when calling functions by pointers.

2016-08-10 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Wed Aug 10 11:25:16 2016 New Revision: 278238 URL: http://llvm.org/viewvc/llvm-project?rev=278238=rev Log: [analyzer] Fix a crash in CloneDetector when calling functions by pointers. CallExpr may have a null direct callee when the callee function is not known in

Re: Modify MallocChecker.cpp to recognize kfree( )

2016-08-10 Thread Artem Dergachev via cfe-commits
Hey, that's a useful patch, i'd like it to be included! There is odd indentation in some places, but in general it looks correct. The usual thing to do nowadays is to put patches on review on Phabricator, would you like to do that? (http://llvm.org/docs/Phabricator.html,

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

2016-08-10 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Wow, i've completely forgot about this one. Sorry about that... I think this checker is good to have in the current shape, and probably incrementally developed. It'd probably move out of alpha whenever it's tweaked to somehow make sure it finds enough real bugs among

r277449 - [analyzer] Respect statement-specific data in CloneDetection.

2016-08-02 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Tue Aug 2 07:21:09 2016 New Revision: 277449 URL: http://llvm.org/viewvc/llvm-project?rev=277449=rev Log: [analyzer] Respect statement-specific data in CloneDetection. So far the CloneDetector only respected the kind of each statement when searching for clones. This

Re: r277449 - [analyzer] Respect statement-specific data in CloneDetection.

2016-08-02 Thread Artem Dergachev via cfe-commits
Wow, i haven't noticed it's mine! Will have a look, sorry. On 8/2/16 5:51 PM, Renato Golin via cfe-commits wrote: On 2 August 2016 at 13:21, Artem Dergachev via cfe-commits <cfe-commits@lists.llvm.org> wrote: Author: dergachev Date: Tue Aug 2 07:21:09 2016 New Revision: 277449 URL

Re: [PATCH] D23014: [analyzer] Model base to derived casts more precisely.

2016-08-02 Thread Artem Dergachev via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:423 @@ +422,3 @@ + +if (!val.isZeroConstant()) { + val = getStoreManager().evalDynamicCast(val, T, Failed); I guess if `val` is a //non-zero// constant, it wouldn't

Re: [PATCH] D23014: [analyzer] Model base to derived casts more precisely.

2016-08-02 Thread Artem Dergachev via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:423 @@ +422,3 @@ + +if (!val.isZeroConstant()) { + val = getStoreManager().evalDynamicCast(val, T, Failed); xazax.hun wrote: > NoQ wrote: > > I guess if `val` is a

Re: [PATCH] D22856: [analyzer] Change -analyze-function to accept qualified names.

2016-08-01 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 66360. NoQ added a comment. Copy-paste the code from CGDebugInfo.cpp to support Objective-C instance and class methods. Add more tests. Move the `analyze_display_progress.cpp` because most tests use dashes in names. https://reviews.llvm.org/D22856 Files:

r277473 - [analyzer] Hotfix for buildbot failure due to unspecified triple in r277449

2016-08-02 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Tue Aug 2 10:16:06 2016 New Revision: 277473 URL: http://llvm.org/viewvc/llvm-project?rev=277473=rev Log: [analyzer] Hotfix for buildbot failure due to unspecified triple in r277449 If a target triple is not specified, the default host triple is used, which is not good

Re: [PATCH] D23316: [analyzer] Fixed the false-positives caused by macro generated code.

2016-08-11 Thread Artem Dergachev via cfe-commits
NoQ added inline comments. Comment at: lib/Analysis/CloneDetection.cpp:436 @@ +435,3 @@ +if (IsInMacro) { + Signature.Complexity = 0; +} omtcyfz wrote: > Do I understand correctly that a code generated by a macro doesn't affect > "complexity" at all

Re: [PATCH] D15227: [analyzer] Valist checkers.

2016-08-11 Thread Artem Dergachev via cfe-commits
NoQ added a comment. The checker's in great shape! I see a few minor things, but that's it. The checks are split into two sections ("uninitialized" and "unterminated"), but there seem to be more auxiliary checks provided (eg. "copies into itself") that are on for both checkers, do you think

Re: [PATCH] D23316: [analyzer] Fixed the false-positives caused by macro generated code.

2016-08-11 Thread Artem Dergachev via cfe-commits
NoQ added a comment. No comments of my own, the patch looks good :) Comment at: lib/Analysis/CloneDetection.cpp:436 @@ +435,3 @@ +if (IsInMacro) { + Signature.Complexity = 0; +} omtcyfz wrote: > omtcyfz wrote: > > omtcyfz wrote: > > > NoQ wrote: >

Re: [PATCH] D23112: [analyzer] Correctly add assumptions based on array bounds.

2016-08-03 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Nice catch! Now, this needs a test. How about this one: // enable the debug.ExprInspection checker? void clang_analyzer_eval(int); void test_asume_after_access(unsigned long x) { char buf[100]; buf[x] = 1; clang_analyzer_eval(x <= 99); //

Re: [PATCH] D23060: [analyzer] Show enabled checker list

2016-08-03 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Like! Do you think that if we rename the ~~bike shed~~ option to `-analyzer-list-enabled-checkers` it'd be more natural and easy to remember? (i barely remember how to spell `-analyzer-checker-help`, it'd be much easier if it was called `-analyzer-list-checkers`).

Re: [PATCH] D22982: [CloneDetector] No longer reporting clones that don't have a common referenced variable pattern.

2016-08-03 Thread Artem Dergachev via cfe-commits
NoQ accepted this revision. This revision is now accepted and ready to land. Comment at: lib/Analysis/CloneDetection.cpp:99 @@ +98,3 @@ + /// Every item in this list is unique. + std::vector Variables; + NoQ wrote: > I've a feeling this is essentially a `map`,

Re: [PATCH] D22374: [analyzer] Copy and move constructors - ExprEngine extended for "almost trivial" copy and move constructors

2016-08-03 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Hmm. I suggest: 1. Change this test's constructor so that it was no longer almost-trivial. Because it isn't significant for this test if the constructor is almost-trivial or not. The test would pass. 2. Add a FIXME-test for this checker, in which a completely undefined

Re: [PATCH] D22862: [analyzer] Fix for PR15623: eliminate unwanted ProgramState checker data propagation.

2016-08-15 Thread Artem Dergachev via cfe-commits
NoQ added a comment. > @dcoughlin, @NoQ, could you, please, tell, how you get dumps of symbolic > expressions and constraints like "(conj_$6{void *}) != 0U"? Tried different > debug.* checkers and clang_analyzer_explain() but failed. That's `SVal.dump()`, `SymbolRef->dump()`,

Re: [PATCH] D23112: [analyzer] Correctly add assumptions based on array bounds.

2016-08-10 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Whoops, forgot to answer: In https://reviews.llvm.org/D23112#508333, @xazax.hun wrote: > I am not sure that the checker is the appropriate way to fix the remaining > issue with this checker. Yeah, there are anyway more problems that require this functionality in the

r275290 - [analyzer] Implement a methond to discover origin region of a symbol.

2016-07-13 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Wed Jul 13 13:07:26 2016 New Revision: 275290 URL: http://llvm.org/viewvc/llvm-project?rev=275290=rev Log: [analyzer] Implement a methond to discover origin region of a symbol. This encourages checkers to make logical decisions depending on value of which region was the

[PATCH] D22622: [analyzer] Add more info to exploded graph dumps

2016-07-21 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin. NoQ added subscribers: xazax.hun, a.sidorin, cfe-commits. `debug.ViewExplodedGraph`, aka `-analyzer-viz-egraph-graphviz`, is often the only way to understand the otherwise confusing analyzer report. Exploded graphs are often

Re: [PATCH] D22622: [analyzer] Add more info to exploded graph dumps

2016-07-21 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Whoops forgot the screenshots: F2187578: 2.png F2187579: 1.png Also not sure how to write tests for these, because we can't choose the directory path for the dump, can we?

[PATCH] D22242: [analyzer] De-duplicate some code for discovering the origin of the symbol.

2016-07-11 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added a reviewer: dcoughlin. NoQ added a subscriber: cfe-commits. Or, more importantly, tell the users that there's a way to discover the region, value of which - at some moment of time - the symbol was introduced to represent. Idea proposed by Devin Coughlin.

Re: [PATCH] D22242: [analyzer] De-duplicate some code for discovering the origin of the symbol.

2016-07-12 Thread Artem Dergachev via cfe-commits
NoQ updated this revision to Diff 63688. NoQ marked an inline comment as done. NoQ added a comment. Added a doxygen comment, made the function virtual instead of branching. http://reviews.llvm.org/D22242 Files: include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h

Re: [PATCH] D22048: [analyzer] Suppress false positives in std::shared_ptr

2016-07-06 Thread Artem Dergachev via cfe-commits
NoQ added inline comments. Comment at: test/Analysis/Inputs/system-header-simulator-cxx.h:349 @@ +348,3 @@ +// No warning is expected as we are suppressing warning coming +// out of std::basic_string. +int z = 0; You mean std::shared_ptr here? Also,

Re: [PATCH] D22048: [analyzer] Suppress false positives in std::shared_ptr

2016-07-06 Thread Artem Dergachev via cfe-commits
NoQ accepted this revision. NoQ added a comment. LGTM, all clear to me now :) http://reviews.llvm.org/D22048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21978: [analyzer] Add LocationContext to SymbolMetadata

2016-07-06 Thread Artem Dergachev via cfe-commits
NoQ added a comment. I also never noticed that the block count gets reset on every stack frame, that's pretty unobvious. Uhm, and it's already there in SymbolConjured. Anyway, fixing the block count itself seems to be pretty complicated. Also, maybe once the ScopeContext thing is ready, we'd

Re: [PATCH] D20795: Added basic capabilities to detect source code clones.

2016-07-11 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Your code is well-written and easy to understand, and makes me want to use it on my code! Added some minor comments, and there seems to be a small logic error in the compound statement hasher. Not sure if that was already explained in detail, but i seem to agree that the

r277757 - [analyzer] Make CloneDetector recognize different variable patterns.

2016-08-04 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Thu Aug 4 14:37:00 2016 New Revision: 277757 URL: http://llvm.org/viewvc/llvm-project?rev=277757=rev Log: [analyzer] Make CloneDetector recognize different variable patterns. CloneDetector should be able to detect clones with renamed variables. However, if variables are

Re: [PATCH] D22982: [CloneDetector] No longer reporting clones that don't have a common referenced variable pattern.

2016-08-04 Thread Artem Dergachev via cfe-commits
NoQ requested changes to this revision. NoQ added a comment. This revision now requires changes to proceed. Whoops, because of a merge conflict while applying the patch, i noticed a few problems in the tests, they seem obvious, but could you have a quick look? Comment at:

Re: [PATCH] D22374: [analyzer] Copy and move constructors - ExprEngine extended for "almost trivial" copy and move constructors

2016-08-04 Thread Artem Dergachev via cfe-commits
NoQ added a comment. In https://reviews.llvm.org/D22374#505672, @baloghadamsoftware wrote: > In https://reviews.llvm.org/D22374#504855, @NoQ wrote: > > > Hmm. I suggest: > > > > 1. Change this test's constructor so that it was no longer almost-trivial. > > Because it isn't significant for this

Re: [PATCH] D22862: [analyzer] Fix for PR15623: eliminate unwanted ProgramState checker data propagation.

2016-08-08 Thread Artem Dergachev via cfe-commits
NoQ added a comment. Hmm. The test in `unwanted-programstate-data-propagation.c` passes on my machine even without the patch, and the return value on the respective path is correctly represented as `(conj_$6{void *}) != 0U`, which comes from the `evalCast()` call in `VisitLogicalExpr()` and is

[PATCH] D23272: [analyzer][Rewrite] Fix boxes and shadows in HTML rewrites in Firefox.

2016-08-08 Thread Artem Dergachev via cfe-commits
NoQ created this revision. NoQ added reviewers: dcoughlin, zaks.anna. NoQ added subscribers: xazax.hun, a.sidorin, cfe-commits. Use the official CSS3 properties `border-radius` and `box-shadow` (not only `-webkit-`specific properties). Fixes analyzer's diagnostic pieces in HTML diagnostics mode

r278018 - [analyzer] Change -analyze-function to accept qualified names.

2016-08-08 Thread Artem Dergachev via cfe-commits
Author: dergachev Date: Mon Aug 8 11:01:02 2016 New Revision: 278018 URL: http://llvm.org/viewvc/llvm-project?rev=278018=rev Log: [analyzer] Change -analyze-function to accept qualified names. Both -analyze-function and -analyzer-display-progress now share the same convention for naming

Re: [PATCH] D23112: [analyzer] Correctly add assumptions based on array bounds.

2016-08-08 Thread Artem Dergachev via cfe-commits
NoQ accepted this revision. This revision is now accepted and ready to land. Comment at: test/Analysis/out-of-bounds.c:153 @@ +152,3 @@ +// The result is unknown for the same reason as above. +void test_asume_after_access(unsigned long x) { + int buf[100]; Yay,

  1   2   3   4   5   6   >