[PATCH] D77150: [Analyzer] New Option for ContainerModeling: AggressiveEraseModeling

2020-07-28 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. Let me double down -- just because we let some select people know about an option, I still don't think it should be user facing. I'm strongly against the philosophy that a

[PATCH] D84494: [Analyzer] Use of BugType in DereferenceChecker (NFC).

2020-07-28 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added reviewers: NoQ, vsavchenko. Szelethus accepted this revision. Szelethus added a comment. Lets make sure that we invite others from the community to participate, here and on other patches as well. Otherwise, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

2020-07-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D82598#2172416 , @NoQ wrote: > I still wonder what made this statement live in my example. There must have > been some non-trivial liveness analysis going on that caused a statement to > be live; probably something to do

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

2020-07-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Wow, I never realized I accidentally landed that assert (D82122#2172360 ), but I guess its great to have that covered. Would you prefer to have that reverted as I'm looking to fix this for good? Repository: rG LLVM Github

[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

2020-07-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. This was accidentally committed as a part of rGb6cbe6cb0399d4671e5384dcc326af56bc6bd122 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82122/new/

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

2020-07-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Thank you so much, you really went out of your way to dig that out! I'll try to patch this somehow. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82598/new/ https://reviews.llvm.org/D82598

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

2020-07-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D84316#2171270 , @NoQ wrote: > Imagine something like re-using the state trait implementation between > `MallocChecker` and `StreamChecker` because they both model "resources that > can be deallocated twice or leaked" -

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

2020-07-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I personally preferred the previous diff, the reporting part of the checker and the string length modeling was separated far more cleanly. In D84316#2171267 , @NoQ wrote: > Mmm, none of these benefits sound like they outweigh

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

2020-07-23 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. You could do it in the code, but if the modeling wouldn't be present from CStringModeling, CStringChecker wouldn't work properly. So you should make it a strong dependency, just as you did in this patch. My comment was mainly a response to @NoQ :)

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

2020-07-23 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus marked an inline comment as done. Szelethus added a comment. Yay! This checker has become a major headache as the analyzer grew. Not on a RetainCount scale, but on a MallocChecker one. Cheap shots, I know :) The patch looks great! I have some miscellaneous comments, but nothing

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

2020-07-22 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Ouch. Let me know how severe this is, because this is a big milestone in my project. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82598/new/ https://reviews.llvm.org/D82598

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

