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

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

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

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 updated this revision to Diff 549978. donat.nagy marked 7 inline comments as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 Files: clang/docs/analyzer/checkers.rst

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 13 inline comments as done. donat.nagy added a comment. I handled the inline comments, I'll check the example code and edit the release notes tomorrow. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:272 + if (const auto ConcreteRight

[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] 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] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 549091. donat.nagy marked 2 inline comments as done. donat.nagy added a comment. Uploading a new version based on the reviews. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 21 inline comments as done. donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:29-30 + +using namespace clang; +using namespace ento; + steakhal wrote: > This is used quite often and hinders

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 4 inline comments as done. donat.nagy added a comment. Thanks for the suggestions, I answered those where I had something to say and I'll implement the rest of them. In D156312#4576120 , @steakhal wrote: > Have you considered checking

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 6 inline comments as done. donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:94-98 +void BitwiseShiftValidator::run() { + if (isValidShift()) { +Ctx.addTransition(State, createNoteTag()); + } +}

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 548626. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 Files: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 2 inline comments as done. donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302 + pluralSuffix(MaximalAllowedShift)); +R->addNote(LeftNote, PathDiagnosticLocation{LHS,

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy planned changes to this revision. donat.nagy marked 5 inline comments as done. donat.nagy added a comment. I'll soon upload a refactored version. Comment at: clang/docs/analyzer/checkers.rst:44-50 +Moreover, if the pedantic mode is activated by +``-analyzer-config

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

donat.nagy added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:1189 +``onwership_returns``: Functions with this annotation return dynamic memory. +The second annotation parameter is the size of the returned memory in bytes. + Szelethus

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

donat.nagy added a comment. Why is this checker placed in the "unix" group (instead of e.g. "core")? As far as I understand it can check some POSIX-specific functions with a flag (that's off by default) but the core functionality checks platform-independent standard library functions.

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

donat.nagy added reviewers: dkrupp, steakhal, Szelethus, gamesh411. donat.nagy added a comment. I'll try to upload results on open source projects, probably on Tuesday (I'm on vacation on Monday). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

donat.nagy abandoned this revision. donat.nagy added a comment. Herald added a subscriber: wangpc. I'm abandoning this commit because it amalgamates several unrelated changes and I think it'd be better to handle them separately: 1. First, there is a very simple, independent improvement related

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

donat.nagy added a comment. Note: this commit is split off from the larger commit https://reviews.llvm.org/D150446, which combined this simple improvement with other, more complex changes that had problematic side-effects. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked an inline comment as done. donat.nagy added a comment. @steakhal Thanks for the review! I answered some of your questions; and I'll handle the rest soon. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:150 + SVB.evalBinOp(State,

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

donat.nagy added inline comments. Comment at: clang/test/Analysis/enum-cast-out-of-range.cpp:203 + +enum class empty_unfixed {}; + steakhal wrote: > donat.nagy wrote: > > Consider using "specified" and "unspecified" instead of "fixed" and > > "unfixed", because

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

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. LGTM, with a little bikeshedding ;-) Comment at: clang/test/Analysis/enum-cast-out-of-range.cpp:203 + +enum class empty_unfixed {}; + Consider using

[PATCH] D155715: [clang][analyzer] Improve StdCLibraryFunctions socket send/recv functions.

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. LGTM, I don't have anything significant to add. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155715/new/

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302 + pluralSuffix(MaximalAllowedShift)); +R->addNote(LeftNote, PathDiagnosticLocation{LHS, Ctx.getSourceManager(), +

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy added a comment. In D156312#4537200 , @NoQ wrote: >> (4) UBOR exhibits buggy behavior in code that involves cast expressions > > Hmm, what makes your check more resilient to our overall type-incorrectness? The code of UBOR

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 544317. donat.nagy added a comment. (Fix incorrect filename mark in the license header.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 Files:

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy added a comment. I tested this commit on several open source projects (in the default mode Pedantic=false), the following table summarizes the results: | vim_v8.2.1920_baseline_vs_vim_v8.2.1920_with_bitwise_shift | New reports

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

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

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

donat.nagy added a comment. Mostly LGTM, but I'd like to ask you to ensure consistent line wrapping (to e.g. 72 or 80 characters) in the free-flowing text that was added or rewritten by this commit. (I know that this is a minor question because the linebreaks won't be visible in the final

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

donat.nagy added a comment. (Just added some side remarks about string handling annoyances.) @balazske Please (1) handle the `dyn_cast` request of @steakhal and also (2) do something with this "how to convert `StringRef` to `char *`" question that we're bikeshedding. I hope that after those

[PATCH] D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker.

donat.nagy added a comment. In D154423#4485009 , @balazske wrote: > It would be more simple to handle the standard streams in `StreamChecker` > only. [...] That's a reasonable plan, especially if we can bring that checker out of alpha in the

[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, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153776/new/ https://reviews.llvm.org/D153776

[PATCH] D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker.

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. Ok, then I think you can finally merge this commit and its two predecessors. I'm happy to see that this improvement is complete. I hope that you can later add a special case for the

[PATCH] D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker.

donat.nagy added a comment. I looked through most of the open source results, and they look promising. However I've seen one questionable tendency: there are many reports (e.g. this one

[PATCH] D154509: [clang][analyzer] StdLibraryFunctionsChecker: Allow NULL buffer in `fread` and `fwrite` if size is zero.

donat.nagy added a comment. By the way here is a concrete example where this patch is needed to squash a false positive:

[PATCH] D153954: [WIP][clang][analyzer] Relax alpha.cplusplus.EnumCastOutOfRange checker

donat.nagy added a comment. In D153954#4480296 , @steakhal wrote: > [...] I'd not recommend moving the actual implementation anywhere. I agree, I mostly spoke figuratively, suggesting that this checker could end up in the optin group when it's

[PATCH] D153954: [WIP][clang][analyzer] Relax alpha.cplusplus.EnumCastOutOfRange checker

donat.nagy added a comment. I think the old "report when the value stored in an enum type is not equal to one of the enumerators" behavior would be useful as an opt-in checker that could be adopted by some projects; while the new "relaxed" version is too bland to be really useful. I'd suggest

[PATCH] D154509: [clang][analyzer] StdLibraryFunctionsChecker: Allow NULL buffer in `fread` and `fwrite` if size is zero.

donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:413-414 +using ValueConstraint::ValueConstraint; +ArgNo SizeArg1N; +

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

donat.nagy added a comment. The commit looks good in general, I have a few minor suggestions and a serious question about the state transition bureaucracy. Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:65-66 + // If set to true, consider getenv

[PATCH] D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker.

donat.nagy added a comment. These code changes seem to be promising. Please upload results on the open source projects (like the ones that you uploaded previously), and I hope that after verifying those I we can finally merge this set of commits. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer

donat.nagy added a comment. I don't have concrete remarks, but I would like to note that I'm happy to see this cleanup :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154325/new/ https://reviews.llvm.org/D154325

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

donat.nagy added a comment. In D153776#4467627 , @balazske wrote: > The success/failure note tags are not added to all functions, `dup` is one of > these, this means at `dup` no note tag is shown. A next patch can be made to > add these messages to all

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

[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

[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

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

[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

[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

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

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

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

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

[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

[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

[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 ) -> std::string { + if (Node->succ_size() > 1) +return Note.str();

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

  1   2   >