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

donat.nagy added a comment. In D158707#4621135 , @steakhal wrote: > 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 doe

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

donat.nagy added a comment. Yes, adding tests that demonstrate the current behavior is a good idea. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158707/new/ https://reviews.llvm.org/D158707 ___ cfe-comm

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

donat.nagy added a comment. 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 larger codebases?) As far as I understand (correct me if I'm wrong), there are situations wh

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

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. This seems to be a straightforward improvement over the current situation; LGTM if you test(ed) it on some real-life code (to ensure that it doesn't introduce a corner case that crashe

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

donat.nagy added a comment. In D159105#4627785 , @steakhal wrote: > In D159105#4627724 , @donat.nagy > wrote: > >> I was thinking about adding an improvement like this for the sake of >> consistency, but I fear

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

donat.nagy added a comment. Good direction of development, this will be useful for providing better bug reports (in addition to ensuring correct behavior some situations). Note that it's also possible to dereference pointers with the operator `->`, which is represented by `MemberExpr`s in the A

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

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. LGTM if the test results are also good. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159109/new/ https://reviews.llvm.org/D159109

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

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. LGTM if the test results are good. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159108/new/ https://reviews.llvm.org/D159108 _

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

donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:34 +if (auto SSize = +SVB.convertToArrayIndex(*Size).getAs()) + return *SSize; danix800 wrote: > donat.nagy wrote: > > I think it's a good conventio

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

donat.nagy added a comment. 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 `&`. If the past-the-end pointer ends up dereferenced later, the current code is sufficient

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

donat.nagy added a comment. In D159107#4630764 , @steakhal wrote: > 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

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

donat.nagy 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-wa

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

donat.nagy added a comment. 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 details of the heuristics. (1) I don't think that code like `int *ptr; scanf("%ld", &ptr);

[PATCH] D159412: [analyzer]FieldRegion in getStaticSize should return size of pointee type

donat.nagy added a comment. Please add a testcase that demonstrates this issue (fails when your change in MemRegion.cpp isn't added) and shows that your commit fixes it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159412/new/ https://reviews.llv

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

donat.nagy added inline comments. Comment at: clang/docs/analyzer/checkers.rst:1793-1803 +.. code-block:: cpp + + Base *create() { + Base *x = new Derived[10]; // note: conversion from derived to base + // happened here + return x; + } -

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

donat.nagy requested changes to this revision. donat.nagy added a comment. This revision now requires changes to proceed. I reviewed this change and collected my suggestions in comments. In general, I feel that the code added by this commit is "sloppy" in the sense that it's designed for the com

[PATCH] D159479: [ASTImport]enhance statement comparing

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. Today we learned that truth is different from falsehood... LGTM, nice catch! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159479/new/

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

donat.nagy added a comment. @vabridgers Thanks for testing these commits! As @balazske wrote on D144622 , I'd suggest handling the three commits (this one = D140562 , D144622 and D144273

[PATCH] D144622: [clang[[ASTImporter] Import TemplateName correctly

donat.nagy added a comment. Basically LGTM (assuming that the TC passes), I added two minor suggestions, but I'm not opposed to merging this in its current state. Comment at: clang/unittests/AST/ASTImporterTest.cpp:8142-8150 + R"( + template + struct A; +

[PATCH] D144273: [clang][ASTImporter] Add VaList declaration to lookup table.

donat.nagy added a comment. @balazske > I tried to find out how to add a correct test but could not check if this > fails or not on AArch64 platform. [...] I want to touch ASTContext only if a > test failure is found on AArch64 that makes it necessary. It's possible to "simulate" the AArch64 b

[PATCH] D144622: [clang][ASTImporter] Import TemplateName correctly

donat.nagy accepted this revision. donat.nagy added a comment. LGTM as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144622/new/ https://reviews.llvm.org/D144622 ___ cfe-commits mailing list cfe-co

[PATCH] D144273: [clang][ASTImporter] Add VaList declaration to lookup table.

donat.nagy accepted this revision. donat.nagy added a comment. LGTM, the test result sounds promising, let's hope that it'll work in the CI as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144273/new/ https://reviews.llvm.org/D144273 __

[PATCH] D145229: [analyzer] Improve the documentation of the alpha.security.taint.TaintPropagation checker

donat.nagy added inline comments. Comment at: clang/docs/analyzer/checkers.rst:78-80 +The ``SuppressAddressSpaces`` option suppresses warnings for null dereferences of all pointers with address spaces. You can disable this behavior with the option Why is this p

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

donat.nagy added a comment. In D152436#4432736 , @balazske wrote: > [...] > From these two solutions, which one is better? (Show many unnecessary notes, > or show only necessary ones but lose some of the useful notes too.) I think we must avoid pollutin

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

donat.nagy added a comment. In D152436#4437822 , @steakhal wrote: > In D152436#4437692 , @donat.nagy > wrote: > >> Personally I think it's completely acceptable if the analyzer sometimes >> emits bug reports tha

[PATCH] D153424: [clang][ASTImporter] Add import of CXXRewrittenBinaryOperator.

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. LGTM, this is a straightforward fix and if the repo can survive three copies of std-compare.h, then it'll also survive four copies. However I created a ticket (in our internal system)

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

donat.nagy added a comment. This is a good and useful commit; but I have some questions connected to the state transition handling code that you had to modify. (The State/ExplodedNode APIs of the engine are really counter-intuitive... :( ) Comment at: clang/lib/StaticAnalyze

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

donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1309 +[Node, Note](PathSensitiveBugReport &BR) -> std::string { + if (Node->succ_size() > 1) +return Note.str(); --

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

donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1299 +// StdLibraryFunctionsChecker. +ExplodedNode *Pred = const_cast(Node); +if (!Case.getNote().empty()) { balazske wrote: > donat.nagy w

[PATCH] D153889: [analyzer][NFC] Fix dangling StringRef in barely used code

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

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

donat.nagy added a comment. The source code changes LGTM. I'll soon check the results on the open source projects and give a final approval based on that. By the way, I think this commit and the "display notes only if interesting" follow-up change (D153776 ) sh

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

donat.nagy requested changes to this revision. donat.nagy added a comment. This revision now requires changes to proceed. As I started to review the code of the follow-up commit D153776 , I noticed a dangling `StringRef` bug which belongs to this review. Moreov

[PATCH] D153674: [dataflow] Disallow implicit copy of Environment, use fork() instead

donat.nagy added a comment. I'm also using `gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0` (which supported according to the LLVM "Getting Started" page ) and I see the same std::optional compilation errors. Please r

[PATCH] D153363: [clang][analyzer] No end-of-file when seek to file begin.

donat.nagy accepted this revision. donat.nagy 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/D153363/new/ https://reviews.llvm.org/D153363 __

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

donat.nagy added a comment. Thanks for the update! I have two minor remarks (string handling is complicated in C++...) but the code looks good otherwise. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1305 + std::string Note = + llv

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

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. LGTM, but please wait until you can merge this together with D153776 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

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

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. LGTM if you rebase these changes onto the most recent version of D153612 . Thanks for adding this interestingness check! Repository: rG LLVM Githu

[PATCH] D153674: [dataflow] Disallow implicit copy of Environment, use fork() instead

donat.nagy added a comment. Thank you for the fix :) ! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153674/new/ https://reviews.llvm.org/D153674 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D153674: [dataflow] Disallow implicit copy of Environment, use fork() instead

donat.nagy added a comment. When I tried to compile with gcc 7.5 after your fix commit (2f7d30dee826 ), I got another very similar compilation error from a different location and I pushed commit cec30e2b190b

[PATCH] D153889: [analyzer][NFC] Fix dangling StringRef in barely used code

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1d75b18843fb: [analyzer][NFC] Fix dangling StringRef in barely used code (authored by donat.nagy). Repository: rG LLVM Github Monorepo CHANGES SI

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

donat.nagy requested changes to this revision. donat.nagy added a comment. This revision now requires changes to proceed. Thanks for attaching the test results! Unfortunately it seems that it was important to look at them, because I noticed that in the posgresql codebase there are many bug repor

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

donat.nagy added a comment. See results and their discussion on the review of the tightly connected follow-up commit D153776 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153612/new/ https://reviews.llvm.org/D15

[PATCH] D145868: [clang][ASTImporter] Fix import of typedef with unnamed structures

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. This commit eliminates crashes caused by a rare corner case (typedefs to types derived from unnamed structs) of the AST import procedure; but introduces some incorrect behavior in a ra

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

donat.nagy created this revision. donat.nagy added reviewers: steakhal, NoQ, gamesh411, Szelethus. donat.nagy added a project: clang-tools-extra. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. Herald added a proje

[PATCH] D124244: [analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

donat.nagy added a comment. Overall this seems to be a promising checker; I added a few minor remarks as inline comments. Unfortunately @steakhal's comment that > The `check::PostCall` handler wants to mark some memory region immutable. > Currently, the checker creates a new //symbolic// memreg

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

donat.nagy added a comment. @steakhal What is your first impression about this commit? Is this the (or at least //a//) good direction? May I continue with creating additional improvements (e.g. better error messages) that depend on this commit? Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D151133: [clang-tidy] Ignore implicit code in bugprone-branch-clone

donat.nagy added a comment. For me it's an unfortunate surprise that Clang Tidy is traversing the concrete //template instantiation// (I would've guessed that it's working on the original template body), but if that's the observed behavior, then this commit is a clean and straightforward fix.

[PATCH] D147889: [clang-tidy] Improve bugprone-branch-clone with support for fallthrough attribute

donat.nagy added a comment. Mostly LGTM, I suggested two cosmetic changes. Comment at: clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp:55 +bool TraverseLambdaExpr(LambdaExpr *, DataRecursionQueue * = nullptr) { + // Ignore lambdas + return true; ---

[PATCH] D151133: [clang-tidy] Ignore implicit code in bugprone-branch-clone

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. If this freshly added testcase passes, then there is no reason to delay merging this commit. Thanks for the contribution :) ! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D147889: [clang-tidy] Improve bugprone-branch-clone with support for fallthrough attribute

donat.nagy accepted this revision. donat.nagy 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/D147889/new/ https://reviews.llvm.org/D147889 __

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

donat.nagy added a comment. @NoQ **Re: ElementRegion hacks:** Your suggestion is very convincing, and I agree that while my idea would bring some clarity to some particular issues, overall it would just worsen the "chaotic heap of classes" situation. The only advantage of my solution is that it

[PATCH] D143917: [clang-tidy] Clarify branch-clone diagnostic message

donat.nagy created this revision. donat.nagy added reviewers: aaron.ballman, njames93, carlosgalvezp. donat.nagy added a project: clang-tools-extra. Herald added subscribers: gamesh411, Szelethus, dkrupp, xazax.hun. Herald added a project: All. donat.nagy requested review of this revision. This si

[PATCH] D143917: [clang-tidy] Clarify bugprone-branch-clone diagnostic message

donat.nagy added a comment. Thanks for the feedback! I can push the commit and I'll do so after a few days (if no changes are suggested). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143917/new/ https://reviews.llvm.org/D143917 _

[PATCH] D138777: [clang-tidy] Add check bugprone-multiple-new-in-one-expression.

donat.nagy added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp:44-45 +return BinOp->getOpcode() == BO_Assign && + BinOp->getRHS()->IgnoreParenCasts() == E; + + return isa(ParentE); In genera

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

donat.nagy added a comment. In D142822#4102795 , @balazske wrote: > I looked at some of the failing tests but can not decide how to fix the > problems. The problem seems to be with the extra `namespace std` that was not > there before. I do not know wha

[PATCH] D144273: [clang][ASTImporter] Add VaList declaration to lookup table.

donat.nagy added a comment. Note that the Debian x64 test failure is from the unit test TEST(ClangdServer, MemoryUsageTest) { MockFS FS; MockCompilationDatabase CDB; ClangdServer Server(CDB, FS, ClangdServer::optsForTest()); auto FooCpp = testPath("foo.cpp"); Server.addDo

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. We reviewed this commit together with @gamesh411 and it looks good to us; we see that it eliminates a possibility where a type object could "acquire" multiple aliases. Repository:

[PATCH] D144273: [clang][ASTImporter] Add VaList declaration to lookup table.

donat.nagy added a comment. @gamesh411 and I tried to understand the consequences of this change. We think it might be problematic that calling `ASTContext::getVaListTagDecl` [1] creates the VaList declaration (and on certain platforms, the declaration of `std`) even in the cases when these dec

[PATCH] D144273: [clang][ASTImporter] Add VaList declaration to lookup table.

donat.nagy added a comment. @balazske Thanks for clarifying the side effects of the current solution and I support creating that side-effect-free getter function -- it sounds to be a good solution for this problem. One minor doubt: I can theoretically imagine the case when the TU object doesn't

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

donat.nagy 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: > > balazske wrote: > > > steakhal wrote:

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

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

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

donat.nagy added a comment. After some thinking and internal discussion I decided that it'd be better to replace the callback used by this checker. This is a clean but rough draft of this concept; for a final version I'd consider: - adding a secondary callback that handles `*ptr` equivalently

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

donat.nagy updated this revision to Diff 521640. donat.nagy added a comment. //(Re-uploaded patch to apply clang-format changes and remove a stray temporary comment.)// Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150446/new/ https://reviews.llvm

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

donat.nagy planned changes to this revision. donat.nagy added a comment. I started to create tests to demonstrate the differences from the baseline, and it turns out that my patch is handling multidimensional arrays incorrectly. I'll debug this issue and upload a fixed version of this patch. R

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

donat.nagy updated this revision to Diff 522544. donat.nagy edited the summary of this revision. donat.nagy added a comment. I managed to debug the issue that plagued my commit (see `test_multidim_zero()` for details, I think I'll fix it in a followup change); and I have uploaded a new version o

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

donat.nagy added a comment. 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) and I'm thinking about introducing a subclass hierarchy where a base class `ElementLikeRegion` has three subclasses called

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

donat.nagy added a comment. I'll move this class hierarchy modification question into a discourse thread after collecting the relevant facts. (By the way, thanks for suggesting discourse, I was not aware of it!) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

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

donat.nagy added a comment. @steakhal Thanks for the background information! I didn't know about D86874 so I indeed ended up with something very similar to it. I reviewed D88359 and I knew that it's a completely general soluti

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

donat.nagy added a comment. @steakhal Then I'll clean up my own solution, which will be completely independent of the patch of Tomasz (except for the obviously identical changes in the test code). The de-duplication that I'm planning is not pure NFC, because it'll probably affect some mostly-t

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

donat.nagy updated this revision to Diff 515326. donat.nagy edited the summary of this revision. donat.nagy added a comment. I'm publishing the extended variant of this commit (which I mentioned in earlier comments). This generalizes the first version of the commit to check for the unsigned-vs-

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

donat.nagy added a comment. I tested this patch on a few open source projects and it significantly reduced the number of alpha.security.ArrayBoundV2 reports: | Project name | memcached | tmux | curl | twin | vim | openssl | | Baseline report #| 0 | 2| 13 | 6

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

donat.nagy added a comment. @steakhal Thanks for the review, it was really instructive :). I'll probably upload the updated commit on Monday. What kind of measurements are you suggesting? I analyzed a few open source projects with the patched Clang SA (see results in my previous commit) and a

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

donat.nagy updated this revision to Diff 516365. donat.nagy marked 11 inline comments as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148355/new/ https://reviews.llvm.org/D148355 Files: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.

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

donat.nagy marked an inline comment as done. donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:173 + const MemSpaceRegion *SR = rawOffset.getRegion()->getMemorySpace(); + if (SR->getKind() != MemRegion::UnknownSpaceRegionKin

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

donat.nagy marked 4 inline comments as done. donat.nagy added a comment. @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 refactoring the optionalness of RegionRawOffsetV2.

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

donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:173 + const MemSpaceRegion *SR = rawOffset.getRegion()->getMemorySpace(); + if (SR->getKind() != MemRegion::UnknownSpaceRegionKind) { +// a pointer to UnknownSpaceRegionK

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

donat.nagy closed this revision. donat.nagy added a comment. Committed in rGde2547329b41ad6ea4ea876d12731bde5a6b64c5 (which accidentally refers to a wrong Phabricator review). Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

donat.nagy added a comment. Commit rGde2547329b41ad6ea4ea876d12731bde5a6b64c5 accidentally refers to this review, but in fact it belongs to D148355 . Repository: rG LLVM Github Monorepo

[PATCH] D149259: [analyzer][NFC] Use std::optional instead of custom "empty" state

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

[PATCH] D149259: [analyzer][NFC] Use std::optional instead of custom "empty" state

donat.nagy updated this revision to Diff 517831. donat.nagy edited the summary of this revision. donat.nagy added a comment. As I was finalizing this commit, I noticed that the logic of `RegionRawOffsetV2::computeOffset()` is presented in a ~~crazy~~ unconventional way; so I rewrote it (while pr

[PATCH] D149259: [analyzer][NFC] Use std::optional instead of custom "empty" state

donat.nagy marked an inline comment as done. donat.nagy added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149259/new/ https://reviews.llvm.org/D149259 ___ cfe-commits mailing list cf

[PATCH] D149460: [analyzer] ArrayBoundCheckerV2: suppress false positives from ctype macros

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

[PATCH] D149259: [analyzer][NFC] Use std::optional instead of custom "empty" state

donat.nagy added a comment. @steakhal Could you review this change when you have some time for it? I'm planning to stabilize and de-alpha this checker with a series of incremental changes; and it'd be good have this commit and the unrelated tweak https://reviews.llvm.org/D149460 merged before I

[PATCH] D149460: [analyzer] ArrayBoundCheckerV2: suppress false positives from ctype macros

This revision was automatically updated to reflect the committed changes. Closed by commit rG8c22cbea87be: [analyzer] ArrayBoundCheckerV2: suppress false positives from ctype macros (authored by donat.nagy). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D149259: [analyzer][NFC] Use std::optional instead of custom "empty" state

donat.nagy added a comment. > It's gonna be bumpy. Yup :) but it's not impossible... > Also, note that this is in production here, so it will take me time to verify > the patches. So, I'd probably prefer to have a patch stack, and do the > evaluation for some selected patches - basically bundl

[PATCH] D149259: [analyzer][NFC] Use std::optional instead of custom "empty" state

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb88023c25729: [analyzer][NFC] Use std::optional instead of custom "empty" state (authored by donat.nagy). Repository: rG LLVM Github Monorepo CHA

[PATCH] D149259: [analyzer][NFC] Use std::optional instead of custom "empty" state

donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:313-315 +} else if (const auto *SRegion = dyn_cast(Region)) { + // NOTE: The dyn_cast<>() is expected to succeed, it'd be very surprising + // to see a MemSpaceRe

[PATCH] D152255: [analyzer][NFC] Pass the diagnostic message to the TrackConstraintBRVisitor

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

[PATCH] D152255: [analyzer][NFC] Pass the diagnostic message to the TrackConstraintBRVisitor

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb16a59328fc5: [analyzer][NFC] Pass the diagnostic message to the TrackConstraintBRVisitor (authored by Endre Fulop ,

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

donat.nagy added a comment. Thanks for the review and the testing! >> [...] What do you think about this design direction? > > Could you elaborate on this part? In short, this review should've been a discourse thread about my "switch to the PreStmt route" plan: I wanted to ask for architectural

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

donat.nagy added a comment. Hmm, I agree that a new checker might be better for the situation when we'd sacrifice a significant amount of TPs, but after reconsidering the situation I think that my concerns are mostly theoretical: if the current code reports an issue, then it's probably not too

<    1   2