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

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 ___ cfe-co

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

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 ___ cfe-c

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

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] D77150: [Analyzer] New Option for ContainerModeling: AggressiveEraseModeling

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] D83120: [Analyzer][StreamChecker] Using BugType::SuppressOnSink at resource leak report.

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) r

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

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) r

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

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 https://lis

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

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] D83226: [analyzer] Add system header simulator a symmetric random access iterator operator+

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] D67381: [analyzer] NFC: Move stack hints to a side map.

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] D67420: [analyzer] NFC: Separate PathDiagnosticConsumer options from AnalyzerOptions.

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 (wh

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

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 for

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

Szelethus added a comment. Herald added subscribers: ASDenysPetrov, martong. In D67422#1667079 , @NoQ wrote: > Casually rename > `ClangDiagPathDiagConsumer` to `TextDiagnostics` according to the lo

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

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 RetT

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

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 for

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

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: clang/test/Analysis/stream.c:2

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

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 cfe-commits@list

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

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 ne

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

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) r

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

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, b

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

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, a.s

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

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

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] D82122: [analyzer][Liveness][RFC][NFC] Propose to turn statement liveness into expression liveness

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] D82256: [analyzer] Enabling ctr in evalCall event

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() << '}'; + ll

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

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 responsi

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

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 do

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

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:309-311 +void ErrorReturnChecker::checkAccess(CheckerContext &C, ProgramStateRef State, + const Stmt *LoadS, SymbolRef CallSym, +

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

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] D81599: [analyzer] SATest: Add 5 more projects for testing

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 project.

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

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, a.s

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

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, balo

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

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 ACTION

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

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 https://reviews.llvm.org/D8

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

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 ma

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

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 thi

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

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 h

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

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 https://reviews.llvm.org/D82598/n

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

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 y

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

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 t

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

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 fo

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

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: clang/include/clang/StaticAnalyzer/Core/CheckerMana

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

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] D81761: [analyzer] Force dependency checkers to be hidden

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: c

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

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 reporter

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

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 different

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus requested changes to this revision. Szelethus added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:152-158 /// Get the value of the stream argument out of the passed call event. /// The c

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:222 {StreamState::FEof, StreamState::FError, StreamState::NoError}}}, - {{"ftell", 1}, {&StreamChecker::preDefault, nullptr, 0, {}}}, + // Note: ftell sets errno on

[PATCH] D77845: [analyzer][CallAndMessage][NFC] Change old callbacks to rely on CallEvent

