[PATCH] D123032: [clang][dataflow] Exclude protobuf types from modeling in the environment.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Since there are many moving pieces and the whole framework is experimental without many users, I'm fine with landing this as is as long we are tracking the improvement opportunities (tic

[PATCH] D127306: [analyzer] Treat system globals as mutable if they are not const

xazax.hun accepted this revision. xazax.hun added a comment. Overall looks good to me, it is a step towards having a more faithful representation of the reality. Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:978 +assert(!Ty.isNull()); +if (Ty.isConstQualified

[PATCH] D127306: [analyzer] Treat system globals as mutable if they are not const

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:978 +assert(!Ty.isNull()); +if (Ty.isConstQualified() && Ty->isArithmeticType()) { // TODO: We could walk the complex types here and see if everything is steak

[PATCH] D125931: [clang][dataflow] Add support for correlated branches to optional model

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:288 +bool isEngagedOptionalValue(const Value &OptionalVal, const Environment &env) { + auto *HasValueVal =

[PATCH] D128060: [clang][dataflow] Extend flow condition in the body of a for loop

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:92 + void VisitForStmt(const ForStmt *S) { +auto *Cond = S->getCond(); Do we support `DoStmt` or is that missing?

[PATCH] D128056: [clang][dataflow] Singleton pointer values for null pointers.

xazax.hun added a comment. I am wondering about the future plans regarding how pointers are represented. What will be the expected behavior when the analysis discovers that the pointer has a null value? E.g.: if (p == nullptr) { } Would we expect `p` in this case to have the same

[PATCH] D127898: [clang][dataflow] Find unsafe locs after fixpoint

xazax.hun added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:66 + SourceLocations Locs; + for (const CFGBlock *Block : Context->getCFG()) { +// Skip blocks that were not evaluated. While doing yet anot

[PATCH] D126973: [clang][dataflow] Relax assumption that `AggregateStorageLocations` correspond to struct type.

xazax.hun added a comment. Herald added a subscriber: martong. With properties moved up in the hierarchy this could be abandoned now, right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126973/new/ https://reviews.llvm.org/D126973 ___

[PATCH] D128357: [clang][dataflow] Store flow condition constraints in a single `FlowConditionConstraints` map.

xazax.hun accepted this revision. xazax.hun added a comment. I found a small nit inline, otherwise looks good. Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:76 + if (!Res.second) { +FlowConditionConstraints[&Token] = +&getOrCreateConjunct

[PATCH] D128356: [clang][dataflow] Use NoopLattice in optional model

xazax.hun accepted this revision. xazax.hun added a comment. Herald added a subscriber: rnkovacs. Should this PR delete `SourceLocationsLattice`? Are there other users left or do we anticipate new users in the future? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D128056: [clang][dataflow] Singleton pointer values for null pointers.

xazax.hun accepted this revision. xazax.hun added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:69 +Res.first->second = +&takeOwnership(std::make_unique(PointeeLoc)); + } ---

[PATCH] D128363: [clang][dataflow] Implement functionality for flow condition variable substitution.

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:197 + /// `Substitutions`, it will be substituted with the value it maps to. + BoolValue &buildAndSubstituteFlowCondition( + AtomicBoolValue &Token, -

[PATCH] D127898: [clang][dataflow] Add API to separate analysis from diagnosis

xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:546 +void diagnoseUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr, +DiagnoseState> &State) { I think this sh

[PATCH] D127898: [clang][dataflow] Add API to separate analysis from diagnosis

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:38-42 +llvm::DenseSet diagnoseCFG( +const ControlFlowContext &CFCtx, +std::vector> &BlockStates, +const Environment &Env, TypeErasedDataflowAnalysis &Analysis, +Dia

[PATCH] D127898: [clang][dataflow] Add API to separate analysis from diagnosis

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:38-42 +llvm::DenseSet diagnoseCFG( +const ControlFlowContext &CFCtx, +std::vector> &BlockStates, +const Environment &Env, TypeErasedDataflowAnalysis &Analysis, +Dia

[PATCH] D128363: [clang][dataflow] Implement functionality for flow condition variable substitution.

xazax.hun accepted this revision. xazax.hun added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:217 +llvm::DenseMap Substitutions) { + llvm::DenseMap SubstitutionsCache; + Substitut

[PATCH] D127898: [clang][dataflow] Add API to separate analysis from diagnosis

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h:85 +private: + ASTContext &Context; + MatchSwitch>> samestep wrote: > samestep wrote: > > xazax.hun wrote: > > > Do we expect this to

[PATCH] D126316: [clang][dataflow] Make limit on fixpoint-algorithm iterations proportional to size of CFG.

xazax.hun added a comment. An alternative approach is to maintain separate counters for back edges and bail if those reach a certain limit. But this global limit is way simpler, so it looks good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D120495: [clang][dataflow] Add transfer functions for structured bindings

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Looks good, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120495/new/ https://reviews.llvm.org/D120495 __

[PATCH] D126759: [clang][dataflow] Model calls returning optionals

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:161 +auto possiblyAliasedOptionalType() { + return hasUnqualifiedDesugaredType( Does alias refers to a type

[PATCH] D126972: [clang][dataflow] Modify `optional` model to handle type aliases.

xazax.hun accepted this revision. xazax.hun added a comment. This patch looks good to me. On the other hand, I was wondering whether functions like this would be handled: pair f(); array g(); MyStructWithOptionals h(); And so on. In general, I wonder if it is a better approach to be able

[PATCH] D126973: [clang][dataflow] Relax assumption that `AggregateStorageLocations` correspond to struct type.

xazax.hun added a comment. Does this mean you want to use `StructValue`s for non-struct types to have access to properties? I wonder if it would be a cleaner approach to be able to assign properties to any type of values instead of weakening the type system. E.g., properties could be a table on

[PATCH] D126973: [clang][dataflow] Relax assumption that `AggregateStorageLocations` correspond to struct type.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. In D126973#3556383 , @ymandel wrote: > I'm generally hesitant about assertions that don't enforce necessary > properties (only "nice"). I think

[PATCH] D153273: [analyzer] Rework support for CFGScopeBegin, CFGScopeEnd, CFGLifetime elements

xazax.hun accepted this revision. xazax.hun added a comment. Thanks! Looks good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153273/new/ https://reviews.llvm.org/D153273 ___ cfe-commits mailing l

[PATCH] D155445: [analyzer][docs] Add CSA release notes

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/docs/ReleaseNotes.rst:908 + (`7cd1f3ad22e4 `_) +- Fixed a null-pointer dereference crash inside the ``MoveChecker``. + (`d172b65ef001 <

[PATCH] D155547: [analyzer] Model lifetime of a variable declared in for condition in CFG correctly

xazax.hun added inline comments. Comment at: clang/test/Analysis/auto-obj-dtors-cfg-output.cpp:1231 A a; - for (A b; A c = b; ) { + for (A b; A c = b; ++c.x) { A d; Would there be any value in also keeping a couple of the original test cases? (When we

[PATCH] D153058: [clang][CFG] Support construction of a weak topological ordering of the CFG.

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:122 + + using BlockOrderTy = llvm::DenseMap; + BlockOrderTy BlockOrder; Would it make sense to have a vector instead and use block ids to get the order? ===

[PATCH] D155547: [analyzer] Model lifetime of a variable declared in for condition in CFG correctly

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LG, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155547/new/ https://reviews.llvm.org/D155547 __

[PATCH] D155547: [analyzer] Model lifetime of a variable declared in for condition in CFG correctly

xazax.hun added inline comments. Comment at: clang/test/Analysis/lifetime-cfg-output.cpp:1 // RUN: %clang_cc1 -fcxx-exceptions -fexceptions -analyze -analyzer-checker=debug.DumpCFG -analyzer-config cfg-lifetime=true,cfg-temporary-dtors=false,cfg-rich-constructors=false -analy

[PATCH] D155694: [NFC][analyzer] Enable implicit destructor for cfg-lifetime tests

xazax.hun added a comment. In D155694#4514322 , @tomasz-kaminski-sonarsource wrote: > Updating this tests illustrated that we are missing a lot of coverage for the > objects with trivial destructors, but I think this should be addressed as a > separate

[PATCH] D155694: [NFC][analyzer] Enable implicit destructor for cfg-lifetime tests

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Thanks! Comment at: clang/test/Analysis/lifetime-cfg-output.cpp:6 +// that has non-trivial destructor. As the behavior for such types is different +// from ones with tr

[PATCH] D155446: [clang][dataflow] Eliminate duplication between `AggregateStorageLocation` and `StructValue`.

xazax.hun accepted this revision. xazax.hun added a comment. I did not do a thorough review checking every line, but I read the design paper and skimmed through this patch. Love the direction, and I am OK with landing this as is. Comment at: clang/include/clang/Analysis/FlowS

[PATCH] D155847: [analyzer] Fix crash in GenericTaintChecker when propagatig taint to AllocaRegion

xazax.hun accepted this revision. xazax.hun added a comment. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155847/new/ https://reviews.llvm.org/D155847 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D155921: [clang][dataflow] Reverse course on `getValue()` deprecation.

xazax.hun accepted this revision. xazax.hun added a comment. Sounds good to me. I believe this make check author's lives easier. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155921/new/ https://reviews.llvm.org/D155921 ___

[PATCH] D153058: [clang][CFG] Support construction of a weak topological ordering of the CFG.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Thanks, looks good to me! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153058/new/ https://reviews.llvm.org/D153058

[PATCH] D155446: [clang][dataflow] Eliminate duplication between `AggregateStorageLocation` and `StructValue`.

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:201 -/// Models a value of `struct` or `class` type, with a flat map of fields to -/// child storage locations, containing all accessible members of base struct -/// and class types.

[PATCH] D156658: [clang][dataflow] When checking `ExprToLoc` convergence, only consider children of block terminator.

xazax.hun added a comment. In D156658#4606400 , @mboehme wrote: > We only do widening at loop heads, and this means that widening only affects > locations and values that flow into the loop from the outside or from a > previous loop iteration. > > But c

[PATCH] D156658: [clang][dataflow] When checking `ExprToLoc` convergence, only consider children of block terminator.

xazax.hun added a comment. > It looks as if, instead, what we should be doing to improve convergence in a > sound manner is to implement widening for ExprToLoc. I'll submit a > corresponding patch shortly. +1, I believe we want `ExprToLoc` to converge. That being said, if we can get away with

[PATCH] D156658: [clang][dataflow] When checking `ExprToLoc` convergence, only consider children of block terminator.

xazax.hun added a comment. In D156658#4552965 , @mboehme wrote: > I've investigated this in more detail. Unfortunately, it turns out that it's > not quite as simple as just implementing widening on `ExprToLoc`. > > One of the reasons for this is that we

[PATCH] D153071: [clang][dataflow] Refactor `DataflowWorklist` types and add a WTO version.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Mea culpa. Looks like I did not anticipate non-RPO worklists. Thanks for cleaning this up, looks good to me! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

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

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:501 public: +const iterator &operator*() const { return *this; } + Is this just a dummy to make sure this sat

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

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Overall looks good to me. I think the changes to the notes already make the analyzer more useful so there are some observable benefits to this patch. Comment at: clan

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

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1282 + LLVM_ATTRIBUTE_RETURNS_NONNULL + const Expr *getExpr() const { return Ex; } + LLVM_ATTRIBUTE_RETURNS_NONNULL ---

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

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:556 + bool IsLocal = + isa_and_nonnull(MR) && + isa(MR->getMemorySpace()); I think strictly speaking this might be "unsound" resulting in false positives in

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

xazax.hun accepted this revision. xazax.hun added a comment. I love it, I think we can land this as is. If there are further comments, we can address those in follow-up PRs. Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:1190 CXXRecordDecl::field_iterator CurFi

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

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:556 + bool IsLocal = + isa_and_nonnull(MR) && + isa(MR->getMemorySpace()); tomasz-kaminski-sonarsource wrote: > xazax.hun wrote: > > I think strictly speakin

[PATCH] D153058: [clang][CFG] Support construction of a weak topological ordering of the CFG.

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:56-64 + template struct NodeData { +// The block from which the interval was constructed. It is either the +// graph's entry block or has at least one predecessor outs

[PATCH] D154478: [analyzer][NFC] Use unique_ptrs for PathDiagnosticConsumers

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

[PATCH] D154935: [clang][dataflow] Introduce `getFieldValue()` test helpers.

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp:94 Environment Env(DAContext, *Fun); - Value *Val = Env.createValue(Ty); - ASSERT_NE(Val, nullptr); - StructValue *SVal = clang

[PATCH] D154965: [clang][dataflow] Fix initializaing a reference field with an `InitListExpr`.

xazax.hun accepted this revision. xazax.hun added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:720-725 for (auto It : llvm::zip(Fields, S->inits())) { const FieldDecl *Field = std::get<

[PATCH] D154969: [dataflow] document flow condition

xazax.hun accepted this revision. xazax.hun added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:528 - /// Returns the token that identifies the flow condition of the environment. +

[PATCH] D155204: [clang][dataflow] Add `refreshStructValue()`.

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:706-711 +/// This function is primarily intended for use by checks that set custom +/// properties on `StructValue`s to model the state

[PATCH] D155204: [clang][dataflow] Add `refreshStructValue()`.

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:706-711 +/// This function is primarily intended for use by checks that set custom +/// properties on `StructValue`s to model the state of these values. Such checks +//

[PATCH] D155445: [analyzer][docs] Add CSA release notes

xazax.hun accepted this revision. xazax.hun added inline comments. This revision is now accepted and ready to land. Comment at: clang/docs/ReleaseNotes.rst:907 + Any use of this flag will result in an error. + (`7cd1f3ad22e4

[PATCH] D153273: [analyzer] Rework support for CFGScopeBegin, CFGScopeEnd, CFGLifetime elements

xazax.hun added inline comments. Comment at: clang/test/Analysis/lifetime-cfg-output.cpp:760 // CHECK-NEXT:2: [B1.1]++ -// CHECK-NEXT:3: [B2.2] (Lifetime ends) -// CHECK-NEXT:4: [B3.1] (Lifetime ends) +// CHECK-NEXT:3: [B2.4] (Lifetime ends) +// CHECK-NEXT:4:

[PATCH] D155445: [analyzer][docs] Add CSA release notes

xazax.hun added inline comments. Comment at: clang/docs/ReleaseNotes.rst:908 + (`7cd1f3ad22e4 `_) +- Fixed a null-pointer dereference crash inside the ``MoveChecker``. + (`d172b65ef001

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

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp:27 class BoolAssignmentChecker : public Checker< check::Bind > { -mutable std::unique_ptr BT; +mutable std::unique_ptr BT; void emitReport(ProgramStateRef stat

[PATCH] D158977: [clang][dataflow] Don't associate prvalue expressions with storage locations.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Thanks! Sometimes I am wondering whether we actually need a full map for PRValues. E.g., once we processed a `MaterializeTemporaryExpr`, we now have a location for the value, and it feel

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

xazax.hun accepted this revision. xazax.hun added a comment. LG! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159108/new/ https://reviews.llvm.org/D159108 ___ cfe-commits mailing list cfe-commits@lists.

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

xazax.hun accepted this revision. xazax.hun added a comment. LG! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159109/new/ https://reviews.llvm.org/D159109 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D159355: [clang][dataflow] Unsoundly treat "Unknown" as "Equivalent" in widening.

xazax.hun accepted this revision. xazax.hun added a comment. Sorry for the late review. This looks good to me, but I hope we will be able to undo it soon :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159355/new/ https://reviews.llvm.org/D159355

[PATCH] D136811: [-Wunsafe-buffer-usage] WIP: RFC: NFC: User documentation.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Overall, the document looks good to me, I like the general direction. I still see some pending comments (mostly small wording fixes) from Aaron. Repository: rC Clang CHANGES SINCE LA

[PATCH] D137348: [-Wunsafe-buffer-usage] Introduce an abstraction for fixable code patterns.

xazax.hun added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:70 + + virtual ~Gadget() {} + Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:107 +/// out of bounds. +class IncrementGadget : public UnsafeGadget { +

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

xazax.hun added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:168-169 + static Matcher matcher() { +// FIXME: What if the index is integer literal 0? Should this be +// a safe gadget in this case? +return stmt( As per some of

[PATCH] D138253: [-Wunsafe-buffer-usage] NFC: Implement fix-strategies and variable-use-claiming.

xazax.hun added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:236 + using UseSetTy = SmallSet; + using DefMapTy = DenseMap; + NoQ wrote: > NoQ wrote: > > This extra payload wasn't advertised but it's very useful because it's hard > > to

[PATCH] D138318: [-Wunsafe-buffer-usage] Improve pointer match pattern

xazax.hun added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:31-39 +hasType(pointerType()), +hasType(autoType( +hasDeducedType(hasUnqualifiedDesugaredType(pointerType(), +// DecayedType, e.g., array type in formal parameter dec

[PATCH] D138321: [-Wunsafe-buffer-usage] Ignore array subscript on literal zero

xazax.hun added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:219 +arraySubscriptExpr(hasBase(ignoringParenImpCasts(hasPointerType())), + unless(hasIndex(integerLiteral(equals(0) .bind("arraySubscr")); ---

[PATCH] D138329: [-Wunsafe-buffer-usage] Add a new recursive matcher to replace `forEachDescendant` in unsafe buffer check

xazax.hun added a comment. Is the problem `forEachDescendant` matching statements inside blocks and lambdas? I wonder if this behavior would surprise people, so I think it would be better to: - Potentially add a template bool parameter to `forEachDescendant` controlling this behavior. - Review

[PATCH] D136603: [analyzer] getBinding should auto-detect type only if it was not given

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Sounds reasonable to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136603/new/ https://reviews.llvm.org/D136603 _

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

xazax.hun added a comment. I did not spend too much time thinking about this yet, but this sounds scary. I wonder if we should target the underlying problem instead, i.e., making sure we never have dead symbols as representatives for eq. classes. What do you think? Repository: rG LLVM Github

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

xazax.hun added a comment. I guess there are some more options. We could try keeping representatives alive no matter what. It could be a good exercise to see if doing that makes any difference in the analysis results. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D138426: Fix #58958 on github

xazax.hun added inline comments. Comment at: clang/lib/Sema/SemaFixItUtils.cpp:136-137 +// Do no take address of const pointer to get void* +const PointerType *FromPtrTy = dyn_cast(FromQTy); +const PointerType *ToPtrTy = dyn_cast(ToQTy); +if (FromPtrTy && FromPtrT

[PATCH] D138426: Fix #58958 on github

xazax.hun added inline comments. Comment at: clang/lib/Sema/SemaFixItUtils.cpp:136-137 +// Do no take address of const pointer to get void* +const PointerType *FromPtrTy = dyn_cast(FromQTy); +const PointerType *ToPtrTy = dyn_cast(ToQTy); +if (FromPtrTy && FromPtrT

[PATCH] D108230: [analyzer] Ignore single element arrays in getStaticSize() conditionally

xazax.hun added a comment. I am a bit conflicted. It is unfortunate that C and C++ compilers regarded single element array members as flexible array members. On the other hand, looking at GCC, it recently added -fstrict-flex-arrays=2

[PATCH] D72810: [LifetimeAnalysis] Add support for lifetime annotations on functions

xazax.hun marked an inline comment as done. xazax.hun added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:7727 +// Move function type attribute to the declarator. +case ParsedAttr::AT_LifetimeContract: aaron.ballman wrote: > xazax.hun wrote:

[PATCH] D63279: [Analyzer] Unroll for-loops where the upper boundary is a variable with know value

xazax.hun added a comment. In D63279#1939349 , @Szelethus wrote: > (note: I forgot to submit this a couple weeks ago) > > LLVM is crashing on me due to the issue mentioned in D75678 > , but on Bitcoin, Xerces, CppCheck and

[PATCH] D73729: [analyzer] AnalyzerOptions: Remove 'fixits-as-remarks'

xazax.hun added a reviewer: Szelethus. xazax.hun added a comment. Herald added subscribers: martong, steakhal. Maybe Kristof has some opinion whether we still need this :) He might be up to date whether CodeChecker is using this feature. Repository: rC Clang CHANGES SINCE LAST ACTION https

[PATCH] D73729: [analyzer] AnalyzerOptions: Remove 'fixits-as-remarks'

xazax.hun added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:310 -ANALYZER_OPTION(bool, ShouldEmitFixItHintsAsRemarks, "fixits-as-remarks", -"Emit fix-it hints as remarks for testing purposes", -false) --

[PATCH] D75163: [analyzer][StreamChecker] Adding precall and refactoring.

xazax.hun added a comment. If we were to refactor this check I wonder if it would make sense to port `evalCall` to `postCall`, so the analyzer engine will conjure the symbol for us. I wonder what @NoQ thinks about the pros and cons of the approaches. As far as I understand the con of evalCall is

[PATCH] D75432: [analyzer][NFC][MallocChecker] Convert many parameters into CallEvent

xazax.hun added a comment. Some nits inline. Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:995 -void MallocChecker::checkBasicAlloc(CheckerContext &C, const CallExpr *CE, -ProgramStateRef State) const { - State = MallocMe

[PATCH] D75431: [analyzer][NFC] Merge checkNewAllocator's paramaters into CXXAllocatorCall

xazax.hun added a comment. Other than a nit looks good to me but wait for @NoQ before committing. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:208 + SValBuilder &getSValBuilder() const { +return State->getStateManager().getSValBuilder();

[PATCH] D75430: [analyzer][NFC] Introduce CXXDeallocatorCall, deploy it in MallocChecker

xazax.hun added a comment. This might be a silly question but is `CXXDeleteExpr` the only way to invoke a deallocator? What would happen if the user would call it by hand? Should we expect `ExprEngine` to trigger a callback in that case? Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D75163: [analyzer][StreamChecker] Adding precall and refactoring.

xazax.hun added a comment. In D75163#1902921 , @balazske wrote: > In D75163#1902816 , @xazax.hun wrote: > > > If we were to refactor this check I wonder if it would make sense to port > > `evalCall` to `postCall`,

[PATCH] D70150: [analyzer] Don't clean up dead symbols from constraints twice.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Nice catch! I wonder if there is a case where the original code is less wasteful, e.g. the majority of the symbols are dead and a lot of checks splitting the execution. But this sounds v

[PATCH] D70470: [analyzer] Add FuchsiaHandleCheck to catch handle leaks, use after frees and double frees

xazax.hun created this revision. xazax.hun added reviewers: NoQ, haowei. xazax.hun added a project: clang. Herald added subscribers: Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity, mgorny. This check is based on ht

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

xazax.hun created this revision. xazax.hun added reviewers: aaron.ballman, NoQ. xazax.hun added a project: clang. Herald added subscribers: Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity. These are three new attribu

[PATCH] D70470: [analyzer] Add FuchsiaHandleCheck to catch handle leaks, use after frees and double frees

xazax.hun marked an inline comment as done. xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:67 +// functions: +// 1. __attribute__((clang::acquire_handle)) |handle will be acquired +// 2. __attribute__((clang::release_hand

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

xazax.hun marked an inline comment as done. xazax.hun added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7287 + case ParsedAttr::AT_AcquireHandle: +handleSimpleAttribute(S, D, AL); +break; NoQ wrote: > The next obvious step here would be

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

xazax.hun marked an inline comment as done. xazax.hun added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7287 + case ParsedAttr::AT_AcquireHandle: +handleSimpleAttribute(S, D, AL); +break; NoQ wrote: > xazax.hun wrote: > > NoQ wrote: > >

[PATCH] D70470: [analyzer] Add FuchsiaHandleCheck to catch handle leaks, use after frees and double frees

xazax.hun marked 13 inline comments as done. xazax.hun added a comment. Thanks for the review! I am not very well versed in Fuchsia's syscalls yet but my understanding is that not all of the syscalls can fail so we do not need all the users to check for errors. But this is something I will veri

[PATCH] D70470: [analyzer] Add FuchsiaHandleCheck to catch handle leaks, use after frees and double frees

xazax.hun updated this revision to Diff 230187. xazax.hun added a comment. - Addressed **some** of the review comments. More changes are planned :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70470/new/ https://reviews.llvm.org/D70470 Files: clang/include/clang/StaticAnalyzer/Check

[PATCH] D70470: [analyzer] Add FuchsiaHandleCheck to catch handle leaks, use after frees and double frees

xazax.hun updated this revision to Diff 230287. xazax.hun marked 10 inline comments as done. xazax.hun added a comment. - Explicitly model "maybe" states. - Fix some escaping issues. - Address most review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70470/new/ https://reviews

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

xazax.hun marked 3 inline comments as done. xazax.hun added inline comments. Comment at: clang/include/clang/Basic/Attr.td:3445 + let Spellings = [Clang<"acquire_handle">]; + let Subjects = SubjectList<[Function, ParmVar], ErrorDiag>; + let Documentation = [AcquireHandleDocs];

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

xazax.hun updated this revision to Diff 230294. xazax.hun added a comment. - Check trivial cases when the acquire_handle attribute is not on an output parameter. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70469/new/ https://reviews.llvm.org/D70469 Files: clang/include/clang/Basic

[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

xazax.hun updated this revision to Diff 230347. xazax.hun marked 8 inline comments as done. xazax.hun added a comment. - Address review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70469/new/ https://reviews.llvm.org/D70469 Files: clang/include/clang/Basic/Attr.td clang/

[PATCH] D70470: [analyzer] Add FuchsiaHandleCheck to catch handle leaks, use after frees and double frees

xazax.hun marked an inline comment as done. xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:19-20 +// handle variable is passed to different function calls or syscalls, its state +// changes. The state changes can be general

[PATCH] D70470: [analyzer] Add FuchsiaHandleCheck to catch handle leaks, use after frees and double frees

xazax.hun updated this revision to Diff 230500. xazax.hun edited the summary of this revision. xazax.hun added a comment. - Make the patch standalone and the check on by default for the fuchsia platform. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70470/new/ https://reviews.llvm.org/

[PATCH] D70470: [analyzer] Add FuchsiaHandleCheck to catch handle leaks, use after frees and double frees

xazax.hun updated this revision to Diff 230668. xazax.hun added a comment. - Fix member operator modeling. - Added new lines to the end of files. - Added documentation. - Minor typo fixes in tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70470/new/ https://reviews.llvm.org/D70470

[PATCH] D70693: [scan-build-py] Set of small fixes

xazax.hun created this revision. xazax.hun added reviewers: NoQ, haowei, rizsotto.mailinglist. Herald added subscribers: Charusso, gamesh411, Szelethus, dkrupp, rnkovacs, whisperity. Herald added a project: clang. xazax.hun added a subscriber: phosek. This patch fix some small errors in scan-buil

[PATCH] D70693: [scan-build-py] Set of small fixes

xazax.hun added a comment. Sorry, there was a brain hiccup on my side. We just do not generate HTMLs for every translation units (which is the case for plists). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70693/new/ https://reviews.llvm.org/D706

<    4   5   6   7   8   9   10   11   12   13   >