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

This revision was automatically updated to reflect the committed changes. Closed by commit rG95a94df5a9c3: [analyzer][NFC] Use CallEvent checker callback in GenericTaintChecker (authored by steakhal). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D720

[PATCH] D69318: [analyzer] Add SufficientSizeArrayIndexingChecker

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:75 + // Should not warn on literal index expressions. + if (dyn_cast(Index->IgnoreParenCasts())) +return; Shoudn't we use `isa<>` if we don

[PATCH] D70596: [analyzer][docs] NFC: Extend documentation for MallocOverflow checker

steakhal created this revision. steakhal added reviewers: Szelethus, NoQ, krememek. Herald added subscribers: cfe-commits, Charusso, donat.nagy, dexonsmith, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a project: clang. Adds and example for `MallocOv

[PATCH] D74735: [analyzer] Add support for CXXInheritedCtorInitExpr.

steakhal added a comment. Herald added a subscriber: danielkiss. This patch introduced a crash while I was analyzing the libpressio . I was using the `CodeChecker` to drive the analysis with the `--enable-all` flag. The exact command was the following:

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

steakhal added a comment. In D71524#1917251 , @Szelethus wrote: > Are we sure this is what we want? If this is a heuristic, we should document > it well, and even then I'm not sure whether we want it. I'm also pretty sure > this would make the eventual c

[PATCH] D73536: [analyzer][taint] Remove taint from symbolic expressions if used in comparisons

steakhal added a subscriber: martong. steakhal added a comment. I'm convinced that we shouldn't remove taint from expressions used in comparisons. With the current configuration files, `sink` functions are not too useful. For now, I would delay developing a mechanism describing constraints here,

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

steakhal created this revision. steakhal added reviewers: NoQ, Szelethus. steakhal added a project: clang. Herald added subscribers: cfe-commits, Charusso, donat.nagy, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun, whisperity. This patch introduces the `clang_analyzer_isTainte

[PATCH] D73536: [analyzer][taint] Remove taint from symbolic expressions if used in comparisons

steakhal added a comment. I genuinely think that in the following case we should warn, since the user already had a chance to express the range assumption using an `assert`. I think that regardless which checker in what condition checks for a given constraint. If the expression is tainted, we s

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

steakhal updated this revision to Diff 243144. steakhal added a comment. - Tests added. - Clang-format-diff applied. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74131/new/ https://reviews.llvm.org/D74131 Files: clang/docs/analyzer/developer-do

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

steakhal updated this revision to Diff 243146. steakhal added a comment. Clarified example usage, especially in contrast of eg.: `debug.TaintTest` checker. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74131/new/ https://reviews.llvm.org/D74131 F

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

steakhal updated this revision to Diff 243492. steakhal added a comment. Rebased on top of master, instead of D71524 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72035/new/ https://reviews.llvm.org/D72035 Files:

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

steakhal added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:300-301 + `cert-con36-c `_, `bugprone-spuriously-wake-up-functions `_, "Yes" + `cert-con54-cpp `_, `bugprone-spuriously-wake-up-functions `_, "Yes" `cert-dcl03-c `_, `misc-s

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

steakhal updated this revision to Diff 245300. steakhal added a comment. Upload the right diff. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74806/new/ https://reviews.llvm.org/D74806 Files: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp clang/test/An

[PATCH] D73536: [analyser][taint] Remove taint from symbolic expressions if used in comparisons

steakhal created this revision. steakhal added reviewers: NoQ, Szelethus. steakhal added a project: clang. Herald added subscribers: cfe-commits, JDevlieghere. steakhal added a subscriber: boga95. **Remove taint from symbolic expressions if used in comparison expressions.** **Problem statement an

[PATCH] D146194: NOT report the warning that unnamed bitfields are uninitialized.

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. Thanks for looking into this. I could not reproduce the issue with the following test code: https://godbolt.org/z/hsY9PTKGz struct A { unsigned x : 3; unsigned : 29;

[PATCH] D146194: NOT report the warning that unnamed bitfields are uninitialized.

steakhal added a comment. Okay, thanks for clarifying. So, it seems like it depends on the language flavor, but why does CSA behave differently in C and C++? This is necessary to understand if we need to fix that instead of patching this particular checker. I'd still add a test to the `clang/t

