[PATCH] D76830: [Analyzer][MallocChecker] No warning for kfree of ZERO_SIZE_PTR.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1687 + if (ArgValKnown) { +if (!KernelZeroSizePtrValue) + KernelZeroSizePtrValue = balazske wrote: > Szelethus wrote: > > Szelethus wrote: > > > martong wro

[PATCH] D76917: [analyzer][MallocChecker] Fix that kfree only takes a single argument

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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

Szelethus added a comment. In D75682#1940901 , @balazske wrote: > So what is missing or wrong in this change to be accepted? This change is functional but is not testable. But the high level idea is great! I think we should maybe merge this with the fol

[PATCH] D76830: [Analyzer][MallocChecker] No warning for kfree of ZERO_SIZE_PTR.

Szelethus added inline comments. Comment at: clang/test/Analysis/kmalloc-linux-1.c:15 + +// FIXME: malloc checker expects kfree with 2 arguments, is this correct? +// (recent kernel source code contains a `kfree(const void *)`) balazske wrote: > Szelethus wrote:

[PATCH] D76830: [Analyzer][MallocChecker] No warning for kfree of ZERO_SIZE_PTR.

Szelethus accepted this revision. Szelethus marked 2 inline comments as done. Szelethus added a comment. LGTM! Mind that I just published D76917 , you can, if you prefer, rebase on top of that. Also, a test case for `delete`ing a `ZERO_SIZE_PTR` valued pointer mi

[PATCH] D63279: [Analyzer] Unroll for-loops where the upper boundary is a variable with know value

Szelethus added a comment. In D63279#1939435 , @xazax.hun wrote: > In D63279#1939349 , @Szelethus wrote: > > > (note: I forgot to submit this a couple weeks ago) > > > > LLVM is crashing on me due to the issue menti

[PATCH] D76790: [analyzer] StdLibraryFunctionsChecker: fix bug with arg constraints

Szelethus added a comment. The fix looks great! In D76790#1941857 , @martong wrote: > I was thinking about the below test, but then I reverted back because I don't > want to add "fake" summaries for testing purposes. Perhaps adding a new > checker optio

[PATCH] D76917: [analyzer][MallocChecker] Fix that kfree only takes a single argument

This revision was automatically updated to reflect the committed changes. Closed by commit rG30a8b77080b9: [analyzer][MallocChecker] Fix that kfree only takes a single argument (authored by Szelethus). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76

[PATCH] D76605: [analyzer] Display the checker name in the text output

Szelethus marked an inline comment as done. Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:313-316 +ANALYZER_OPTION(bool, ShouldDisplayCheckerNameForText, "display-checker-name", +"Display the checker name

[PATCH] D75271: [analyzer][NFC] Change LangOptions to CheckerManager in the shouldRegister* functions.

This revision was automatically updated to reflect the committed changes. Closed by commit rGbda3dd0d986b: [analyzer][NFC] Change LangOptions to CheckerManager in the shouldRegister*… (authored by Szelethus). Herald added a subscriber: ASDenysPetrov. Changed prior to commit: https://reviews.llv

[PATCH] D76830: [Analyzer][MallocChecker] No warning for kfree of ZERO_SIZE_PTR.

Szelethus accepted this revision. Szelethus added a comment. LGTM, thanks! Comment at: clang/test/Analysis/kmalloc-linux-1.c:1 +// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=unix.Malloc -verify %s + Oh, I almost forgot, the `core` p

[PATCH] D77012: [analyzer] Fix StdLibraryFunctionsChecker NotNull Constraint Check

Szelethus added a comment. In D77012#1948550 , @NoQ wrote: > @Szelethus can we make this checker depend on undefined value checker > (probably CallAndMessage) so that uninitialized arguments were handled first? I'll drop some pointers to previous disc

[PATCH] D77012: [analyzer] Fix StdLibraryFunctionsChecker NotNull Constraint Check

Szelethus accepted this revision. Szelethus added a comment. Oh, yea, LGTM! Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77012/new/ https://reviews.llvm.org/D77012 ___ cfe-commits mailing list

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

This revision was automatically updated to reflect the committed changes. Szelethus marked 5 inline comments as done. Closed by commit rG703a1b8caf09: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap (authored by ldionne, committed by Szelethus). Herald added a sub

[PATCH] D77061: [analyzer] Add core.CallAndMessage to StdCLibraryFunctionArgsChecker's dependency

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM! It would be great if we could separate the modeling portions of the `CallAndMessage` checker out, but hey, there are only so many hours in each day :^) Repository: rG LLVM Gith

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

Szelethus added a comment. In D68165#1949954 , @ldionne wrote: > > Closed by commit rG703a1b8caf09 > > : > > [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy > > CallDescr

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

Szelethus added a comment. In D68165#1950451 , @ldionne wrote: > Always with me? Maybe you should double-check that you don't have something > weird in your git config, aliases or other? I don't know *why* it would be me > in particular, but the fact tha

[PATCH] D75171: [Analyzer] Fix for incorrect use of container and iterator checkers

Szelethus added a comment. In D75171#1954279 , @sylvestre.ledru wrote: > @baloghadamsoftware @Szelethus it would be great to have the name of the > checkers in the error message > The error is "error: checker cannot be enabled with analyzer option > 'a

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

Szelethus added a comment. Some tests would be appreciated, It really helps me understand whats going on. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:620-624 + "AggressiveEraseModeling", + "Enables exploration of the pas

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

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:692-699 + std::tie(StateEnd, StateNotEnd) = State->assume(*RetEnd); + if (StateEnd) { +C.addTransition(StateEnd); + } + if (StateNo

[PATCH] D76510: [analyzer] Change the default output type to PD_TEXT_MINIMAL in the frontend, error if an output loc is missing for PathDiagConsumers that need it

Szelethus marked an inline comment as done. Szelethus added a comment. Lets highlight then that this change affects the analyzer in **frontend** mode, not in the **driver** configuration. I might make a patch for that too, btw. @NoQ, were you able to check whether **this** change could break any

[PATCH] D76790: [analyzer] StdLibraryFunctionsChecker: fix bug with arg constraints

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. Yup, looks great, sorry for the slack! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76790/new/ https://reviews.llvm.org/D76790 ___

[PATCH] D75045: [analyzer] Improved check of `fgetc` in StreamChecker.

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. I'm a bit confused as to what this patch aims to do. Once again, I'd kindly ask you to discuss for a bit before updating this patch. --- The rule states that after reaching `E

[PATCH] D74973: [analyzer] StdLibraryFunctionsChecker refactor w/ inheritance

Szelethus added a comment. FYI I've been seeing your patches to this checker and I will respond to them, but I need to do some learning on my own before having the confidence to accept or request changes. Working on it! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D72035: [analyzer][NFC] Use CallEvent checker callback in GenericTaintChecker

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. Herald added a subscriber: martong. Wow. Its a joy to see you do C++. LGTM. Are you planning to introduce `CallDescriptionMap` at one point? :) Comment at: clang/lib/S

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

Szelethus requested changes to this revision. Szelethus added a reviewer: steakhal. Szelethus added a comment. This revision now requires changes to proceed. This patch is really cool, but I still feel anxious a bit about duplicating so much functionality, especially since we're working very hard

[PATCH] D74131: [analyzer][taint] Add isTainted debug expression inspection check

Szelethus added a reviewer: boga95. Szelethus added a comment. In D74131#1884372 , @steakhal wrote: > If this patch is good to go, could someone commit it? > I don't have commit access (yet). I think you can apply for a commit access, you have a history

[PATCH] D74760: [Analyzer] Fix for iterator modeling and checkers: handle negative numbers correctly

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. This patch is a good testament to how well those debug functions turned out. LGTM. Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:203 auto &SVB = State-

[PATCH] D69662: [Checkers] Avoid using evalCall in StreamChecker.

Szelethus added a comment. Herald added a subscriber: martong. In D69662#1744479 , @NoQ wrote: > In D69662#1736601 , @balazske wrote: > > > Anyway the checks that do not use BindExpr (all except the open functions)

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

Szelethus added a comment. In D71524#1889566 , @boga95 wrote: > @steakhal's revision is on the top of this. Changing the order will only > cause unnecessary work on both sides. He recently rebased on top of master. I'm no fan of creating unnecessary wor

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG9fd7ce7f4449: [analyzer][MallocChecker][NFC] Communicate the allocation family to auxiliary… (authored by Szelethus). Herald added subscribers: martong, steakhal. Changed prior to commit: https://review

[PATCH] D75045: [analyzer] Improved check of `fgetc` in StreamChecker.

Szelethus added a comment. In D75045#1891064 , @balazske wrote: > In D75045#1891056 , @NoQ wrote: > > > In D75045#1891055 , @NoQ wrote: > > > > > How many such platforms can

[PATCH] D73729: [analyzer] AnalyzerOptions: Remove 'fixits-as-remarks'

Szelethus accepted this revision. Szelethus added a comment. Looks great! Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73729/new/ https://reviews.llvm.org/D73729 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D69662: [Checkers] Avoid using evalCall in StreamChecker.

Szelethus added a comment. In D69662#1890007 , @NoQ wrote: > In D69662#1889545 , @Szelethus wrote: > > > Sorry for the slack :) > > > > One should never count on the invocation order of callback funcions in > > bet

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

Szelethus marked 5 inline comments as done. Szelethus added inline comments. Herald added subscribers: martong, steakhal. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:259 /// calls. bool isCalled(const CallDescription &CD) const;

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGe5513336aee4: [analyzer][MallocChecker][NFC] Change the use of IdentifierInfo* to… (authored by Szelethus). Changed prior to commit: https://reviews.llvm.org/D68163?vs=222791&id=246441#toc Repository:

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

Szelethus marked an inline comment as done. Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:259 /// calls. bool isCalled(const CallDescription &CD) const; NoQ wrote: > Szelethus wrote: > > Sz

[PATCH] D75021: [clang][analyzer] Enable core.builtin even with -no-default-checks

Szelethus added reviewers: NoQ, Szelethus, baloghadamsoftware. Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. This doesn't seem to be in line with the current idea behind `core` checkers. Turning them off is strongly disenco

[PATCH] D75021: [clang][analyzer] Enable core.builtin even with -no-default-checks

Szelethus added a comment. @Charusso implemented a flag that can //silence// checkers in D66042 . I can offer that as an alternative while we're working on separating modeling and reporting in the checker insfrastructure. Repository: rG LLVM Github Monorepo

[PATCH] D75158: [analyzer][StreamChecker] Using function description objects - NFC.

Szelethus accepted this revision. Szelethus added reviewers: baloghadamsoftware, NoQ, martong, Charusso, xazax.hun. Szelethus added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. Cool! Though this patch says little without followups, so maybe we s

[PATCH] D75163: [analyzer][StreamChecker] Adding precall and refactoring.

Szelethus added a comment. Aha, so we're moving every check as to whether the stream is closed to `preCall`? Makes sense, if not much else changed. Could you please run `clang-format-diff.py` after adding the tests? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D75171: [Analyzer] Fix for incorrect use of container and iterator checkers

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. Im sorry, i though we talked about internal code and we're stuck with the checker interface -- this should definitely be solved by changing the parameter of the `shouldRegister

[PATCH] D75163: [analyzer][StreamChecker] Adding precall and refactoring.

Szelethus added a comment. To summarize, we're moving every stream check (illegal call to a stream management function with a null stream, or a closed stream, etc) to `preCall`, and leave only the modeling portions in `evalCall`. Makes sense! You did an amazing job here! I like the new infrastr

[PATCH] D75271: [Analyzer][NFC] Add AnalyzerOptions parameter to shouldRegisterXXX() functions

Szelethus added a reviewer: Charusso. Szelethus added a comment. In D75271#1895900 , @Charusso wrote: > I am so sorry to mention, but we need the `AnalysisManager` to pass around > which manages the analysis, therefore it knows both the `LangOptions` and

[PATCH] D75271: [Analyzer][NFC] Add AnalyzerOptions parameter to shouldRegisterXXX() functions

Szelethus added a comment. In D75271#1896182 , @Szelethus wrote: > > Also this entire callback should be removed ideally: it has to be a virtual > > function defaulting to `return true;` and if someone needs this feature > > could rewrite the behavior. I

[PATCH] D75271: [Analyzer][NFC] Add AnalyzerOptions parameter to shouldRegisterXXX() functions

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. Let's change this to either `CheckerManger` or `AnalysisManager`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75271/new/ https

[PATCH] D75163: [analyzer][StreamChecker] Adding precall and refactoring.

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM! It makes much more sense to do the checking in `preCall` and the modeling in `evalCall`, the comments in the patch are precise and helpful, the tests cover everything. While I feel

[PATCH] D75360: [analyzer][NFC] Tie CheckerRegistry to CheckerManager, allow CheckerManager to be constructed for non-analysis purposes

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

[PATCH] D75271: [Analyzer][NFC] Add AnalyzerOptions parameter to shouldRegisterXXX() functions

Szelethus commandeered this revision. Szelethus edited reviewers, added: baloghadamsoftware; removed: Szelethus. Szelethus added a comment. Yoink. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75271/new/ https://reviews.llvm.org/D75271 _

[PATCH] D75360: [analyzer][NFC] Tie CheckerRegistry to CheckerManager, allow CheckerManager to be constructed for non-analysis purposes

Szelethus updated this revision to Diff 247291. Szelethus added a comment. Unforget git add. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75360/new/ https://reviews.llvm.org/D75360 Files: clang/include/clang/StaticAnalyzer/Core/CheckerManager.h clang/include/clang/StaticAnalyzer/F

[PATCH] D75360: [analyzer][NFC] Tie CheckerRegistry to CheckerManager, allow CheckerManager to be constructed for non-analysis purposes

Szelethus added a comment. Phabricator didn't pick up on it (because I accidentally used `mv` instead of `git mv`), but `CheckerRegistration.h` was moved to `AnalyzerHelpFlags.h`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75360/new/ https://reviews.llvm.org/D75360 __

[PATCH] D75271: [analyzer][NFC] Change LangOptions to CheckerManager in the shouldRegister* functions.

Szelethus updated this revision to Diff 247292. Szelethus retitled this revision from "[Analyzer][NFC] Add AnalyzerOptions parameter to shouldRegisterXXX() functions" to "[analyzer][NFC] Change LangOptions to CheckerManager in the shouldRegister* functions.". Szelethus edited the summary of this

[PATCH] D75271: [analyzer][NFC] Change LangOptions to CheckerManager in the shouldRegister* functions.

Szelethus added a comment. Mind that `AnalysisManager`, similarly to `CheckerManager` before D75360 , must be constructed with an `ASTContext` which is not available at the moment when the `shouldRegister*` functions run, and they must run no later than when the

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

Szelethus marked 5 inline comments as done. Szelethus added a comment. Herald added subscribers: martong, steakhal. I've spent a lot of time on this code, and I don't think it'd be a great idea to do everything in a single step. Collapsing `CallExpr, ProgramState` into `CallEvent` is a shockingl

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

Szelethus updated this revision to Diff 247512. Szelethus added a reviewer: balazske. Szelethus set the repository for this revision to rG LLVM Github Monorepo. Szelethus added a comment. Address inlines. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

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

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

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

Szelethus created this revision. Szelethus added reviewers: balazske, martong, NoQ, xazax.hun, rnkovacs, dcoughlin, baloghadamsoftware. Herald added subscribers: cfe-commits, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet, whisperity. Herald added a project:

[PATCH] D75431: [analyzer][NFC] Merge checkNewAllocator's paramaters into CXXAllocatorCall

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

[PATCH] D75432: [analyzer][NFC][MallocChecker] Convert many parameters into CallEvent

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

[PATCH] D74973: [analyzer] StdLibraryFunctionsChecker refactor w/ inheritance

Szelethus added a comment. I have some high level questions, you have spent far more time with this code and I'm happy to be proven wrong! :) In D74973#1889188 , @martong wrote: > > Is really more kind of constraint needed than range constraint? > > Yes,

[PATCH] D73898: [analyzer] StdLibraryFunctionsChecker: Add argument constraints

Szelethus added a comment. The high level idea and the implementation of the checker seems great. In general, things that you want to address in later patches should be stated in the code with a `TODO`. I wrote a couple nits that I don't want to delete, but maybe it'd be better to address them

[PATCH] D75356: [Analyzer][StreamChecker] Introduction of stream error state handling.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:50-68 +struct StreamErrorState { + // The error state of an opened stream. + // EofError: EOF condition (feof returns true) + // OtherError: other (non-EOF) error (ferror returns t

[PATCH] D74131: [analyzer][taint] Add isTainted debug expression inspection check

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM, thanks! Feel free to commit as you're ready. Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:97 .Case("clang_analyzer_express", &Expr

[PATCH] D63279: [Analyzer] Unroll for-loops where the upper boundary is a variable with know value

Szelethus added a comment. Herald added subscribers: martong, steakhal. In D63279#1630510 , @Szelethus wrote: > In D63279#1630491 , @NoQ wrote: > > > the only reason it's not enabled by default yet is because we for

[PATCH] D75356: [Analyzer][StreamChecker] Introduction of stream error state handling.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:50-68 +struct StreamErrorState { + // The error state of an opened stream. + // EofError: EOF condition (feof returns true) + // OtherError: other (non-EOF) error (ferror returns t

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

Szelethus added a comment. In D73720#1874014 , @baloghadamsoftware wrote: > In case of multiple container-related bugs only mark the container > interesting on the correct bug path. Also a typo fixed. ~~Uh-oh, I'm not sure why this would ever be an iss

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

Szelethus added a comment. See my related comment here: D73720#1901453 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74541/new/ https://reviews.llvm.org/D74541 ___ cfe-commits mailing list cfe-com

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. Apologies -- I'd definitely prefer to address the debug related changes in a separate pack, similarly to D74541 . CHANGES SINCE LAST ACTION

[PATCH] D75356: [Analyzer][StreamChecker] Introduction of stream error state handling.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:92-125 +class MakeRetVal { + const CallExpr *CE = nullptr; + std::unique_ptr RetVal; + SymbolRef RetSym; + +public: + MakeRetVal(const CallEvent &Call, CheckerContext &C)

[PATCH] D75432: [analyzer][NFC][MallocChecker] Convert many parameters into CallEvent

Szelethus marked 2 inline comments as done. Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:995 -void MallocChecker::checkBasicAlloc(CheckerContext &C, const CallExpr *CE, -ProgramStateRef Stat

[PATCH] D75163: [analyzer][StreamChecker] Adding precall and refactoring.

Szelethus added a comment. In D75163#1902816 , @xazax.hun wrote: > The name of the patch implies refactoring but some functional changes were > also done. Is it possible to separate the functional changes into a separate > patch? If it is not a big effor

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. The scope of the patch is small and is well tested. I suggest to move the discussion about debug messages and `NoteTag`s to the next revision. LGTM! CHANGES SINCE LAST ACTION https://

[PATCH] D75514: [Analyzer] Only add container note tags to the operations of te affected container

Szelethus added a comment. In D75514#1903153 , @baloghadamsoftware wrote: > Alternative approach for debugging (instead of checking the source range): > `clang_analyzer_express()` from `ExprInspection` marks its argument as > interesting in the bug repo

[PATCH] D75356: [Analyzer][StreamChecker] Introduction of stream error state handling.

Szelethus added a comment. Okay, I think we're mostly in agreement so far -- could we implement a warning and add some test files for unchecked stream states after a failed `fseek` call? The title of the revision is "[Analyzer][StreamChecker] Introduction of stream error state handling.", yet i

[PATCH] D69602: [analyzer] Test case for lambda capture by value modelling

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM, can we remove the open projects entry under the same breath? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69602/new/ https://review

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

Szelethus added a comment. Its becoming a bit difficult to navigate inlines, could you please mark them as done as you address them? Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:61 + using FnCheck = bool (StreamChecker::*)(const CallEvent &, +

[PATCH] D69308: [analyzer] Test cases for the unsupported features for Clang Static Analyzer

Szelethus added a comment. LGTM given that the inlines are fixed. In D69308#1722139 , @NoQ wrote: > Thanks for the tests! > > Both of these features are relatively hard. > > ... Would love to see this comment in its entirety on the open projects page :^

[PATCH] D65912: [clang-tidy] Add new check for math constants

Szelethus resigned from this revision. Szelethus added a comment. I have no authority in clang-tidy, but the idea is nice! You may wanna reupload with full context though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65912/new/ https://reviews.ll

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

Szelethus accepted this revision. Szelethus added a comment. Let's make it 3! Thank you so much for sticking by, I guess one of the reasons why a patch so obviously great and desired took so long is that we still didn't figure out how we imagine the `CallDescriptionMap` to be integrated into lar

[PATCH] D69662: [Checkers] Avoid using evalCall in StreamChecker.

Szelethus added a comment. In D69662#1731974 , @balazske wrote: > I wanted to remove `eval::Call` because only one checker can do this > otherwise it is undefined behavior (according to the not very new "Analyzer > Guide"). If it is essentially needed in

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

Szelethus added a comment. Changes to `MallocChecker` really highlight the positive effects of this patch. Nice! Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:451 static ProgramStateRef MallocMemAux(CheckerContext &C, const CallExpr *CE, -

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp:199 + /// + /// This callback should be used to construct the checker's fields. + /// Szelethus wrote: > Hmmm, what use case do you have in mind? Do we actu

[PATCH] D69745: [analyzer] Checker: check::BeginAnalysis

Szelethus added a comment. YES PLEASE. Debug checkers that only dump from `check::EndAnalysis` won't rely on the analysis not actually crashing. Which is ironically exactly when we want to use them. Comment at: clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp:199 +

[PATCH] D69813: [analyzer] CERTStrChecker: Model gets()

Szelethus added a comment. Hmm, so this checker is rather a collection of CERT rule checkers, right? Shouldn't the checker name contain the actual rule name (STR31-C)? User interfacewise, I would much prefer smaller, leaner checkers than a big one with a lot of options, which are barely support

[PATCH] D69813: [analyzer] CERTStrChecker: Model gets()

Szelethus added a comment. In D69813#1734272 , @Charusso wrote: > In D69813#1734193 , @Szelethus wrote: > > > Hmm, so this checker is rather a collection of CERT rule checkers, right? > > Shouldn't the checker name

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

Szelethus added a comment. Nice catch! Though, wouldn't the memory sanitizer buildbots break on this reliably? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69962/new/ https://reviews.llvm.org/D69962 ___ cfe-commits

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

Szelethus added inline comments. Comment at: clang/lib/Analysis/CFG.cpp:5882 + // FIXME: Should we return the terminator here? + if (size() == 0) What would that even be? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69962/ne

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

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM, provided that the inlines are addressed! Thanks! Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:102-103 /// system call etc. - bool che

[PATCH] D70047: [Analyzer] Use a reference in a range-based for

Szelethus accepted this revision. Szelethus added a comment. Whoo! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70047/new/ https://reviews.llvm.org/D70047 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. In D69962#1741503 , @NoQ wrote: > In D69962#1739440 , @NoQ wrote: > > > In D69962#1738618

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

Szelethus added a comment. Accepted accidentally. Well, in any case, I trust your judgement on this! I'd prefer not to commit the test file as-is. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69962/new/ https://reviews.llvm.org/D69962 ___

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

Szelethus added a comment. In D69962#1744679 , @NoQ wrote: > In D69962#1742291 , @Szelethus wrote: > > > Try to add a non-sanitizer built tablegen: > > `-DCLANG_TABLEGEN=/path/to/non/sanitized/clang-tblgen` > > `-

[PATCH] D70439: [Analyzer][Docs][NFC] Add CodeChecker to the command line tools

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. For reasons other than being a part of the project, CodeChecker is objectively an amazing tool to use with the analyzer. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D71791: [CFG] Fix an assertion failure with static initializers

Szelethus added a comment. > I am not sure what would be the best way to test this change here. I'll admit, I'm a bit out of touch with Clang and will unfortunately be for a while, but the short answer is that the unit tests we have for the CFG are kinda bad. The constructed CFG has a longer li

[PATCH] D71791: [CFG] Fix an assertion failure with static initializers

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. In D71791#1795093 , @xazax.hun wrote: > I decided to fix the unittests. Having CFGs full of dangling pointers to the > AST does not look fun at a

[PATCH] D75612: [Analyzer][StreamChecker] Adding PreCall and refactoring (NFC).

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM! Lets have a link to the original discussion: D75163 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D75163: [analyzer][StreamChecker] Adding precall and refactoring.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:64-70 +/// Get the value of the stream argument out of the passed call event. +/// The call should contain a function that is described by Desc. +SVal getStreamArg(const FnDescription

[PATCH] D75612: [Analyzer][StreamChecker] Adding PreCall and refactoring (NFC).

Szelethus added a comment. Wouldn't it be better just to upload this diff to D75163 by the way? It feels like we're discarding much of the discussion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75612/new/ http

[PATCH] D75356: [Analyzer][StreamChecker] Introduction of stream error state handling.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:37-40 + // NoError: No error flag is set or stream is not open. + // EofError: EOF condition (feof returns true) + // OtherError: other (non-EOF) error (ferror returns true) + //

[PATCH] D75514: [Analyzer] Only add container note tags to the operations of the affected container

Szelethus added a comment. In D75514#1907392 , @baloghadamsoftware wrote: > In D75514#1905268 , @Szelethus wrote: > > > But why is this related to `UndefinedVal`? > > > Because `UndefinedVal` seems to be the "null"

<    9   10   11   12   13   14   15   16   >