[PATCH] D136603: [analyzer] Model cast after LValueToRValueBitCasts

martong added a comment. In D136603#3888065 , @steakhal wrote: > Previously, even in the `RegionStoreManager::getBinding()` even if `T` was > non-null, we replaced it with `TVR->getValueType()` in case the `MR` was > `TypedValueRegion`. Yeah, that

[PATCH] D133698: [clang][dataflow] Implement transferBranch

martong added a comment. In D133698#3886329 , @aaron.ballman wrote: > FWIW, it looks like this change broke the AIX bot: > https://lab.llvm.org/buildbot/#/builders/214/builds/3932 Yes, I've received many build bot failures complaining about the unused

[PATCH] D133698: [clang][dataflow] Implement transferBranch

martong added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:165 + TypeErasedDataflowAnalysis const StmtToEnvMap martong wrote: > Actually, this member is not used, I am not sure why was this added. Anyway,

[PATCH] D133698: [clang][dataflow] Implement transferBranch

martong added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:165 + TypeErasedDataflowAnalysis const StmtToEnvMap Actually, this member is not used, I am not sure why was this added. Anyway, I am removing

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong added a comment. In D101526#3883871 , @NoQ wrote: > Looks great to me, thanks!! Thanks for the review! Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp:32-33

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. martong marked an inline comment as done. Closed by commit rG82a50812f7e5: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg (authored by martong).

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

martong added a comment. In D135360#3885494 , @balazske wrote: > If we look only at the C standard we can not tell much about if the functions > should set `errno`. It seems that setting `errno` is totally > implementation-dependent, except for a few

[PATCH] D136684: [clang][ASTImporter] Remove use of ParentMapContext.

martong added a comment. In D136684#3884983 , @balazske wrote: > This is really a NFC-like change but not NFC because it has visible effects > of removing some crashes. I could not produce a test that provokes the wrong > case (when `getParents`

[PATCH] D133698: [clang][dataflow] Implement transferBranch

martong added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp:73 + .getName())}), + /*VerifyResults=*/Match), + llvm::Succeeded()); ymandel wrote: > nit: maybe just

[PATCH] D133698: [clang][dataflow] Implement transferBranch

This revision was automatically updated to reflect the committed changes. martong marked an inline comment as done. Closed by commit rGbb72d0dde29e: [clang][dataflow] Implement transferBranch (authored by martong). Changed prior to commit: https://reviews.llvm.org/D133698?vs=470748=470802#toc

[PATCH] D136668: [clang][dataflow] Add initial sign analysis

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG93ce23adb548: [clang][dataflow] Add initial sign analysis (authored by martong). Changed prior to commit:

[PATCH] D136668: [clang][dataflow] Add initial sign analysis

martong updated this revision to Diff 470796. martong marked 2 inline comments as done. martong added a comment. - Address ymandel's comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136668/new/ https://reviews.llvm.org/D136668 Files:

[PATCH] D136668: [clang][dataflow] Add initial sign analysis

martong marked 13 inline comments as done. martong added a comment. In D136668#3883241 , @ymandel wrote: > Nice!! Just nits. > > At a high level, I'm a little concerned about this as a demo, since I > wouldn't recommend this implementation in practice

[PATCH] D133698: [clang][dataflow] Implement transferBranch

martong updated this revision to Diff 470748. martong marked an inline comment as done. martong added a comment. - Address ymandel's comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133698/new/ https://reviews.llvm.org/D133698 Files:

[PATCH] D133698: [clang][dataflow] Implement transferBranch

