[PATCH] D52423: [analyzer] Make ConversionChecker load StdCLibraryFunctionsChecker

donat.nagy added a comment. Yes, moving StdCLibraryFunctionsChecker to an always-loaded package is probably a better solution than adding this one particular dependency link. (Evaluating these functions may be useful for other checkers as well, although it does not seem to change the results

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

donat.nagy added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:164 +// double and possibly long double on some systems +RepresentsUntilExp = 53; break; + case 32: xazax.hun wrote: > A link to the source of

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

donat.nagy added a comment. Herald added a subscriber: dkrupp. polite ping Repository: rC Clang https://reviews.llvm.org/D52730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

donat.nagy added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:163 + +switch (FloatingSize) { + case 64: NoQ wrote: > Continuing the float semantics discussion on the new revision - Did you > consider `llvm::APFloat`?

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

donat.nagy updated this revision to Diff 171678. donat.nagy added a comment. Use APFloat to determine precision of floating point type Additionally, fix a typo in the tests. Repository: rC Clang https://reviews.llvm.org/D52730 Files: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

donat.nagy marked 7 inline comments as done. donat.nagy added a comment. I found a solution for determining the precision value of a floating point type. Is this acceptable? Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:163 + +switch (FloatingSize) { +

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

donat.nagy updated this revision to Diff 170169. donat.nagy added a comment. Give a reference for the significand size values of the IEEE 754 floating point types; cleanup comments and formatting. Repository: rC Clang https://reviews.llvm.org/D52730 Files:

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

donat.nagy marked 2 inline comments as done. donat.nagy added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:164 +// double and possibly long double on some systems +RepresentsUntilExp = 53; break; + case 32:

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

donat.nagy updated this revision to Diff 172922. donat.nagy marked an inline comment as done. donat.nagy added a comment. Use ASTContext::getFloatTypeSemantics() Repository: rC Clang https://reviews.llvm.org/D52730 Files: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

donat.nagy added a comment. Could someone with commit rights commit this patch (if it is acceptable)? I don't have commit rights myself. Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:158-162 +unsigned FloatingSize = AC.getTypeSize(DestType); +//

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

donat.nagy marked 6 inline comments as done. donat.nagy added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:175 + + if (RepresentsUntilExp >= sizeof(unsigned long long)*8) { return false; NoQ wrote: > Szelethus wrote: > >

[PATCH] D52742: [analyzer][WIP] Add macro expansions to the plist output

donat.nagy added a comment. Does this checker handle the expansion of variadic macros introduced in C++11 (see syntax (3) and (4) here ) and the `#` and `##` preprocessor operators? Comment at:

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

donat.nagy updated this revision to Diff 167929. donat.nagy added a comment. Fix formatting in tests; add a comment. Repository: rC Clang https://reviews.llvm.org/D52730 Files: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp test/Analysis/conversion.c Index:

[PATCH] D52423: [analyzer] Make ConversionChecker load StdCLibraryFunctionsChecker

donat.nagy abandoned this revision. donat.nagy added a comment. I submitted a new patch, which moves stdCLibraryFunctions to apiModeling (https://reviews.llvm.org/D52722). Repository: rC Clang https://reviews.llvm.org/D52423 ___ cfe-commits

[PATCH] D52722: [analyzer] Move StdCLibraryFunctions to apiModeling

donat.nagy created this revision. donat.nagy added a reviewer: NoQ. Herald added subscribers: cfe-commits, Szelethus, mikhail.ramalho, a.sidorin, szepet, eraman, xazax.hun. Herald added a reviewer: george.karpenkov. StdCLibraryFunctionsChecker models the evaluation of several well-known

[PATCH] D52730: [analysis] ConversionChecker: handle floating point

donat.nagy created this revision. donat.nagy added reviewers: NoQ, george.karpenkov, rnkovacs, baloghadamsoftware, mikhail.ramalho. Herald added subscribers: cfe-commits, Szelethus. Extend the alpha.core.Conversion checker to handle implicit converions where a too large integer value is

[PATCH] D52722: [analyzer] Move StdCLibraryFunctions to apiModeling

donat.nagy added a subscriber: martong. donat.nagy added a comment. I don't have commit rights, please commit for me @NoQ or @martong Repository: rC Clang https://reviews.llvm.org/D52722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D52423: Make ConversionChecker load StdCLibraryFunctionsChecker

donat.nagy created this revision. donat.nagy added a reviewer: dergachev.a. Herald added a subscriber: cfe-commits. ConversionChecker produces false positives when it encounters the idiomatic usage of certain well-known functions (e.g. getc() and the character classification functions like

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked an inline comment as done. donat.nagy added inline comments. Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4 +void test_basic1(int in, int ) { + if (in > 77) +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy updated this revision to Diff 176408. donat.nagy added a comment. Minor correction in documentation Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54757/new/ https://reviews.llvm.org/D54757 Files:

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked 2 inline comments as done. donat.nagy added a comment. I applied this check to the llvm + clang codebase, and I was surprised to see that it produced about 600 results (for comparison, the clang-tidy checks which are enabled by default produce approximately 6000 results

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked 8 inline comments as done. donat.nagy added a comment. At the end of the documentation I stated that the check examines the code after macro expansion. Is this note clear enough? Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4 +void test_basic1(int

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy updated this revision to Diff 176066. donat.nagy added a comment. Handle ternary operators, improve documentation Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54757/new/ https://reviews.llvm.org/D54757 Files:

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy added a comment. Macro-generated long switches: I cannot access the check results right now, but if I recall correctly, the check assigned these errors to certain lines in the `.inc` files. This means that (1) it is not very easy to to suppress them with `//NOLINT` comments, but (2)

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy updated this revision to Diff 174943. donat.nagy added a comment. Implement suggested improvements Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54757 Files: clang-tidy/bugprone/BranchCloneCheck.cpp clang-tidy/bugprone/BranchCloneCheck.h

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked 6 inline comments as done. donat.nagy added a comment. An example of duplicated code in Clang (it appears in llvm/tools/clang/lib/Frontend/Rewrite/RewriteMacros.cpp starting from line 128): // If this is a #warning directive or #pragma mark (GNU extensions), // comment the

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy updated this revision to Diff 175502. donat.nagy added a comment. Remove braces, move if parent checking to ASTMatcher, other minor improvements Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54757/new/ https://reviews.llvm.org/D54757

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked 15 inline comments as done. donat.nagy added a comment. I will add tests for macro handling later. Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:71 +// Check whether this `if` is part of an `else if`: +if (const auto *IP = +

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy created this revision. donat.nagy added reviewers: alexfh, hokein, aaron.ballman, xazax.hun, whisperity. Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs, mgorny. Implement a check for detecting if/else if/else chains where two or more branches are Type I

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy updated this revision to Diff 175697. donat.nagy marked 5 inline comments as done. donat.nagy added a comment. Minor simplifications, add tests for macro handling Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54757/new/

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy updated this revision to Diff 175701. donat.nagy added a comment. Correct a typo (ELSE instead of else) Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54757/new/ https://reviews.llvm.org/D54757 Files:

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

donat.nagy marked 3 inline comments as done. donat.nagy added a comment. **AstDiff:** I looked at the AstDiff library, but I think that it is overkill for my goals. The main feat of the AstDiff library is analyzing and presenting the differences between two AST subtrees, while this checker

[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

[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

[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

[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] 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] 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] 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/

[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] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

donat.nagy added a comment. Thanks for creating this commit, it's a useful improvement! I added some inline comments on minor implementation details; moreover, note that "Releted checkers:" (instead of "related") is a typo in the commit message. I also have a less concrete question about the

[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); //

[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 ___

[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", );`

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

donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:47 +protected: + class PtrCastVisitor : public BugReporterVisitor { public: I like that yo switched to a more descriptive class name :)

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

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

[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

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

donat.nagy added a comment. I don't think that the `[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 to

[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 `[N]` issue is too serious: we can just increment >> the array extent

[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

[PATCH] D159519: [clang][AST][ASTImporter] improve AST comparasion on VarDecl & GotoStmt

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. LGTM. I'm not very familiar with this area, but the change seems to be a very clean improvement. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

donat.nagy added a comment. Looks good, thanks for the improvements! One minor remark: your commit uses backticks (`) around the class names in the bug reports, which is commonly used to mark code fragments e.g. in commit messages; however, in the bug reports IIRC most other checkers use

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

donat.nagy added a comment. By the way, what would you think about putting the implementation of the two checkers (DeleteWithNonVirtualDtor, ArrayDelete) into two separate checker classes and placing the shared code into a common base class of these classes? I think this could be a significant

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

donat.nagy added a comment. A (somewhat delayed) ping to @steakhal because you asked me to tell you when I have the results. (If you've already seen the table that I uploaded on Friday, then you've nothing to do.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3e014038b373: [analyzer] Improve underflow handling in ArrayBoundV2 (authored by donat.nagy). Changed prior to commit:

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

donat.nagy added a comment. Yup, there was a superfluous line break that was corrected by git clang-format; thanks for bringing it to my attention. As this is a very trivial change, I'll merge the fixed commit directly, without uploading it into Phabricator. Comment at:

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

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy added a comment. In D156312#4589251 , @steakhal wrote: > I don't think we should do anything about it unless it's frequent enough. > Try to come up with a heuristic to be able to measure how often this happens, > if you really care. > Once

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

donat.nagy added a comment. The change looks promising, I only have minor remarks. Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:125 - const NoteTag *Note = - C.getNoteTag([SymbolicEnvPtrRegion, FunctionName]( -

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

donat.nagy added a comment. In D158499#4608974 , @danix800 wrote: > One of the observable issue with inconsistent size type is > > void clang_analyzer_eval(int); > > typedef unsigned long long size_t; > void *malloc(unsigned long size); > void

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 550703. donat.nagy added a comment. Updated the release notes. This could be the final version of this commit if you agree that it's good enough. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 Files:

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 550318. donat.nagy edited the summary of this revision. donat.nagy added a comment. I verified that the checker handles the examples in the documentation correctly (and added them to the test suite). However, as I was tweaking the examples in the

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy added a comment. (The release notes entry is still missing, I'll try to add it tomorrow.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 ___

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG25b9696b61e5: [analyzer] Upstream BitwiseShiftChecker (authored by donat.nagy). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

donat.nagy added a comment. This checker is a straightforward, clean implementation of the SEI-CERT rule EXP51-CPP, it'll be a good addition to Static Analyzer. I'm satisfied with the implementation, although there might be others who have a more refined taste in C++ coding style. The only

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

donat.nagy added a comment. The results on open-source projects are depressing, but acceptable. This checker is looking for a serious defect, so it doesn't find any true positives on stable versions of open-source projects; however it produces a steady trickle of false positives because the

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 2 inline comments as done. donat.nagy added inline comments. Comment at: clang/docs/analyzer/checkers.rst:81 + +Ensure the shift operands are in proper range before shifting. + donat.nagy wrote: > steakhal wrote: > > We should exactly elaborate

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 550706. donat.nagy added a comment. A few minutes ago I accidentally re-uploaded the previous version of the patch; now I'm fixing this by uploading the actually updated variant. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/

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

donat.nagy marked an inline comment as done. donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp:27 class BoolAssignmentChecker : public Checker< check::Bind > { -mutable std::unique_ptr BT; +mutable

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

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. LGTM. I'm not familiar with the Iterator checker family, but this is a very straightforward change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. Thanks for the updates! I didn't spot any important issue, so I'm accepting this commit, but let's wait for the opinion of @steakhal before merging this. In D158707#4613955

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

This revision was automatically updated to reflect the committed changes. donat.nagy marked an inline comment as done. Closed by commit rG8a5cfdf7851d: [analyzer][NFC] Remove useless class BuiltinBug (authored by donat.nagy). Changed prior to commit:

[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

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

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

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

donat.nagy added inline comments. Herald added a subscriber: ormris. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:35-36 public Checker { - mutable std::unique_ptr BT; + mutable std::unique_ptr BT; mutable std::unique_ptr TaintBT;

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

donat.nagy updated this revision to Diff 549881. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157104/new/ https://reviews.llvm.org/D157104 Files: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp clang/test/Analysis/out-of-bounds.c

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

donat.nagy marked 2 inline comments as done. donat.nagy added a comment. I didn't upload the test results yet because right now there is some incompatibility between our local fork and the upstream. I hope that it'll be fixed soon, but until then I can't use our automated tools to run the

[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");

[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

[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

[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

[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

[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

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

[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

[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

[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() !=

[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:

[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 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 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

[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

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

  1   2   >