[PATCH] D154478: [analyzer][NFC] Use unique_ptrs for PathDiagnosticConsumers

steakhal abandoned this revision. steakhal added a comment. Okay, I'm done. It's just a complete mess. I'll come back to this once I have some time, but not now. Here is what I found: Unittests call `AnalysisConsumer->addDiagnosticConsumer()` after the `AnalysisConsumer` is constructed, but befo

[PATCH] D154827: [analyzer] NonParamVarRegion should prefer definition over canonical decl

steakhal created this revision. steakhal added reviewers: xazax.hun, donat.nagy, NoQ. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware. Herald added a reviewer: Szelethus. Herald added a project: All. steakhal reques

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

steakhal added a comment. Please, let me have a look at this stack tomorrow or the day after prior landing it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153612/new/ https://reviews.llvm.org/D153612

[PATCH] D154871: [clang] Satisfy clang v12

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Looks reasonable. Unfortunate that it regresses readability by a pair of parents. Anyway LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D154827: [analyzer] NonParamVarRegion should prefer definition over canonical decl

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGffcf214b5d27: [analyzer] NonParamVarRegion should prefer definition over canonical decl (authored by steakhal). Changed prior to commit: https://r

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1305 + std::string Note = + llvm::formatv(Case.getNote().str().c_str(), +cast(Call.getDecl())->getDeclName()); dona

[PATCH] D153273: [analyzer] Rework support for CFGScopeBegin, CFGScopeEnd, CFGLifetime elements

steakhal added inline comments. Comment at: clang/lib/Analysis/CFG.cpp:1819-1826 + for (std::size_t idx = 1, count = LocalScopeEndMarkers.size(); idx < count; + ++idx) { +LocalScope::const_iterator B = LocalScopeEndMarkers[idx]; +LocalScope::const_iterator E = Loca

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

steakhal added inline comments. Comment at: clang/test/Analysis/stream-note.c:61-62 FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}} + // stdargs-note@-1 {{'fopen' is successful}} + // stdargs-note@-2 {{'fopen' is successful}} if (!F1)

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

steakhal added inline comments. Comment at: clang/test/Analysis/stream-note.c:61-62 FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}} + // stdargs-note@-1 {{'fopen' is successful}} + // stdargs-note@-2 {{'fopen' is successful}} if (!F1)

[PATCH] D154838: [analyzer] Add check for null pointer passed to %p of printf family

steakhal added a comment. Expect a review on around the next weekend due to vacations from me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154838/new/ https://reviews.llvm.org/D154838 ___ cfe-commits m

[PATCH] D155445: [analyzer][docs] Add CSA release notes

steakhal created this revision. steakhal added reviewers: NoQ, xazax.hun, Szelethus, donat.nagy, balazske, gamesh411, tripleCC, tomasz-kaminski-sonarsource, OikawaKirie. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware. Herald

[PATCH] D155445: [analyzer][docs] Add CSA release notes

steakhal added inline comments. Comment at: clang/docs/ReleaseNotes.rst:907 + Any use of this flag will result in an error. + (`7cd1f3ad22e4 `_) +- Fixed a null-pointer dereference crash inside the ``MoveChecker``. -

[PATCH] D155445: [analyzer][docs] Add CSA release notes

steakhal updated this revision to Diff 541175. steakhal marked 4 inline comments as done. steakhal added a comment. Currentl look: F28285701: image.png let me know if you like it. Feel free to propose changes. I'm not sure about the relative ordering. We shou

