[PATCH] D67480: [analyzer] Add 'freopen' support to SimpleStreamChecker.

NoQ added a comment. `SimpleStreamChecker` is a historical tutorial example , i don't think we should be updating it other than for modernizing checker API use. It was supposed to handle a few simple examples correctly but it wasn't supposed to suppo

[PATCH] D66714: [analyzer] Don't run the analyzer for -analyzer-list-enabled-checkers

NoQ added a comment. Umm, this test fails for me locally because i see `osx` checkers displayed in the list. Which is weird because they're supposed to be enabled based on the target platform, not on the host platform. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D66714: [analyzer] Don't run the analyzer for -analyzer-list-enabled-checkers

NoQ added a comment. rC371781 . Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66714/new/ https://reviews.llvm.org/D66714 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D59516: [analyzer] Add custom filter functions for GenericTaintChecker

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:517 +if (V) + State = addTaint(State, *V, TaintTagNotTainted); + } Why not simply remove taint? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D5951

[PATCH] D67567: [clang-tidy] New check to warn when storing dispatch_once_t in non-static, non-global storage

NoQ added a comment. FTR, we already have a similar Static Analyzer check, eg.: https://github.com/llvm-mirror/clang/blob/release_80/test/Analysis/dispatch-once.m#L15 Your check is a bit more aggressive but i don't see why didn't we do it that way in the first place :) CHANGES SINCE LAST ACTI

[PATCH] D67480: [analyzer] Add 'freopen' support to SimpleStreamChecker.

NoQ added a comment. Btw, `evalCall` is not deprecated. In fact, there are no alternatives for it in many cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67480/new/ https://reviews.llvm.org/D67480 __

[PATCH] D67706: [clang][analyzer] Using CallDescription in StreamChecker.