2020-07-21 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus marked an inline comment as done. Szelethus added inline comments. Comment at: clang/lib/Analysis/LiveVariables.cpp:317-320 for (Stmt *Child : S->children()) { -if (Child) - AddLiveStmt(val.liveStmts, LV.SSetFact, Child); +if (const auto *E =

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

2020-07-21 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus marked an inline comment as done. Szelethus added a comment. In D82598#2162496 , @xazax.hun wrote: > In D82598#2162239 , @Szelethus wrote: > > > I chased my own tail for weeks before realizing that there

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

2020-07-20 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I chased my own tail for weeks before realizing that there is indeed another instance when a live **statement** is stored, other then `ObjCForCollectionStmt`... void clang_analyzer_eval(bool); void test_lambda_refcapture() { int a = 6; [&](int ) { a =

[PATCH] D83961: [Analyzer] Fix bug report source locations in minimal output.

2020-07-20 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I find the summary here a bit lacking. We detected this issue in D83120 , where a lot of discussion can be found, so its worth linking in. On its own, I'm not immediately sold on whether this is the correct solution, and even if it

[PATCH] D83407: [analyzer][StdLibraryFunctionsChecker] Add POSIX networking functions

2020-07-20 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! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83407/new/ https://reviews.llvm.org/D83407

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

2020-07-18 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I hope you don't mind if I bring up a point you mentioned during a daily meeting. > The error checking shouldn't be present on one of execution, but rather on > all of them. I think your recent comment highlights why that wouldn't be sufficient, unfortunately.

[PATCH] D83942: [analyzer][tests] Add a notion of project sizes

2020-07-16 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I suppose LLVM could be a HUGE project? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83942/new/ https://reviews.llvm.org/D83942 ___ cfe-commits mailing list

[PATCH] D83942: [analyzer][tests] Add a notion of project sizes

2020-07-16 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I don't speak snek, but I approve this message! Its also great that we have dedicated tiny/small projects to analyze locally. Comment at: clang/utils/analyzer/ProjectMap.py:31 +SMALL: 1min-10min +BIG: 10min-1h +HUGE: >1h

[PATCH] D70411: [analyzer] CERT STR rule checkers: STR31-C

2020-07-15 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Please do not bypass the previous comments that hadn't reached a conclusion -- littering inlines about miscellaneous stuff at this stage does more harm then good, and derails the discussion. Its not the first time I took a look, and I made some progress today again,

[PATCH] D83120: [Analyzer][StreamChecker] Use BugType::SuppressOnSink at resource leak report.

2020-07-15 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/test/Analysis/stream.c:274-284 // Check that "location uniqueing" works. // This results in reporting only one occurence of resource leak for a stream. void check_leak_noreturn_2() { FILE *F1 = tmpfile(); if (!F1)

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

2020-07-15 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. While this patch spans across a lot of files, it is actually rather straightforward. I'm stunned to see that we never really had a proper dynamic size support, especially that this patch has been out there for a good long time :) Oh well, better learn that now than

[PATCH] D83120: [Analyzer][StreamChecker] Use BugType::SuppressOnSink at resource leak report.

2020-07-15 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. Now that we found the answer to the only lingering question this revision raised, I think you can safely land it while we start looking into fixing this newfound bug. LGTM. Comment at:

[PATCH] D83120: [Analyzer][StreamChecker] Use BugType::SuppressOnSink at resource leak report.

2020-07-15 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Unless @NoQ has anything else to add :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83120/new/ https://reviews.llvm.org/D83120 ___ cfe-commits mailing list

[PATCH] D83717: [clang-tidy] Add check fo SEI CERT item ENV32-C

2020-07-14 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added reviewers: Szelethus, martong, steakhal, dkrupp. Szelethus added a comment. @Eugene.Zelenko Thanks for cleaning up revisions -- same as D69560#1725399 , we are working in the same office and have worked on some forms of static analysis

[PATCH] D83407: [analyzer][StdLibraryFunctionsChecker] Add POSIX networking functions

2020-07-14 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus marked an inline comment as done. Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:328-329 struct Signature { -const ArgTypes ArgTys; -const QualType RetTy; +ArgTypes ArgTys; +QualType

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

2020-07-14 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Herald added subscribers: ASDenysPetrov, martong. In D67422#1667079 , @NoQ wrote: > Casually rename > `ClangDiagPathDiagConsumer` to `TextDiagnostics` according to the

[PATCH] D67421: [analyzer] NFC: Move IssueHash to libAnalysis.

2020-07-14 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Herald added subscribers: ASDenysPetrov, martong. Comment at: clang/include/clang/Analysis/IssueHash.h:35 +/// /// In case a new hash is introduced, the old one should still be maintained for /// a while. One should not introduce a new hash

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

2020-07-14 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus marked 2 inline comments as done. Szelethus added a comment. This revision is now accepted and ready to land. LGTM! Very well done! In D67420#2149461 , @vsavchenko wrote: > I strongly believe that decoupling

[PATCH] D67381: [analyzer] NFC: Move stack hints to a side map.

2020-07-14 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Herald added subscribers: ASDenysPetrov, martong. Aren't `StackHint`s basically ancient `NoteTag`s? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67381/new/ https://reviews.llvm.org/D67381

[PATCH] D83226: [analyzer] Add system header simulator a symmetric random access iterator operator+

2020-07-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. Yay! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83226/new/ https://reviews.llvm.org/D83226

[PATCH] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

2020-07-14 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I've seen you resurrecting the thread, I'll just take a bit of time to remember what happened in the previous episodes! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67420/new/ https://reviews.llvm.org/D67420

[PATCH] D83701: [analyzer][tests] Add 5 more projects for testing

2020-07-14 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Post-commit LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83701/new/ https://reviews.llvm.org/D83701 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D83120: [Analyzer][StreamChecker] Using BugType::SuppressOnSink at resource leak report.

2020-07-13 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/test/Analysis/stream.c:274-284 // Check that "location uniqueing" works. // This results in reporting only one occurence of resource leak for a stream. void check_leak_noreturn_2() { FILE *F1 = tmpfile(); if (!F1)

[PATCH] D83120: [Analyzer][StreamChecker] Using BugType::SuppressOnSink at resource leak report.

2020-07-13 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/test/Analysis/stream.c:274-284 // Check that "location uniqueing" works. // This results in reporting only one occurence of resource leak for a stream. void check_leak_noreturn_2() { FILE *F1 = tmpfile(); if (!F1)

[PATCH] D77150: [Analyzer] New Option for ContainerModeling: AggressiveEraseModeling

2020-07-13 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. There is a parallel discussion in this patch on how we should cater to user requests, I wrote a lengthy comment about my opinion, but I just don't think it adds much -- at the end of the day, it is fair that if we implement an feature for a small subset of users that

[PATCH] D83407: [analyzer][StdLibraryFunctionsChecker] Add POSIX networking functions

2020-07-10 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus marked an inline comment as done. Szelethus added a comment. I'm yet to go over line-by-line, but the overall logic looks great. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:328-329 struct Signature { -const ArgTypes ArgTys; -

[PATCH] D78280: [Analyzer][StreamChecker] Track streams that were not found to be opened.

2020-07-10 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Its a bit hard to judge this. Have you tested this on open source projects? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78280/new/ https://reviews.llvm.org/D78280 ___

[PATCH] D81061: [Analyzer][VLASizeChecker] Fix problem with zero index assumption.

2020-07-10 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I don't have much to say here, this goes a bit outside my expertise. @NoQ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81061/new/ https://reviews.llvm.org/D81061 ___

[PATCH] D83115: [Analyzer] Report every bug if only uniqueing location differs.

2020-07-10 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added reviewers: NoQ, vsavchenko, xazax.hun, martong. Szelethus added a subscriber: NoQ. Szelethus added inline comments. Comment at: clang/lib/Analysis/PathDiagnostic.cpp:1136-1137 ID.Add(getLocation()); + ID.Add(getUniqueingLoc()); +

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

2020-07-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. I'd prefer if you moved `f_leak_2` to `stream-notes.c`. Otherwise, LGTM. Comment at: clang/test/Analysis/stream.c:147-150 + FILE *p1 = fopen("foo1.c", "r"); + if

[PATCH] D83120: [Analyzer][StreamChecker] Using BugType::SuppressOnSink at resource leak report.

2020-07-10 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a reviewer: NoQ. Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM! Though, the test file change is interesting. You could add a test that behaves differently from the previous implementation: { FILE *f =

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

2020-07-10 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D72705#2143405 , @balazske wrote: > We must check on every execution path that a specific condition on a value is > satisfied (or: find one where the condition is not satisfied). This would be > the most simple form of this

[PATCH] D83407: [analyzer][StdLibraryFunctionsChecker] Add POSIX networking functions

2020-07-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1735-1747 + if (!addToFunctionSummaryMap( + "accept", Summary(ArgTypes{IntTy, *StructSockaddrPtrRestrictTy, +

[PATCH] D83295: [Analyzer] Hotfix for various crashes in iterator checkers

2020-07-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. LGTM, but if we knowingly a subpar solution, we should make that clear in the surrounding code. I know the followup patch is around the corner, but just to be sure. Comment at:

[PATCH] D83407: [analyzer][StdLibraryFunctionsChecker] Add POSIX networking functions

2020-07-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1735-1747 + if (!addToFunctionSummaryMap( + "accept", Summary(ArgTypes{IntTy, *StructSockaddrPtrRestrictTy, +

[PATCH] D83475: [analyzer] Add CTUImportCppThreshold for C++ files

2020-07-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I don't like this solution t much, but I respect the urgency, and since we don't introduce a compatibility issue, I trust we can remove this if we decide to change to TableGen. Post-commit LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D83475: [analyzer] Add CTUImportCppThreshold for C++ files

2020-07-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:48-50 #ifndef ANALYZER_OPTION_DEPENDS_ON_USER_MODE /// Create a new analyzer option, but dont generate a method for it in /// AnalyzerOptions. It's value depends on the

[PATCH] D83295: [Analyzer] Hotfix for various crashes in iterator checkers

2020-07-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:275-276 } else if (isRandomIncrOrDecrOperator(OK)) { +if (!BO->getRHS()->getType()->isIntegralOrEnumerationType()) + return; handlePtrIncrOrDecr(C, BO->getLHS(),

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

2020-07-09 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. After reading @martong's comment D72705#2035844 and @balazske's comment D72705#2086994 , I think there is a need to argue across all paths of execution within the function, and as such

[PATCH] D81750: [analyzer] Don't allow hidden checkers to emit diagnostics

2020-07-07 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rGcfd6b4b811aa: [analyzer] Dont allow hidden checkers to emit diagnostics (authored by Szelethus). Changed prior to commit: https://reviews.llvm.org/D81750?vs=270447=275607#toc Repository: rG LLVM

[PATCH] D78126: [analyzer][NFC] Don't allow dependency checkers to emit diagnostics

2020-07-07 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rGb2956076976c: [analyzer][NFC] Dont allow dependency checkers to emit diagnostics (authored by Szelethus). Changed prior to commit: https://reviews.llvm.org/D78126?vs=270372=275605#toc Repository: rG

[PATCH] D81761: [analyzer] Force dependency checkers to be hidden

2020-07-07 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG690ff37a2869: [analyzer] Force dependency checkers to be hidden (authored by Szelethus). Changed prior to commit: https://reviews.llvm.org/D81761?vs=274466=275598#toc Repository: rG LLVM Github

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-06 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D82585#2133260 , @teemperor wrote: > Fixed here to get the bot running again: > https://github.com/llvm/llvm-project/commit/7308e1432624f02d4e652ffa70e40d0eaa89fdb3 Thank you so much! Kind of ironic considering that this

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-06 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I'm on it! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82585/new/ https://reviews.llvm.org/D82585 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D81750: [analyzer] Don't allow hidden checkers to emit diagnostics

2020-07-06 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rGcfd6b4b811aa: [analyzer] Dont allow hidden checkers to emit diagnostics (authored by Szelethus). Changed prior to commit: https://reviews.llvm.org/D81750?vs=270447=275697#toc Repository: rG LLVM

[PATCH] D78126: [analyzer][NFC] Don't allow dependency checkers to emit diagnostics

2020-07-06 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rGb2956076976c: [analyzer][NFC] Dont allow dependency checkers to emit diagnostics (authored by Szelethus). Changed prior to commit: https://reviews.llvm.org/D78126?vs=270372=275687#toc Repository: rG

[PATCH] D81761: [analyzer] Force dependency checkers to be hidden

2020-07-06 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG690ff37a2869: [analyzer] Force dependency checkers to be hidden (authored by Szelethus). Changed prior to commit: https://reviews.llvm.org/D81761?vs=274466=275651#toc Repository: rG LLVM Github

[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

2020-07-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. LGTM! You packed a lot of punch into this patch, and I like it very much. I read the code and everything looks great. I nitpicked on one thing, the `updateTrackedRegion` function is a

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-04 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rGb6cbe6cb0399: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core… (authored by Szelethus). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-03 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h:216 + /// This output is not intended to be machine-parseable. + void printCheckerWithDescList(const AnalyzerOptions , raw_ostream , +

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-07-03 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus updated this revision to Diff 275378. Szelethus marked 6 inline comments as done. Szelethus added a comment. Small fixes according to reviewer comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82585/new/ https://reviews.llvm.org/D82585 Files:

[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

2020-07-02 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I only looked at the checker naming issue, and that seems to have been resolved perfectly, thanks! :) Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:582 + "false", + InAlpha>, + ]>,

[PATCH] D82288: [analyzer][StdLibraryFunctionsChecker] Add POSIX file handling functions

2020-07-01 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. The changes proposed in mailing list thread (http://lists.llvm.org/pipermail/cfe-dev/2020-June/066017.html) seem ortogonal to this change. Sorry for the slack today, I just peeked around in MallocChecker to look for conflicts, but it

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

2020-06-30 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D82845#2123283 , @baloghadamsoftware wrote: > In D82845#2122984 , @balazske wrote: > > > I checked it with simple debug print, the `LeakedSyms` will contain 2 items > > with

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

2020-06-30 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D82845#2122984 , @balazske wrote: > I checked it with simple debug print, the `LeakedSyms` will contain 2 items > with different `StreamOpenNode`. So I think these should be independent > problem reports for the bug

[PATCH] D81761: [analyzer] Force dependency checkers to be hidden

2020-06-30 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus updated this revision to Diff 274466. Szelethus added a comment. Revert changes to plugins and to the related tests, allow the `example` package to bypass the new restriction. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81761/new/ https://reviews.llvm.org/D81761 Files:

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-06-30 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus marked 6 inline comments as done. Szelethus added a comment. In D82585#2122634 , @balazske wrote: > It looks like that the dependency manipulation and computation functions I guess there are two ways of looking at this: - CheckerRegistry only

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-06-30 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus updated this revision to Diff 274445. Szelethus added a comment. Fixes according to reviewer comments! edit: from @gamesh411. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82585/new/ https://reviews.llvm.org/D82585 Files:

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

2020-06-30 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus marked 2 inline comments as done. Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h:83 + +using CmdLineOptionList = llvm::SmallVector; + gamesh411 wrote: > Could you please explain why use zero

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

2020-06-30 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D82845#2121940 , @balazske wrote: > Why does this not work? > I get only warning for the first resource leak (in the test `f_leak_2`). How > to fix this? First off, are two errors detected by the checker? You could check

[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

2020-06-29 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:577-583 + CheckerOptions<[ +CmdLineOption, + ]>, NoQ wrote: > Szelethus wrote: > > This goes against D81750 -- Sorry for not bringing this up earlier, but

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

2020-06-29 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Note to self: rename `debug.DumpLiveStmts` to `debug.DumpLiveExprs`. Thanks for the review btw! I don't immediately have something to add to your discussion though :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D82741: [Analyzer][StreamChecker] Use BugType instead of BuiltinBug (NFC) .

2020-06-29 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. I just checked `BuiltinBug` -- why do we even have this??? Anyways, LGTM. We should probably delete the whole thing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

2020-06-29 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. I have a few pending patches that enforce some long desired restrictions on which checkers can emit diagnostics, and I'd prefer not to mess with your checker after you land

[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

2020-06-25 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus abandoned this revision. Szelethus added a comment. This revision has done its job. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82122/new/ https://reviews.llvm.org/D82122 ___ cfe-commits

[PATCH] D82561: [analyzer][CrossTU] Lower CTUImportThreshold default value

2020-06-25 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. Based on discussions I heard in between @dkrupp, @martong and You, this seems appropriate. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

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

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

2020-06-25 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I didn't run this on big projects just yet, and judging from the fact that `Environment` only contained ObjetiveC examples of non-expression statements, I might need a tip on how to test on ObjC code :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D82585: [analyzer][NFC] Move the data structures from CheckerRegistry to the Core library

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

[PATCH] D81599: [analyzer] SATest: Add 5 more projects for testing

2020-06-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. AFAIK CppCheck has little to no dependencies, you just need to clone and compile it, that might also be worth a shot. Rtags, tmux, capnproto are also relatively painless, and are also quick to analyze. Xerces also has very few dependencies, and is a mid-sized

[PATCH] D82288: [analyzer][StdLibraryFunctionsChecker] Add POSIX file handling functions

2020-06-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I went through this a lot more thoroughly, as well as the previous patch, and this looks great, especially for an alpha option. I will admit, I'm a bit concerned by the lack of individual tests (what if a stupid interaction with another checker causes issues?), but I

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

2020-06-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:309-311 +void ErrorReturnChecker::checkAccess(CheckerContext , ProgramStateRef State, + const Stmt *LoadS, SymbolRef CallSym, +

[PATCH] D82385: [Analyzer] Fix errors in iterator modeling

2020-06-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:530-532 const auto *Pos = getIteratorPosition(State, LHS); if (!Pos) return; I fear this might be a stupid question, but what's up with `5 + it`? Why

[PATCH] D82288: [analyzer][StdLibraryFunctionsChecker] Add POSIX file handling functions

2020-06-24 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I see a lot of `NoEvalCall`, but I wonder whether modifying `errno` warrants this. Shouldn't we have a alongside `NoEvalCall` and `EvalCallAsPure` an `EvalCallAsPureButInvalidateErrno` invalidation kind? Also, I'm kind of worried by this checker taking on the

[PATCH] D82256: [analyzer] Enabling ctr in evalCall event

2020-06-21 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp:130 +llvm::errs() << " (" << ND->getQualifiedNameAsString() << ')'; + llvm::errs() << " {" << Call.getNumArgs() << '}'; +

[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

2020-06-20 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Cheers! I'll leave this here for conversation's sake, and start working on refactoring LivenessAnalysis. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82122/new/ https://reviews.llvm.org/D82122

[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

2020-06-20 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus marked an inline comment as done. Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/Environment.cpp:193 + +assert((isa(BlkExpr.getStmt()) || !IsBlkExprLive) && + "Only Exprs can be live, LivenessAnalysis argues about the liveness

[PATCH] D81407: [Analyzer][StreamChecker] Add note tags for file opening.

2020-06-19 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I see where you're coming from @NoQ. What do you think, @balazske? I think there is is still value in this implementation as a //debug// option to gather data, so that we don't invest a lot of time creating a robust infrastructure for an idea that might not work out.

[PATCH] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

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

[PATCH] D81407: [Analyzer][StreamChecker] Add note tags for file opening.

2020-06-18 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. Balázs, could you please add the checker option within this patch? If we find that the option works well (removes a lot of useless reports) I'd be happy to help implement that uniqueing pass. CmdLineOption, Comment at:

[PATCH] D81407: [Analyzer][StreamChecker] Add note tags for file opening.

2020-06-17 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus marked 2 inline comments as done. Szelethus added a comment. This revision is now accepted and ready to land. Yay! Getting so close to enabling this by default. I'm a big fan of your work on this checker. Comment at:

[PATCH] D81761: [analyzer] Force dependency checkers to be hidden

2020-06-17 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. In D81761#2097561 , @balazske wrote: > My observation is, if there is an example checker, it should be really > "example" and not "test". (The "custom" is probably good to rename to "test" > but not the "example".) (The names

[PATCH] D81745: [analyzer][MallocChecker] PR46253: Correctly recognize standard realloc

2020-06-16 Thread Kristóf Umann via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG1614e3540827: [analyzer][MallocChecker] PR46253: Correctly recognize standard realloc (authored by Szelethus). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D81407: [Analyzer][StreamChecker] Add note tags for file opening.

2020-06-14 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. I'd still like to see more `NoteTag`s such as "File read failed, end-of-file indicator set on 'F'", and a final evaluation would be nice, but otherwise this checker looks amazing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D78126: [analyzer][NFC] Don't allow dependency checkers to emit diagnostics

2020-06-12 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus reopened this revision. Szelethus added a comment. This revision is now accepted and ready to land. Reverted in rGe64059828f19f629081220bffeb3ef7582870111 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D81761: [analyzer] Force dependency checkers to be hidden

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

[PATCH] D81752: Revert "[analyzer][NFC] Don't allow dependency checkers to emit diagnostics"

2020-06-12 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus accepted this revision. Szelethus added a comment. Yup, I agree. Detailed my answer here a bit: D78126#inline-751477 . Thanks for taking initiative! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D78126: [analyzer][NFC] Don't allow dependency checkers to emit diagnostics

2020-06-12 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus marked an inline comment as done. Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:43 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h" +#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h" #include

[PATCH] D81750: [analyzer] Don't allow hidden checkers to emit diagnostics

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

  1   2   3   4   5   6   7   8   9   10   >