[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

2019-09-03 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL370767: [analyzer] Add a checker option to detect nested dead stores (authored by Szelethus, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message

2019-09-03 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL370798: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker… (authored by Szelethus, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Change

[PATCH] D67024: [analyzer] NFC: Replace intrusive list of bug reports with a vector of pointers.

2019-09-03 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. Other then the things @gribozavr mentioned, LGTM. Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:458 +public: + typedef llvm::SmallVector, 4> ReportList; + using iterator = ReportList

[PATCH] D66572: [analyzer] NFC: BugReporter Separation Ep.I.

2019-09-03 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Also, thank you @gribozavr for your comments -- its very nice to have someone review a bigger part of our development interface who is knowledgeable about Clang, but not the Static Analyzer specifically. Looking at your inlines, these are very fair criticisms, I found

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

2019-09-03 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:42 + return MR->StripCasts(); +} + NoQ wrote: > Charusso wrote: > > Probably a consistent way of `MemRegion` handling is more appropriate. > Pointer casts are quite a can

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

2019-09-03 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:133 std::unique_ptr> InEH; + const bool WarnForDeadNestedAssignments; NoQ wrote: > I suggest we adopt the idiom of passing the `Checker` object around a

[PATCH] D67140: [analyzer][NFC] Fix inconsistent references to checkers as "checks"

2019-09-03 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus created this revision. Szelethus added reviewers: NoQ, xazax.hun, Charusso, baloghadamsoftware, rnkovacs, dcoughlin. Szelethus added a project: clang. Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet, whisperity. Traditionally, cl

[PATCH] D67140: [analyzer][NFC] Fix inconsistent references to checkers as "checks"

2019-09-03 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added reviewers: gribozavr, alexfh. Szelethus added subscribers: gribozavr, alexfh. Szelethus added a comment. @gribozavr, @alexfh this isn't a particularly exciting patch, but it does highlight a fundamental difference in between the analyzer and clang-tidy, so I added you for the sa

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

2019-09-03 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:133 std::unique_ptr> InEH; + const bool WarnForDeadNestedAssignments; Szelethus wrote: > NoQ wrote: > > I suggest we adopt the idiom of passing the `Che

[PATCH] D59637: [analyzer] Use the custom propagation rules and sinks in GenericTaintChecker

2019-09-05 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Also, please mark inlines done as you fix them :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59637/new/ https://reviews.llvm.org/D59637 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm

[PATCH] D59637: [analyzer] Use the custom propagation rules and sinks in GenericTaintChecker

2019-09-05 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a reviewer: steakhal. Szelethus added a comment. This revision is now accepted and ready to land. Some nits inline, otherwise LGTM. @steakhal, do you have anything to add to this? Comment at: lib/StaticAnalyzer/Checkers/Generic

[PATCH] D67140: [analyzer][NFC] Fix inconsistent references to checkers as "checks"

2019-09-05 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D67140#1658365 , @aaron.ballman wrote: > Then again, with the recent resurfacing of discussions about renaming > everything under the sun, maybe we've changed our community opinion here. :-D > I guess I don't see Check vs

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

2019-09-05 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I'm still working on this, just been kinda busy. I'll try to get it out of the way asap. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66716/new/ https://reviews.llvm.org/D66716 ___

[PATCH] D67140: [analyzer][NFC] Fix inconsistent references to checkers as "checks"

2019-09-05 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I don't have strong feelings on capitalization itself either, but the idea of longer, multi-sentence bug report messages (maybe this could finally be used? D66572#inline-602143 ) sound pretty cool. Repository: rG LLVM

[PATCH] D67140: [analyzer][NFC] Fix inconsistent references to checkers as "checks"

2019-09-05 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D67140#1659907 , @NoQ wrote: > In D67140#1659872 , @aaron.ballman > wrote: > > > If we're okay with the inconsistency but still feel like giving ourselves > > more work, adding proper

[PATCH] D59637: [analyzer] Use the custom propagation rules and sinks in GenericTaintChecker

2019-09-06 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. I'm fine with moving in-class function into anonymous namespace later. LGTM, feel free to commit! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59637/new/ https://reviews.llvm.org/D59637 ___

[PATCH] D66572: [analyzer] NFC: BugReporter Separation Ep.I.

2019-09-06 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. This patch set the goal of splitting `BugReport` into two different report kinds, and I think it did that well. Not only that, we drastically improved the documentation, formalized many

[PATCH] D66572: [analyzer] NFC: BugReporter Separation Ep.I.

2019-09-06 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. But, of course, let's wait for @gribozavr to give his blessings as well, I'm only accepting because removing/changing other parts of the API seems to deserve a separate revision :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66572/new/ https://reviews.llvm

[PATCH] D67156: [Analyzer] Debug Checkers for Container and Iterator Inspection

2019-09-06 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Nice! Thank you so much for sorting this out! I think this will make reviewing far more accessible for newcomers to the IteratorChecker family. I have a couple nits to comment on, but I won't clutter the code just yet. @NoQ, do you have any high level objections t

[PATCH] D67156: [Analyzer] Debug Checkers for Container and Iterator Inspection

2019-09-06 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I'm sadly not knowledgeable enough with `CallDescriptionMap`, so let's have another round of review on this, otherwise, its perfect. We talked about dividing this checker into multiple files, which would also make reviewing a bit easier. With that done, combined with

[PATCH] D64991: [analyzer][WIP] Implement a primitive reaching definitions analysis

2019-09-07 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Hmm, we could make a redundant assignments checker: if a variable has multiple reaching definitions, but those all assign the same value, emit a warning. We could even use fixits with that. void t(int a) { if (coin()) a = 2; // note: reaching def else

[PATCH] D67335: [analyzer][NFC] Refactor the checker registration unit test file

2019-09-08 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus created this revision. Szelethus added reviewers: NoQ, xazax.hun, rnkovacs, Charusso, dcoughlin, baloghadamsoftware. Szelethus added a project: clang. Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet, whisperity. Nothing exciting

[PATCH] D67336: [analyzer][NFC] Introduce SuperChecker<>, a convenient alternative to Checker<> for storing subcheckers

2019-09-08 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus created this revision. Szelethus added reviewers: NoQ, xazax.hun, rnkovacs, dcoughlin, Charusso, baloghadamsoftware, dkrupp. Szelethus added a project: clang. Herald added subscribers: cfe-commits, gamesh411, donat.nagy, mikhail.ramalho, a.sidorin, szepet, whisperity. Please don't take

[PATCH] D67335: [analyzer][NFC] Refactor the checker registration unit test file

2019-09-08 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus marked an inline comment as done. Szelethus added inline comments. Comment at: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp:84 -} -} -} +} // namespace +} // namespace ento Well, according to `clangd`, this is how it's supposed to be

[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

2020-09-11 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus updated this revision to Diff 291199. Szelethus marked 4 inline comments as done. Szelethus added a comment. Reinstate the assert removed in rG032b78a0762bee129f33e4255ada6d374aa70c71 , add a test. Enforce that `Envir

[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

2020-09-11 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rGb9bca883c970: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a… (authored by Szelethus). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

2020-09-11 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus updated this revision to Diff 291231. Szelethus added a comment. Rename the live statements checker to live expressions checker. The test file added in a revert commit changed rather heavily, but it makes sense that these entries are removed IMO. Unless anyone objects, I'll intend to l

[PATCH] D87518: [analyzer][Liveness][NFC] Remove an unneeded pass to collect variables that appear in an assignment

2020-09-11 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus created this revision. Szelethus added reviewers: xazax.hun, NoQ, vsavchenko, martong, balazske, baloghadamsoftware, steakhal. Szelethus added a project: clang. Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnk

[PATCH] D87519: [analyzer][Liveness][NFC] Enqueue the CFGBlocks post-order

2020-09-11 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus created this revision. Szelethus added reviewers: xazax.hun, NoQ, vsavchenko, balazske, martong, baloghadamsoftware, steakhal. Szelethus added a project: clang. Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnk

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

2020-09-14 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. @balazske may have some closing words. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1262 Getline(LongLongTy

[PATCH] D86533: (Urgent!) [release][docs][analyzer] Add 11.0.0. release notes

2020-09-15 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Sorry for the slack (which is kind of ironic with my attention grabbing title :) ). Landed in rG791b7e9f73e0064153a7c3db8045a7333a8c390c . Thanks everyone! CHANGES SINCE LAST ACTION https://revie

[PATCH] D86532: (Urgent!) [docs][analyzer] Add documentation for alpha.fuchsia.Lock and alpha.core.C11Lock

2020-09-15 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG7c6f5b7fbf5a: [analyzer] Add documentation for alpha.fuchsia.Lock and alpha.core.C11Lock (authored by Szelethus). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D82598: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing doesn't exist

2020-09-15 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rGdd1d5488e47d: [analyzer][Liveness][NFC] Get rid of statement liveness, because such a thing… (authored by Szelethus). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

2020-09-15 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Post-commit LGTM on the post-commit changes! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87081/new/ https://reviews.llvm.org/D87081 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

2020-09-20 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I figured you're still working on this, sorry! I'd really like to chat about my earlier comment D71524#1917251 , as it kind of challenges the high level idea. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71524/new/ h

[PATCH] D87942: [Analyzer] GNU named variadic macros in Plister

2020-09-20 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. The fix is great, thank you so much! The test seems to be annoying, it might be worth looking into it some other time, but that is definitely orthogonal to this patch. Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1211-1212 -

[PATCH] D87942: [Analyzer] GNU named variadic macros in Plister

2020-09-21 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM! Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87942/new/ https://reviews.llvm.org/D87942 __

[PATCH] D88092: [analyzer][StdLibraryFunctionsChecker] Fix getline/getdelim signatures

2020-09-22 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. A joy of reviewing C++ code is that you get to marvel in all the great things the language has, without having to pull fistfuls of hair out to get get to that point. These patches are al

[PATCH] D88100: [analyzer][StdLibraryFunctionsChecker] Separate the signature from the summaries

2020-09-22 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. Yup, we've talked about this before. This is indeed a better interface design. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88100/n

[PATCH] D87043: [Analyzer] Fix for dereferece of smart pointer after branching on unknown inner pointer

2020-09-25 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Congrats on your GSoC! Unless I missed it, it seems like you haven't posted your final evaluation on cfe-dev, even though its an amazing looking document with a lot of examples and explanations. I think it would be great to spread the message, its a work to be proud o

[PATCH] D88312: "ErrorReturn checker" WIP review

2020-09-28 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:505 + Report->markInteresting(CallSym); Report->addRange(CallLocation); C.emitReport(std::move(Report)); This still causes more harm then good

[PATCH] D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size

2020-08-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I really like the patch, but have nothing to add to what other reviewers already said. Nice! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86295/new/ https://reviews.llvm.org/D86295 _

[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

2020-08-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Do I sense correctly that the only information `CSrtingLengthModeling.cpp` requires from the actual `CStringChecker` is a checker tag? Because if so, I think we should just separate them even more cleanly -- we could just make a `CStringLengthModeling` checker impleme

[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

2020-08-25 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D86135#2233611 , @martong wrote: >> The fundamental problem is, we simply can't ask Preprocessor what a macro >> expands into without hacking really hard. > > Can you summarize what is the exact problem (or give a link to a d

[PATCH] D73376: [analyzer] Add FuchsiaLockChecker and C11LockChecker

2020-08-25 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Herald added subscribers: ASDenysPetrov, martong. Documentation under `clang/docs/analyzer/checkers.rst` seems to be missing. Could we get that fixed before 11.0.0 releases? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D7

[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

2020-08-25 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus planned changes to this revision. Szelethus marked an inline comment as done. Szelethus added a comment. Thanks! I'll get these fixed. Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:901 + + void next(Token &Result) { +if (CurrTokenIt == TokenRange

[PATCH] D86532: (Urgent!) [docs][analyzer] Add documentation for alpha.fuchsia.Lock and alpha.core.C11Lock

2020-08-25 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus created this revision. Szelethus added reviewers: xazax.hun, haowei, NoQ, martong, balazske, steakhal. Szelethus added a project: clang. Herald added subscribers: cfe-commits, ASDenysPetrov, phosek, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, b

[PATCH] D86533: (Urgent!) [release][docs][analyzer] Add 11.0.0. release notes

2020-08-25 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus created this revision. Szelethus added reviewers: hans, NoQ, vsavchenko, dcoughlin, xazax.hun, baloghadamsoftware, martong, balazske, steakhal, Charusso, jkorous, dkrupp, gamesh411. Szelethus added a project: clang. Herald added subscribers: cfe-commits, ASDenysPetrov, phosek, donat.nag

[PATCH] D86532: (Urgent!) [docs][analyzer] Add documentation for alpha.fuchsia.Lock and alpha.core.C11Lock

2020-08-25 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a reviewer: hans. Szelethus added a comment. We will need to push this to the 11.0.0. release branch as well, sorry for the trouble. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86532/new/ https://reviews.llvm.org/D86532

[PATCH] D86533: (Urgent!) [release][docs][analyzer] Add 11.0.0. release notes

2020-08-25 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. @vsavchenko I just realized your work for this release is the following list: git log llvmorg-11-init..llvmorg-11.0.0-rc2 --oneline -- clang/utils/analyzer/ Is it a correct guess that while your primary audience are the analyzer developers, but it wouldn't hurt to m

[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

2020-08-25 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus marked an inline comment as done and an inline comment as not done. Szelethus added a comment. I'll get the nits I didn't object to fixed, thats the status you're looking for :) Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1338-1339 void TokenPrint

[PATCH] D72705: [analyzer] Added new checker 'alpha.unix.ErrorReturn'.

2020-08-26 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus requested changes to this revision. Szelethus added a subscriber: bruntib. Szelethus added a comment. This revision now requires changes to proceed. I debated this

[PATCH] D86533: (Urgent!) [release][docs][analyzer] Add 11.0.0. release notes

2020-08-26 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus updated this revision to Diff 287930. Szelethus marked 14 inline comments as done. Szelethus added a comment. Fixes according to reviewer comments! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86533/new/ https://reviews.llvm.org/D86533 Files: clang/docs/ReleaseNotes.rst

[PATCH] D86533: (Urgent!) [release][docs][analyzer] Add 11.0.0. release notes

2020-08-26 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/docs/ReleaseNotes.rst:471 + +- Improve the pre- and post condition modeling of several hundred more standard + C functions. whisperity wrote: > martong wrote: > > Umm, this is still an alpha command line option,

[PATCH] D86691: [analyzer] Fix wrong parameter name in printFormattedEntry

2020-08-27 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. Nice, thank you! Did you stumble across this, or found it with a tool? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86691/new/ https://re

[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

2020-08-27 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus created this revision. Szelethus added reviewers: NoQ, vsavchenko, dcoughlin, baloghadamsoftware, martong, balazske, xazax.hun, steakhal. Szelethus added a project: clang. Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.s

[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

2020-08-27 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:896-898 +RawLexer.reset(new Lexer(SM.getLocForStartOfFile(LocInfo.first), LangOpts, + MB->getBufferStart(), MacroNameTokenPos, +

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

2020-08-31 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Herald added a subscriber: danielkiss. I'm not thrilled by this solution. As I understand it, the assertion was put there to enforce our ability to create a new assumption, and its great that we had it, because we managed to catch a fault in that. Seems like this patch

[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

2020-08-31 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Herald added a subscriber: danielkiss. In D86736#2244365 , @martong wrote: >> For any other loops, in order to know whether we should analyze another >> iteration, among other things, we evaluate it's condition. Which is a >> p

[PATCH] D85351: [Analyzer] Fix for `ExprEngine::computeObjectUnderConstruction()` for base and delegating consturctor initializers

2020-08-31 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D85351#2247037 , @baloghadamsoftware wrote: > In D85351#2215547 , @Szelethus wrote: > >> Shouldn't we create a new test care for this, instead of expanding an >> existing one? Btw, th

[PATCH] D86533: (Urgent!) [release][docs][analyzer] Add 11.0.0. release notes

2020-09-01 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I'd prefer to have a couple more green lights. In particular, another look on the constraint manager and Objective C stuff would be great. Comment at: clang/docs/ReleaseNotes.rst:487 +- While still in alpha, ``alpha.unix.PthreadLock``, the iterator a

[PATCH] D86873: [analyzer][NFC] Refactor ArrayBoundCheckerV2

2020-09-01 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Before we dive into this too much, if you can point to discussion that explains why we have a 2 versions of the same checker, that would be nice. Why did you chose to work on this one, and not the other? I recall us talking about this in a meeting, but its always grea

[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

2020-09-01 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/ProgramState.cpp:327 +using ObjCForLctxPair = +std::pair; + martong wrote: > Why it is not enough to simply have ObjCForCollectionStmt* as a key? `Environment` is the data structure we

[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

2020-09-01 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D86736#2249920 , @martong wrote: > I don't have anymore immediate concerns, but I will need more time to comb > through the rest of the patch in more details... hopefully I can do that in > the following days. Thank you so

[PATCH] D72705: [analyzer] Added new checker 'alpha.unix.ErrorReturn'.

2020-09-02 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D72705#2241542 , @balazske wrote: > The summary of this last discussion is that it is not acceptable to have only > the simple check for the explicit comparison with a fixed constant. At least > not for return types where th

[PATCH] D87004: [analyzer] Evaluate PredefinedExpressions

2020-09-02 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM! Nice! Comment at: clang/test/Analysis/eval-predefined-exprs.cpp:1 +// RUN: %clang_analyze_cc1 --std=c++17 -analyzer-checker=core,debug.ExprInspection -verify %s

[PATCH] D87081: [analyzer][StdLibraryFunctionsChecker] Elaborate the summary of fread and fwrite

2020-09-07 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. The patch looks great, in fact, it demonstrates how well thought out your summary crafting machinery is. In D87081#2258579 , @martong wrote: > However, in a similar case with the CallAndMessage Checker, we decided to > list th

[PATCH] D85351: [Analyzer] Fix for `ExprEngine::computeObjectUnderConstruction()` for base and delegating consturctor initializers

2020-09-07 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. The tests look great, thanks! I still lack the confidence to accept, unfortunately. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85351/new/ https://reviews.llvm.org/D85351 ___ cfe-commits mailing list cfe-commits@

[PATCH] D84415: [analyzer][StdLibraryFunctionsChecker] Add POSIX pthread handling functions

2020-09-07 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. @balazske seems to be very involved, he might have some closing words -- from my end, LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D76590: [Analyzer] Model `empty()` member function of containers

2020-09-07 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I'm in favor of most, if not all of the changes, though I will admit that this patch seems pretty cluttered, you are doing a lot of refactoring under the same hood. You're moving, adding, removing and changing helper functions and their invocations. Would be possible

[PATCH] D87118: Add an explicit toggle for the static analyzer in clang-tidy

2020-09-07 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a reviewer: NoQ. Szelethus added a comment. Herald added a subscriber: Charusso. NoQ in particular has been working hard on the common infrastructure in between the static analyzer and clang-tidy, I'll add him :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87118/new/

[PATCH] D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer

2020-09-07 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I can only imagine how long it took for you to write all this, because reading it wasn't that short either. Thank you so much! It really gave me a perspective on how you see this problem, as well as what is actually happening (and should happen) in the checker. In D8

[PATCH] D86533: (Urgent!) [release][docs][analyzer] Add 11.0.0. release notes

2020-09-07 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/docs/ReleaseNotes.rst:453 + equal or known to be non-equal. + +- Added :ref:`on-demand parsing ` capability to Cross Translation ASDenysPetrov wrote: > I've added the patch "Reasoning about comparison expressio

[PATCH] D86533: (Urgent!) [release][docs][analyzer] Add 11.0.0. release notes

2020-09-07 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus updated this revision to Diff 290280. Szelethus marked 4 inline comments as done. Szelethus added a comment. Added a line about D78933 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86533/new/ https://reviews.llvm.org/D86533 Files: clang/do

[PATCH] D87240: [analyzer][StdLibraryFunctionsChecker] Have proper weak dependencies

2020-09-07 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. This is exactly how I imagines weak dependencies to work. LGTM on my end. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87240/new/ https:/

[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

2020-09-07 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D86135#2259325 , @steakhal wrote: > Perfectly clear, thank you. However, I would still rely on the others to > accept this :| > > BTW why does the `plist-macros-with-expansion.cpp.plist` change? It makes the > diff somewhat

[PATCH] D83678: [analyzer][ReturnPtrRangeChecker] Fix a false positive on end() iterator

2020-09-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. LGTM on my end! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83678/new/ https://reviews.llvm.org/D83678 ___ cfe-commits mailing list cfe-comm

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

2020-09-10 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. In D79431#2263693 , @martong wrote: > In D79431#2263690 , @martong wrote: > >> What if we'd add a `toStri

[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

2020-09-10 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D86736#2257880 , @NoQ wrote: > i'm very glad that we now consistently use expressions in our Environment. *except for `ReturnStmt`, but I haven't dug myself into that pit just yet :) Thanks! Repository: rG LLVM Github Mo

[PATCH] D72705: [analyzer] Added new checker 'alpha.unix.ErrorReturn'.

2020-09-29 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D72705#2274568 , @balazske wrote: > I am not sure if this checker is worth to further development. A part of the > found bugs can be detected with other checkers too, specially if the > preconditions of many standard functio

[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

2020-10-12 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a subscriber: aaron.ballman. Szelethus added inline comments. Comment at: clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp:307 .Case(FULLNAME, HELPTEXT) #include "clang/StaticAnalyzer/Checkers/Checkers.inc" #undef CHECKER NoQ wrote: > Szele

[PATCH] D89414: clang/StaticAnalyzer: Stop using SourceManager::getBuffer

2020-10-14 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. Sounds good! Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89414/new/ https://reviews.llvm.org/D89414 ___ cfe-commits mailin

[PATCH] D91948: [WIP][analyzer][doc] Add Container- and IteratorModeling developer docs

2020-11-23 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I don't have the time to comb through this doc, unfortunately, but I want to applaud this effort to make the iterator checker family more accessible. Its certainly a forerunner in modeling tricky C++ constructs, and I can't wait to be a more valuable reviewer after be

[PATCH] D89987: [analyzer] [NFC] Rename SymbolRef to SymExprRef

2020-10-27 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D89987#2350649 , @NoQ wrote: > Honestly i'd rather eliminate `SymExpr` and go with `Symbol` everywhere. It's > an overloaded term but appending "Expr" to it doesn't really make it > significantly less overloaded. Why? I thi

[PATCH] D83678: [analyzer][ReturnPtrRangeChecker] Fix a false positive on end() iterator

2020-11-02 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG22e7182002b5: [analyzer][ReturnPtrRangeChecker] Fix a false positive on end() iterator (authored by Szelethus). Changed p

[PATCH] D96163: [analyzer] Add 12.0.0. release notes

2021-05-13 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus closed this revision. Szelethus added a comment. Pushed this to the 12.0.0 release branch, just forgot to close it! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96163/new/ https://reviews.llvm.org/D96163 ___ cfe-commits mailing lis

[PATCH] D102643: [analyzer][Z3][NFC] Use GTEST_SKIP instead of hacks

2021-05-18 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. Cheers! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102643/new/ https://reviews.llvm.org/D102643 ___ cfe-commits mailing list cfe-

[PATCH] D102683: [analyzer] Check the checker name, rather than the ProgramPointTag when silencing a checker

2021-05-18 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. While I still see checker silencing as a solution to a problem that shouldn't exist, I'm unsure about the amount of tedious and invasive work required to make the underlying infrastructure elegant just for the sake of it being elegant. Most notably, checker silencing

[PATCH] D102683: [analyzer] Check the checker name, rather than the ProgramPointTag when silencing a checker

2021-05-19 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG479ea2a8ed95: [analyzer] Check the checker name, rather than the ProgramPointTag when… (authored by Szelethus). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D102835: [analyzer] Correctly propagate ConstructionContextLayer thru ParenExpr

2021-05-20 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. This looks great! Comment at: clang/test/Analysis/NewDelete-checker-test.cpp:41-52 +// RUN: %clang_analyze_cc1 -std=c++17 -fblocks %s \ +// RUN: -verify=expected,newdelete \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=cplusplus.

[PATCH] D102835: [analyzer] Correctly propagate ConstructionContextLayer thru ParenExpr

2021-05-20 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/test/Analysis/NewDelete-checker-test.cpp:54-57 +// RUN: %clang_analyze_cc1 -std=c++17 -fblocks -verify %s \ +// RUN: -verify=expected,leak \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=cplusplus.NewDeleteLe

[PATCH] D102914: [analyzer] Make checker silencing work for non-pathsensitive bug reports

2021-05-21 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus created this revision. Szelethus added reviewers: Charusso, NoQ, steakhal, martong, vsavchenko. Szelethus added a project: clang. Herald added subscribers: manas, ASDenysPetrov, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, w

[PATCH] D102914: [analyzer] Make checker silencing work for non-pathsensitive bug reports

2021-05-21 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus updated this revision to Diff 347003. Szelethus added a comment. Fix the test file. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102914/new/ https://reviews.llvm.org/D102914 Files: clang/lib/StaticAnalyzer/Core/BugReporter.cpp clang/test/Analysis/silence-checkers-malloc.

[PATCH] D96163: [analyzer] Add 12.0.0. release notes

2021-02-05 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus created this revision. Szelethus added reviewers: tstellar, NoQ, vsavchenko, dcoughlin, vrnithinkumar, xazax.hun, baloghadamsoftware, martong, balazske, steakhal, jkorous, dkrupp, gamesh411, ASDenysPetrov, aabbaabb, isthismyaccount. Szelethus added a project: clang. Herald added subscri

[PATCH] D108912: [release][analyzer] Add 13.0.0 release notes

2021-09-23 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus closed this revision. Szelethus added a comment. Commited in rGee6913cc8317c08b603daed64b07a17a95ec926a to `release/13.x`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108912/new/ https://reviews.llvm.org

[PATCH] D96163: [analyzer] Add 12.0.0. release notes

2021-02-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus updated this revision to Diff 322550. Szelethus added a comment. Fix'd! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96163/new/ https://reviews.llvm.org/D96163 Files: clang/docs/ReleaseNotes.rst clang/docs/analyzer/checkers.rst Index: clang/docs/analyzer/checkers.rst =

[PATCH] D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis

2021-02-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. Herald added a subscriber: nullptr.cpp. This is amazing. We longed for a sensible implementation for this for a long time. Really liking the unit tests as well! There are a number of inlines you didn't mark as done but seem to have ad

[PATCH] D93223: [analyzer] Create MacroExpansionContext member in AnalysisConsumer

2021-02-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. Herald added a subscriber: nullptr.cpp. My no1. thought is that I wish the new functionality you're implementing was in the interface of the `Preprocessor`. I found, and still find it so hard to believe that you couldn't just retrieve

[PATCH] D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists

2021-02-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. Herald added a subscriber: nullptr.cpp. Happy to see this mess go. It was obvious even after the first few hurdles that it wouldn't work out in the long term. Comment at: clang/test/Analysis/plist-macros-with-expansi

[PATCH] D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis

2021-02-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/include/clang/Analysis/MacroExpansionContext.h:105 + using MacroExpansionText = SmallString<40>; + using ExpansionMap = llvm::DenseMap; + using ExpansionRangeMap = llvm::DenseMap; Hmm, I'm by no means an exper

<    7   8   9   10   11   12   13   14   15   16   >