Szelethus marked an inline comment as done. Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:113-114 const CheckerContext &C) { - assert(Call.getDecl()); + if (!Call.getDecl(

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

Szelethus added a comment. The high level idea seems great after surveying the analyzer for similar issues, but I might need to think about this a bit longer. @baloghadamsoftware, IteratorChecker needs to solve similar problems, right? Do you have any input on this? Repository: rG LLVM Gith

[PATCH] D75430: [analyzer][NFC] Introduce CXXDeallocatorCall, deploy it in MallocChecker

Szelethus added a comment. Ping^2 :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75430/new/ https://reviews.llvm.org/D75430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commit

[PATCH] D78638: [analyzer] Consider array subscripts to be interesting lvalues

Szelethus added a comment. How come rGe20b388e2f923bfc98f63a13fea9fc19aeaec425 doesn't solve this? Or, rather, how come it even worked if this patch is needed? Is the index being a global variable the issue? The change looks

[PATCH] D78638: [analyzer] Consider array subscripts to be interesting lvalues

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. Everything's clear! Nice detective work! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78638/new/ https://reviews.llvm.org/D78638 _

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus added a comment. I gave a lot of thought to the high level logic, but I need some time to really internalize whats happening here. In D78374#1993858 , @balazske wrote: > Finally I had to make the decision to remove the `ErrorKindTy` enum and us

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

Szelethus added a comment. I think you can go ahead and commit. You seem to have a firm grasp on this project, I believe your experience with ASTImporter has more then prepared you for digging functions out of the `std` namespace, or whatever else that might come up :^) Repository: rG LLVM

[PATCH] D75430: [analyzer][NFC] Introduce CXXDeallocatorCall, deploy it in MallocChecker

Szelethus added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75430/new/ https://reviews.llvm.org/D75430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

Szelethus added a comment. In D67422#2192407 , @NoQ wrote: > One bit that's not directly related but i decided to keep it from the > original patch was moving the plist-html diagnostic builder function into its > own file. That is a great call indeed.

[PATCH] D84520: [Analyzer] Improve invalid dereference bug reporting in DereferenceChecker.

Szelethus added a comment. We definitely need more tests. The idea overall however is amazing, thanks! Comment at: clang/test/Analysis/misc-ps-region-store.m:1160 struct list_pr8141 *items; - for (;; items = ({ do { } while (0); items->tail; })) // expected-warning{{Derefe

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

Szelethus added a comment. Would it be possible to publish these results on a public CodeChecker server? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72705/new/ https://reviews.llvm.org/D72705 ___ cfe-c

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

Szelethus accepted this revision. Szelethus added a reviewer: gamesh411. Szelethus added a comment. LGTM on my end, but please wait for approval from either @gamesh411 or @NoQ. In D77150#2191049 , @baloghadamsoftware wrote: > However, I think we should c

[PATCH] D85424: [Analyzer] Crash fix for alpha.cplusplus.IteratorRange

Szelethus added a comment. Yeah, this looks straightforward, but how come you didn't manage to get a small test case? creduce gave up? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85424/new/ https://reviews.llvm.org/D85424 __

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

Szelethus added a comment. In D72705#2199333 , @balazske wrote: > Test results for tmux are available here >

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

Szelethus added a comment. Layering violations are a running theme in the analyzer -- CheckerRegistry and the entire MallocChecker fiasco are two glaring examples. Luckily, this isn't a severe case so I wouldn't worry about it much. I've been follo

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

Szelethus added a comment. The patch looks great. I guess the only remaining discussion remains as to whether this should be in `libAnalysis`, or somewhere else. Here is my take: Since clang-tidy, CSA, and some other parts of the compiler (like uninitialized variable warnings) are static analyz

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

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. Tests c: I'm still not a huge fan of the warning message. Now it describes //what kind// of constraint was violated, but not //how// (too large? too small?). Also, while I res

[PATCH] D84248: [analyzer][StdLibraryFunctionsChecker] Add POSIX time handling functions

Szelethus added reviewers: NoQ, vsavchenko. Szelethus accepted this revision. Szelethus added a comment. Lets make sure we invite the wider community to see whats going on. Otherwise, LGTM! It seems like this patch adds two new additions at once -- infrastructure for when the arg constraint siz

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

Szelethus added a comment. Shouldn't we create a new test care for this, instead of expanding an existing one? Btw, this looks great, but I lack the confidence to accept. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85351/new/ https://reviews.llv

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

Szelethus added a comment. Ah, okay, silly me. Didn't catch that. Then the message is OK for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79431/new/ https://reviews.llvm.org/D79431 ___ cfe-commits

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

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: > This thing still worries me but this definitely is

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

Szelethus accepted this revision. Szelethus added a comment. Unless anyone else objects, I'm happy to see this landed! Nice work! ^-^ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67422/new/ https://reviews.llvm.org/D67422 ___ cfe-commits mai

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

Szelethus added a comment. In D67422#2216137 , @NoQ wrote: > I agree that there's something here and also that it's not that > urgent/critical to rename things at this point. It's not that people suffer > horribly from having to link to only some of thes

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

Szelethus created this revision. Szelethus added reviewers: NoQ, vsavchenko, xazax.hun, martong, balazske, baloghadamsoftware, gamesh411. Szelethus added a project: clang. Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnk

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

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1226 +TStream.injextRange( +const_cast(PrevParamMap)[__VA_ARGS__II]); +TStream.next(TheTok); Oh, this has to be fixed as well.

[PATCH] D79358: [analyzer] CERT: STR37-C

Szelethus added a reviewer: martong. Szelethus added a comment. Herald added a subscriber: rnkovacs. Adding @martong, because I fear that this is colliding with StdLibraryFunctionsChecker. The warnings added here seem to be, in essence, identical to D73898 . Re

[PATCH] D79415: [analyzer][MalloChecker] When modeling realloc-like functions, don't early return if the argument is symbolic

Szelethus created this revision. Szelethus added reviewers: NoQ, balazske, martong, baloghadamsoftware, vsavchenko, xazax.hun, dcoughlin. Szelethus added a project: clang. Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.s

[PATCH] D79420: [analyzer] Make NonNullParamChecker as dependency for StdCLibraryFunctionsChecker

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. I'm a bit unsure about the naming, because it's not technically true that `NonNullParamChecker` is a dependency of `StdCLibraryFunctionsChecker`. The actual relationship is different, be

[PATCH] D79358: [analyzer] CERT: STR37-C

Szelethus added a comment. I'm afraid so. The patch otherwise looked really clean, sorry to ruin the day! :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79358/new/ https://reviews.llvm.org/D79358 __

[PATCH] D77148: [analyzer] ApiModeling: Add buffer size arg constraint with multiplier involved

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1041 + EvalCallAsPure) + .ArgConstra

[PATCH] D78118: [analyzer] StdLibraryFunctionsChecker: Add option to display loaded summaries

Szelethus added a comment. It would be nice to have a user-facing option that lists the functions this checker is **capable** of modeling in another patch :) Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:299 + "DisplayLoadedSummaries", +

[PATCH] D79432: [analyzer] StdLibraryFunctionsChecker: Add summaries for libc

Szelethus added a comment. There a couple functional lines commented out, and we need more tests, so I take that the patch is a proof of concept for now? Because its pretty cool if so. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79432/new/ h

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

