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

[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

[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

[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

[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

[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

[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

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

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

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

[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

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

[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

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

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

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

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

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

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

[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

[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

[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

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

[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 *() const { return *this; } + Is this just a dummy to make sure this satisfies

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

xazax.hun added inline comments. Comment at: clang/lib/Analysis/IntervalPartition.cpp:121 +Index.emplace(N, ID); + Graph.Intervals.emplace_back(ID, Header, std::move(Data.Nodes)); } It would probably take for me some time to understand what is going on

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

xazax.hun added inline comments. Comment at: clang/unittests/Analysis/IntervalPartitionTest.cpp:21 +namespace { +template using NodeData = CFGInterval::NodeData; +} // namespace Another redundant anonymous namespace here. Repository: rG LLVM Github

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

xazax.hun added a comment. Could you rebase this patch to the latest tip of tree? Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:73 + + // Whether this node is the head of a feedback edge within the interval. + bool IsFeedbackHead = false;

[PATCH] D152263: [clang][CFG] Add support for partitioning CFG into intervals.

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

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

xazax.hun accepted this revision. xazax.hun added a comment. Nice, looks like this change did catch some unintentional copies! Already paying dividends :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153674/new/ https://reviews.llvm.org/D153674

[PATCH] D153491: [dataflow] Avoid copying environment

xazax.hun added a comment. In D153491#4445051 , @ymandel wrote: > In D153491#4443704 , @xazax.hun > wrote: > >> This sounds extremely error-prone to me. In case copying the analysis state >> has side effects

[PATCH] D153584: [dataflow] Make SAT solver deterministic

xazax.hun added a comment. Is there a measurable perf cost for this determinism? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153584/new/ https://reviews.llvm.org/D153584 ___ cfe-commits mailing list

[PATCH] D153488: [dataflow] HTMLLogger: meaningful names for flow condition variables

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Should we have some documentation or tooltip explaining the users what the meaning of those names are? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D153476: [dataflow] document & test determinism of formula structure

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/Arena.cpp:13 +// Compute a canonical key for an unordered poir of operands, so that we +// can intern (A & B) and (B

[PATCH] D152263: [clang][CFG] Add support for partitioning CFG into intervals.

xazax.hun accepted this revision. xazax.hun added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Analysis/IntervalPartition.cpp:26 + + std::queue Worklist; + for (const CFGBlock *S : Header.succs()) ymandel wrote: >

[PATCH] D153491: [dataflow] Avoid copying environment

xazax.hun added a comment. This sounds extremely error-prone to me. In case copying the analysis state has side effects like this, I would argue we want such operations to be really explicit. What do you think? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D152263: [clang][CFG] Add support for partitioning CFG into intervals.

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:35 + + std::set Blocks; + Nit: I wonder if we want something like `llvm::DenseSet` when we use smaller types like pointers. Same for `Successors`.

[PATCH] D153006: [clang][dataflow] Perform deep copies in copy and move operations.

xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/RecordOps.cpp:56-57 + + llvm::iterator_range< + llvm::DenseMap::const_iterator> + DstChildren = DstVal->children(); I know we usually like to mention types at least once,

[PATCH] D153006: [clang][dataflow] Perform deep copies in copy and move operations.

xazax.hun added a comment. I am not opposed to this solution, but I do anticipate some performance problems in the future. I wonder if copy-on-write, or some persistent data structures where copying is cheap would be a better strategy long term. But getting it right first and fast after sounds

[PATCH] D152813: [clang][dataflow] Create `Value`s for integer literals.

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/Arena.h:89 + /// `Value`. These literals are the same every time. + IntegerValue (llvm::APInt Value); + gribozavr2 wrote: > Should we be taking the type into account? If

[PATCH] D152732: [clang][dataflow] Support limits on the SAT solver to force timeouts.

xazax.hun added a comment. In D152732#4414707 , @ymandel wrote: > Ultimately what matters for a user is the global limit. I am not 100% sure about that. While it is true that the user cares about the process not hanging, but global vs local limits can

[PATCH] D152732: [clang][dataflow] Support limits on the SAT solver to force timeouts.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Huge +1, I think most solvers need to have some resource limits in place as the runtime can explode. I am just not 100% what is the best approach here, putting a global limit on the

[PATCH] D152263: [clang][CFG] Add support for partitioning CFG into intervals.

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:22 + +struct CFGInterval { + CFGInterval(const CFGBlock *Header) : Header(Header), Blocks({Header}) {} A concise definition of what is an interval with a

[PATCH] D151818: [clang][analyzer][NFC] Use the operator new directly with the `BumpPtrAllocator`

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/D151818/new/ https://reviews.llvm.org/D151818

[PATCH] D151194: [clang][dataflow] Add support for return values of reference type.

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:196 + void popCall(const CallExpr *Call, const Environment ); + void popCall(const CXXConstructExpr *Call, const Environment ); mboehme wrote: >

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

xazax.hun added a comment. I generally support the idea of a CXXLifetimeExtendedObj region, definitely cleaner than a CXXTempObjectRegion with static lifetime. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151325/new/

[PATCH] D151194: [clang][dataflow] Add support for return values of reference type.

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:196 + void popCall(const CallExpr *Call, const Environment ); + void popCall(const CXXConstructExpr *Call, const Environment );

[PATCH] D151201: [clang][dataflow] Fix a crash in `getLogicOperatorSubExprValue()`.

xazax.hun added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:5064 +bool target() { + return true || false || false || false; +} Should we also test that the value of the expression is `true` in the analysis

[PATCH] D151183: [clang][dataflow] Add a `ControlFlowContext::build()` overload taking a `FunctionDecl`.

xazax.hun accepted this revision. xazax.hun added inline comments. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. Comment at: clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:39 + /// Builds a ControlFlowContext from an

[PATCH] D150655: [clang][dataflow] Use `Strict` accessors in more places in Transfer.cpp.

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:717-723 +Value *SubExprVal = Env.getValueStrict(*SubExpr); +if (SubExprVal == nullptr) return; -Env.setStorageLocation(*S,

[PATCH] D150657: [clang][dataflow] Use `Strict` accessors in SignAnalysisTest.cpp.

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:134 LatticeTransferState ) { - StorageLocation *Loc = State.Env.getStorageLocation(*BO, SkipPast::None); - if (!Loc)

[PATCH] D150656: [clang][dataflow] Use `Strict` accessors in TypeErasedDataflowAnalysis.cpp.

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:303 - auto *InitStmtLoc = Env.getStorageLocation(*InitStmt, SkipPast::Reference); - if (InitStmtLoc == nullptr) -return; - -

[PATCH] D149144: [clang][dataflow] Eliminate intermediate `ReferenceValue`s from `Environment::DeclToLoc`.

xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:329 +static void +builtinTransferScopeEnd(const CFGScopeEnd , +TypeErasedDataflowAnalysisState ) { mboehme wrote: > xazax.hun

[PATCH] D149144: [clang][dataflow] Eliminate intermediate `ReferenceValue`s from `Environment::DeclToLoc`.

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:329 +static void +builtinTransferScopeEnd(const CFGScopeEnd , +TypeErasedDataflowAnalysisState ) {

[PATCH] D149144: [clang][dataflow] Eliminate intermediate `ReferenceValue`s from `Environment::DeclToLoc`.

xazax.hun added a subscriber: dcoughlin. xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:616 + bool erased = DeclToLoc.erase(); + if (!erased) +D.dump(); Would we dump this in release builds? Do we want

[PATCH] D149464: [clang][dataflow] Expose DataflowAnalysisContext from DataflowEnvironment.

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:189 + /// Deprecated. Use *getDataflowAnalysisContext().getOptions().Log instead. Logger () const { return *DACtx->getOptions().Log; } bazuzi

[PATCH] D149464: [clang][dataflow] Expose DataflowAnalysisContext from DataflowEnvironment.

xazax.hun accepted this revision. xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:189 + /// Deprecated. Use *getDataflowAnalysisContext().getOptions().Log instead. Logger () const { return

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

xazax.hun accepted this revision. xazax.hun added a comment. I am a bit overloaded at the moment, feel free to commit. I can still add comments later that could be addressed in a follow up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146591/new/

[PATCH] D148344: [clang][dataflow] Refine matching of optional types to anchor at top level.

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/D148344/new/ https://reviews.llvm.org/D148344

[PATCH] D148344: [clang][dataflow] Refine matching of optional types to anchor at top level.

xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:262 /// Returns true if and only if `Type` is an optional type. bool isOptionalType(QualType Type) { if (!Type->isRecordType()) Why do we

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp:77 + +void escape(char C, llvm::raw_ostream ) { + switch (C) { We already sort of have a way to escape HTML here:

[PATCH] D147302: [clang][dataflow] Add `create()` methods to `Environment` and `DataflowAnalysisContext`.

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/DataflowAnalysisContext.h:100 +// used `StorageLocation` subclasses and make them use a

[PATCH] D147326: [clang][dataflow][NFC] Share code between Environment ctor and pushCallInternal().

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:465 + /// referenced in `FuncDecl`. `FuncDecl` must have a body. + void initVars(const FunctionDecl *FuncDecl); I wonder if we should rename this to

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

xazax.hun added a comment. In D146591#4213614 , @sammccall wrote: > (e.g. does the MLIR dataflow framework have the same idea about how the > analysis timeline relates to the CFG and elements within it? And if we want > to show the clang AST or the

[PATCH] D144730: [FlowSensitive] Log analysis progress for debugging purposes

xazax.hun added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/Logger.cpp:17 +Logger ::null() { + struct NullLogger : Logger {}; + static auto *Instance = new NullLogger(); Adding `final`? Just in case it can help with devirtualization.

[PATCH] D144730: [FlowSensitive] Log analysis progress for debugging purposes

xazax.hun added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:186 + Logger () const { +return DACtx->getOptions().Log ? *DACtx->getOptions().Log : Logger::null(); + } If we already have a `NullLogger`, I

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

xazax.hun added a comment. While I am OK with this approach, I wonder if it would be too much work to split the HTML generation and emitting data up. E.g., the logger could emit YAML or JSON that could be parsed by a python script to create the HTML (or the HTML itself could use JS to parse it

[PATCH] D146514: [clang][dataflow] Fix crash when RHS of `&&` or `||` calls `noreturn` func.

xazax.hun added a comment. In D146514#4212528 , @mboehme wrote: > No, I think this is a different case. Sorry, I might have been unclear. I am 100% agree that these are different cases. I was wondering whether we can/should have a single code path

[PATCH] D146507: [clang][dataflow][NFC] Eliminate StmtToEnvMap interface.

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

[PATCH] D146538: [analyzer] Fix crashing getSValFromInitListExpr for nested initlists

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Ugh :/ I wonder if we should also open tickets on GitHub to reduce the chance of forgetting addressing the root cause for these. What do you think? Repository: rG LLVM Github

[PATCH] D146514: [clang][dataflow] Fix crash when RHS of `&&` or `||` calls `noreturn` func.

xazax.hun added a comment. This fix looks good for me for this particular problem, but I wonder whether the solution is general enough. In case the analysis figures out that a call would not return (e.g., the `value` method is called on a provably empty optional, and it would throw instead of

[PATCH] D145069: [analyzer][NFC] Split the no state change logic and bug report suppression into two visitors

xazax.hun added a comment. Do you plan to selectively enable warnings coming from the STL to catch misuses of certain STL types? I think we should probably discuss this direction first. It is, in general, a fragile approach. Users might use Clang with GCC's, LLVM's, or MSVC's standard

[PATCH] D135495: [clang-tidy] handle exceptions properly in `ExceptionAnalyzer`

xazax.hun added a comment. I think at this point it is ok to merge. Any other comments can be addressed in follow-up commits. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135495/new/ https://reviews.llvm.org/D135495 ___ cfe-commits mailing

[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`

xazax.hun accepted this revision. xazax.hun added a comment. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135495/new/ https://reviews.llvm.org/D135495 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`

xazax.hun added a comment. Overall, I like the structure of this patch and the references back to the standard. But every time we compare type pointers, they might compare inequal when one of the types have sugar on it while the other does not. Please review all of those comparisons to see

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

xazax.hun added a comment. I am ok with committing this to unblock people hitting this assert, but at the same time I wonder if we want to open a ticket on GitHub that we might want to rethink how some of this works. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D143750: [clang-tidy] Clarify documention of `bugprone-unchecked-optional-access`.

xazax.hun added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst:278 + +Given that ``value()`` has well-defined program termination behavior, why treat +it the same as ``operator*()`` which causes undefined behavior

[PATCH] D142710: [clang][dataflow] Relax validity assumptions in `UncheckedOptionalAccessModel`.

xazax.hun added a comment. In D142710#4096325 , @ymandel wrote: > In D142710#4094934 , @xazax.hun > wrote: > >> This change looks good to me. I wonder, however, whether the behavior should >> be parameterized

[PATCH] D140897: [clang][dataflow] Fix handling of `DeclRefExpr`s to `BindingDecl`s.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. I hope we will be able to get rid of this `SkipPast` thing at some point by looking at the value categories of the AST instead. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D142710: [clang][dataflow] Relax validity assumptions in `UncheckedOptionalAccessModel`.

xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. This change looks good to me. I wonder, however, whether the behavior should be parameterized in the future. E.g., whether the user of the analysis should be able to make a decision

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

xazax.hun added a comment. Overall looks good to me, I wonder if the tests could be less manual though. E.g., instead of asserting true/false, checking if the assignment would compile. This way we can be sure that the method in ASTContext matches the behavior of the compiler (and we get

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

xazax.hun added a reviewer: erichkeane. xazax.hun added a comment. Since now the patch touches Clang proper, adding one more reviewer. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135495/new/ https://reviews.llvm.org/D135495 ___ cfe-commits

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

xazax.hun added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:81 + FromPointeeTy->getUnqualifiedDesugaredType(); + const Type *ToPointeeUQTy = ToPointeeTy->getUnqualifiedDesugaredType(); + Here I am also

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

xazax.hun added a comment. Also, could you open a bug report about the strange exception behavior on GitHub? Hopefully someone working on conformance can take a look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135495/new/ https://reviews.llvm.org/D135495

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

xazax.hun accepted this revision. xazax.hun added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:60 +// Checks if T1 is convertible to T2. +static bool isMultiLevelConvertiblePointer(QualType

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

xazax.hun added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:95 + + if (P1->isArrayType() && P2->isArrayType()) { +// At every level that array type is involved in, at least If none of the functions I recommended

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

xazax.hun added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:60 +// Checks if T1 is convertible to T2. +static bool isMultiLevelConvertiblePointer(QualType P1, QualType P2, + unsigned CurrentLevel

  1   2   3   4   5   6   7   8   9   10   >