[PATCH] D28445: [Analyzer] Extend taint propagation and checking

2017-02-01 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a subscriber: a.sidorin. NoQ added a comment. Hello! Sorry, i'm very distracted recently. I think your new approach should work, however it would be much easier and more straightforward to just ask the store manager to provide the default-bound symbol for a region directly, instead

[PATCH] D28955: [analyzer] Enable support for symbolic extension/truncation

2017-01-21 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. In https://reviews.llvm.org/D28955#652443, @ddcc wrote: > When I was testing this patch, it was on top of both > https://reviews.llvm.org/D28952 and https://reviews.llvm.org/D28953. For > `malloc.c`, the change on line 1708 from `int` to `size_t` is necessary to > prevent

[PATCH] D28952: [analyzer] Add new Z3 constraint manager backend

2017-01-21 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Late-joining the round of applause for the awesome progression of this project! Not sure how to deal with the massive test run-line changes at the moment, will take some extra time to think. In https://reviews.llvm.org/D28952#652436, @ddcc wrote: > Another issue is that

[PATCH] D28946: [analyzer] Fix memory space for block-captured static locals.

2017-01-24 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 85588. NoQ added a comment. Reopening due to a revert. This time i reduce the scope of the fix to the checker, and add FIXMEs to the problems that showed up while testing it. https://reviews.llvm.org/D28946 Files:

[PATCH] D28905: [analyzer] Consider function call arguments while building CallGraph

2017-01-26 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D28905 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29183: [Analysis] Fix for call graph to correctly handle nested call expressions

2017-01-26 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Hmm, we should have done better work reviewing https://reviews.llvm.org/D28905. https://reviews.llvm.org/D29183 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30157: [analyzer] Improve valist check

2017-02-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:178 +VaListModelledAsArray = Cast->getCastKind() == CK_ArrayToPointerDecay; + const MemRegion *Reg = SV.getAsRegion(); + if (const auto *DeclReg = Reg->getAs()) { I

[PATCH] D28445: [Analyzer] Extend taint propagation and checking

2017-02-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Yeah, i think this is now as easy as i expected it to be :) Still, the new API is in need of better documentation, because the notion of default binding is already confusing, and the new use-case we now have for this API is even more confusing. I don't instantly see a good

[PATCH] D30289: [Analyzer] Add bug visitor for taint checker

2017-02-23 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Yay, this is awesome! It's actually possible to test visitors with the `-analyzer-output=text` option. This option converts path notes to `note:` diagnostics, which you can catch with `expected-note{{}}`, see `test/Analysis/inlining/path-notes.c` for an example (well,