[PATCH] D146194: NOT report the warning that unnamed bitfields are uninitialized.

steakhal added a comment. BTW we request full context for git patches. That way the reviewer can scroll and see the context around your hunks. You can request git to emit the full context by passing the `-U99` commandline option. See

[PATCH] D146538: [analyzer] Fix crashing getSValFromInitListExpr for nested initlists

steakhal created this revision. steakhal added reviewers: xazax.hun, NoQ. 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] D146538: [analyzer] Fix crashing getSValFromInitListExpr for nested initlists

steakhal added a comment. I think this one also deserves backporting to clang-16.0.1 @xazax.hun Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146538/new/ https://reviews.llvm.org/D146538 ___ cfe-commits

[PATCH] D146538: [analyzer] Fix crashing getSValFromInitListExpr for nested initlists

steakhal added a comment. In D146538#4210037 , @xazax.hun wrote: > Ugh :/ > > I wonder if we should also open tickets on GitHub to reduce the chance of > forgetting addressing the root cause for these. What do you think? Thanks for the quick review! I

[PATCH] D146538: [analyzer] Fix crashing getSValFromInitListExpr for nested initlists

This revision was automatically updated to reflect the committed changes. Closed by commit rG558b46fde2db: [analyzer] Fix crashing getSValFromInitListExpr for nested initlists (authored by steakhal). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1465

[PATCH] D146538: [analyzer] Fix crashing getSValFromInitListExpr for nested initlists

steakhal added a comment. Speaking of issue tracking, I think it would be much more valuable to our users if we had a proper release notes section for CSA. Right now it is so ad-hoc. That is also true for crash fixes and backports. Maybe we should put more emphasis on that part for the next majo

[PATCH] D146538: [analyzer] Fix crashing getSValFromInitListExpr for nested initlists

steakhal added a comment. This has been backported to clang-16.0.1 as 1172ed57d8234990b277281b3084969fcdb38602 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146

[PATCH] D144780: Explicit cast on customized offsetof should not be ignored when evaluating as const

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. I love it. Short, to the point. Thanks. I think this is the right way to fix this. Good job. Can you merge the change? Or should I do it on you behalf? Repository: rG LLVM Github Monorep

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. First and foremost, I want to deeply apologize about my rushed response. When I say that the `Taint originated here` note remained, I **wrongly** draw conclusions. Won't happen again. __Taint-flow IDs__: I would challenge that we need a flow ID (number) because for me

[PATCH] D144780: Explicit cast on customized offsetof should not be ignored when evaluating as const

steakhal added a comment. In D144780#4154619 , @danix800 wrote: > In D144780#4154487 , @steakhal > wrote: > >> I love it. Short, to the point. Thanks. >> I think this is the right way to fix this. >> Good job. >>

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. If we worry about having taint-related reports without a Note message explaining where the taint was introduced, we could just assert that in a `BugReportVisitor` at the `finalizeVisitor()` callback. I think such an assertion would make a lot of sense. To achieve this,

[PATCH] D140891: [analyzer] Fix assertion failure in SMT conversion for unary operator on floats.

steakhal added a comment. I'm proposing to backport this fix to clang-16. https://github.com/llvm/llvm-project/issues/61097 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140891/new/ https://reviews.llvm.org/D140891

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

steakhal added a comment. I think we should backport this to `release/16.x`. Here is the ticket for it: https://github.com/llvm/llvm-project/issues/61115 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138713/new/ https://reviews.llvm.org/D138713 __

[PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

steakhal added a comment. Herald added a subscriber: thopre. This patch breaks `llvm::StringSet` equality. The following code would no longer compile: llvm::StringSet LHS; llvm::StringSet RHS; bool equal = LHS == RHS; Such code might be used as gtest assertions like `EXPECT_EQ(LHS, RHS)`.

[PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

steakhal added a comment. Oh, I forgot to mention why this change breaks the equality operator of `llvm::StringSet`. It's because the `StringMap::operator==` will try to compare the `value` as well, which is of type `std::nullopt_t` and that has no equality comparison operator. Previously, the

[PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

steakhal added a comment. Hm, it would be easier to use `llvm::StringMap` instead of `std::nullopt_t`, or some adhoc empty struct. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138539/new/ https://reviews.llvm.org/D138539

[PATCH] D142627: [analyzer] Fix crash exposed by D140059

steakhal added a comment. I'm backporting this to clang-16. https://github.com/llvm/llvm-project/issues/61149 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142627/new/ https://reviews.llvm.org/D142627 __

[PATCH] D142328: [clang][Interp] Fix compound assign operator types

steakhal added a comment. Hi @tbaeder, I was looking for commits that mentions "fix" "assertion" or "crash" in the title, that are part of the `main` branch but not backported to `release/16.x` to be eventually released as `clang-16`. I wonder what's the status of this and the related patches gi

[PATCH] D142384: [C++20] Fix a crash with modules.

steakhal added a comment. @usaxena95 @ilya-biryukov, @hans, I wonder if we should backport this change to `release/16.x`. Otherwise, clang-16 won't have this fix. WDYT? I'm also a bit worried that we don't have tests. And that we have "unexpected" sideeffects like what you mentioned @hans. Give

[PATCH] D144546: [clang][dataflow] Fix assert for CXXConstructExpr argument number

steakhal added a comment. By looking at the title, I get the impression that this fixes an assertion violation. I also observed that this commit is part of `main` but not part of `release/16.x`, hence the `clang-16` would be released without this fix. I want to raise awareness of backporting cr

[PATCH] D144546: [clang][dataflow] Fix assert for CXXConstructExpr argument number

steakhal added a comment. In D144546#4171238 , @ymandel wrote: > In D144546#4167161 , @steakhal > wrote: > >> By looking at the title, I get the impression that this fixes an assertion >> violation. >> I also ob

[PATCH] D144546: [clang][dataflow] Fix assert for CXXConstructExpr argument number

steakhal added a comment. FYI the release schedule is here: https://llvm.org/docs/HowToReleaseLLVM.html#annual-release-schedule Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144546/new/ https://reviews.llvm.org/D144546 ___

[PATCH] D144780: Explicit cast on customized offsetof should not be ignored when evaluating as const

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG53f75425b3fe: [analyzer] Explicit cast on customized offsetof should not be ignored when… (authored by danix800, committed by steakhal). Changed pr

[PATCH] D144780: Explicit cast on customized offsetof should not be ignored when evaluating as const

steakhal added a comment. Ah sorry that it took me so long to push this. I completely forgot about this one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144780/new/ https://reviews.llvm.org/D144780 __

[PATCH] D144780: Explicit cast on customized offsetof should not be ignored when evaluating as const

steakhal added a comment. As this resolves a crash, I'm backporting it: https://github.com/llvm/llvm-project/issues/61245 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144780/new/ https://reviews.llvm.org/D144780 _

[PATCH] D152435: [analyzer][CStringChecker] Adjust the invalidation operation on the super region of the destination buffer during string copy

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:286-287 + SVal V, + std::function); I'd highly suggest making this a template taking the functor that way. Given that we m

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

steakhal resigned from this revision. steakhal added a comment. I resign as a reviewer as I'm not deeply connected to this checker, thus I won't block it or accept it. However, my opinion is that a checker should be "released" if they have clear diagnostics (which includes that it doesn't flood

[PATCH] D152435: [analyzer][CStringChecker] Adjust the invalidation operation on the super region of the destination buffer during string copy

steakhal added a comment. In D152435#4433147 , @OikawaKirie wrote: > BTW, what does the `Done` checkbox mean in the code comments? "Done" means "Resolved" on GitHub - which is basically that some action was taken or the person who raised the issue with

[PATCH] D153776: [clang][analyzer] Display notes in StdLibraryFunctionsChecker only if interesting

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1325 if (!Pred) -break; +continue; } Why do you continue here? Do you have a case for this? Repository: rG LLVM Github Mo

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. In D144269#4237539 , @dkrupp wrote: > This is a totally rewritten version of the patch which solely relies on the > existing "interestingness" utility to track back the taint propagation. (And > does not introduce a new FlowI

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. Looks even better. Only minor concerns remained, mostly about style and suggestions of llvm utilities. Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:129-130 /// Given a pointer/reference argument, return the value it refers to

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. I think as we converge, now it could be time to update the summary of the patch to reflect the current implementation. (e.g. flowids etc.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144269/new/ https://reviews.llvm.org/D144269 _

[PATCH] D151325: [analyzer] Differentiate lifetime extended temporaries

steakhal added a comment. Please leave a link in the summary to the RFC to discuss. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151325/new/ https://reviews.llvm.org/D151325 ___ cfe-commits mailing list

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

steakhal added a comment. I'll come back one or two weeks later. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150446/new/ https://reviews.llvm.org/D150446 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D151225: [clang][analyzer] Merge apiModeling.StdCLibraryFunctions and StdCLibraryFunctionArgs checkers into one.

steakhal added a comment. I think I'm also for this change. I also wonder why we can't enable posix modeling by default. If we still can't, then when will we? Do you think it would make sense to enable them in the future? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Yea, it still looks okay. Let me know if you don't have commit access. In that case, also let me know what should be used for the commit author. Repository: rG LLVM Github Monorepo CHA

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

steakhal added a comment. Ah, I can see that pre-merge bots failed due to clang-format violations. Please make all the touched lines as clang-formatted prior to committing anything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150430/new/ https:/

[PATCH] D151651: [StaticAnalyzer] Fix block pointer type nullability check

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Looks good. That pesky ObjC :D Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151651/new/ https://reviews.llvm.org/D151651 _

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

steakhal accepted this revision. steakhal added a comment. Can you commit this change or should I do it on your behalf? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150430/new/ https://reviews.llvm.org/D150430

[PATCH] D151651: [StaticAnalyzer] Fix block pointer type nullability check

This revision was automatically updated to reflect the committed changes. Closed by commit rG993060e1d31d: [StaticAnalyzer] Fix block pointer type nullability check (authored by tripleCC, committed by steakhal). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D151727: [clang] Replace dyn_cast with cast in MemRegion::getMemorySpace

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

[PATCH] D151726: [clang] Remove unnecessary casts around Allocate function calls

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

[PATCH] D151725: [clang] Move dyn_cast's into if statements for readability

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. I think should mention "NFC" for such commits. Also, we tend to use the "analyzer" tag for CSA related changes. BTW thanks for these simplifications. Repository: rG LLVM Github Monorep

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

steakhal added a comment. TBH we don't really have processes there. And its a bit of a mess, regarding CSA. Usually we forgot to update the release notes, and at best we aggregate the changes just prior a release for the release notes. But we haven't done it for the last two releases for sure.

[PATCH] D143867: [analyzer] Fix SARIF column location assertion crash

steakhal added a comment. Looks sensible to me. Do you have a test for triggering the previous assertion? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143867/new/ https://reviews.llvm.org/D143867 ___ cf

[PATCH] D143867: [analyzer] Fix SARIF column location assertion crash

steakhal added a comment. In D143867#4121841 , @Scarlet1ssimo wrote: > In D143867#4121835 , @steakhal > wrote: > >> Looks sensible to me. >> Do you have a test for triggering the previous assertion? > > I do hav

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

steakhal added a comment. @NoQ @xazax.hun I plan to merge this by the end of the week if you have no objection. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138037/new/ https://reviews.llvm.org/D138037 ___

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

steakhal added a comment. I would also suggest backporting this to the `release/16.x` branch as that has already branched off. WDYT? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138037/new/ https://reviews.llvm.org/D138037 __

[PATCH] D143867: [analyzer] Fix SARIF column location assertion crash

steakhal added a comment. In D143867#4121874 , @Scarlet1ssimo wrote: >> The test passes on main. Are you sure about the reproducer? > > This happens on versions from release/10.x to release/15.x. Since 16.x, this > file gets refactored and no such probl

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGafcf70aa6de7: [analyzer] Remove unjustified assert from EquivalenceClass::simplify (authored by steakhal). Changed prior to commit: https://reviews.llvm.org/D138037?vs=475480&id=498301#toc Repository:

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

steakhal added a comment. I'm backporting this as #60834 to the 16.x branch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138037/new/ https://reviews.llvm.org/D138037 __

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

steakhal added a comment. In D138037#4129160 , @xazax.hun wrote: > I am ok with committing this to unblock people hitting this assert, but at > the same time I wonder if we want to open a ticket on GitHub that we might > want to rethink how some of this

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

steakhal added a comment. In D138037#4134504 , @steakhal wrote: > I'm backporting this as #60834 > to the 16.x branch. Backported with 1ab37a25e463ea144289a1cbcf32a7659b2d60bb

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. I haven't checked the implementation, but fundamentally patching the TaintBugVisitor is not how we should improve the diagnostic for taint issues. I saw that this patch is not about NoteTags, so I didn't go any further that point. What we should do instead, to add a fa

[PATCH] D149158: [clang][analyzer] Cleanup tests of StdCLibraryFunctionsChecker (NFC)

steakhal added inline comments. Comment at: clang/test/Analysis/Inputs/std-c-library-functions.h:1-2 +typedef typeof(sizeof(int)) size_t; +typedef signed long ssize_t; +typedef struct { donat.nagy wrote: > balazske wrote: > > steakhal wrote: > > > balazske wrote:

[PATCH] D149158: [clang][analyzer] Cleanup tests of StdCLibraryFunctionsChecker (NFC)

steakhal added inline comments. Comment at: clang/test/Analysis/Inputs/std-c-library-functions.h:1-2 +typedef typeof(sizeof(int)) size_t; +typedef signed long ssize_t; +typedef struct { balazske wrote: > steakhal wrote: > > donat.nagy wrote: > > > balazske wrote:

[PATCH] D149158: [clang][analyzer] Cleanup tests of StdCLibraryFunctionsChecker (NFC)

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. LGTM, premerge checks are green AFAICT. Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149158/new/ https://reviews.llvm.org/D149158

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

steakhal added a comment. I still struggle to see the motivation. Please consider adding a test demonstraing a case when the diagnostic is not relevant or easily misunderstood by the users. Alternatively, give an evaluation of this change so that I can see it myself. In the end it would still m

[PATCH] D149160: [clang][analyzer] Handle special value AT_FDCWD in affected standard functions

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

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

steakhal added a comment. > In C++17 the initialization rules for enum classes are relaxed. In what way are they relaxed compared to regular enums? Anyway, IMO those are indeed FPs, see https://godbolt.org/z/7z984bz6v I'm looking forward to the fix. Thanks. Repository: rG LLVM Github Monore

[PATCH] D150552: [analyzer] Fix QTimer::singleShot NewDeleteLeaks false positive

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Good job. Looks good to me. This is pretty much analogous to D27717 . The same reasoning applies. Thanks for the patch. Should I commit this on your behalf?

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

steakhal added a comment. I like it. How about an implementation like this: void CStringChecker::evalSprintfCommon(CheckerContext &C, const CallExpr *CE, bool IsBounded) const { ProgramStateRef State = C.getState(); DestinationArgExpr Dest = {CE

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

steakhal added a comment. Here is the migrated version of the mentioned discussion: https://discourse.llvm.org/t/static-or-dynamic-code-analysis-for-undefined-behavior-in-sprintf/59624 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150430/new/ http

[PATCH] D150531: Fix start index for sprintf ovlerap check + tests

steakhal added a comment. I think you should squash this into D150430 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150531/new/ https://reviews.llvm.org/D150531 ___ cf

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

steakhal added a comment. In D150446#4345723 , @donat.nagy wrote: > By the way, I'm fed up with the hack that ElementRegion is used for three > separate things ("real" array indexing, casts and pointer arithmetic). To fix > this I'm thinking about intr

[PATCH] D150552: [analyzer] Fix QTimer::singleShot NewDeleteLeaks false positive

steakhal closed this revision. steakhal added a comment. And I managed to make a typo in the commit message :D Closed by 3b6a368d763e812024ca6ba4024855603f693291 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[PATCH] D150430: Implement BufferOverlap check for sprint/snprintf

steakhal added a comment. In D150430#4347365 , @ArnaudBienner wrote: > Thanks for the review :) > > I implemented the suggested changes. > > Just one question: `PointeeTy.isNull()`: is this guaranteed to always work > even though `getType()->isAnyPointe

[PATCH] D150744: [NFC][CLANG] Fix uninitialized scalar field issues found by Coverity

steakhal added a comment. Speaking of the `StaticAnalyzer`, I think setting some default value is a fair point, and I think it's a good practice. In our case, we initialized that field from the registration function, like this: void ento::registerMoveChecker(CheckerManager &mgr) { MoveChe

[PATCH] D148355: [analyzer] Fix comparison logic in ArrayBoundCheckerV2

steakhal added a comment. This subject haunted us for quite some time now, and there is more behind what it seems at first. If I'm not mistaken, this patch is pretty much what is in D86874 except that it doesn't give up and still checks the upper-bound in this

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. Nice improvement! I only have minor nitpicks and some recommendations for the taint API. Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:2

[PATCH] D148355: [analyzer] Fix comparison logic in ArrayBoundCheckerV2

steakhal added a subscriber: tomasz-kaminski-sonarsource. steakhal added a comment. In D148355#4279866 , @donat.nagy wrote: > @steakhal Thanks for the background information! > > I didn't know about D86874 so I indeed >

[PATCH] D148639: [NFC][clang] Fix static analyzer concerns about AUTO_CAUSES_COPY

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1053 - for (auto I : CleanedState->get()) { + for (const auto &I : CleanedState->get()) { if (SymbolRef Sym = I.second.getAsSymbol()) I think this is supposed to be

[PATCH] D148355: [analyzer] Fix comparison logic in ArrayBoundCheckerV2

steakhal added a comment. Sounds good. Thanks for clarifying. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148355/new/ https://reviews.llvm.org/D148355 ___ cfe-commits mailing list cfe-commits@lists.llv

[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. I liked the idea of sorting these options at compile time. I think the most dummy sort should work just fine inthis case, as the number of elements are not that large. Do you think worth it

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. I didn't go through the whole revision this time, but I think the next steps are already clear for the next round. My impression was that I might not expressed my intent and expec

[PATCH] D148355: [analyzer] Fix comparison logic in ArrayBoundCheckerV2

steakhal added a comment. I like this. Looks really solid. I only had style nitpicks. Have you run measurements? After that we could land it, I think. FYI: I'll also schedule a test run with this patch. You should expect the results in a week - not because it takes that long, but because I need

[PATCH] D148355: [analyzer] Fix comparison logic in ArrayBoundCheckerV2

steakhal added a comment. Okay, so you just added some numbers :D Now it's my turn I guess. I'll compare this version against the version I shared, and use in our product. Stay tuned. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148355/new/ https

[PATCH] D148355: [analyzer] Fix comparison logic in ArrayBoundCheckerV2

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:167-168 - NonLoc rawOffsetVal = rawOffset.getByteOffset(); - - // CHECK LOWER BOUND: Is byteOffset < extent begin? - // If so, we are doing a load/store - // before the f

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal added a comment. To conclude the review, please respond to the "Not Done" inline comments, and mark them "Done" if you think they are resolved. Thank you for your patience. Comment at: clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp:108-109 + if ((stateNotZero &

[PATCH] D148355: [analyzer] Fix comparison logic in ArrayBoundCheckerV2

steakhal added a comment. Please mark comments "Done" where applicable. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:173 + const MemSpaceRegion *SR = rawOffset.getRegion()->getMemorySpace(); + if (SR->getKind() != MemRegion::UnknownSpaceRegionKind) {

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. LGTM. About formatting the tests: Personally, I would have preferred to "clean as you code", but I can see your point. Leave it as-is. Land it, please. CHANGES SINCE LAST ACTION https:/

[PATCH] D148355: [analyzer] Fix comparison logic in ArrayBoundCheckerV2

steakhal added a comment. In D148355#4294738 , @donat.nagy wrote: > @steakhal I marked a few comments as Done (I accidentally missed some when I > was creating the most recent patch) and now the only not-Done thing is the > followup commit for refactor

[PATCH] D148355: [analyzer] Fix comparison logic in ArrayBoundCheckerV2

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. In D148355#4294798 , @steakhal wrote: > In D148355#4294738 , @donat.nagy > wrote: > >> @steakhal I marke

[PATCH] D149158: [clang][analyzer] Cleanup tests of StdCLibraryFunctionsChecker (NFC)

steakhal added inline comments. Comment at: clang/test/Analysis/Inputs/std-c-library-functions.h:1-2 +typedef typeof(sizeof(int)) size_t; +typedef signed long ssize_t; +typedef struct { `ssize_t`'s size should match the size of `size_t`. In this implementation, i

<    8   9   10   11   12   13   14   15   16   >