martong marked 9 inline comments as done. martong added a comment. Thanks for the assiduous review @ymandel ! Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:78-79 public: - TerminatorVisitor(const StmtToEnvMap , Environment , + // The return

[PATCH] D133698: [clang][dataflow] Implement transferBranch

martong added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp:9-11 +// This file defines a simplistic version of Sign Analysis as an example +// of a forward, monotonic dataflow analysis. The analysis tracks all +// variables in the

[PATCH] D136684: [clang][ASTImporter] Remove use of ParentMapContext.

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. It is very good that we can get rid of the `ParentMapContext` because it was sub-optimal to build that up for **//all//** declaration contexts of the

[PATCH] D136603: [analyzer] Model cast after LValueToRValueBitCasts

martong added a comment. > PS: I'm not sure how/when we should get rid of the LocAsInteger and > represent this by a SymbolCast. > Maybe @ASDenysPetrov or @martong could help me review this. Whenever this https://reviews.llvm.org/D117229 gets accepted and when we emit all symbolCasts for all

[PATCH] D136603: [analyzer] Model cast after LValueToRValueBitCasts

martong added a comment. I am not sure, if the `ExprEngine::VisitCast` is the proper place to add these new modifications and call SValBuilder's evalCast. I think it might be better positioned in `RegionStoreManager::getBinding`. Considering, we already do a cast evaluation for certain kind of

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

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. In D135375#3882491 , @Szelethus wrote: > Seems like the issues mentioned above are real, but orthogonal to this patch. > Would it be okay to

[PATCH] D136668: [clang][dataflow] Add initial sign analysis

martong created this revision. martong added reviewers: xazax.hun, gribozavr2, sgatev, ymandel. Herald added subscribers: steakhal, rnkovacs. Herald added a reviewer: Szelethus. Herald added a project: All. martong requested review of this revision. Herald added a project: clang. Herald added a

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101526/new/ https://reviews.llvm.org/D101526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D136162: [analyzer] Fix assertion failure with conflicting prototype calls

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Thanks for the update. Nice Work! Comment at: clang/test/Analysis/region-store.c:66 + // expected-warning@+1 {{passing arguments to 'b' without a prototype is deprecated

[PATCH] D133698: [clang][dataflow] Implement transferBranch

martong added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:127 + // Default implementation is a Noop. + virtual void branchTransfer(bool Branch, const Stmt *S, Lattice , + Environment ) {}

[PATCH] D133698: [clang][dataflow] Implement transferBranch

martong updated this revision to Diff 469636. martong edited the summary of this revision. martong added a comment. - Rebase to latest llvm/main Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133698/new/ https://reviews.llvm.org/D133698 Files:

[PATCH] D133698: [clang][dataflow] SignAnalysis, edgeTransfer, branchTransfer

martong updated this revision to Diff 469625. martong marked 8 inline comments as done. martong added a comment. - Add comments - Assumption -> ConditionValue - Use CRTP - branchTransfer -> transferBranch - Make just one simple test case for transferBranch Repository: rG LLVM Github Monorepo

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

martong added a comment. In D135247#3867445 , @balazske wrote: > If `StdCLibraryFunctionsChecker` is added as dependency to `StreamChecker` > and no other thing is changed these tests fail, and I could not find out the > reason: > Errors in test

[PATCH] D136162: [analyzer] Fix assertion failure in RegionStore within bindArray()

martong added a comment. > A similar situation could happen if we reinterpret cast pointers, etc. so the > situation is not limited to conflicting function prototypes. Please provide tests for those cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D136162: [analyzer] Fix assertion failure in RegionStore within bindArray()

martong added a comment. Hmm, seems like the conflicting prototype (i.e. the obsolescent use of zero parameters) is needed to reproduce the assertion failure. That makes me wonder, how does the redecl chain of `b` looks like? Is `void b()` chained with `void b(int*)`, or are they represented

[PATCH] D135136: [analyzer] Make directly bounded LazyCompoundVal as lazily copied

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2843 -// subregions of the `LCS.getRegion()` also lazily copied. -if (const MemRegion *R =

[PATCH] D135136: [analyzer] Make directly bounded LazyCompoundVal as lazily copied

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2290 List.insert(List.end(), InnerList.begin(), InnerList.end()); - continue; } steakhal wrote: > Here is the `continue` which previously prevented

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1818-1857 +// int fgetpos(FILE *restrict stream, fpos_t *restrict pos); +// From 'The Open Group Base Specifications Issue 7, 2018 edition': +// "The

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

martong added a comment. In D135360#3839890 , @balazske wrote: > I found some anomalies during development: > > - If the checker **StdCLibraryFunctions** is added as dependency for > **alpha.unix.Stream** in //checkers.td// I get some "unexplainable"

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

martong added a comment. Thanks for patch and the many test cases. You still haven't answered my below concern: > Is there a way to stop the analysis with an error if "ModelPOSIX=true" is not > set but the 'alpha.unix.Stream' checker is enabled? Comment at:

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