NoQ added a comment. Thanks! Woohoo, tests! You can go one step further to have a `CallDescriptionMap`, like in D62557 . This would replace the whole chain of `if`s with a map lookup (which is currently still a chain of `if`s under the hood, but a lot less code

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

NoQ added a comment. Is it just me or phabricator somehow refuses to display the changes since the last diff here? That's probably the commit diff is involved somehow. So i'm confused what exactly has changed >.< Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D67932: [analyzer] Fix accidentally skipping the call during inlined defensive check suppression.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus, baloghadamsoftware, Charusso. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet. Herald added a project: clang. Got myself distracted with a tiny fix

[PATCH] D67932: [analyzer] Fix accidentally skipping the call during inlined defensive check suppression.

NoQ marked an inline comment as done. NoQ added a comment. Yeah, i think we should avoid such peeking and instead try to do everything in one pass. I.e., if we need to peek at the node above us, just make a visitor that delays the decision until it has precisely the information it needs. I gues

[PATCH] D67706: [clang][analyzer] Using CallDescription in StreamChecker.

NoQ added a comment. In D67706#1685963 , @Szelethus wrote: > It seems like this patch is diffed against your latest commit, not the master > branch. Yeah, seems so. The code looks great tho, thanks! Comment at: clang/lib/StaticAnalyz

[PATCH] D68172: Don't install example analyzer plugins

NoQ added a reviewer: Szelethus. NoQ added a comment. +@Szelethus because i'm a bit out-of-the-loop on plugins: i very vaguely remember that we decided to put them into tests(?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68172/new/ https://revi

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

NoQ marked an inline comment as done. NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:378-379 + + using CheckFn = void (MallocChecker::*)(CheckerContext &C, const CallExpr *CE, + ProgramStateRef

[PATCH] D68162: [analyzer][MallocChecker][NFC] Communicate the allocation family information to auxiliary functions with template parameters

NoQ added a comment. Thank you, fantastic finding! > in fact we know it //compile time// Yeah, but is it accidental or is there a good reason behind always having this information at compile time? 'Cause i don't want to restrict the code to always provide this information at compile time if we

[PATCH] D68163: [analyzer][MallocChecker][NFC] Change the use of IdentifierInfo* to CallDescription

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Fantastic, thanks!! Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:259 /// calls. bool isCalled(const CallDescription &CD) const; --

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:397-398 + CallDescriptionMap NonFreeingMemFnMap{ + {{"alloca", 1}, &MallocChecker::checkAlloca}, + {{"_alloca", 1}, &MallocChecker::checkAlloca}, + {{"malloc", 1}, &MallocCh

[PATCH] D67706: [clang][analyzer] Using CallDescription in StreamChecker.

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:90-106 + FnCheck identifyCall(const CallEvent &Call, CheckerContext &C, + const CallExpr *CE) const; + + void evalFopen(CheckerContext &C, const CallExpr *CE) const;

[PATCH] D68199: [analyzer] DynamicTypeInfo: Simplify the API

NoQ added a comment. I actually like the idea, it makes it consistent with other maps. But you'll need to clean up memory management here. Given that you can't modify the state in `getDynamicTypeInfo()`, i guess you'll have to resort to smart pointers. Comment at: clang/lib/S

[PATCH] D67079: [analyzer] CastValueChecker: Model inheritance

NoQ added a comment. In D67079#1687577 , @Charusso wrote: > - `CastVisitor` is the new facility which decides whether the assumptions are > appropriate. > - The math is still WIP. You need the math to decide whether delaying the decision to a visitor is

[PATCH] D67079: [analyzer] CastValueChecker: Model inheritance

NoQ added inline comments. Comment at: clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp:25-27 + if (isa(a)) +if (isa(a)) + clang_analyzer_warnIfReached(); // no-warning Charusso wrote: > NoQ wrote: > > Why is `(isa(a) && isa(a))` deemed possi

[PATCH] D67079: [analyzer] CastValueChecker: Model inheritance

NoQ added inline comments. Comment at: clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp:25-27 + if (isa(a)) +if (isa(a)) + clang_analyzer_warnIfReached(); // no-warning Charusso wrote: > NoQ wrote: > > Charusso wrote: > > > NoQ wrote: > > > >

[PATCH] D67079: [analyzer] CastValueChecker: Model inheritance

NoQ added inline comments. Comment at: clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp:25-27 + if (isa(a)) +if (isa(a)) + clang_analyzer_warnIfReached(); // no-warning NoQ wrote: > Charusso wrote: > > NoQ wrote: > > > Charusso wrote: > > > >

[PATCH] D67706: [clang][analyzer] Using CallDescription in StreamChecker.

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:60-61 private: - void Fopen(CheckerContext &C, const CallExpr *CE) const; - void Tmpfile(CheckerContext &C, const CallExpr *CE) const; - void Fclose(CheckerContext &C, const CallExpr *C

[PATCH] D68199: [analyzer] DynamicTypeInfo: Simplify the API

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yup, optional sounds perfect. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68199/new/ https://reviews.llvm.org/D68199 ___ cfe-commits mailing

[PATCH] D41938: [Analyzer] SValBuilder Comparison Rearrangement (with Restrictions and Analyzer Option)

NoQ added a comment. We've found a crash on our internal buildbot, would you like to have a look?: **`$ cat repro.c`** int foo(int x, int y) { short a = x - 1U; return a - y; } **`$ clang -cc1 -analyze -analyzer-checker=core repro.c`** Assertion failed: (APSIntType(LInt) == BV.ge

[PATCH] D45557: [Analyzer] Fix for SValBuilder expressions rearrangement

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:443 return Sym->getType() == Ty && +APSIntType(Int) == BV.getAPSIntType(Sym->getType()) && (!BinaryOperator::isComparisonOp(Op) || 6. Therefore i conclude that this c

[PATCH] D45557: [Analyzer] Fix for SValBuilder expressions rearrangement

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yup thanks! https://reviews.llvm.org/D45557 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

[PATCH] D45650: [CFG] [analyzer] Don't treat argument constructors as temporary constructors.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet. Herald added subscribers: cfe-commits, rnkovacs. Function argument constructors (that are used for passing objects into functions by value) are completely unlike temporary object constructor

[PATCH] D45650: [CFG] [analyzer] Don't treat argument constructors as temporary constructors.

NoQ updated this revision to Diff 142505. NoQ added a comment. Add tests where the argument is passed by reference. These tests work correctly (i.e. they correctly identify the argument constructor as a temporary constructor) because such constructors would include a `MaterializeTemporaryExpr`

[PATCH] D45491: [analyzer] Do not invalidate the `this` pointer.

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yeah, i think this makes sense, thanks! It feels a bit weird that we have to add it as an exception - i wonder if there are other exceptions that we need to make. Widening over the stack memory spac

[PATCH] D45416: [analyzer] ExprEngine: model GCC inline asm rvalue cast outputs

NoQ accepted this revision. NoQ added a comment. Wow, you actually did that. Ok, now we can decide if we want this to be analyzer-only (with a `CFG::BuildOptions` flag) or get someone else to have a look at that as a global CFG change (i.e. it may potentially affect compiler warnings). Or at l

[PATCH] D45416: [analyzer] ExprEngine: model GCC inline asm rvalue cast outputs

NoQ added a comment. (i'd much rather do the latter) Repository: rC Clang https://reviews.llvm.org/D45416 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45407: [StaticAnalyzer] Added notes to the plist output

NoQ added a comment. Sorry, overwhelmed a bit, i'll try to get to this as soon as possible. Btw, what sort of UI are you trying to make these extra note pieces of mine work with? Was `-analyzer-config notes-as-events=true` of any help? Repository: rC Clang https://reviews.llvm.org/D45407

[PATCH] D45698: [analyzer] When we fail to evaluate a pointer cast, escape the pointer.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet. Herald added subscribers: cfe-commits, rnkovacs. If a pointer cast fails (evaluates to an `UnknownVal`) and such cast is the last use of the pointer, the pointer is no longer referenced by t

[PATCH] D45698: [analyzer] When we fail to evaluate a pointer cast, escape the pointer.

NoQ added a comment. Hmm, yeah, indeed, i must have overlooked it, nice one :) I'll see if i can fix the other place as well. Repository: rC Clang https://reviews.llvm.org/D45698 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lis

[PATCH] D45416: [analyzer] ExprEngine: model GCC inline asm rvalue cast outputs

NoQ added a comment. (also we'll need CFG dump tests) Repository: rC Clang https://reviews.llvm.org/D45416 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45706: [CFG] [analyzer] Add construction contexts for loop condition variables.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet. Herald added subscribers: cfe-commits, rnkovacs. Loop condition variables (eg. `while (shared_ptr P = getIntPtr()) { ... }`) weren't handled in https://reviews.llvm.org/D42699 because they d

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet. Herald added subscribers: cfe-commits, rnkovacs, baloghadamsoftware. Since recently, WebKit uses a peculiar build system that compiles multiple translation units at once by automatically joi

[PATCH] D45117: [analyzer] Fix diagnostics in callees of interesting callees.

NoQ added a comment. Herald added a subscriber: baloghadamsoftware. https://reviews.llvm.org/rC330375 has nothing to do with this review; i misplaced the link in the commit message again. Repository: rL LLVM https://reviews.llvm.org/D45117 ___ c

[PATCH] D45335: [analyzer] RetainCount: Accept more "safe" CFRetain wrappers.

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Herald added a subscriber: baloghadamsoftware. Committed in https://reviews.llvm.org/rC330375 but i misplaced the phabricator link again, sorry. Repository: rC Clang https://reviews.llvm.org/D45

[PATCH] D45698: [analyzer] When we fail to evaluate a pointer cast, escape the pointer.

NoQ added a comment. In https://reviews.llvm.org/D45698#1069121, @xazax.hun wrote: > I am in favour of this approach. This is what I suggested back than in > https://reviews.llvm.org/D23014 but it was somehow overlooked. I'll try to follow-up on this. Repository: rC Clang https://reviews.

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

NoQ updated this revision to Diff 143195. NoQ added a comment. Hopefully a more portable way of testing this stuff. https://reviews.llvm.org/D45839 Files: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h lib/StaticAnalyzer/Core/CallEvent.cpp lib/StaticAnalyzer/Core/ExprEn

[PATCH] D45774: [analyzer] cover more cases where a Loc can be bound to constants

NoQ added a comment. Looks great, thanks! > CXXDynamicCastExpr: I don't think evalCast would handle this correctly, does > it? Seems so. It is partially supported by `StoreManager::attemptDownCast()`. I also see relatively little urgency in handling it here because the whole point of `dynamic

[PATCH] D45517: [analyzer] WIP: False positive refutation with Z3

NoQ added a comment. > The visitor currently checks states appearing as block edges in the exploded > graph. The first idea was to filter states based on the shape of the exploded > graph, by checking the number of successors of the parent node, but > surprisingly, both `succ_size()` and `pred_

[PATCH] D45407: [StaticAnalyzer] Added notes to the plist output

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Checked this out, seems to be safely ignored for now. The `.plist` format is pretty resistant to this sort of stuff because most APIs to handle it are almost treating it as native arrays/dictionarie

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h:144 +// includes the full path. +if (SM.getFilename(IL).contains("UnifiedSource")) { + StringRef Name = SM.getFilename(SL); george.karpenkov wro

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

NoQ updated this revision to Diff 143396. NoQ marked 3 inline comments as done. NoQ added a comment. Address most comments. https://reviews.llvm.org/D45839 Files: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h lib/StaticAnalyzer/Core/CallEvent.cpp lib/StaticAnalyzer/Cor

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h:145-147 + if (Name.endswith_lower(".c") || Name.endswith_lower(".cpp") || + Name.endswith_lower(".cc") || Name.endswith_lower(".cxx") || + Name.endswit

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

NoQ updated this revision to Diff 143416. NoQ marked an inline comment as done. NoQ added a comment. A more accurate extension check. https://reviews.llvm.org/D45839 Files: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h lib/StaticAnalyzer/Core/CallEvent.cpp lib/StaticAn

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

NoQ added a comment. Guys, what do you think about a checker that warns on uninitialized fields only when at least one field is initialized? I'd be much more confident about turning such check on by default. We can still keep a `pedantic` version. > I can say with confidence that CodeChecker do

[PATCH] D45920: [analyzer] Move RangeSet related declarations into the RangedConstraintManager header.

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/RangedConstraintManager.h:130 +template <> +struct ProgramStateTrait + : public ProgramStatePartialTrait { george.karpenkov wrote: > Why not also `REGISTER_TRAIT_WITH_PROGRAMSTATE` here? ``` 33

[PATCH] D45920: [analyzer] Move RangeSet related declarations into the RangedConstraintManager header.

NoQ added a comment. In https://reviews.llvm.org/D45920#1074439, @george.karpenkov wrote: > Another approach would be to instead teach `RangedConstraintManager` to > convert it's constraints to Z3. That would be an unwanted dependency, but the > change would be much smaller, and the internals o

[PATCH] D45517: [analyzer] WIP: False positive refutation with Z3

NoQ added a comment. In https://reviews.llvm.org/D45517#1074422, @rnkovacs wrote: > In https://reviews.llvm.org/D45517#1074057, @NoQ wrote: > > > So, yeah, that's a good optimization that we're not invoking the solver on > > every node. But i don't think we should focus on improving this > > op

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

NoQ marked 2 inline comments as done. NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h:145-147 + if (Name.endswith_lower(".c") || Name.endswith_lower(".cpp") || + Name.endswith_lower(".cc") || Name.endswith_lo

[PATCH] D45920: [analyzer] Move RangeSet related declarations into the RangedConstraintManager header.

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D45920#1074437, @george.karpenkov wrote: > > I could also move RangedConstraintManager.h under include > > We probably don't want to do that: currently it can only be impo

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h:144 +// includes the full path. +if (SM.getFilename(IL).contains("UnifiedSource")) { + StringRef Name = SM.getFilename(SL); probinson wrote: > g

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

NoQ added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h:144 +// includes the full path. +if (SM.getFilename(IL).contains("UnifiedSource")) { + StringRef Name = SM.getFilename(SL); NoQ wrote: > probins

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

NoQ added a comment. Aha, ok, yeah, that sounds like a lot, thank you. I think i'll follow up with a separate commit that will enable first-level-code-file-include analysis in all files under an on-by-default `-analyzer-config` flag, would that make sense? https://reviews.llvm.org/D45839 __

[PATCH] D45407: [StaticAnalyzer] Added notes to the plist output

NoQ added a comment. Yup thanks!~ Repository: rC Clang https://reviews.llvm.org/D45407 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46037: [analyzer] pr37166, pr37139: Disable constructor inlining when lifetime extension through aggregate initialization occurs.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet. Herald added subscribers: cfe-commits, rnkovacs, baloghadamsoftware. This hotfix is similar to https://reviews.llvm.org/D43689 (and needs a follow-up similar to https://reviews.llvm.org/D442

[PATCH] D46037: [analyzer] pr37166, pr37139: Disable constructor inlining when lifetime extension through aggregate initialization occurs.

NoQ updated this revision to Diff 143850. NoQ added a comment. Add a test similar to pr37166. It should be actually fixed now with this patch. Essentially, we may also be lifetime-extended by an array of aggregate structures (with arbitrary amounts of interleaving array and aggregate structure

[PATCH] D46115: [ASTImporter] properly import SrcLoc of Attr

NoQ added a reviewer: a.sidorin. NoQ added a comment. +Alexey because he's the `ASTImporter` guy. Repository: rC Clang https://reviews.llvm.org/D46115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D46146: [analyzer] pr37152: Fix operator delete[] array-type-sub-expression handling.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet. Herald added subscribers: cfe-commits, rnkovacs, baloghadamsoftware. When operator `delete[]` receives a sub-expression of array type, it destroys the array correctly. Even if it's multi-dim

[PATCH] D46146: [analyzer] pr37152: Fix operator delete[] array-type-sub-expression handling.

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1089-1091 +// Yes, it may even be a multi-dimensional array. +while (const auto *AT = getContext().getAsArrayType(DTy)) + DTy = AT->getElementType(); alexfh wrote: > Maybe

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

NoQ added inline comments. Comment at: clang-tidy/ClangTidy.cpp:373-376 // FIXME: Remove this option once clang's cfg-temporary-dtors option defaults // to true. AnalyzerOptions->Config["cfg-temporary-dtors"] = Context.getOptions().AnalyzeTemporaryDtors ? "true" :

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

NoQ added a comment. Am i understanding correctly that the new `-enable-alpha-checks` flag doesn't enable alpha checks, but it only allows enabling them with a separate command? Also wanted to mention that developers who are interested in running analyzer alpha checkers over a compilation datab

[PATCH] D45177: CStringChecker, check strlcpy/strlcat

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:106 +bool isAppending, +bool canOverlap = false) const; The fact that the regular `strcpy`/`strcat` isn't checked for overlaps

[PATCH] D45416: [analyzer] ExprEngine: model GCC inline asm rvalue cast outputs

NoQ accepted this revision. NoQ added a subscriber: rsmith. NoQ added a comment. Woohoo LGTM. Heads up to @rsmith because we're about to break the CFG again, and also yay we've found another use case for rewriting AST in our CFG. Repository: rC Clang https://reviews.llvm.org/D45416 _

[PATCH] D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize.

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Looks great, thanks! I think the overall plan for any taint work would be to remove it from the program state API and move getters/setters into its own translation unit (like dynamic type propagati

[PATCH] D46224: [analyzer] pr37209: Fix casts of lvalues to references.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet. Herald added subscribers: cfe-commits, rnkovacs, baloghadamsoftware. In the provided test case the expression `(int *&)*(int *)p` is evaluated as follows: 1. Take an lvalue `p` of type `cha

[PATCH] D35110: [Analyzer] Constraint Manager Negates Difference

NoQ added a comment. LGTM! Minor nitpicking in comments. Currently there's no such problem, but as `GetRange` becomes more complicated, we'll really miss the possibility of saying something like "if there's a range for negated symbol, `return GetRange(the negated symbol)`", so that all other s

[PATCH] D35110: [Analyzer] Constraint Manager Negates Difference

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:265-276 + const llvm::APSInt &from = i->From(), &to = i->To(); + const llvm::APSInt &newFrom = (to.isMinSignedValue() ? + BV.getMaxValue(to) : +

[PATCH] D45177: CStringChecker, check strlcpy/strlcat

NoQ added a comment. Whoops - this isn't quite correct because there's one more difference between strlcpy/strlcat and the standard strcpy/strcat/strncpy/strncat: the return value. After this patch the new functions are modeled as if they return a pointer into the string, which is incorrect and

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

NoQ added a comment. Had a look at the actual code, came up with some comments, most are fairly minor, so good job! You did a good job explaining the overall idea, but a lot of specifics were difficult to understand, so i made a few comments on how to make the code a bit easier to read. > I kn

[PATCH] D45611: [analyzer] Fix filename in cross-file HTML report

NoQ added a comment. Thanks! I noticed the problem but never investigated it. Repository: rC Clang https://reviews.llvm.org/D45611 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45177: CStringChecker, check strlcpy/strlcat

NoQ reopened this revision. NoQ added a comment. This revision is now accepted and ready to land. I reverted this for now. I'm sorry - i would have looked into this myself, but unfortunately there are other important fixes that distract me at the moment. Repository: rC Clang https://reviews.

[PATCH] D35110: [Analyzer] Constraint Manager Negates Difference

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:265-276 + const llvm::APSInt &from = i->From(), &to = i->To(); + const llvm::APSInt &newFrom = (to.isMinSignedValue() ? + BV.getMaxValue(to) : +

[PATCH] D46368: [analyzer] pr18953: Zeroing constructors, store binding extents, ASTRecordLayouts.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, rnkovacs. Herald added subscribers: cfe-commits, baloghadamsoftware. As the old assertion says, > "Thou shalt not initialize the same memory twice." Namely, the `State->bindDefault()`/`Sto

[PATCH] D45177: CStringChecker, check strlcpy/strlcat

NoQ added a comment. Yay that was fast, thanks! Could we have tests for the return value? Like, concatenate a couple of concrete string literals and verify the return value via `clang_analyzer_eval()`. https://reviews.llvm.org/D45177 ___ cfe-comm

[PATCH] D46393: Remove explicit cfg-temporary-dtors=true

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Thanks! Just curious - did these flags bother you? Cause we never really care about cleaning up run lines after flipping the flag, so we have a lot of such stale flags in our tests. We could start

[PATCH] D46415: [analyzer] pr36458: Fix retrieved value cast for symbolic void pointers.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, rnkovacs. Herald added subscribers: cfe-commits, baloghadamsoftware. Through C cast magic it's possible to put a raw void pointer into a variable of non-void pointer type. It is fine - gene

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260 +Report->addNote(NoteBuf, +PathDiagnosticLocation::create(FieldChain.getEndOfChain(), + Context.get

[PATCH] D46415: [analyzer] pr36458: Fix retrieved value cast for symbolic void pointers.

NoQ updated this revision to Diff 145260. NoQ added a comment. Fix test names. Add one more test, just to make sure it works. https://reviews.llvm.org/D46415 Files: lib/StaticAnalyzer/Core/Store.cpp test/Analysis/casts.c Index: test/Analysis/casts.c ===

[PATCH] D45774: [analyzer] cover more cases where a Loc can be bound to constants

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yup, thanks! I'll commit. https://reviews.llvm.org/D45774 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

[PATCH] D45774: [analyzer] cover more cases where a Loc can be bound to constants

NoQ added a comment. Two questions for the future: - Should we skip the initializer binding for local variables (and fields/elements of local variables) entirely? Cause we can load from them anyway. And keeping the Store small might be good for performance. - Just in case, do you accidentally

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

NoQ added a comment. In https://reviews.llvm.org/D46159#1088050, @alexfh wrote: > As folks noted here, some users prefer to use clang-tidy as a frontend for > the static analyzer. If this helps them test experimental CSA features and > CSA maintainers are willing to accept bug reports and poten

[PATCH] D46081: [analyzer] Expand conversion check to check more expressions for overflow and underflow

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Yep, looks good. This limitation to `DeclRefExpr`s was super conservative. I'll follow up regarding the bugprone category in a week or so on the mailing lists. It's been a long-standing historical d

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260 +Report->addNote(NoteBuf, +PathDiagnosticLocation::create(FieldChain.getEndOfChain(), + Context.get

[PATCH] D46224: [analyzer] pr37209: Fix casts of glvalues to references.

NoQ added a comment. > i'll most likely try to investigate it more carefully, or at least see if it > causes regressions on large codebases, before committing Seemed fine, no visible change. I hope this means that i pattern-matched carefully enough. Repository: rC Clang https://reviews.llv

[PATCH] D46368: [analyzer] pr18953: Zeroing constructors, store binding extents, ASTRecordLayouts.

NoQ added a comment. > We could work around that by invalidating object contents every time we > detect something fishy. The only reasonable detection i can come up with > would be to detect if we have a coinciding binding (i.e. what the assertion > was previously protecting us from), eg.: > >

[PATCH] D46187: [Analyzer] getRegisteredCheckers(): handle debug checkers too.

NoQ added a comment. It seems that you're using debug checkers of the analyzer to gain some free tools for exploring the source code (such as displaying a call graph) for free, right? I believe we could also benefit from a method of extracting the analyzer's `clang -cc1` run-line from clang-ti

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

NoQ added a comment. Hmm, ok, it seems that i've just changed the API in https://reviews.llvm.org/D46368, and i should have thought about this use case. Well, at least i have some background understanding of these problems now. Sorry for not keeping eye on this problem. In https://reviews.llvm

[PATCH] D46159: [clang-tidy] Add a flag to enable alpha checkers

NoQ added a comment. Ok, i personally think this patch should go in. I think it makes it clear enough that an unsuspecting user would suspect something by reading the flag, and makes the feature hard enough to discover. And i'm happy to support anybody who's going to work on this stuff. -

[PATCH] D45177: CStringChecker, check strlcpy/strlcat

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1485 + state = CheckOverlap(C, state, CE->getArg(2), Dst, srcExpr); + This crashes on the old tests for the checker. I guess that's because the normal `strcpy()` doesn't hav

[PATCH] D43928: [analyzer] Correctly measure array size in security.insecureAPI.strcpy

NoQ added a comment. Sorry, i completely forgot about this one :( I think this patch needs `lit` tests, eg. tell the analyzer to analyze a simple strcpy() call on any `-target` with non-8-bit chars and see if it's still crashes or behaves incorrectly. Comment at: lib/StaticA

[PATCH] D48205: [analyzer] Assert that nonloc::SymbolVal always wraps a non-Loc-type symbol.

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1241 if (const llvm::APSInt *I = - SVB.getKnownValue(State, nonloc::SymbolVal(S))) + SVB.getKnownValue(State, SVB.makeSymbolVal(S))) return Loc::isLocTyp

[PATCH] D48205: [analyzer] Assert that nonloc::SymbolVal always wraps a non-Loc-type symbol.

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1241 if (const llvm::APSInt *I = - SVB.getKnownValue(State, nonloc::SymbolVal(S))) + SVB.getKnownValue(State, SVB.makeSymbolVal(S))) return Loc::isLocTyp

[PATCH] D48608: [CFG] [analyzer] Add construction contexts for C++ objects returned from Objective-C messages.

NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, george.karpenkov. Herald added subscribers: cfe-commits, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware. This patch extends https://reviews.llvm.org/D44120 to Objective-C messages that can also sometimes retu

[PATCH] D48285: [analyzer][UninitializedObjectChecker] Added "NotesAsWarnings" flag

NoQ accepted this revision. NoQ added a comment. Looks great, thanks! Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:696 void ento::registerUninitializedObjectChecker(CheckerManager &Mgr) { auto Chk = Mgr.registerChecker(); Chk->IsPedantic = Mgr.g

[PATCH] D48291: [analyzer][UninitializedObjectChecker] Fixed captured lambda variable name

NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:699 +for (const LambdaCapture &L : CXXParent->captures()) { + if (L.getLocation() == Field->getLocation()) +return L.getCapturedVar()->getName(); Uh

<    11   12   13   14   15   16   17   18   19   20   >