[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

steakhal added a comment. This patch appears to introduce a bug in some source ranges. Reported the regression at https://github.com/llvm/llvm-project/issues/71161. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64087/new/ https://reviews.llvm.org/D

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

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. It's good enough already, given you apply my last suggestion. References would be nice to support for notes, but not a blocker. Comment at: clang/lib/StaticAnalyzer/Check

[PATCH] D159549: [analyzer] Fix false negative when accessing a nonnull property from a nullable object

steakhal added a comment. For new revisions, prefer creating a PR at GitHub. Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:903-904 if (!TrackedNullability && - getNullabilityAnnotation(ReturnType) == Nullability::Nullable) { + getNullabilityA

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

steakhal added a comment. The code looks good now. Let's have one last round. Do you have real-world reports? Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:762-763 +def CXXArrayDeleteChecker : Checker<"ArrayDelete">, + HelpText<"Reports destructions of a

[PATCH] D154603: [analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. Thanks for the ping. I have some concerns inline. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:1000-1008 + CheckerOptions<[ +CmdLine

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

steakhal added inline comments. Comment at: clang/test/Analysis/ArrayDelete.cpp:85-88 +Derived *d3 = new DoubleDerived[10]; +Base *b3 = d3; // expected-note{{Conversion from derived to base happened here}} +delete[] b3; // expected-warning{{Deleting an array of polym

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG0954dc3fb921: [analyzer] CStringChecker buffer access checks should check the first bytes (authored by steakhal). Repository: rG LLVM Github Monor

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGc3a87ddad62a: [analyzer] CStringChecker should check the first byte of the destination of… (authored by steakhal). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

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

steakhal added a comment. I have concerns mostly about the cast visitor. Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:192 + + const Stmt *S = N->getStmtForDiagnostics(); + if (!S) Discookie wrote: > steakhal wrote: > > Aren't you actuall

[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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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. 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] 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. 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#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] 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 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] 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] 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 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 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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

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] 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] 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] 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] 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] 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] 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] 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] 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] 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. 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] D157385: [clang][CFG] Cleanup functions

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. This is a great improvement. When I saw that clang now supports it and e.g. the CSA operates on the CFG, I also considered adding this. Now I don't need to do it, so many thanks!