Szelethus added a comment. Sure, this is an improvement because we display more information, but I'd argue that it isn't really a more readable warning message :) How about th argument to the call to - cannot be represented with a character - is a null pointer - ... , which violates the funct

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478 + + // FIXME: Check for stream in EOF state? + balazske wrote: > Szelethus wrote: > > Will that be the next patch? :D Eager to see it! > It may be too big change

[PATCH] D77066: [analyzer] ApiModeling: Add buffer size arg constraint

Szelethus added a comment. I'm not familiar enough with `DynamicSize.cpp` to judge the changes there, but aside from a few nits, this LGTM. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:249-250 + // cannot apply the constraint. Actually, ot

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478 + + // FIXME: Check for stream in EOF state? + balazske wrote: > balazske wrote: > > Szelethus wrote: > > > balazske wrote: > > > > Szelethus wrote: > > > > > Wi

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

Szelethus updated this revision to Diff 262387. Szelethus added a comment. Make the solution handle dependencies from plugins as well via `CheckerRegistry`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78126/new/ https://reviews.llvm.org/D78126 Files: clang/include/clang/StaticAnal

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

Szelethus marked 3 inline comments as done. Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2126-2130 + + const CheckerRegistry &Registry = ErrorNode->getState() +->getAnalysisManager() +

[PATCH] D79072: [Analyzer][VLASizeChecker] Check VLA size in typedef and sizeof.

Szelethus added a comment. We check now whether the argument of `typedef` and `sizeof` is an incorrect VLA, but what other examples are we potentially forgetting? The warning message states that "Declared variable-length array (VLA) has negative size", but we are not actually looking for declar

[PATCH] D75430: [analyzer][NFC] Introduce CXXDeallocatorCall, deploy it in MallocChecker

Szelethus marked 5 inline comments as done. Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:59 enum CallEventKind { + CE_CXXDeallocator, CE_Function, xazax.hun wrote: > Szelethus wrote: > > I nee

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus added a comment. > Currently, parameters of functions without their definition present cannot be > represented as regions because it would be difficult to ensure that the same > declaration is used in every case. To overcome this, we introduce a new kind > of region called ParamRegion

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus added a comment. In D79704#2029305 , @baloghadamsoftware wrote: > Thank you for quickly looking into this. > > In D79704#2029230 , @Szelethus wrote: > > > - What identifies a `MemRegion`, `TypedValueRegio

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus added a comment. In D79704#2032271 , @NoQ wrote: > Blanket reply! `ParamRegion` is not a `DeclRegion` because it does not > necessarily have a corresponding `Decl`. For instance, when calling a > function through an unknown function pointer, yo

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. I'm sorry for the late review -- please know that this isn't the first time me taking a look, this is a complex issue. I find navigating your phabricator comments a bit difficu

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. Yeah, this patch should definitely have unit tests. All similar patches should. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D7970

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:435-442 + const VarDecl *VD; + if (const auto *VR = + dyn_cast(cast(Sym)->getRegion())) { +VD = cast(VR->getDecl()); + } else if (const auto *

[PATCH] D79330: [Analyzer][VLASizeChecker] Check for VLA size overflow.

Szelethus added inline comments. Comment at: clang/test/Analysis/vla-overflow.c:10 +// Size of this array should be the first to overflow. +size_t s = sizeof(char[x][x][x][x]); // expected-warning{{Declared variable-length array (VLA) has too large size}} +return s;

[PATCH] D79072: [Analyzer][VLASizeChecker] Check VLA size in typedef and sizeof.

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. In D79072#2025120 , @martong wrote: > I'd split this patch into two as well. > > 1. [NFC] Refactoring parts > 2. The actual extra additions about

[PATCH] D79330: [Analyzer][VLASizeChecker] Check for VLA size overflow.

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. > Variable-length array (VLA) should have a size that fits into a size_t value. > At least if the size is queried with sizeof, but it is better (and more > simple) to check it

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus added a comment. In D78374#2033826 , @balazske wrote: > I think this is a expected way of how it works, failure of a stream operation > does not necessarily depend on result of a previous operation. So any > operation can fail or not, independe

[PATCH] D78120: [analyzer][StreamChecker] Don't make StreamTestChecker depend on StreamChecker for the time being

Szelethus added a comment. In D78120#1983756 , @balazske wrote: > LGTM > But a better approach can be to make a new kind of dependency. (Or split the > StreamChecker.) Definitely the latter, I just didn't wanna mess with your project :) But I'd be hap

[PATCH] D78120: [analyzer][StreamChecker] Don't make StreamTestChecker depend on StreamChecker for the time being

Szelethus added a comment. In D78120#2034455 , @Szelethus wrote: > In D78120#1983756 , @balazske wrote: > > > LGTM > > But a better approach can be to make a new kind of dependency. (Or split > > the StreamChecker

<    5   6   7   8   9   10   11   12   13   14   >