[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

steakhal added a comment. Maybe its too early for the patch, but if a nonlocal change, like changing some Core functionality, we usually measure the real world implications and compare it against some baseline to get an idea how this affects the whole system. It also helps uncovering corner cas

[PATCH] D158285: [NFC][CLANG] Fix wrong orders of function arguments positions

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1468-1477 std::optional getRangeForNegatedSymSym(const SymSymExpr *SSE) { return getRangeForNegatedExpr( [SSE, State = this->State]() -> SymbolRef { if

[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

steakhal added a comment. As a general comment on requiring all extents to be of unsigned APSInts. Checkers, like the `MallocChecker`, binds the result of arbitrary expression's values (the argument of malloc, for example) as extents of some regions. Usually, that argument is of an `unsigned` sta

[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

steakhal added a comment. In D158707#4614268 , @danix800 wrote: > In D158707#4614100 , @steakhal > wrote: > >> As a general comment on requiring all extents to be of unsigned APSInts. >> Checkers, like the `Mallo

[PATCH] D158813: [analyzer] MPIChecker: MPI_Waitall should respect count arg

steakhal added a comment. Let me try to summarize some of the variables: So, given an invocation like `MPI_Waitall(C, Requests, Statues)`: - `MR` is `lval(Requests)` - `ElementCount` is the number of elements `Requests` consist of. - `ArrSize` is basically `ElementCount` but an APSInt. - `MROffs

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

steakhal added a comment. I checked out the code to see how does the Static Analyzer work after this. I'm impressed that it seems to work. Do you mind adding my test file to this patch? `clang/test/Analysis/cxx2b-deducing-this.cpp`: // RUN: %clang_analyze_cc1 -std=c++2b -verify %s \ // RUN:

[PATCH] D158858: [analyzer] MPIChecker: add defensive checking (check against nullptr)

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Makes sense. Would thus test crash without the early returns? And now they wouldn't? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158858/new

[PATCH] D158858: [analyzer] MPIChecker: add defensive checking (check against nullptr)

steakhal added a comment. Thanks. Feel free to merge this (before any of the others). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158858/new/ https://reviews.llvm.org/D158858 ___ cfe-commits mailing li

[PATCH] D158953: [analyzer] MmapWriteExecChecker: use getAs instead of castAs

steakhal added a comment. Could you please simplify the test case? You could basically get rid of everything there, except for forwarding one top level parameter as the proto argument to the mmap call. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D158953: [analyzer] MmapWriteExecChecker: use getAs instead of castAs

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Land it, whenever you wish. Thanks! PS: usually we don't have a comment like that. The name of the function should give enough context, along with a // no-crash comment at the line where it

[PATCH] D158968: [analyzer] Fix assertion on casting SVal to NonLoc inside the IteratorRange checker

steakhal created this revision. steakhal added reviewers: NoQ, donat.nagy, xazax.hun, Szelethus. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware. Herald added a project: All. steakhal requested review of this revision. Herald

[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. I haven't checked the details, but it makes sense. It's reassuring that all the tests pass :D, which is good enough for me. Thanks for the cleanup! Repository: rG LLVM Github Monorepo

[PATCH] D158968: [analyzer] Fix assertion on casting SVal to NonLoc inside the IteratorRange checker

This revision was automatically updated to reflect the committed changes. Closed by commit rG985e399647d5: [analyzer] Fix assertion on casting SVal to NonLoc inside the IteratorRange… (authored by steakhal). Changed prior to commit: https://reviews.llvm.org/D158968?vs=553858&id=553879#toc Repo

[PATCH] D158968: [analyzer] Fix assertion on casting SVal to NonLoc inside the IteratorRange checker

steakhal added a comment. In D158968#4620721 , @donat.nagy wrote: > LGTM. I'm not familiar with the Iterator checker family, but this is a very > straightforward change. Thanks for the review! Well, it's an Ericsson checker ;) Repository: rG LLVM G

[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

steakhal added a comment. Oh, so we would canonicalize towards a signed extent type. I see. I think I'm okay with that. However, I think this needs a little bit of polishing and testing when the region does not point to the beginning of an array or object, but rather inside an object, or like t

[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

steakhal added a comment. In D158707#4621270 , @donat.nagy wrote: > In D158707#4621135 , @steakhal > wrote: > >> Oh, so we would canonicalize towards a signed extent type. I see. I think >> I'm okay with that.

[PATCH] D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well

steakhal created this revision. steakhal added reviewers: NoQ, xazax.hun, donat.nagy, Szelethus. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware. Herald added a project: All. steakhal requested review of this revision. Herald

[PATCH] D159106: [analyzer] ArrayBoundCheckerV2 should listen to check::Bind as well

steakhal created this revision. steakhal added reviewers: NoQ, xazax.hun, donat.nagy, Szelethus. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware. Herald added a project: All. steakhal requested review of this revision. Herald

[PATCH] D159107: [analyzer] ArrayBoundCheckerV2 should disallow forming lvalues to out-of-bounds locations

steakhal created this revision. steakhal added reviewers: NoQ, xazax.hun, donat.nagy, Szelethus. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware. Herald added a project: All. steakhal requested review of this revision. Herald

[PATCH] D159108: [analyzer] CStringChecker should check the first byte of the destination of strcpy, strncpy

steakhal created this revision. steakhal added reviewers: NoQ, xazax.hun, donat.nagy, Szelethus. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware. Herald added a project: All. steakhal requested review of this revision. Herald

[PATCH] D159109: [analyzer] CStringChecker buffer access checks should check the first bytes

steakhal created this revision. steakhal added reviewers: NoQ, xazax.hun, donat.nagy, Szelethus. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware. Herald added a project: All. steakhal requested review of this revision. Herald

[PATCH] D159163: [analyzer][NFC] Workaround miscompilation on recent MSVC

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Feel free to merge it. Thanks! I'd be curious to see if this bug is tracked at Microsoft. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D15916

[PATCH] D159163: [analyzer][NFC] Workaround miscompilation on recent MSVC

steakhal added a comment. Please add the exact versions to the commit message of Visual Studio where it worked, and where it didn't, thus we applied this change so that it would work on all (both) Visual Studio versions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

[PATCH] D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well

steakhal added a comment. In D159105#4627724 , @donat.nagy wrote: > I was thinking about adding an improvement like this for the sake of > consistency, but I fear that this might cause a surprising amount of false > positives. (Did you test it on large

[PATCH] D159106: [analyzer] ArrayBoundCheckerV2 should listen to check::Bind as well

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:46 + void impl(SVal Loc, bool isLoad, const Stmt *S, CheckerContext &C) const; + donat.nagy wrote: > I'd call this function `performCheck` or something simila

[PATCH] D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well

steakhal planned changes to this revision. steakhal added a comment. In D159105#4627883 , @donat.nagy wrote: > In D159105#4627785 , @steakhal > wrote: > >> In D159105#4627724

[PATCH] D159106: [analyzer] ArrayBoundCheckerV2 should listen to check::Bind as well

steakhal planned changes to this revision. steakhal added a comment. This patch would cause FPs on this code: struct S {}; void zero_size_array() { S arr[0]; (void)arr; } Being short on time, I'll just drop this commit from the stack and come back in a late future. This might be r

[PATCH] D159107: [analyzer] ArrayBoundCheckerV2 should disallow forming lvalues to out-of-bounds locations

steakhal abandoned this revision. steakhal added a comment. In D159107#4627903 , @donat.nagy wrote: > Good direction of development, this will be useful for providing better bug > reports (in addition to ensuring correct behavior some situations). > No

[PATCH] D154838: [analyzer] Add check for null pointer passed to the %p of printf family

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. I'd suggest renaming the checker to `alpha.unix.NullFormatSpecifier`. Maybe add tests where multiple format specifiers are null, and also one test where no variadic args are prese

[PATCH] D159107: [analyzer] ArrayBoundCheckerV2 should disallow forming lvalues to out-of-bounds locations

steakhal added a comment. In D159107#4630573 , @donat.nagy wrote: > I don't think that the `&arr[N]` issue is too serious: we can just increment > the array extent when the parent expression of the array subscript operator > is the unary operator `&`.

[PATCH] D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well

steakhal added a comment. I'm thinking of a heuristic like this: bool IsAtOffsetZero = [ByteOffset] { const auto *Int = ByteOffset.getAsInteger(); return Int && Int->isZero(); }(); // ... if (state_precedesLowerBound && state_withinLowerBound && !IsAtOffsetZero && isTainted(

[PATCH] D159107: [analyzer] ArrayBoundCheckerV2 should disallow forming lvalues to out-of-bounds locations

steakhal added a comment. In D159107#4631069 , @donat.nagy wrote: > In D159107#4630764 , @steakhal > wrote: > >> In D159107#4630573 , @donat.nagy >> wrote: >> >>> I don

[PATCH] D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well

steakhal added a comment. There are still a few FPs of the kind, where they iterate over the result of `getenv` in a loop, and continuously checks the character against the zero terminator. I refined the suppression heuristic as follows: - If the offset is zero, don't report taint issue. (as I

[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

steakhal added inline comments. Comment at: clang/test/Analysis/memory-model.cpp:167 + clang_analyzer_dumpExtent(a); // expected-warning {{0 S64b}} + clang_analyzer_dumpElementCount(a); // expected-warning {{5 S64b}} + clang_analyzer_dumpExtent(t); // expected-warn

[PATCH] D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well

steakhal added a comment. In D159105#4631504 , @steakhal wrote: > There are still a few FPs of the kind, where they iterate over the result of > `getenv` in a loop, and continuously checks the character against the zero > terminator. > I refined the sup

[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

steakhal added inline comments. Comment at: clang/test/Analysis/memory-model.cpp:167 + clang_analyzer_dumpExtent(a); // expected-warning {{0 S64b}} + clang_analyzer_dumpElementCount(a); // expected-warning {{5 S64b}} + clang_analyzer_dumpExtent(t); // expected-warn

[PATCH] D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well

steakhal added a comment. In D159105#4633864 , @donat.nagy wrote: > As I thought more about this commit I realized that I have two vague but > significant concerns. I'm sorry that I wasn't able to describe these before > you started to dig into the det

[PATCH] D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well

steakhal added a comment. In D159105#4633899 , @steakhal wrote: > The measurement is still running, and I'll post a few words about that once I > had a look; but anyway, I feel that this patch is controversial and needs > more discussion before it could

[PATCH] D158499: [analyzer] Compute FAM dynamic size

steakhal added a comment. @danix800 FYI I think you used the wrong revision link in the commit. Maybe mark this revision as "abandoned" again, to reflect the actual status. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158499/new/ https://reviews.

[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

steakhal closed this revision. steakhal added a comment. Landed as https://github.com/llvm/llvm-project/commit/12559064e05a11e8418425de59d8745f0cfb1122 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158707/new/ https://reviews.llvm.org/D158707 ___

[PATCH] D157385: [clang][CFG] Cleanup functions

steakhal added a comment. When I added `-analyzer-config cfg-lifetime=true` to `clang/test/Analysis/scopes-cfg-output.cpp`, suddenly duplicated lifetime ends entries appeared where we have `CleanupFunctions`. My output is: void test_cleanup_functions() [B2 (ENTRY)] Succs (1): B1

[PATCH] D157385: [clang][CFG] Cleanup functions

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. In D157385#4634591 , @tbaeder wrote: > @steakhal Double lifetime ends should be fixed now. I haven't verified, but it should be good now. CHANGE

[PATCH] D159397: [StaticAnalyzer] Use switch statement in MallocChecker::performKernelMalloc. NFC

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. It doesn't look much of an improvement TBH, but I won't stand against it either. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159397/ne

[PATCH] D158156: [analyzer] Add C++ array delete checker

steakhal added a comment. Add a test where we have multiple imp derived to base casts but only one leads to a bug. Do it for both ways, once that the first is the offending, and once the second. Comment at: clang/docs/analyzer/checkers.rst:1793-1803 +.. code-block:: cpp + + B

[PATCH] D158813: [analyzer] MPIChecker: MPI_Waitall should respect count arg

steakhal added a comment. I don't have time this week, sorry. Maybe others can take this over. @donat.nagy could you please have a look? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158813/new/ https://reviews.llvm.org/D158813 ___

[PATCH] D159479: [ASTImport]enhance statement comparing

steakhal added a comment. Make sure the commit message and content complies with the guidelines. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159479/new/ https://reviews.llvm.org/D159479 ___ cfe-commits

[PATCH] D138037: [analyzer] Remove unjustified assertion from EQClass::simplify

steakhal added a subscriber: vsavchenko. steakhal added a comment. In D138037#3941632 , @xazax.hun wrote: > I did not spend too much time thinking about this yet, but this sounds scary. > I wonder if we should target the underlying problem instead, i.e.,

[PATCH] D138037: [analyzer] Remove unjustified assertion from EQClass::simplify

steakhal added a comment. In D138037#3941777 , @xazax.hun wrote: > I guess there are some more options. We could try keeping representatives > alive no matter what. It could be a good exercise to see if doing that makes > any difference in the analysis

[PATCH] D108230: [analyzer] Ignore single element arrays in getStaticSize() conditionally

steakhal added a comment. In D108230#3942374 , @xazax.hun wrote: > I am a bit conflicted. It is unfortunate that C and C++ compilers regarded > single element array members as flexible array members. On the other hand, > looking at GCC, it recently adde

[PATCH] D136811: [-Wunsafe-buffer-usage] WIP: RFC: NFC: User documentation.

steakhal added a comment. As I was reading I'll highlighted some typos. `compile time` -> `compile-time` /g By the looks of it, this document is not referenced anywhere. I believe `clang/docs/index.rst` should refer to this document in some place. Thanks for the huge effort driving this you all!

[PATCH] D137348: [-Wunsafe-buffer-usage] Introduce an abstraction for fixable code patterns.

steakhal added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:40 + + /// Determine if a kind is a safe kind. Slower than calling isSafe(). + static bool isSafeKind(Kind K) { We could have a `GADGET_RANGE(UnsafeGadgets, Increment, Decremen

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

steakhal added inline comments. Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13 +void foo(...); + +void * bar(void); +char * baz(void); I would expect this test file to grow quite a bit. As such, I think we should have more self-descriptive name

[PATCH] D138253: [-Wunsafe-buffer-usage] NFC: Implement fix-strategies and variable-use-claiming.

steakhal added inline comments. Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:30 + /// of primitive fixits (individual insertions/removals/replacements). + using FixItList = llvm::SmallVector; + Unless we have a well-formed idea of how m

[PATCH] D136603: [analyzer] getBinding should auto-detect type only if it was not given

This revision was automatically updated to reflect the committed changes. Closed by commit rG93b98eb399a1: [analyzer] getBinding should auto-detect type only if it was not given (authored by steakhal). Changed prior to commit: https://reviews.llvm.org/D136603?vs=471112&id=477493#toc Repository

[PATCH] D138319: [analyzer] Prepare structures for integral cast feature introducing

steakhal added a comment. I spent some time on this patch. But I'm left thinking about why we need the extra indirection? Couldn't we just use a `std::pair` pair as a key instead? You mention //effective// bitwidths in your summary, but the code does not mention this concept. Maybe you could ela

[PATCH] D138329: [-Wunsafe-buffer-usage] Add a new recursive matcher to replace `forEachDescendant` in unsafe buffer check

steakhal added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:54-55 + bool TraverseDecl(Decl *Node) { +if (!Node) + return true; +if (!match(*Node)) Can `Node` be ever null if the visitor is initiated only be `AST_MATCHER_P`?

[PATCH] D137258: [clang] Optimize storage and lookup of analyzer options

steakhal added a comment. Are you sure about the speedup? When I looked at the implementation of the `StringSwitch`, it boiled down to an `if-else` chain under the hood. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137258/new/ https://reviews.llv

[PATCH] D138253: [-Wunsafe-buffer-usage] NFC: Implement fix-strategies and variable-use-claiming.

steakhal added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:90 + /// returns an empty list if no fixes are necessary. + virtual Optional getFixits(const Strategy &) const { +return None; steakhal wrote: > I wonder if we should prefe

[PATCH] D138657: [analyzer] Consider single-elem arrays as FAMs by default

steakhal created this revision. steakhal added reviewers: NoQ, xazax.hun. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware. Herald added a reviewer: Szelethus. Herald added a project: All. steak

[PATCH] D138659: [analyzer] Deprecate FAM analyzer-config, recommend -fstrict-flex-arrays instead

steakhal created this revision. steakhal added reviewers: NoQ, xazax.hun. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware. Herald added a reviewer: Szelethus. Herald added a project: All. steak

[PATCH] D108230: [analyzer] Ignore single element arrays in getStaticSize() conditionally

steakhal added a comment. I proposed the D138657 and D138659 patches for getting rid of this flag. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108230/new/ https://reviews.llvm

[PATCH] D112621: [analyzer][solver] Introduce reasoning for not equal to operator

steakhal added a comment. In D112621#3915822 , @manas wrote: > Ping `Analysis/constant-folding.c` seems to fail. Please run the `check-clang-analysis` build target to see what fails and investigate it. Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D138668: Correct typos

steakhal accepted this revision. steakhal added a comment. Looks good. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138668/new/ https://reviews.llvm.org/D138668 ___ cfe-commits mailing list cfe-commits@

[PATCH] D137258: [clang] Optimize storage and lookup of analyzer options

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. BTW the change itself looks good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137258/new/ https://reviews.llvm.org/D137258

[PATCH] D138657: [analyzer] Consider single-elem arrays as FAMs by default

This revision was automatically updated to reflect the committed changes. Closed by commit rG36481758390c: [analyzer] Consider single-elem arrays as FAMs by default (authored by steakhal). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138657/new/ ht

[PATCH] D138659: [analyzer] Deprecate FAM analyzer-config, recommend -fstrict-flex-arrays instead

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG097ce7616527: [analyzer] Deprecate FAM analyzer-config, recommend -fstrict-flex-arrays instead (authored by steakhal). Repository: rG LLVM Github

[PATCH] D138713: Fix assertion failure "PathDiagnosticSpotPiece's must have a valid location." in ReturnPtrRange checker on builtin functions

steakhal resigned from this revision. steakhal added a comment. Herald added a subscriber: steakhal. I already reviewed this internally. For me, it looks great. @arseniy-sonar consider adding the `Fixes #55347` marker to the summary, so the commit will auto-reference the fixed issue. Also mentio

[PATCH] D112621: [analyzer][solver] Introduce reasoning for not equal to operator

steakhal added a comment. I haven't looked at the implementation. I only checked the tests and something is not quite right there. See my comments inline. BTW the line-coverage is good. I found only two branches which I want to have tests for - see inline. Comment at: clang/l

[PATCH] D138713: Fix assertion failure "PathDiagnosticSpotPiece's must have a valid location." in ReturnPtrRange checker on builtin functions

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp:48 + dyn_cast_or_null(C.getStackFrame()->getCallSite()); + CE && CE->getBuiltinCallee() != 0) +return; NoQ wrote: > I suspect that you mi

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

steakhal added inline comments. Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13 +void foo(...); + +void * bar(void); +char * baz(void); NoQ wrote: > ziqingluo-90 wrote: > > steakhal wrote: > > > I would expect this test file to grow quite a bit.

[PATCH] D136811: [-Wunsafe-buffer-usage] WIP: RFC: NFC: User documentation.

steakhal added inline comments. Comment at: clang/docs/SafeBuffers.rst:31 +convert large amounts of old code to conform to the warning; + - Attribute ``[[unsafe_buffer_usage]]`` lets you annotate custom functions as +unsafe, while providing a safe alternative that can of

[PATCH] D139195: [-Wmissing-noreturn] Detect non-void noreturn function candidates

steakhal created this revision. steakhal added reviewers: NoQ, xazax.hun, aaron.ballman. Herald added subscribers: martong, rnkovacs. Herald added a reviewer: Szelethus. Herald added a project: All. steakhal requested review of this revision. Herald added a project: clang. Herald added a subscriber

[PATCH] D139195: [-Wmissing-noreturn] Detect non-void noreturn function candidates

steakhal planned changes to this revision. steakhal added a comment. In D139195#3966675 , @aaron.ballman wrote: >> Previously, only void returning functions were considered for noreturn >> attribute candidates. This patch removes this artificial restric

[PATCH] D74806: [analyzer] NFCi: Refactor CStringChecker: use strongly typed internal API

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

[PATCH] D74806: [analyzer] NFCi: Refactor CStringChecker: use strongly typed internal API

steakhal marked an inline comment as done. steakhal added a comment. In D74806#1882300 , @NoQ wrote: > This is fantastic, i love it! > > > all tests are preserved and passing; only *the message changed at some > > places*. In my opinion, these messages ar

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

steakhal added a comment. Herald added a subscriber: martong. If this patch is good to go, could someone commit it? I don't have commit access (yet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74131/new/ https://reviews.llvm.org/D74131 __

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

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:407 +// FIXME Add detailed diagnostic. +std::string Msg = "Function argument constraint is not satisfied"; +auto R = std::make_unique(BT, Msg, N);

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

steakhal updated this revision to Diff 246420. steakhal added a comment. Declaring the purpose of this checker: > [...] > This would help to reduce the *noise* that the `TaintTest` debug checker > would > introduce and let you focus on the `expected-warning`s that you really care > about

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

steakhal marked 2 inline comments as done. steakhal added a comment. Marking comments done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74131/new/ https://reviews.llvm.org/D74131 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

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

steakhal marked 5 inline comments as done. steakhal added a comment. In D72035#1889320 , @Szelethus wrote: > Wow. Its a joy to see you do C++. LGTM. Are you planning to introduce > `CallDescriptionMap` at one point? :) Yes, definitely. ==

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

steakhal 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. I would happily rebase this patch if you want. Comment at

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

This revision was automatically updated to reflect the committed changes. steakhal marked an inline comment as done. Closed by commit rG859bcf4e3bb9: [analyzer][taint] Add isTainted debug expression inspection check (authored by steakhal). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

steakhal added a comment. I don't have any technical comments on this patch since I haven't used `NoteTags` yet, only a couple of readability ones. Comment at: clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp:97-103 + auto *PSBR = dyn_cast(&BR); +

[PATCH] D75063: [analyzer] StdLibraryFunctionsChecker: Add NotNull Arg Constraint

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:684 +.Case({ +ReturnValueCondition(LessThanOrEq, ArgNo(2)), +}) Two lines below you are using the `{0U}` initialization syntax

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

steakhal added a comment. In D71524#1902654 , @steakhal wrote: > In D71524#1889566 , @boga95 wrote: > > > @steakhal's revision is on the top of this. Changing the order will only > > cause unnecessary work on both

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

steakhal added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:958 + + unsigned getNumArgs() const override { return getDecl()->getNumParams(); } + NoQ wrote: > Charusso wrote: > > `return getOriginExpr()->getNumAr

[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

steakhal added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:33-50 +void SignalInMultithreadedProgramCheck::registerMatchers(MatchFinder *Finder) { + auto signalCall = + callExpr( + ignoringImpCasts(hasDes

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