[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker

2017-02-23 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530 + auto value = RVal; + if (auto loc = value.getAs()) { +value = State->getRawSVal(*loc); baloghadamsoftware wrote: > NoQ wrote: > > baloghadamsoftware wrote:

[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker

2017-02-23 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530 + auto value = RVal; + if (auto loc = value.getAs()) { +value = State->getRawSVal(*loc); NoQ wrote: > baloghadamsoftware wrote: > > NoQ wrote: > > >

[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker

2017-02-09 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530 + auto value = RVal; + if (auto loc = value.getAs()) { +value = State->getRawSVal(*loc); baloghadamsoftware wrote: > NoQ wrote: > > Is there a test case for

[PATCH] D29419: [Analyzer] Checker for mismatched iterators

2017-02-09 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Hello, thanks for another useful checker! I make quite a few of these mistakes regularly. This one looks similar to the `IteratorPastEnd` checker, so much that i'd definitely advice re-using some code. At the very least, functions like `isIterator()` should definitely

[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker

2017-02-09 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530 + auto value = RVal; + if (auto loc = value.getAs()) { +value = State->getRawSVal(*loc); baloghadamsoftware wrote: > NoQ wrote: > > baloghadamsoftware wrote:

[PATCH] D29884: [analyzer] Proper caching in CallDescription objects.

2017-02-15 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:73 + : II(nullptr), IsLookupDone(false), FuncName(FuncName), +RequiredArgs(RequiredArgs) {} Maybe `assert(FuncName.size() > 0)` here?

[PATCH] D29884: [analyzer] Proper caching in CallDescription objects.

2017-02-15 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:73 + : II(nullptr), IsLookupDone(false), FuncName(FuncName), +RequiredArgs(RequiredArgs) {} xazax.hun wrote: > NoQ wrote: > > Maybe

[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker

2017-02-15 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. I sometimes wish ASTContext methods just didn't crash :) Oh well, let's just see if more problems show up. Comment at: lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp:93

[PATCH] D29884: [analyzer] Proper caching in CallDescription objects.

2017-02-15 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yep, seems that somebody has missed these issues :) I guess there's no way to test the operator case, because nobody made a CallDescription with an empty name for us (maybe we should even assert

[PATCH] D27202: [analyzer] Do not conjure a symbol for return value of a conservatively evaluated function

2017-01-18 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ planned changes to this revision. NoQ added a comment. Yep, because there are a lot more places in which the value of the expression suddenly changes, i think i'd want to either fix all of them, or (if i find some of those really reasonable) make a callback for checkers to subscribe to

[PATCH] D27710: [analyzer] Prohibit ExplodedGraph's edges duplicating

2017-01-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. In https://reviews.llvm.org/D27710#650396, @ilya-palachev wrote: > I don't understand why the following test passes, because each of two > checkers: `MallocChecker` and `SimpleStreamChecker` are using > `generateNonFatalErrorNode` method: That's because error nodes

[PATCH] D28946: [analyzer] Fix memory space for block-captured static locals.

2017-01-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. When a block that is being analyzed as top-level captures static local variables declared in a function (which itself is not being analyzed) surrounding the block, it puts such variables into the unknown memory space, similarly to the stack locals of those functions

[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker

2017-01-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Thanks for the update, most welcome! Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:234 +if (Call.getNumArgs() >= 1) { + const auto = static_cast(Call); + handleRandomIncrOrDecr(C,

[PATCH] D30406: [Analyzer] Add support for displaying cross-file diagnostic paths in HTML output

2017-02-28 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added subscribers: a.sidorin, NoQ. NoQ added a comment. I think this is great. We've been hearing a lot of complaints on the mailing lists recently about that problem. Did you check that scan-build properly de-duplicates cross-file reports that originate from different clang runs but point

[PATCH] D30534: [analyzer] When creating a temporary object copy, properly copy the value into it.

2017-03-02 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. The test case provided demonstrates a regression due to my earlier attempt to fix temporaries in https://reviews.llvm.org/D26839. Neither before nor after, we did not properly copy the symbolic rvalue of the object to the newly created memory region; in different

[PATCH] D25475: [analyzer] Add a new SVal to support pointer-to-member operations.

2016-11-28 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. This looks good to me (bikeshedded a bit), but i think Devin should have another look, because his comments were way deeper than mine. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:217 + + llvm::ImmutableList

[PATCH] D27409: [analyzer] RetainCountChecker: The callback in dispatch_data_create() doesn't free the return symbol.

2016-12-07 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ retitled this revision from "[analyzer] RetainCountChecker: Improve support for libdispatch APIs." to "[analyzer] RetainCountChecker: The callback in dispatch_data_create() doesn't free the return symbol.". NoQ updated the summary for this revision. NoQ updated this revision to Diff 80606.

[PATCH] D27535: [analyzer] Add ObjCPropertyChecker - check for autosynthesized copy-properties of mutable types.

2016-12-08 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp:68 +return; + + BR.EmitBasicReport( dcoughlin wrote: > You'll also want to make sure to not warn on the following idiom, in which > programmers use @synthesize to

[PATCH] D27535: [analyzer] Add ObjCPropertyChecker - check for autosynthesized copy-properties of mutable types.

2016-12-08 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 80756. NoQ added a comment. Don't construct a `StringRef` to a temporary `std::string`. https://reviews.llvm.org/D27535 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/CMakeLists.txt

[PATCH] D27535: [analyzer] Add ObjCPropertyChecker - check for autosynthesized copy-properties of mutable types.

2016-12-08 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 80751. NoQ marked an inline comment as done. NoQ added a comment. - Address comments - Richer warning message (is it in good shape?) - Add tests for lack of implementation https://reviews.llvm.org/D27535 Files:

[PATCH] D27535: [analyzer] Add ObjCPropertyChecker - check for autosynthesized copy-properties of mutable types.

2016-12-08 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 80811. NoQ added a comment. Don't yell at read-only properties (no -copy is performed). Single-quote 'copy' in the error messages. https://reviews.llvm.org/D27535 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td

[PATCH] D27535: [analyzer] Add ObjCPropertyChecker - check for autosynthesized copy-properties of mutable types.

2016-12-07 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin. NoQ added a subscriber: cfe-commits. Herald added a subscriber: mgorny. When an ObjC property is declared as `(copy)`, and the type of the property is mutable (eg. `NSMutableSomething`), the autosynthesized code for the

[PATCH] D27599: [analyzer] Teach the analyzer that pointers can escape into __cxa_demangle

2016-12-09 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. LGTM! This could be probably modeled as realloc, and/or we could look for leaks of its return value, if we wanted. https://reviews.llvm.org/D27599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

2016-12-09 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. A quick example of how a bug reporter visitor for this checker may look like - it needs to be expanded significantly but here's a start. F2675703: report-11.html <== example of how it looks. See, for example, `MallocChecker` to

[PATCH] D27535: [analyzer] Add ObjCPropertyChecker - check for autosynthesized copy-properties of mutable types.

2016-12-12 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 81083. NoQ added a comment. - Fix crashes on checking properties in categories. - Address both comments. https://reviews.llvm.org/D27535 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/CMakeLists.txt

[PATCH] D27717: [analyzer] Suppress a leak false positive in Qt's QObject::connectImpl()

2016-12-13 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin, xazax.hun, a.sidorin. NoQ added a subscriber: cfe-commits. A quick fix to address the false positive posted by Tiago Macarios in the mailing lists: http://lists.llvm.org/pipermail/cfe-dev/2016-December/051738.html

[PATCH] D27409: [analyzer] RetainCountChecker: Improve support for libdispatch APIs.

2016-12-06 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ retitled this revision from "[analyzer] RetainCountChecker: The callback in dispatch_data_create() doesn't free the return symbol." to "[analyzer] RetainCountChecker: Improve support for libdispatch APIs.". NoQ updated the summary for this revision. NoQ updated this revision to Diff 80432.

[PATCH] D27408: [analyzer] RetainCountChecker: remove unused enum value; NFC.

2016-12-06 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 80431. NoQ added a comment. Remove OwnedAllocatedSymbol instead. https://reviews.llvm.org/D27408 Files: include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp Index:

[PATCH] D27854: [analyzer] Add check for mutex acquisition during interrupt context in Magenta kernel

2016-12-17 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. This checker looks good to me! I don't see any obvious problems, and i think we can land it into non-alpha (enabled by default) once reviewers' comments are addressed. Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:78 +def Magenta :

[PATCH] D28445: [Analyzer] Extend taint propagation and checking

2017-01-10 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Thanks for working on the taint! I really wish the taint analysis in the analyzer to flourish, and the part you've digged into is particularly sensitive, so i'd dump some thoughts here, hopefully helpful. **What works as it should: ** > In the second example, the SVal

[PATCH] D28602: [analyzer] Don't dereference the array bound to a reference.

2017-01-12 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin, xazax.hun, a.sidorin. NoQ added a subscriber: cfe-commits. An attempt to fix pr31592 (https://llvm.org/bugs/show_bug.cgi?id=31592), more sensible than what i did in the hot pre-4.0-release fix in r291754

[PATCH] D27918: [analyzer] OStreamChecker

2017-01-11 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Hello, Thanks for the patch, and for the impressing amount of work you put into this. Because the patch is huge, so it's going to take some time for somebody to understand how everything works. I'd probably like to learn a bit more about the motivation behind the checker.

[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker

2017-01-11 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a subscriber: zaks.anna. NoQ added a comment. Looks good. I assume the crash is in `getTypeInfo()`; do you have any idea what are exact prerequisites for using this method? So that there were no more crashes here. Repository: rL LLVM https://reviews.llvm.org/D28297

[PATCH] D27365: [analyzer] Print type for SymbolRegionValues when dumping to stream

2016-12-02 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ accepted this revision. NoQ added a comment. Thanks, i love dumps! Btw, did you encounter cases when types are unobvious from the parent region? Comment at: lib/StaticAnalyzer/Core/SymbolManager.cpp:89 + os << "reg_$" << getSymbolID() + << "<" <<

[PATCH] D26836: [analyzer] SValExplainer: Support ObjC ivars and __block variables.

2016-11-30 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ abandoned this revision. NoQ added a comment. Decided to go ahead and commit as r288260. https://reviews.llvm.org/D26836 ___ 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.

2016-11-29 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ accepted this revision. NoQ added a reviewer: NoQ. NoQ added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D25660#598576, @baloghadamsoftware wrote: > In https://reviews.llvm.org/D25660#590778, @NoQ wrote: > > > - Agree on the `evalAssume()`

[PATCH] D26839: [analyzer] An attempt to fix pr19539 - crashes on temporaries life-extended via members

2016-11-29 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 79540. NoQ marked an inline comment as done. NoQ added a comment. Update the comment. https://reviews.llvm.org/D26839 Files: lib/StaticAnalyzer/Core/ExprEngine.cpp test/Analysis/lifetime-extension.cpp Index: test/Analysis/lifetime-extension.cpp

[PATCH] D26837: [analyzer] Litter the SVal/SymExpr/MemRegion class hierarchy with asserts.

2016-11-29 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 79538. NoQ marked 3 inline comments as done. NoQ added a comment. Thanks for the comments! Addressed. https://reviews.llvm.org/D26837 Files: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h

[PATCH] D26837: [analyzer] Litter the SVal/SymExpr/MemRegion class hierarchy with asserts.

2016-11-29 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h:319 public: - SymbolVal(SymbolRef sym) : NonLoc(SymbolValKind, sym) {} + SymbolVal() = delete; + SymbolVal(SymbolRef sym) : NonLoc(SymbolValKind, sym) { assert(sym); }

[PATCH] D27202: [analyzer] Do not conjure a symbol for return value of a conservatively evaluated function

2016-11-29 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin, xazax.hun, a.sidorin. NoQ added subscribers: cfe-commits, baloghadamsoftware. Instead, return a `nonloc::LazyCompoundVal` of a temporary region for the call expression, with a trivial store in which the temporary region is

[PATCH] D26838: [analyzer] Enforce super-region classes for various memory regions through compile-time and run-time type checks.

2016-11-29 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1279 /// associated element type, index, and super region. const ElementRegion *getElementRegion(QualType elementType, NonLoc Idx, +

[PATCH] D26838: [analyzer] Enforce super-region classes for various memory regions through compile-time and run-time type checks.

2016-11-29 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 79537. NoQ marked 5 inline comments as done. NoQ added a comment. Thanks for the comments! Addressed. https://reviews.llvm.org/D26838 Files: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h include/clang/StaticAnalyzer/Core/PathSensitive/Store.h

[PATCH] D26588: Add LocationContext to members of check::RegionChanges

2016-11-30 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ commandeered this revision. NoQ added a reviewer: k-wisniewski. NoQ added a comment. Seems to become outdated with https://reviews.llvm.org/D27090. https://reviews.llvm.org/D26588 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D26589: Add static analyzer checker for finding infinite recursion

2016-11-30 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ commandeered this revision. NoQ added a reviewer: k-wisniewski. NoQ added a comment. Seems to become outdated with https://reviews.llvm.org/D27092. https://reviews.llvm.org/D26589 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D27091: Add the way to extract SVals of arguments used in a call for a given StackFrameCtx

2016-11-30 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Hmm, i'm thinking of just the opposite - refactor the CallEvent methods to use the respective new ProgramState methods. This way we'd avoid touching the CallEvent allocator for a simple Environment lookup, while still avoiding code duplication.

[PATCH] D26760: Add the way to extract SVals of arguments used in a call for a given StackFrameCtx

2016-11-30 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ commandeered this revision. NoQ edited reviewers, added: k-wisniewski; removed: NoQ. NoQ added a comment. Seems to become outdated with https://reviews.llvm.org/D27091. https://reviews.llvm.org/D26760 ___ cfe-commits mailing list

[PATCH] D27408: [analyzer] RetainCountChecker: remove unused enum value; NFC.

2016-12-05 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin. NoQ added a subscriber: cfe-commits. `OwnedSymbol` is not used anywhere; `OwnedAllocatedSymbol` is used instead. I couldn't grasp the intended difference between the two, maybe we should remove it?

[PATCH] D27409: [analyzer] RetainCountChecker: The callback in dispatch_data_create() doesn't free the return symbol.

2016-12-05 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin. NoQ added a subscriber: cfe-commits. The hack that was previously applied to `CGBitmapContextCreateWithData()` is now extended to another function, `dispatch_data_create()`. This function accepts a callback block, and the

[PATCH] D27409: [analyzer] RetainCountChecker: The callback in dispatch_data_create() doesn't free the return symbol.

2016-12-05 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 80264. NoQ added a comment. Update the comments in the surrounding code accordingly. https://reviews.llvm.org/D27409 Files: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp test/Analysis/dispatch-data-leak.m Index: test/Analysis/dispatch-data-leak.m

[PATCH] D26694: [analyzer] Drop explicit mention of range constraint solver

2016-11-30 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Approve! And probably `-analyzer-store=region` is worth dropping as well eventually. https://reviews.llvm.org/D26694 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D27091: Add the way to extract SVals of arguments used in a call for a given StackFrameCtx

2016-12-01 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Chatted about this a bit more. Because `CallEvent` is an API designed mostly for convenience, and `ProgramState` is a core API that needs to stay as clean and separate from other entities and easy to understand as possible, it would be the best to 1. construct a

[PATCH] D28330: [analyzer] Fix false positives in Keychain API checker

2017-01-05 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. > 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. Aha, i see! So

[PATCH] D28330: [analyzer] Fix false positives in Keychain API checker

2017-01-09 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ accepted this revision. NoQ added a reviewer: NoQ. NoQ added a comment. In https://reviews.llvm.org/D28330#637075, @zaks.anna wrote: > I did not think of solution #1! It's definitely better than the pattern > matching I've added here. However, this checker fires so infrequently, that I >

[PATCH] D28023: [analyzer] Fix leak false positives before no-return functions caused by incomplete analyses.

2016-12-21 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin, xazax.hun, a.sidorin. NoQ added a subscriber: cfe-commits. Consider an example: void foo(int y) { void *x = malloc(1); assert(y); // macro that expands to "if (!y) exit(1);" free(x); } In CFG block

[PATCH] D27710: [analyzer] Prohibit ExplodedGraph's edges duplicating

2016-12-21 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. I think `MallocChecker` should be (or should have been) affected by the problem you describe, so we can use it as an example. In https://reviews.llvm.org/D27710#628939, @zaks.anna wrote: > Do we support attaching multiple bug reports to the same node? That's what

[PATCH] D30909: [Analyzer] Finish taint propagation to derived symbols of tainted regions

2017-03-24 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Thanks again for the awesome stuff! It took years for me to even come up with the idea. Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:494 + SymbolManager = C.getSymbolManager(); + return SM.getDerivedSymbol(Sym, LCV.getRegion()); }

[PATCH] D31289: [analyzer] Fix symbolication for unknown unary increment/decrement results.

2017-03-23 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. If result of an unary increment or decrement is unknown, conjure a symbol to represent it based on the operator expression, not on the sub-expression. In this particular test case, result of a LocAsInteger increment is unknown, and it gets symbolicated to an `int

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

2017-03-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. I think this is awesome. Would anybody like to move concrete constraint class definitions to a separate header + translation unit, so that to highlight how simple the primary interface is to a person who haven't gone through all the discussion? Comment

[PATCH] D30534: [analyzer] When creating a temporary object copy, properly copy the value into it.

2017-03-22 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 92618. NoQ marked 4 inline comments as done. NoQ added a comment. Fix review comments! Add a callback order test to ensure that `checkRegionChanges` is called as many (or rather as few) times as necessary. https://reviews.llvm.org/D30534 Files:

[PATCH] D30534: [analyzer] When creating a temporary object copy, properly copy the value into it.

2017-03-22 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:281 + // Try to recover some path sensitivity in case we couldn't compute the value. + if (ExV.isUnknown()) +ExV = getSValBuilder().conjureSymbolVal(Result, LC, Ex->getType(),

[PATCH] D31840: [analyzer] Fix crash on access to property

2017-04-10 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. This defensive check looks reasonable regardless of how well we model unions (we don't). Thanks for the patch! Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:706 + } Values.push_back(IvarLVal); return; I believe

[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

2017-04-10 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Yeah, of course, ideally sticking this into scan-build, one way or another, would be great. I understand that it'd require quite a bit of communication with Laszlo. Otherwise we're just duplicating a whole lot of things. Thanks for digging into this, guys. Really.

[PATCH] D31886: [analyzer] Simplify values in binary operations more aggressively

2017-04-10 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. SValBuilder tries to constant-fold symbols in the left-hand side of the symbolic expression whenever it fails to evaluate the expression directly. However, it only constant-folds them when they are atomic expressions, not when they are complicated expressions

[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

2017-04-10 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. One more obvious observation regarding scan-build: because you are already reading a compilation database, the whole tool is essentially usable in combination with the current scan-build-py (which can create compilation databases). So it's already quite usable, but you're

[PATCH] D31650: [Analyzer] Detect when function pointer is freed

2017-04-04 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Hello, thanks for the patch! Because we already warn on freeing concrete function pointers, eg. void foo() { free(); } this is a useful addition. Is freeing function pointers always undefined? I wonder what happens if we take some JIT-enabled javascript engine,

[PATCH] D25596: alpha.core.Conversion - Fix false positive for 'U32 += S16; ' expression, that is not unsafe

2017-04-04 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. > There are several undetected "TODO: loss of precision" right now in the tests > that I would like to fix. I think it's a good idea to have `// no-warning` comments as well when testing for lack

[PATCH] D27918: [analyzer] OStreamChecker

2017-04-04 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Hello, Sorry again for the delays, thank you for your patience. Your checker is in good shape, very well-structured and easy to follow, you definitely know your stuff, and my comments here are relatively minor. Are you planning on making more varied warning messages, eg.

[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker

2017-04-04 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Hello, Because i couldn't reproduce the loc-nonloc problem on my standard library, could you share a preprocessed file with the issue? I'm really curious at what's going on here, and it's the only issue remaining, so maybe we could squash it together.

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

2017-04-04 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Hello, sorry for your having to wait on me. The implementation looks good, i'm only having a couple of public API concerns. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:46-49 +bool

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

2017-04-04 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ accepted this revision. NoQ added a comment. Yay, i think this is good to go. Sorry again for being slow>< I guess i'd go ahead and land this patch soon. Comment at: include/clang/Analysis/CloneDetection.h:228 +/// custom constraints. +class CloneConstraint { +public:

[PATCH] D31886: [analyzer] Simplify values in binary operations more aggressively

2017-04-12 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. In https://reviews.llvm.org/D31886#724796, @xazax.hun wrote: > maybe we want to skip this kind of simplification in case of Z3? Hmm, that depends on how would we want to use it eventually. - If Z3 acts all alone and fires only over actual bug reports, then yeah, it turns

[PATCH] D31886: [analyzer] Simplify values in binary operations more aggressively

2017-04-12 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 94955. NoQ marked 5 inline comments as done. NoQ added a comment. Thanks for the comments! Updated. Performance didn't degrade in my test runs. https://reviews.llvm.org/D31886 Files: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h

[PATCH] D31840: [analyzer] Fix crash on access to property

2017-04-12 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yep, looks good now, thanks! Repository: rL LLVM https://reviews.llvm.org/D31840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.

2017-04-12 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. In code void foo(int *p) { if (p) {} } void bar(int *p) { foo(p); *p = 5; } we suppress the null dereference warning in `*p = 5` because the state split within the inlined function is essentially irrelevant, as this is merely a defensive check

[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.

2017-04-12 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:965 + +// Performing operator `&' on an lvalue expression is essentially a no-op. +// Then, if we are taking addresses of fields or elements, these are also alexshap

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

2017-04-05 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ reopened this revision. NoQ added a comment. This revision is now accepted and ready to land. Hmm, reverted because i'm seeing crashes on some buildbots (works for me though). It's crashing somewhere in `saveHash`, seems that some `Stmt`s are null. For instance,

[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

2017-04-06 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. $ cat test.c #include int main() { int i = 5; { int i = i; printf("%d\n", i); } return 0; } $ clang test.c $ ./a.out 32767 $ clang test.c -Xclang -ast-dump ... `-FunctionDecl 0x7ff82d8eac78 line:2:5 main

[PATCH] D31975: [Analyzer] Iterator Checkers

2017-04-21 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. I had a look at how hard it is to split the patch. In ~3 hours i reached this far: F3239478: 0001-Delete-the-old-checker.patch F3239481: 0002-Reduced-patch-with-the-following-changes.patch F3239475:

[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.

2017-04-13 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 95082. NoQ added a comment. Remove accidentally added braces in unrelated code. https://reviews.llvm.org/D31982 Files: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp lib/StaticAnalyzer/Core/Store.cpp test/Analysis/explain-svals.cpp

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

2017-04-13 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. This looks correct! Thanks for taking this up. One of the possible improvements for future work here would be to actually bind the second argument value to the buffer instead of just invalidating it. Like, after `memset(buf, 0, sizeof(buf))` the analyzer should know that

[PATCH] D31541: [analyzer] MisusedMovedObjectChecker: Add a printState() method.

2017-04-13 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Hmm, i've been thinking of writing a test for this via `ExprInspection`'s `clang_analyzer_printState()`, however `printState()` functionality is only enabled in debug builds, and i'm not seeing how to enable the test only on debug builds (there's `REQUIRES:` but it only

[PATCH] D32291: [analyzer] Implement handling array subscript into null pointer, improve null dereference checks for array subscripts

2017-04-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ created this revision. When encountering an array-to-pointer-decay and the array base is null (or any other concrete pointer value) (eg. it's a member array in a structure, and the structure pointer is null; of course it wouldn't happen to stack-based or global arrays), do not yield

[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.

2017-04-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:977 + Ex = Op->getSubExpr()->IgnoreParenCasts(); + while (true) { +if (const auto *ME = dyn_cast(Ex)) { zaks.anna wrote: > Why do we need the

[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.

2017-04-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/Store.cpp:440 // well, although in reality we should return the offset added to that - // value. + // value. See also the similar FIXME in getLValueFieldOrIvar(). if (Base.isUnknownOrUndef() ||

[PATCH] D31868: [analyzer] Check NULL pointer dereference issue for memset function

2017-04-18 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Wow, so you're doing the binding thing now? Thanks! It was not critical for landing this patch, so you could have fixed comments here, allowing us to commit what's already done, and then proceed with further improvements. It's also easier to review and aligns with the

[PATCH] D30909: [Analyzer] Finish taint propagation to derived symbols of tainted regions

2017-04-18 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:494 + SymbolManager = C.getSymbolManager(); + return SM.getDerivedSymbol(Sym, LCV.getRegion()); } vlad.tsyrklevich wrote: > NoQ wrote: > > vlad.tsyrklevich wrote: > >

[PATCH] D31975: [Analyzer] Iterator Checkers

2017-04-19 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. I think for this case it'd be great to (instead) add comments all over the place (especially in checker callbacks, eg. `check[Pre|Post]Call()` function bodies) to indicate what check every piece of code is for. That'd be equally easy to review, at least for me, but it'd

[PATCH] D30771: [analyzer] Teach the MallocChecker about Glib API for two arguments

2017-04-19 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Yay, now that's a lot cleaner! Regarding style, recently we tend to ask for updating the style in the new code, and i don't think the Golden Rule (http://llvm.org/docs/CodingStandards.html#introduction) does much in our case, but i don't have a strong opinion on that.

[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.

2017-04-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ updated this revision to Diff 95890. NoQ marked 5 inline comments as done. NoQ added a comment. Fix comments! https://reviews.llvm.org/D31982 Files: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp lib/StaticAnalyzer/Core/Store.cpp test/Analysis/explain-svals.cpp

[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.

2017-04-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:965 + +// Performing operator `&' on an lvalue expression is essentially a no-op. +// Then, if we are taking addresses of fields or elements, these are also zaks.anna

[PATCH] D31975: [Analyzer] Iterator Checkers

2017-04-20 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Hmm. 1. Could you do renaming the checker file in a separate patch, so that we saw an actual diff, not a whole greenish file, here on phabricator? 2. The invalidated iterator checker looks relatively small (a single check and a few rounds of iterator invalidations), would

  1   2   3   4   5   6   7   8   9   10   >