[PATCH] D154603: [analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker

steakhal added a comment. In D154603#4609872 , @gamesh411 wrote: > - try to consolidate the multiple warnings coming from this checker's > `checkLocation` callback > > category based filtering ( example from > lib/StaticAnalyzer/Checkers/GenericTaintChe

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

steakhal added a comment. In D158499#4608847 , @danix800 wrote: > We have `getDynamicExtentWithOffset` to use, which can handle more general > dynamic memory based cases than this fix. > > I'll abandon this one. > > There are issues worth clarifying and f

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

steakhal added a comment. In D158499#4606337 , @danix800 wrote: > In D158499#4606291 , @steakhal > wrote: > >> Thanks for submitting this. >> A funny thing is that in my free time I was also working on this last

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

steakhal added a comment. Thanks for submitting this. A funny thing is that in my free time I was also working on this last week. I'll have a look at this more in depth during the week. For the mean time here is my version, committed pretty much a couple days ago to my fork. https://github.com/l

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

steakhal added a comment. One more thing to mention. Its usually illadvised to rename files or do changes that would seriously impact git blame, unless we have a really good reason doing so. Aesthetics usually don't count one, especially if the name is not user-facing. However, maintainability

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

steakhal added a comment. I share the raised concerns. And I think we are aligned. PS: sorry if any of my comments are dups, or outdated. I only had a little time last week, and things have progressed since then I noticed. I still decided to submit my possibly outdated and partial review commen

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

steakhal added a comment. In D158285#4603477 , @aaron.ballman wrote: >> I've seen a few similar patches from you @Manna and probably some related >> folks. >> Could you please clarify the motivation and the expectations? Or feel free >> to point me to

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

steakhal added a comment. Hold on, make sure this is clang-format compliant before committing. (Check buildkite) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157104/new/ https://reviews.llvm.org/D157104 ___

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

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

[PATCH] D158360: [docs] Update the static analyzer bug reporting page

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Makes sense to me, thanks! I'm not sure though how much we wanna invest into maintaining these handwritten htmls. I would prefer moving away from these. It would likely also fit more nice

[PATCH] D158360: [docs] Update the static analyzer bug reporting page

steakhal added a comment. Makes sense to me. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158360/new/ https://reviews.llvm.org/D158360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

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

steakhal requested changes to this revision. steakhal added subscribers: tbaeder, ABataev. steakhal added a comment. This revision now requires changes to proceed. I think we have two 2 TPs, that were interesting to see. I've CCd the code owners of the affected parts to confirm this. As a genera

[PATCH] D158295: [NFC][clang][analyzer] Avoid potential dereferencing of null pointer value

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. Thanks for the PR. I went over the code and concluded that it can never be null. However, the code could be improved, while making this explicit. Comment at: c

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal added a comment. 🎉 🚀 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal accepted this revision. steakhal added a comment. In D156312#4588406 , @donat.nagy wrote: > After investigating this issue, I added the testcases > `signed_aritmetic_{good,bad}` which document the current sub-optimal state. > The root cause of

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

steakhal added a comment. Thanks for the xrefs! It turns out I've already seen that one :D I'll look into it once more, with a fresh set of eyes. Thanks again! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126864/new/ https://reviews.llvm.org/D1268

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. The test coverage is also impressive. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 __

[PATCH] D157888: [NFC][Clang] Fix static analyzer concern about null value dereference

steakhal added a comment. If `getStmtForDiagnostics()` in general, never returns null, then shouldn't we mark the API with an appropriate attribute? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157888/new/ https://reviews.llvm.org/D157888 _

[PATCH] D157888: [NFC][Clang] Fix static analyzer concern about null value dereference

steakhal added a comment. Hmm. I guess the assertion is to silence some tool. And I think actually that function might very well also return null in some cases. Why do you think it cannot or at least should not return null in your context? I couldn't infer this from the context, neither from the

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

steakhal added a comment. I notices some inconsistency between this `-fstrict-flex-arrays=N` flag and what the `RecordDecl::hasFlexibleArrayMember()` returns for an example like this: typedef unsigned long size_t; void *malloc(size_t); void free(void *); void field(void) { struct

[PATCH] D150647: [WIP][analyzer] Fix EnumCastOutOfRangeChecker C++17 handling

steakhal added a comment. Isn't D153954 superseded this one? If so, consider abandoning it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150647/new/ https://reviews.llvm.org/D150647

[PATCH] D154603: [analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker

steakhal added a comment. I'm sorry starting the review of this one only now, but I'm quite booked. Is it still relevant? If so, I'll continue. Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:116-117 +const NoteTag *Note = +C.getNoteTag([Reg

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. As I don't use this checker, thus I cannot speak of my experience. However, the reasons look solid and I'm fine with moving this checker to `unix.StdCLibraryFunctions`. Let some time for ot

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

steakhal accepted this revision. steakhal added a comment. Looks safe and good. I'm interested in the diff though. Let me know once you have the results. I wanna have a look before we land this. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:184 +// s

[PATCH] D157702: [clang][analyzer][NFC] Change PlacementNewChecker into PlacementNew in documentation

steakhal added a comment. In D157702#4580384 , @PiotrZSL wrote: > In D157702#4580310 , @steakhal > wrote: > >> Do we have other typos like this? > > I don't think so, only mess in overall documentation: > https:

[PATCH] D157702: [clang][analyzer][NFC] Change PlacementNewChecker into PlacementNew in documentation

steakhal added a comment. Do we have other typos like this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157702/new/ https://reviews.llvm.org/D157702 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D157702: [clang][analyzer][NFC] Change PlacementNewChecker into PlacementNew in documentation

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Correct. Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157702/new/ https://reviews.llvm.org/D157702 ___

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal added a comment. Great progress. Now, moving to the docs. Have you checked that the docs compile without errors and look as you would expect? Please post how it looks. Please make a note about this new checker in the clang's release docs, as we usually announce new checkers and major c

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

steakhal added a comment. Looks pretty good. Comment at: clang/docs/analyzer/checkers.rst:704 +optin.portabilityMinor.UnixAPI +" +Finds non-severe implementation-defined behavior in UNIX/Posix functions. This line should be just as long,

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal added a comment. I haven't gone deep into the implementation, but by only looking at the quality of the code and the structure I assume it implements the right behavior. I've checked the tests though, and they look good. I only found irrelevant nits. Awesome work. --- Have you consider

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

steakhal added a comment. Additionally, it might make sense to first "release" the checker, and after another llvm release, turn this into a Core checker. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152436/new/ https://reviews.llvm.org/D152436

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

steakhal added a comment. Thanks for the sample reports. I'm fine if we want to make it a non-alpha (released) checker. An orthogonal question is, whether we want to have it under the code package. I'm not sure there are official guidance for elevating a checker to code, but here are my assumpti

[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. I went over the patch and I found only a single debatable case for taking by reference. To clarify why I would prefer taking by value: value semantics is good for local reasoning

[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. I can only vouch for the SttaticAnalyzer portion. Those parts makes sense to me. However, I've seen cases where it doesn't resonate. As a rule of thumb, if we are "copying" only at

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

steakhal added a comment. In D150446#4560892 , @donat.nagy wrote: > I'm abandoning this commit because it amalgamates several unrelated changes > and I think it'd be better to handle them separately: > > 1. First, there is a very simple, independent imp

[PATCH] D157114: [clang][ASTImporter] Improve StructuralEquivalence algorithm on repeated friends

steakhal resigned from this revision. steakhal added a comment. In general, I'm not really involved with the ASTImporter. Please, put me as a reviewer if the ASTImporter directly affects the Static Analyzer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal added inline comments. Comment at: clang/docs/analyzer/checkers.rst:35 +core.BitwiseShift (C, C++) +""" + Comment at: clang/docs/analyzer/checkers.rst:44-50 +Moreover, if the pedantic mode is activated by +

[PATCH] D153954: [clang][analyzer] Fix empty enum handling in EnumCastOutOfRange checker

steakhal accepted this revision. steakhal added a comment. LGTM Comment at: clang/test/Analysis/enum-cast-out-of-range.cpp:203 + +enum class empty_unfixed {}; + donat.nagy wrote: > Consider using "specified" and "unspecified" instead of "fixed" and > "unfixed"

[PATCH] D156724: [StaticAnalyzer] Fix incorrect link to "note" diagnostics in HTML output

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe73ae745b0d6: [analyzer] Fix incorrect link to "note" diagnostics in HTML output (authored by gruuprasad, committed by steakhal). Repository: rG

[PATCH] D156724: [StaticAnalyzer] Fix incorrect link to "note" diagnostics in HTML output

steakhal added a comment. In D156724#4556774 , @gruuprasad wrote: > @steakhal, ok, this is my first contribution, I don't have the commit access > yet. > Can you please merge this? What should be the commit author? Repository: rG LLVM Github Monor

[PATCH] D156724: [StaticAnalyzer] Fix incorrect link to "note" diagnostics in HTML output

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Looks correct. Please merge it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156724/new/ https://reviews.llvm.org/D156724

[PATCH] D156787: [analyzer][attr] Add docs to ownership_* attributes

steakhal added inline comments. Comment at: clang/test/Analysis/malloc-annotations.c:151-155 +void af6(void) { + int *p = my_malloc(12); + my_hold(p); + *p = 4; +} Please, consider elaborating on this test with some comments, as it's not clear to me at first

[PATCH] D156201: [ASTImporter] Fix corrupted RecordLayout introduced by circular referenced fields

steakhal added a comment. Have you considered back porting this to clang-17? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156201/new/ https://reviews.llvm.org/D156201 ___ cfe-commits mailing list cfe-co

[PATCH] D156787: [analyzer][attr] Add docs to ownership_* attributes

steakhal added a comment. I haven't looked at the content but I have the feeling that we should probably backport this to release/17.x Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156787/new/ https://reviews.llvm.org/D156787

  1   2   3   4   5   6   7   8   9   10   >