martong added a comment. I am happy with the change and with the result. However, [[

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

martong added a comment. In D135375#3849261 , @Szelethus wrote: > Some early results: > >

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong added a comment. In D101526#3804623 , @NoQ wrote: > Hi, looks great! I found a couple of typos and the amount of changes in tests > is suspiciously low. And I want to make sure that the promise to change "arg" > -> "argument" isn't lost (but

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong updated this revision to Diff 467773. martong added a comment. - Changed to be more verbose: "arg" -> "argument" - Fixed "less than" to "greater than" - Added new tests - Fixed typos Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101526/new/

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong updated this revision to Diff 467772. martong marked 3 inline comments as done. martong added a comment. - Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101526/new/ https://reviews.llvm.org/D101526 Files:

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

martong added a comment. > The two checkers work together and 'apiModeling.StdCLibraryFunctions' > and its 'ModelPOSIX=true' option should be now a dependency of > checker 'alpha.unix.Stream'. Is there a way to stop the analysis with an error if "ModelPOSIX=true" is not set but the

[PATCH] D126481: [analyzer] Handle SymbolCast in SValBuilder

martong added a comment. I don't see this case different to the unary expressions. Consider the unary minus for example. Let's say, the symbol `a` is constrained to `[1, 1]` and then `-a` is constrained to `[-2, -2]`. This is clearly an infeasible state and the analyzer will terminate the

[PATCH] D134947: [analyzer] Fix liveness of Symbols for values in regions reffered by LazyCompoundVal

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Thanks for the updates. I am okay with it now. LGTM. But please wait for NoQ's approval. So, this is a gentle ping for you @NoQ :) Comment at:

[PATCH] D134941: [analyzer][NFC] Add tests for D132236

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Thanks for the update! LGTM. Comment at: clang/test/Analysis/trivial-copy-struct.cpp:57 + + // Dead code should be unreachable + clang_analyzer_warnIfReached(); //

[PATCH] D134947: [analyzer] Fix liveness of Symbols for values in regions reffered by LazyCompoundVal

martong added a comment. I like the approach of this patch and I think this is somewhat aligned with @NoQ's ideas about > a list of explicitly-live compound values and > "weak region roots" that aren't necessarily live themselves but anything > derived from them ... is live Coupled with the

[PATCH] D134941: [analyzer][NFC] Add tests for D132236

martong added a comment. Thank you! Increasing coverage in tests is always great. Comment at: clang/test/Analysis/NewDeleteLeaks.cpp:196 + +namespace symbol_reaper_lifetime { +struct Nested { Could you please add some explanation for the test case? Thinking of

[PATCH] D133119: [clang-tidy] Add checker 'bugprone-suspicious-realloc-usage'.

martong accepted this revision. martong added a comment. In D133119#3827432 , @balazske wrote: > I added a simple detection of create a copy of `p` before `p = realloc(p, > ...)`. This can remove the warning at very obvious cases when a copy of `p` >

[PATCH] D126481: [analyzer] Handle SymbolCast in SValBuilder

martong added a comment. Actually, you already have a logic for checking if there is a contradiction in your D103096 patch, in ConstraintAssignor::updateExistingConstraints: // Update constraints in the map which bitwidth is equal or lower then //

[PATCH] D126481: [analyzer] Handle SymbolCast in SValBuilder

martong added a comment. > Suppose we have found the way to handle it. But what if we find a > contradiction while simplifying, like (short)(int x) = 0 and (int x) = 1. So > that we would already know that it is impossible. What SVal should we return > in such case? Undef? Unknown? Original

[PATCH] D132249: [clang][analyzer] Add some functions to StreamChecker with errno modeling.

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h:110 +/// \arg \c InvalE Expression that causes invalidation of \c errno. +ProgramStateRef setErrnoStdMustBeChecked(ProgramStateRef State, +

[PATCH] D132017: [clang][analyzer] Add errno modeling to StreamChecker

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:465 + /// post-call event. + class NoErrnoConstraint : public

[PATCH] D126481: [analyzer] Handle SymbolCast in SValBuilder

martong added a comment. Yeah okay. I get it now. Thank you for your patience and your time on elaborating the issue. First, I think we'd need to fabricate a test case that shows us the bug even without applying your patch (D103096 ). Then we can iterate

[PATCH] D126481: [analyzer] Handle SymbolCast in SValBuilder

martong added a comment. > Assume we have `(int)(short)(int x)`. `VisitSymbolCast` will try to get the > constant recursively in the next order: > > - `(short)(int x)` > - `(int x)` > > And here is my concern. Whether it is correct to get the range for `int x` > and consider it as a correct

[PATCH] D133119: [clang-tidy] Add checker 'bugprone-suspicious-realloc-usage'.

martong accepted this revision. martong added a reviewer: whisperity. martong added a subscriber: whisperity. martong added a comment. This revision is now accepted and ready to land. We had a discussion about this with @dkrupp . We think that the `p = realloc(p, var)` construct in itself is an

[PATCH] D134549: [clang] Add fix-it note to defaulted-function-deleted warning

martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:3707 ToFunction->setRangeEnd(ToEndLoc); + ToFunction->setDefaultLoc(D->getDefaultLoc()); This should be imported very similarly to `EndLoc`. Repository: rG LLVM Github Monorepo

[PATCH] D134303: [AST] Preserve more structure in UsingEnumDecl node.

martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:4866 auto ToUsingLoc = importChecked(Err, D->getUsingLoc()); - auto ToEnumLoc = importChecked(Err, D->getEnumLoc()); + auto ToEnumLoc = importChecked(Err, D->getEnumType()); auto ToEnumDecl =

[PATCH] D133698: [clang][dataflow] SignAnalysis, edgeTransfer, branchTransfer

martong planned changes to this revision. martong added a comment. Aligned with the RFC, I am going to dissect this patch into two patches: 1. This patch will remain a simple infrastructural change that introduces transferBranch. This could be useful for complex lattices (e.g integer value

[PATCH] D133945: [clang][ASTImporter] DeclContext::localUncachedLookup: Continue lookup into decl chain when regular lookup fails

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. LGTM, Thanks! Comment at: clang/lib/AST/DeclBase.cpp:1781 if (Name && !hasLazyLocalLexicalLookups() && !hasLazyExternalLexicalLookups()) { if

[PATCH] D133931: [clang][dataflow] Replace `transfer(const Stmt *, ...)` with `transfer(const CFGElement *, ...)` in `clang/Analysis/FlowSensitive`.

martong added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp:137-140 +auto CS = E->getAs(); +if (!CS) + return; +auto S = CS->getStmt(); This is exactly the same code that you have in

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong added a comment. Gentle ping @steakhal @NoQ Trying to revive this after a year :) I am sorry it took so long to get back to this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101526/new/ https://reviews.llvm.org/D101526

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong updated this revision to Diff 460345. martong marked 2 inline comments as done. martong added a comment. - Rebase - move Msg into the lambda Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101526/new/ https://reviews.llvm.org/D101526 Files:

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong marked 7 inline comments as done. martong added inline comments. Herald added a project: All. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:156-157 // the bug is reported. -virtual std::string describe(ProgramStateRef State, +

[PATCH] D133851: [analyzer] Initialize ShouldEmitErrorsOnInvalidConfigValue analyzer option

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Okay, I don't see any problem with this change. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133851/new/

[PATCH] D133698: [clang][dataflow] SignAnalysis, edgeTransfer, branchTransfer

martong created this revision. martong added reviewers: xazax.hun, gribozavr2, sgatev, ymandel, samestep. Herald added subscribers: steakhal, gamesh411, Szelethus, dkrupp, rnkovacs, mgorny. Herald added a reviewer: Szelethus. Herald added a reviewer: NoQ. Herald added a project: All. martong

[PATCH] D132109: [analyzer] Dump the environment entry kind as well

martong accepted this revision. martong 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/D132109/new/ https://reviews.llvm.org/D132109

[PATCH] D131858: [clang] Track the templated entity in type substitution.

martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:3748-3751 + // If it is a template, import all related things. + if (Error Err = ImportTemplateInformation(D, ToFunction)) +return std::move(Err); + What's the reason of moving this

[PATCH] D132142: [analyzer] Prefer wrapping SymbolicRegions by ElementRegions

martong added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:799 + /// we actually know their types. + QualType getApproximatedType() const { +return sym->getType()->getPointeeType(); steakhal wrote: >

[PATCH] D132142: [analyzer] Prefer wrapping SymbolicRegions by ElementRegions

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. I am okay with this change, it does give a proper canonical form, which is good. On the other hand, I agree that (on the long term) base regions and offsets would be a better memory model

[PATCH] D131879: [clang][analyzer] Errno modeling code refactor (NFC).

martong accepted this revision. martong added a comment. Herald added a subscriber: rnkovacs. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131879/new/ https://reviews.llvm.org/D131879 ___

[PATCH] D131707: [analyzer][NFC] Cache the result of getLocationType in TypedValueRegion

martong added a comment. > So I think the most valuable optimizations are low-level optimizations to > `ImmutableMap`. There were a few suggestions on the mailing list to use > something more modern than the AVL trees under the hood but I don't think > authors found much success with those.

[PATCH] D130705: [clang][ASTImporter] Improve import of functions with auto return type.

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Thank you for the update! LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130705/new/ https://reviews.llvm.org/D130705

[PATCH] D131006: [analyzer] Use DisequalityMap while inferring constraints

martong added inline comments. Comment at: clang/test/Analysis/range-inferring-from-disequality-map.cpp:50-51 +if(x != tmp1 && x != tmp2) + // TODO: This condition should be infeasible. + // Thus, the branch should be unreachable. +

[PATCH] D131067: [analyzer] Treat values passed as parameter as escaped

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131067/new/ https://reviews.llvm.org/D131067

[PATCH] D130705: [clang][ASTImporter] Improve import of functions with auto return type.

martong added a comment. Thank you, nice and assiduous work! The changes look good to me, but I think we should have some more tests where variable and function template specializations are used as return types. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D131067: [analyzer] Treat values passed as parameter as escaped

martong added a comment. Thanks, looks good with some nits. Comment at: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:556-558 +unsigned numParams = FD->getNumParams(); +for (unsigned i = 0; i < numParams; ++i) { +

[PATCH] D130470: [clang][analyzer] Add more wide-character functions to CStringChecker

martong accepted this revision. martong added a comment. I think there's only some very minor style nits which are remained, but that should not block this any further. Let's land it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130470/new/

[PATCH] D131006: [analyzer] Use DisequalityMap while inferring constraints

martong added inline comments. Comment at: clang/test/Analysis/range-inferring-from-disequality-map.cpp:10-11 + clang_analyzer_value(x); // expected-warning {{32s:{ [0, 0] }}} + // TODO: TODO: Keep x range correct even if associated disequalities are + // already dead. +

[PATCH] D130372: [analyzer] Add a new factory function RangeSet::Factory::invert

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. LGTM! Nice work! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130372/new/ https://reviews.llvm.org/D130372 ___ cfe-commits mailing

[PATCH] D131006: [analyzer] Use DisequalityMap while inferring constraints

martong added a comment. Awesome! Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1512-1516 +if (IsFirst) { + IsFirst = false; + RS = *RSPtr; +} else + RS = RangeFactory.unite(RS, *RSPtr); `unite`

[PATCH] D130705: [clang][ASTImporter] Improve import of functions with auto return type.

martong added a comment. In D130705#3685299 , @balazske wrote: > It is still not perfect, analysis of qtbase is not better than before and > some of the unreachable assertions were encountered. Does it mean you plan some updates on this patch?

[PATCH] D130372: [analyzer] Add a new factory function RangeSet::Factory::invert

martong added a comment. In D130372#3683275 , @ASDenysPetrov wrote: > Now I'm working on the next patch and you'll see a motivation soon. Thanks for the update! Looks good, but I am still waiting with my formal approve to understand the motivation.

[PATCH] D130372: [analyzer] Add a new factory function RangeSet::Factory::invert

martong added a comment. > The motivation is to extend the set of operations that can be performed on > range sets. Specifically, this function is needed for the next patch in the > stack. Would be great to see the motivation clearly and early. What will be the next patch about?

[PATCH] D130470: [clang][analyzer] Add more wide-character functions to CStringChecker

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. In D130470#3682131 , @balazske wrote: > A related question: > Is it better to have a more generic function like

[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/AST/ASTImporter.cpp:3237 + ParentMapContext = DC->getParentASTContext().getParentMapContext(); + DynTypedNodeList P = ParentC.getParents(*S);

[PATCH] D130091: [clang][analyzer] Added partial wide character support to CStringChecker

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. Okay, thanks for the update. LGTM! Comment at: clang/test/Analysis/wstring.c:385 + wchar_t a[32]; + // FIXME: This should work with

[PATCH] D130091: [clang][analyzer] Added partial wide character support to CStringChecker

martong added a comment. Nice improvement and the tests are meaningful! > clang/test/Analysis/cstring.c Hadn't we have already a test file for this checker? What about `string.c` and `bstring.c`? You might have added redundant test cases in the new test file. Comment at:

[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.

martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:3269-3273 +// Note: It is possible that T can be get as both a RecordType and a +// TemplateSpecializationType. + } + if (const auto *TST = T->getAs()) { +return

[PATCH] D112621: [analyzer][solver] Introduce reasoning for not equal to operator

martong added a comment. In D112621#3660256 , @manas wrote: > Considering @ASDenysPetrov 's example of `LHS = [1, 2] U [8, 9]` and `RHS = > [5, 6]`, I constructed a test case as following: > > `(((u1 >= 1 && u1 <= 2) || (u1 >= 8 && u1 <= 9)) && u2 >= 5

[PATCH] D130029: [analyzer][NFC] Use `SValVisitor` instead of explicit helper functions

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

[PATCH] D129498: [analyzer] Add new function `clang_analyzer_value` to ExprInspectionChecker

martong added inline comments. Comment at: clang/test/Analysis/print-ranges.cpp:1 +// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config eagerly-assume=false -verify %s +// REQUIRES: no-z3 ASDenysPetrov wrote: > martong wrote:

[PATCH] D129280: [analyzer] PlacementNewChecker, properly handle array overhead (cookie)

martong added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129280/new/ https://reviews.llvm.org/D129280 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers

martong added a comment. Thanks Denys for your continued work on this. These are very good questions that must be answered, we need exactly such thinking to implement this properly. I believe we can advance gradually. In D103096#3642681 ,

[PATCH] D129498: [analyzer] Add new function `clang_analyzer_value` to ExprInspectionChecker

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129498/new/ https://reviews.llvm.org/D129498 ___ cfe-commits mailing list

[PATCH] D129678: [analyzer][NFC] Tidy up handler-functions in SymbolicRangeInferrer

martong accepted this revision. martong added a comment. In D129678#3652859 , @ASDenysPetrov wrote: > Fixed a typo that caused `constraint_manager_negate.c` and `unary-sym-expr.c` > tests crashes. Good. Still LGTM. CHANGES SINCE LAST ACTION

[PATCH] D104647: [analyzer] Support SVal::getType for pointer-to-member values

martong added a comment. In D104647#3643060 , @ASDenysPetrov wrote: > @vsavchenko Is this alive? I think, we could commit this and set Valery as the author (he is no longer working on CSA). Unless you find something wrong with the patch; in that case

[PATCH] D129678: [analyzer][NFC] Tidy up handler-functions in SymbolicRangeInferrer

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Took a while to accept that this is indeed an NFC, but looks good! And indeed the code is better structured this way. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D129498: [analyzer] Add new function `clang_analyzer_value` to ExprInspectionChecker

martong added a comment. Very good! In D129498#3650595 , @NoQ wrote: > In D129498#3647348 , @ASDenysPetrov > wrote: > >> In D129498#3644222 , @NoQ wrote: >> >>> Maybe

[PATCH] D129640: [clang][ASTImporter] Improved handling of functions with auto return type.

martong added a comment. Thanks, nice work! Comment at: clang/lib/AST/ASTImporter.cpp:3237 + ParentMapContext = DC->getParentASTContext().getParentMapContext(); + DynTypedNodeList P = ParentC.getParents(*S); + while (!P.empty()) {

[PATCH] D129737: [analyzer] Fixing SVal::getType returns Null Type for NonLoc::ConcreteInt in boolean type

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. Thanks! LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129737/new/ https://reviews.llvm.org/D129737

[PATCH] D129269: [analyzer] Fix use of length in CStringChecker

martong accepted this revision. martong 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/D129269/new/ https://reviews.llvm.org/D129269

[PATCH] D129269: [analyzer] Fix use of length in CStringChecker

martong added a comment. Thanks Vince for the patch! I think the implementation is correct. But I am worried that it might do sub-optimal computations on the string literals. I mean, getting the length would require O(N) steps each time, and that could be problematic with long literals,

  1   2   3   4   5   6   7   8   9   10   >