[PATCH] D148554: [dataflow] Extract arena for Value/StorageLocation out of DataflowAnalysisContext

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Arena.h:91-97
+  /// Returns a boolean value as a result of substituting `Val` and its sub
+  /// values based on entries in `SubstitutionsCache`. Intermediate results are
+  /// stored in `SubstitutionsCache` to avoid reprocessing values that have
+  /// already been visited.
+  BoolValue (
+  BoolValue ,
+  llvm::DenseMap );

This can be removed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148554/new/

https://reviews.llvm.org/D148554

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:52
+  /// Returns whether `B` is reachable from the entry block.
+  bool isBlockReachable(const CFGBlock *B) const {
+return BlockReachable[B->getBlockID()];

What do you think about making this a const reference? Alternatively, let's 
document that it must not be null?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146514/new/

https://reviews.llvm.org/D146514

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:524
 
-void transferSwap(const StorageLocation ,
-  const StorageLocation ,
-  LatticeTransferState ) {
-  auto *OptionalVal1 = State.Env.getValue(OptionalLoc1);
-  assert(OptionalVal1 != nullptr);
+void transferSwap(const Expr , SkipPast E1Skip, const Expr ,
+  Environment ) {

What do you think about passing `const StorageLocation*` instead of `const 
Expr&`? This way we don't need to pass `E1Skip`.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:534
+if (Loc2 != nullptr)
+  Env.setValue(*Loc2, createOptionalValue(Env, Env.makeAtomicBoolValue()));
+return;

Any reason to not set a fresh value for `Loc1` in this case (similarly a fresh 
value for `Loc2` below)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142710/new/

https://reviews.llvm.org/D142710

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142468: [clang][dataflow] Fix bug in handling of reference-typed fields.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:209
 if (VD->getType()->isReferenceType()) {
+  assert(isValidReferenceLoc(*DeclLoc, Env) &&
+ "reference-typed declarations map to `ReferenceValue`s");

Can we use `isa_and_nonnull(Env.getValue(Loc))` instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142468/new/

https://reviews.llvm.org/D142468

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141716: [clang][dataflow] Add (initial) debug printing for `Value` and `Environment`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:792
+  // fields are printed.
+  llvm::dbgs() << "DeclToLoc:\n";
+  for (auto [D, L] : DeclToLoc)

Shouldn't this be `OS`? Same for those below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141716/new/

https://reviews.llvm.org/D141716

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140859: [clang][dataflow] Allow analyzing multiple functions in unit tests

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp:89-104
+  unsigned FunctionBeginOffset =
+  SourceManager.getFileOffset(Func->getBeginLoc());
+  unsigned FunctionEndOffset = SourceManager.getFileOffset(Func->getEndLoc());
+
   unsigned I = 0;
-  auto Annotations = AnnotatedCode.ranges();
+  std::vector Annotations = AnnotatedCode.ranges();
+  llvm::erase_if(Annotations, [=](llvm::Annotations::Range R) {





Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:177
 /// Runs dataflow specified from `AI.MakeAnalysis` and `AI.PostVisitCFG` on the
 /// body of the function that matches `AI.TargetFuncMatcher` in `AI.Code`.
 /// Given the analysis outputs, `VerifyResults` checks that the results from 
the

"bodies of all functions that match"



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:269
 /// Runs dataflow specified from `AI.MakeAnalysis` and `AI.PostVisitCFG` on the
 /// body of the function that matches `AI.TargetFuncMatcher` in `AI.Code`. 
Given
 /// the annotation line numbers and analysis outputs, `VerifyResults` checks

"bodies of all functions that match"



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:295
 /// Runs dataflow specified from `AI.MakeAnalysis` and `AI.PostVisitCFG` on the
 /// body of the function that matches `AI.TargetFuncMatcher` in `AI.Code`. 
Given
 /// the state computed at each annotated statement and analysis outputs,

"bodies of all functions that match"



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:367
 VerifyResults(AnnotationStates, AO);
+AnnotationStates.clear();
   });

Can you please add a comment to describe why this needs to be cleared?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140859/new/

https://reviews.llvm.org/D140859

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137948: [clang][dataflow] Add widening API and implement it for built-in boolean model.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:467
+
+  //`DeclContext` of the block being analysed if provided.
   std::vector CallStack;





Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:418-419
+Effect = LatticeJoinEffect::Changed;
+
+
+  return Effect;




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137948/new/

https://reviews.llvm.org/D137948

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137334: [clang][dataflow] Generalize custom comparison to return tri-value result.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:52
+/// Indicates the result of a tentative comparison.
+enum class ComparisonStatus {
+  Same,

Alternative that I slightly prefer - `ComparisonResult`.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:75
+///   `Different`:`Val1` is distinct from `Val2`, according to the model.
+///   `Unknown`: The model does not determine a relationship between `Val1`
+///and `Val2`.

Perhaps replace "does not" with "can't"?



Comment at: 
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:365
 if (IsSet1 == nullptr)
-  return true;
+  return IsSet2 ? ComparisonStatus::Same : ComparisonStatus::Different;
 

Why is it same if `IsSet1` is null and `IsSet2` isn't null?



Comment at: 
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:1185
-
-  bool compareEquivalent(QualType Type, const Value ,
- const Environment , const Value ,

Why remove this? It's not the same as the default implementation, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137334/new/

https://reviews.llvm.org/D137334

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135964: [clang][dataflow] Add equivalence relation for `Value` type.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/Value.cpp:15
+#include "llvm/Support/Casting.h"
+#include 
+

This seems unnecessary.



Comment at: clang/unittests/Analysis/FlowSensitive/ValueTest.cpp:30
+  EXPECT_TRUE(areEquivalentValues(V1, V2));
+  // Symmetry.
+  EXPECT_TRUE(areEquivalentValues(V2, V1));

I suggest dropping the symmetry comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135964/new/

https://reviews.llvm.org/D135964

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135397: [clang][dataflow] Add support for a Top value in boolean formulas.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:156
 
+  TopBoolValue () {
+return takeOwnership(std::make_unique());

Please add a comment.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:291
 
+  BoolValue () const {
+return DACtx->createTopBoolValue();

Please add a comment.



Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:100
+public:
+  explicit TopBoolValue() : BoolValue(Kind::TopBool) {}
+

`explicit` seems unnecessary.



Comment at: clang/lib/Analysis/FlowSensitive/WatchedLiteralsSolver.cpp:237-238
+  case Value::Kind::TopBool:
+// Nothing more to do. Each `TopBool` instance is mapped to a fresh
+// variable.
+break;

Where? Does that mean that `TopBool` never reaches the solver? I think it'd be 
good to clarify.



Comment at: 
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:1220
+  (void)0;
+  /*[[p2]]*/
+}

Why do we need to check two code points here and in the test below? It's not 
obvious what the difference between `p1` and `p2` is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135397/new/

https://reviews.llvm.org/D135397

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133935: [clang][dataflow] Refactor `clang/Analysis/FlowSensitive/MatchSwitchTest.cpp`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/unittests/Analysis/FlowSensitive/MatchSwitchTest.cpp:139
 TEST(MatchSwitchTest, ReturnNonVoid) {
   using namespace ast_matchers;
 

Let's move this below line 27 and remove it from the other tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133935/new/

https://reviews.llvm.org/D133935

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133933: [clang][dataflow] Modify `transfer` in `DataflowModel` to take `CFGElement` as input instead of `Stmt`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:235
+  /// Return value indicates whether the model processed the `Element`.
+  virtual bool transfer(const CFGElement *Elt, Environment ) = 0;
 };

In `DataflowAnalysis` we use the name `Element`. Can we use the same here too?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133933/new/

https://reviews.llvm.org/D133933

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h:75
+  std::vector
+  diagnose(ASTContext , const CFGElement *Elt, const Environment );
 

Why not `Ctx` like above?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133930/new/

https://reviews.llvm.org/D133930

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133865: [clang][dataflow] Replace usage of the deprecated overload of `checkDataflow`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupportTest.cpp:83-88
+AnalysisInputs(
+Code, hasName(Target),
+[](ASTContext , Environment &) {
+  return NoopAnalysis(
+  Context, /*ApplyBuiltinTransfer=*/false);
+}),

Do we need `.withASTBuildArgs({"-fsyntax-only", "-std=c++17"})` here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133865/new/

https://reviews.llvm.org/D133865

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132377: [clang][dataflow] Add `SetupTest` parameter for `AnalysisInputs`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:88
+/// Arguments for building the dataflow analysis.
+template  struct AnalysisInputs {
+  /// Input code that is analyzed.

Why move this? It makes it hard to tell if there are other changes. If there 
are other changes, let's keep it where it is to have a clean diff and move it 
in a separate commit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132377/new/

https://reviews.llvm.org/D132377

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132147: [clang][dataflow] Refactor `TestingSupport.h`

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:60
+/// Arguments for building the dataflow analysis.
+template  struct AnalysisInputs {
+  /// Input code that is analyzed.

Could you please indicate which members are mandatory and which are optional. 
Perhaps comments could start either with `/// Mandatory. ...` or `/// Optional. 
...`.



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:244
+///
+/// Requirements:
+///

Should requirements include the full list of the requirements of 
`checkDataflow` below?



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:295-296
 
-// Runs dataflow on the body of the function that matches `TargetFuncMatcher` 
in
-// code snippet `Code`. Requires: `AnalysisT` contains a type `Lattice`.
+// FIXME: Remove this function after usage has been updated to the overload
+// which uses the `AnalysisInputs` struct.
+//

Let's clearly indicate that it's deprecated.



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:339-340
 
-// Runs dataflow on the body of the function named `target_fun` in code snippet
-// `code`.
+// FIXME: Remove this function after usage has been updated to the overload
+// which uses the `AnalysisInputs` struct.
+//

Let's clearly indicate that it's deprecated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132147/new/

https://reviews.llvm.org/D132147

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131616: [clang][dataflow] Generalise match switch utility to other AST types and add a `CFGMatchSwitch` which currently handles `CFGStmt` and `CFGInitializer`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:24-25
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/CFG.h"

These seem unnecessary.



Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:28
+#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
+#include "llvm/ADT/StringRef.h"
+#include 

This seems unnecessary.



Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:30
+#include 
+#include 
+#include 

This seems unnecessary.



Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:32
+#include 
+#include 
+

This seems unnecessary.



Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:55
+MSActionT A) && {
+std::move(std::move(StmtBuilder).template CaseOf(M, A));
+return std::move(*this);

Is the outer move necessary? Also, aren't we supposed to assign the result back 
to `StmtBuilder`? Same comment for `InitBuilder` below.



Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:1
 //=== MatchSwitch.h -*- C++ 
-*-===//
 //

Let's add a FIXME to rename the file to ASTMatchSwitch.h and update the comment 
below.



Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:47-51
+template  using MSMatcherT = ast_matchers::internal::Matcher;
+
+template 
+using MSActionT = std::function;

What do you think about calling these `MatchSwitchMatcher` and 
`MatchSwitchAction`?



Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:64
+/// callbacks, which together define a switch that can be applied to a node
+/// whose type can be derived from `BaseT`. This structure can simplify the
+/// definition of `transfer` functions that rely on pattern-matching.





Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:90
   ///
-  ///  `Node` should be a subclass of `Stmt`.
-  template 
-  MatchSwitchBuilder &&
-  CaseOf(ast_matchers::internal::Matcher M,
- std::function
- A) && {
+  ///  `NodeT` should be derivable from `BaseT`.
+  template 

Let's add a static assert.



Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:90
   ///
-  ///  `Node` should be a subclass of `Stmt`.
-  template 
-  MatchSwitchBuilder &&
-  CaseOf(ast_matchers::internal::Matcher M,
- std::function
- A) && {
+  ///  `NodeT` should be derivable from `BaseT`.
+  template 

sgatev wrote:
> Let's add a static assert.




Comment at: clang/unittests/Analysis/FlowSensitive/CFGMatchSwitchTest.cpp:12
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Stmt.h"

This seems unnecessary.



Comment at: clang/unittests/Analysis/FlowSensitive/CFGMatchSwitchTest.cpp:14-15
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/CFG.h"

These seem unnecessary.



Comment at: clang/unittests/Analysis/FlowSensitive/CFGMatchSwitchTest.cpp:20-24
+#include 
+#include 
+#include 
+#include 
+#include 

These seem unnecessary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131616/new/

https://reviews.llvm.org/D131616

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131614: [clang][dataflow] Extend transfer functions for other `CFGElement`s

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:103
+  /// code being analysed.
+  virtual void transferCFGElement(const CFGElement *Element, Lattice ,
+  Environment ) {}

Any reason not to call this one `transfer` too?



Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:157
 
+// Holds data structures required for running dataflow analysis.
+struct AnalysisContext {

Let's start struct and member comments with `///`.



Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:184
 ///   All predecessors of `Block` except those with loop back edges must have
-///   already been transferred. States in `BlockStates` that are set to
+///   already been transferred. States in `TC.BlockStates` that are set to
 ///   `llvm::None` represent basic blocks that are not evaluated yet.





Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:312
+/// Transfers `State` by evaluating each element in the `Block` based on the
+/// `TC.Analysis` specified.
+///





Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:314
+///
+/// Built in transfer functions (if the option for `ApplyBuiltinTransfer` is 
set
+/// by the analysis) will be applied to the element before evaluation by the





Comment at: 
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:328-340
+  switch (Element.getKind()) {
+  case CFGElement::Statement: {
+builtinTransfer(Element.castAs(), State, AC);
+break;
+  }
+  case CFGElement::Initializer: {
+builtinTransfer(Element.castAs(), State);

The level of abstraction for built-in and user code is different in this 
function which makes it hard to follow. Let's move this in a 
`builtinTransfer(const CFGElement &, TypeErasedDataflowAnalysisState &)` 
function that dispatches to one of the other built-in transfer functions. This 
way `transferCFGBlock` won't mix implementation details with the high level 
algorithm.



Comment at: 
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:341-347
+}
+// User-provided analysis
+AC.Analysis.transferTypeErased(, State.Lattice, State.Env);
+// Post processing
+if (PostVisitCFG) {
+  PostVisitCFG(Element, State);
+}





Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:164-170
+std::function VerifyResults, ArrayRef 
Args,
+const tooling::FileContentMappings  = {}) {
+
+  std::function
+  PostVisitCFG = nullptr;
+  if (PostVisitStmt) {




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131614/new/

https://reviews.llvm.org/D131614

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131809: [clang][dataflow] Add an option for context-sensitive depth

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:395
   // `DeclContext` of the block being analysed if provided.
-  const DeclContext *DeclCtx = nullptr;
+  std::vector CallString;
 





Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:210
+ const DeclContext *Callee) const {
+  return CallString.size() <= MaxDepth &&
+ std::find(CallString.begin(), CallString.end(), Callee) ==

If `canDescend` is supposed to return false for `MaxDepth = 0`, shouldn't this 
be `<`?



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:4003
 
-TEST(TransferTest, ContextSensitiveSetTwoLayers) {
+TEST(TransferTest, ContextSensitiveSetTwoLayersDepthOne) {
   std::string Code = R"(

Let's add a similar test with `Depth` set to 0.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131809/new/

https://reviews.llvm.org/D131809

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131779: [clang][dataflow] Encode options using llvm::Optional

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:70
+   ? TransferOptions{}
+   : llvm::Optional()}) {}
 





Comment at: clang/include/clang/Analysis/FlowSensitive/Transfer.h:31
+  /// unsupported.
+  llvm::Optional ContextSensitiveOpts;
 };

Perhaps keep the name `ContextSensitive`? In the context of `TransferOptions` 
it seems clear what this is. 



Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:322
 case CFGElement::Initializer:
-  if (Analysis.applyBuiltinTransfer())
+  if (Analysis.builtinTransferOptions().hasValue())
 transferCFGInitializer(*Element.getAs(), State);

For consistency.



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:69
+  {ApplyBuiltinTransfer ? TransferOptions{}
+: llvm::Optional()},
+  Std, TargetFun);





Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:3902
   },
-  {/*.ApplyBuiltinTransfer=*/true,
-   /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/false}});
+  {TransferOptions{llvm::Optional()}});
 }

xazax.hun wrote:
> I think `llvm::None` is a more concise way to create an empty optional (akin 
> to `std::nullopt`).



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131779/new/

https://reviews.llvm.org/D131779

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131646: [clang][dataflow] Restructure loops to call widen on back edges

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:157
 
+// Returns whether `Block` is a "back edge" in the CFG. Such a block has only
+// one successor, the start of the loop.

Let's start function comments with `///` throughout the file.



Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:164
+
+// Returns the predecessor to `Block` that is a "back edge", if one exists.
+//





Comment at: 
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:167-168
+// If this function returns a non-null pointer, that means `Block` dominates 
the
+// back edge block. (That is, all paths from the entry block to the back edge
+// block must go through `Block`.) It also means that there are only two
+// predecessors; the other is along the path from the entry block to `Block`.





Comment at: 
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:168-169
+// back edge block. (That is, all paths from the entry block to the back edge
+// block must go through `Block`.) It also means that there are only two
+// predecessors; the other is along the path from the entry block to `Block`.
+static const CFGBlock *findBackEdge(const CFGBlock *Block) {

li.zhe.hua wrote:
> xazax.hun wrote:
> > Is this also true when we have multiple `continue` statements in the loop?
> Yes. The end of the loop, and each of the `continue` statements, target the 
> back edge block. They all get funneled through that back edge to `Block`, 
> such that `Block` only has two predecessors. However, I haven't verified this 
> in the CFG code, only by not finding a counterexample.
Does that hold for back edges stemming from `goto` statements?



Comment at: 
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:270
+
+TEST(DataflowAnalysisTest, ConvergesOnWidenAnalysis) {
+  std::string Code = R"(

There's already a set of "widening" tests - 
http://google3/third_party/llvm/llvm-project/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp;l=527-712;rcl=462638952

What do you think about refactoring those so that we have tests that exercise 
the framework with both `join` and `widen`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131646/new/

https://reviews.llvm.org/D131646

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131438: [clang][dataflow] Analyze constructor bodies

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:379
+  /// Shared implementation of `pushCall` overloads.
+  void pushCallInternal(const FunctionDecl *FuncDecl,
+ArrayRef Args);

Let's add a note that unlike `pushCall`, this member is invoked on the 
environment of the callee.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:380
+  void pushCallInternal(const FunctionDecl *FuncDecl,
+ArrayRef Args);
+

`#include "llvm/ADT/ArrayRef.h"`



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:667-670
+const ControlFlowContext *CFCtx = Env.getControlFlowContext(F);
+
+if (!CFCtx)
+  return;




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131438/new/

https://reviews.llvm.org/D131438

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131438: [clang][dataflow] Analyze constructor bodies

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:138
   ///
+  ///  `Call` must be either a `CallExpr` or a `CXXConstructExpr`.
+  ///

How about we define overloads that take these types instead of taking an `Expr` 
here? This should remove the need for type-checking and guarding against bad 
input in the implementation. `transferInlineCall` can be a template if 
necessary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131438/new/

https://reviews.llvm.org/D131438

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131280: [clang][dataflow] Parameterize analysis by explicit map of analyzable functions.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:83
+/// overload sets. change the key to support uniqueness.
+llvm::Expected>
+buildFunctionMapFromAST(ASTUnit );

`#include "llvm/ADT/StringMap.h"`



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:27
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Compiler.h"

This should be `StringMap`.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:60
   ///
+  /// Paramter `AnalyzableFunctions` is a map of analyzable function bodies
+  /// (represented as `ControlFlowContext`s), keyed by the fully-qualified

How about `InlineableFunctions`?



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:60
   ///
+  /// Paramter `AnalyzableFunctions` is a map of analyzable function bodies
+  /// (represented as `ControlFlowContext`s), keyed by the fully-qualified

sgatev wrote:
> How about `InlineableFunctions`?




Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:549
 
-  // Note that it is important for the storage location of `S` to be set
-  // before `pushCall`, because the latter uses it to set the storage
-  // location for `return`.
-  auto  = Env.createStorageLocation(*S);
-  Env.setStorageLocation(*S, ReturnLoc);
-  auto CalleeEnv = Env.pushCall(S);
+  const FunctionDecl *FuncDecl = CFCtx->getDecl()->getAsFunction();
+  assert(FuncDecl != nullptr && "ControlFlowContexts in the environment "

How is that different from `F`? Why not let `Environment::pushCall` get this 
from the `CallExpr` argument?



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:74
 
+/// `Code` must be the source code from `ASTUnit` was built.
 template 




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131280/new/

https://reviews.llvm.org/D131280

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131065: [clang][dataflow] Store DeclContext of block being analysed in Environment if available.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:537-539
+  auto *Caller = Env.getDeclCtx();
+  Env = Environment(ExitState->Env);
+  Env.setDeclCtx(Caller);

I believe you'll need to rebase because this has changed: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/Transfer.cpp#L567


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131065/new/

https://reviews.llvm.org/D131065

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131170: [clang][dataflow] Analyze method bodies

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:217
+const Expr *Arg = MethodCall->getImplicitObjectArgument();
+Env.ThisPointeeLoc = Env.getStorageLocation(*Arg, SkipPast::Reference);
+  }

samestep wrote:
> sgatev wrote:
> > samestep wrote:
> > > sgatev wrote:
> > > > What if `Arg` is null?
> > > Good point, thanks; under what circumstances can that happen? In any 
> > > case, I'm adding a guard for this.
> > It can be null if the argument isn't modeled and there's no value assigned 
> > to its storage location.
> Hmm I don't quite follow; `Arg` just comes from calling 
> `getImplicitObjectArgument` on the `CXXMemberCallExpr`, right? So shouldn't 
> it only depend on the AST, and be completely independent of what we do or 
> don't model?
Ah, right. Well, I'm not familiar with the details, but apparently it returns 
null in some cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131170/new/

https://reviews.llvm.org/D131170

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130600: [clang][dataflow] Handle return statements

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:208
   Environment Env(*this);
+  Env.ReturnLoc = Env.getStorageLocation(*Call, SkipPast::Reference);
 

Let's add a FIXME to support references here.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:543-545
+  auto  = Env.createStorageLocation(*S);
+  Env.setStorageLocation(*S, ReturnLoc);
   auto CalleeEnv = Env.pushCall(S);

Now there's a hidden connection - `ReturnLoc` gets assigned a value in `Env` 
because we implicitly use the same storage location in `Env.pushCall(S)`. I 
suggest adding a comment about this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130600/new/

https://reviews.llvm.org/D130600

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131170: [clang][dataflow] Analyze method bodies

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:217
+const Expr *Arg = MethodCall->getImplicitObjectArgument();
+Env.ThisPointeeLoc = Env.getStorageLocation(*Arg, SkipPast::Reference);
+  }

samestep wrote:
> sgatev wrote:
> > What if `Arg` is null?
> Good point, thanks; under what circumstances can that happen? In any case, 
> I'm adding a guard for this.
It can be null if the argument isn't modeled and there's no value assigned to 
its storage location.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131170/new/

https://reviews.llvm.org/D131170

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131170: [clang][dataflow] Analyze method bodies

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:383
   StorageLocation *ReturnLoc = nullptr;
-  // FIXME: Move `ThisPointeeLoc` here from `DataflowAnalysisContext`.
+  StorageLocation *ThisPointeeLoc = nullptr;
 

Let's add a brief comment explaining what this is and when it is expected to be 
non-null.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:217
+const Expr *Arg = MethodCall->getImplicitObjectArgument();
+Env.ThisPointeeLoc = Env.getStorageLocation(*Arg, SkipPast::Reference);
+  }

What if `Arg` is null?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131170/new/

https://reviews.llvm.org/D131170

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130600: [clang][dataflow] Handle return statements

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:348
+auto *Loc = Env.getReturnStorageLocation();
+assert(Loc != nullptr);
+// FIXME: Model NRVO.

samestep wrote:
> sgatev wrote:
> > Let's do `if (Loc == nullptr) return;`
> I don't think we want to do that, right? Shouldn't the `return` storage 
> location always be set? Or is this about the "analyzing fragments rather than 
> full functions" thing we discussed yesterday?
I think it's related. If we are going with always initializing the `return` 
storage location then I guess at some point we should be able to make 
`Environment::getReturnStorageLocation` return a reference? In that case I'm 
fine with keeping the assert around in the meantime.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:564
+  assert(ReturnLoc != nullptr);
+  Env.setStorageLocation(*S, *ReturnLoc);
+  Env.popCall(ExitEnv);

samestep wrote:
> sgatev wrote:
> > We use stable storage locations to ensure convergence. In that spirit, 
> > shouldn't we assign `ReturnLoc`'s value to `S`'s storage location instead 
> > of changing the storage location? Alternatively, we can pass `S`'s storage 
> > location to `pushCall` so that it can store it as `ReturnLoc`.
> Could you clarify how this hurts convergence? My understanding is that 
> `ReturnLoc` here is already stable, so this would make `S`'s storage location 
> stable too.
If I follow correctly, `ReturnLoc` here is the result of 
`Env.createStorageLocation(ReturnType)` which isn't stable. Each call to 
`createStorageLocation` returns a fresh storage location.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130600/new/

https://reviews.llvm.org/D130600

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130726: [clang][dataflow] Handle multiple context-sensitive calls to the same function

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:250
+  // scope, we do not propagate the maps.
+  this->LocToVal = std::move(CalleeEnv.LocToVal);
+  this->MemberLocToStruct = std::move(CalleeEnv.MemberLocToStruct);

Does `std::move` help here? `CalleeEnv` is a const reference.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130726/new/

https://reviews.llvm.org/D130726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130600: [clang][dataflow] Handle return statements

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:345
+// FIXME: Support reference-type returns.
+assert(Val->getKind() != Value::Kind::Reference);
+

Let's do `if (Val->getKind() == Value::Kind::Reference) return;`. Otherwise the 
framework will be unusable in practice.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:348
+auto *Loc = Env.getReturnStorageLocation();
+assert(Loc != nullptr);
+// FIXME: Model NRVO.

Let's do `if (Loc == nullptr) return;`



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:564
+  assert(ReturnLoc != nullptr);
+  Env.setStorageLocation(*S, *ReturnLoc);
+  Env.popCall(ExitEnv);

We use stable storage locations to ensure convergence. In that spirit, 
shouldn't we assign `ReturnLoc`'s value to `S`'s storage location instead of 
changing the storage location? Alternatively, we can pass `S`'s storage 
location to `pushCall` so that it can store it as `ReturnLoc`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130600/new/

https://reviews.llvm.org/D130600

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131109: [clang][dataflow][NFC] Fix outdated comment on getStableStorageLocation

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131109/new/

https://reviews.llvm.org/D131109

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131039: [clang][dataflow] Add cache of `ControlFlowContext`s for function decls.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:34
+  /// Builds a ControlFlowContext from an AST node. `D` is the function in 
which
+  /// `S` resides. All arguments must be non-null.
   static llvm::Expected build(const Decl *D, Stmt *S,

ymandel wrote:
> sgatev wrote:
> > Can we make them references?
> Turns out I was wrong - `D` can be null. Would that CFG has comments about 
> what its parameters do... We have a test that excercises this (though I can't 
> say why):
> 
> https://github.com/llvm/llvm-project/blob/9a976f36615dbe15e76c12b22f711b2e597a8e51/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp#L70
> 
> The other two -- yes -- but then we'd have to update the various callers as 
> well. I'd rather not do that in this patch, but I'll add the new overload and 
> we can follow up with cleanups in another patch.
> 
> 
Weird. I think we should change the test to pass the decl.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:369
+
+  llvm::DenseMap FunctionModels;
 };

Perhaps `FunctionContexts`?



Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:19
 #include "clang/Analysis/FlowSensitive/Value.h"
+#include "clang/Basic/IdentifierTable.h"
 #include "llvm/Support/Debug.h"

Is this still needed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131039/new/

https://reviews.llvm.org/D131039

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131065: [clang][dataflow] Store DeclContext of block being analysed in Environment if available.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:170
 
+  /// Returns the `DeclCtx` of the block being analysed if provided, otherwise
+  /// returns nullptr.





Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:370
 
+  // `DeclCtx` of the block being analysed if provided.
+  const DeclContext *DeclCtx;





Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:170-172
+ const DeclContext )
 : Environment(DACtx) {
+  DeclCtx = 




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131065/new/

https://reviews.llvm.org/D131065

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131021: [clang][dataflow] Rename member to make it clear that it isn't stable

[Stanislav Gatev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG817dd5e3fd6b: [clang][dataflow] Rename member to make it 
clear that it isnt stable (authored by sgatev).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131021/new/

https://reviews.llvm.org/D131021

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
  clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp


Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -337,7 +337,7 @@
 }
 
 StorageLocation ::createStorageLocation(QualType Type) {
-  return DACtx->getStableStorageLocation(Type);
+  return DACtx->createStorageLocation(Type);
 }
 
 StorageLocation ::createStorageLocation(const VarDecl ) {
Index: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
@@ -24,15 +24,14 @@
 namespace clang {
 namespace dataflow {
 
-StorageLocation &
-DataflowAnalysisContext::getStableStorageLocation(QualType Type) {
+StorageLocation ::createStorageLocation(QualType Type) 
{
   if (!Type.isNull() &&
   (Type->isStructureOrClassType() || Type->isUnionType())) {
 // FIXME: Explore options to avoid eager initialization of fields as some 
of
 // them might not be needed for a particular analysis.
 llvm::DenseMap FieldLocs;
 for (const FieldDecl *Field : getObjectFields(Type))
-  FieldLocs.insert({Field, (Field->getType())});
+  FieldLocs.insert({Field, (Field->getType())});
 return takeOwnership(
 std::make_unique(Type, 
std::move(FieldLocs)));
   }
@@ -43,7 +42,7 @@
 DataflowAnalysisContext::getStableStorageLocation(const VarDecl ) {
   if (auto *Loc = getStorageLocation(D))
 return *Loc;
-  auto  = getStableStorageLocation(D.getType());
+  auto  = createStorageLocation(D.getType());
   setStorageLocation(D, Loc);
   return Loc;
 }
@@ -52,7 +51,7 @@
 DataflowAnalysisContext::getStableStorageLocation(const Expr ) {
   if (auto *Loc = getStorageLocation(E))
 return *Loc;
-  auto  = getStableStorageLocation(E.getType());
+  auto  = createStorageLocation(E.getType());
   setStorageLocation(E, Loc);
   return Loc;
 }
@@ -63,7 +62,7 @@
   PointeeType.isNull() ? PointeeType : PointeeType.getCanonicalType();
   auto Res = NullPointerVals.try_emplace(CanonicalPointeeType, nullptr);
   if (Res.second) {
-auto  = getStableStorageLocation(CanonicalPointeeType);
+auto  = createStorageLocation(CanonicalPointeeType);
 Res.first->second =
 (std::make_unique(PointeeLoc));
   }
Index: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
@@ -90,12 +90,12 @@
 return *cast(Vals.back().get());
   }
 
-  /// Returns a stable storage location appropriate for `Type`.
+  /// Returns a new storage location appropriate for `Type`.
   ///
   /// Requirements:
   ///
   ///  `Type` must not be null.
-  StorageLocation (QualType Type);
+  StorageLocation (QualType Type);
 
   /// Returns a stable storage location for `D`.
   StorageLocation (const VarDecl );


Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -337,7 +337,7 @@
 }
 
 StorageLocation ::createStorageLocation(QualType Type) {
-  return DACtx->getStableStorageLocation(Type);
+  return DACtx->createStorageLocation(Type);
 }
 
 StorageLocation ::createStorageLocation(const VarDecl ) {
Index: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
@@ -24,15 +24,14 @@
 namespace clang {
 namespace dataflow {
 
-StorageLocation &
-DataflowAnalysisContext::getStableStorageLocation(QualType Type) {
+StorageLocation ::createStorageLocation(QualType Type) {
   if (!Type.isNull() &&
   (Type->isStructureOrClassType() || Type->isUnionType())) {
 // FIXME: Explore options to avoid eager initialization of fields as some of
 // them might not be needed for a particular analysis.
 llvm::DenseMap FieldLocs;
 

[PATCH] D131014: [clang][dataflow] Make the type of the post visit callback consistent

[Stanislav Gatev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc44c71843f3e: [clang][dataflow] Make the type of the post 
visit callback consistent (authored by sgatev).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131014/new/

https://reviews.llvm.org/D131014

Files:
  clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.h
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1245,9 +1245,10 @@
   return UncheckedOptionalAccessModel(Ctx, Options);
 },
 [, Diagnoser = UncheckedOptionalAccessDiagnoser(Options)](
-ASTContext , const Stmt *Stmt,
+ASTContext , const CFGStmt ,
 const TypeErasedDataflowAnalysisState ) mutable {
-  auto StmtDiagnostics = Diagnoser.diagnose(Ctx, Stmt, State.Env);
+  auto StmtDiagnostics =
+  Diagnoser.diagnose(Ctx, Stmt.getStmt(), State.Env);
   llvm::move(StmtDiagnostics, std::back_inserter(Diagnostics));
 },
 [](AnalysisData AnalysisData) {
Index: clang/unittests/Analysis/FlowSensitive/TestingSupport.h
===
--- clang/unittests/Analysis/FlowSensitive/TestingSupport.h
+++ clang/unittests/Analysis/FlowSensitive/TestingSupport.h
@@ -76,7 +76,7 @@
 llvm::StringRef Code,
 ast_matchers::internal::Matcher TargetFuncMatcher,
 std::function MakeAnalysis,
-std::function
 PostVisitStmt,
 std::function VerifyResults, ArrayRef Args,
@@ -112,11 +112,11 @@
   Environment Env(DACtx, *F);
   auto Analysis = MakeAnalysis(Context, Env);
 
-  std::function
+  std::function
   PostVisitStmtClosure = nullptr;
   if (PostVisitStmt != nullptr) {
 PostVisitStmtClosure = [, ](
-   const Stmt *Stmt,
+   const CFGStmt ,
const TypeErasedDataflowAnalysisState ) {
   PostVisitStmt(Context, Stmt, State);
 };
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -333,7 +333,8 @@
 runTypeErasedDataflowAnalysis(
 const ControlFlowContext , TypeErasedDataflowAnalysis ,
 const Environment ,
-std::function
+std::function
 PostVisitStmt) {
   PostOrderCFGView POV(());
   ForwardDataflowWorklist Worklist(CFCtx.getCFG(), );
@@ -398,12 +399,9 @@
   // Skip blocks that were not evaluated.
   if (!BlockStates[Block->getBlockID()])
 continue;
-  transferBlock(
-  CFCtx, BlockStates, *Block, InitEnv, Analysis,
-  [](const clang::CFGStmt ,
-   const TypeErasedDataflowAnalysisState ) {
-PostVisitStmt(Stmt.getStmt(), State);
-  });
+
+  transferBlock(CFCtx, BlockStates, *Block, InitEnv, Analysis,
+PostVisitStmt);
 }
   }
 
Index: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -138,7 +138,8 @@
 runTypeErasedDataflowAnalysis(
 const ControlFlowContext , TypeErasedDataflowAnalysis ,
 const Environment ,
-std::function
+std::function
 PostVisitStmt = nullptr);
 
 } // namespace dataflow
Index: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -125,14 +125,14 @@
 runDataflowAnalysis(
 const ControlFlowContext , AnalysisT ,
 const Environment ,
-std::function &)>
+std::function &)>
 PostVisitStmt = nullptr) {
-  std::function
+  std::function
   PostVisitStmtClosure = nullptr;
   if (PostVisitStmt != nullptr) {
 PostVisitStmtClosure = [](
-   const Stmt *Stmt,
+   const CFGStmt ,
 

[PATCH] D131039: [clang][dataflow] Add cache of `ControlFlowContext`s for function decls.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:34
+  /// Builds a ControlFlowContext from an AST node. `D` is the function in 
which
+  /// `S` resides. All arguments must be non-null.
   static llvm::Expected build(const Decl *D, Stmt *S,

Can we make them references?



Comment at: clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:50
 private:
-  ControlFlowContext(std::unique_ptr Cfg,
+  // `D` must not be null.
+  ControlFlowContext(const Decl *D, std::unique_ptr Cfg,

Can we make it a reference?



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:369
+
+  // Keyed on the function's fully qualified name. No leading "::".
+  llvm::StringMap FunctionModels;

Can we make the keys `FunctionDecl *` and use `FunctionDecl::getDefinition` in 
`getControlFlowContext` to obtain a canonical pointer?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131039/new/

https://reviews.llvm.org/D131039

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131014: [clang][dataflow] Make the type of the post visit callback consistent

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added a comment.

In D131014#3694852 , @samestep wrote:

> Seems reasonable  I'm curious though, why does this not require 
> `clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp` to 
> be updated?

It does, thanks! Fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131014/new/

https://reviews.llvm.org/D131014

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131014: [clang][dataflow] Make the type of the post visit callback consistent

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 449449.
sgatev added a comment.
Herald added a subscriber: carlosgalvezp.
Herald added a project: clang-tools-extra.

Address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131014/new/

https://reviews.llvm.org/D131014

Files:
  clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.h
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1245,9 +1245,10 @@
   return UncheckedOptionalAccessModel(Ctx, Options);
 },
 [, Diagnoser = UncheckedOptionalAccessDiagnoser(Options)](
-ASTContext , const Stmt *Stmt,
+ASTContext , const CFGStmt ,
 const TypeErasedDataflowAnalysisState ) mutable {
-  auto StmtDiagnostics = Diagnoser.diagnose(Ctx, Stmt, State.Env);
+  auto StmtDiagnostics =
+  Diagnoser.diagnose(Ctx, Stmt.getStmt(), State.Env);
   llvm::move(StmtDiagnostics, std::back_inserter(Diagnostics));
 },
 [](AnalysisData AnalysisData) {
Index: clang/unittests/Analysis/FlowSensitive/TestingSupport.h
===
--- clang/unittests/Analysis/FlowSensitive/TestingSupport.h
+++ clang/unittests/Analysis/FlowSensitive/TestingSupport.h
@@ -76,7 +76,7 @@
 llvm::StringRef Code,
 ast_matchers::internal::Matcher TargetFuncMatcher,
 std::function MakeAnalysis,
-std::function
 PostVisitStmt,
 std::function VerifyResults, ArrayRef Args,
@@ -112,11 +112,11 @@
   Environment Env(DACtx, *F);
   auto Analysis = MakeAnalysis(Context, Env);
 
-  std::function
+  std::function
   PostVisitStmtClosure = nullptr;
   if (PostVisitStmt != nullptr) {
 PostVisitStmtClosure = [, ](
-   const Stmt *Stmt,
+   const CFGStmt ,
const TypeErasedDataflowAnalysisState ) {
   PostVisitStmt(Context, Stmt, State);
 };
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -333,7 +333,8 @@
 runTypeErasedDataflowAnalysis(
 const ControlFlowContext , TypeErasedDataflowAnalysis ,
 const Environment ,
-std::function
+std::function
 PostVisitStmt) {
   PostOrderCFGView POV(());
   ForwardDataflowWorklist Worklist(CFCtx.getCFG(), );
@@ -398,12 +399,9 @@
   // Skip blocks that were not evaluated.
   if (!BlockStates[Block->getBlockID()])
 continue;
-  transferBlock(
-  CFCtx, BlockStates, *Block, InitEnv, Analysis,
-  [](const clang::CFGStmt ,
-   const TypeErasedDataflowAnalysisState ) {
-PostVisitStmt(Stmt.getStmt(), State);
-  });
+
+  transferBlock(CFCtx, BlockStates, *Block, InitEnv, Analysis,
+PostVisitStmt);
 }
   }
 
Index: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -138,7 +138,8 @@
 runTypeErasedDataflowAnalysis(
 const ControlFlowContext , TypeErasedDataflowAnalysis ,
 const Environment ,
-std::function
+std::function
 PostVisitStmt = nullptr);
 
 } // namespace dataflow
Index: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -125,14 +125,14 @@
 runDataflowAnalysis(
 const ControlFlowContext , AnalysisT ,
 const Environment ,
-std::function &)>
+std::function &)>
 PostVisitStmt = nullptr) {
-  std::function
+  std::function
   PostVisitStmtClosure = nullptr;
   if (PostVisitStmt != nullptr) {
 PostVisitStmtClosure = [](
-   const Stmt *Stmt,
+   const CFGStmt ,
const TypeErasedDataflowAnalysisState ) {
   

[PATCH] D131021: [clang][dataflow] Rename member to make it clear that it isn't stable

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 449436.
sgatev added a comment.

Update commit message


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131021/new/

https://reviews.llvm.org/D131021

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
  clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp


Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -337,7 +337,7 @@
 }
 
 StorageLocation ::createStorageLocation(QualType Type) {
-  return DACtx->getStableStorageLocation(Type);
+  return DACtx->createStorageLocation(Type);
 }
 
 StorageLocation ::createStorageLocation(const VarDecl ) {
Index: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
@@ -24,15 +24,14 @@
 namespace clang {
 namespace dataflow {
 
-StorageLocation &
-DataflowAnalysisContext::getStableStorageLocation(QualType Type) {
+StorageLocation ::createStorageLocation(QualType Type) 
{
   if (!Type.isNull() &&
   (Type->isStructureOrClassType() || Type->isUnionType())) {
 // FIXME: Explore options to avoid eager initialization of fields as some 
of
 // them might not be needed for a particular analysis.
 llvm::DenseMap FieldLocs;
 for (const FieldDecl *Field : getObjectFields(Type))
-  FieldLocs.insert({Field, (Field->getType())});
+  FieldLocs.insert({Field, (Field->getType())});
 return takeOwnership(
 std::make_unique(Type, 
std::move(FieldLocs)));
   }
@@ -43,7 +42,7 @@
 DataflowAnalysisContext::getStableStorageLocation(const VarDecl ) {
   if (auto *Loc = getStorageLocation(D))
 return *Loc;
-  auto  = getStableStorageLocation(D.getType());
+  auto  = createStorageLocation(D.getType());
   setStorageLocation(D, Loc);
   return Loc;
 }
@@ -52,7 +51,7 @@
 DataflowAnalysisContext::getStableStorageLocation(const Expr ) {
   if (auto *Loc = getStorageLocation(E))
 return *Loc;
-  auto  = getStableStorageLocation(E.getType());
+  auto  = createStorageLocation(E.getType());
   setStorageLocation(E, Loc);
   return Loc;
 }
@@ -63,7 +62,7 @@
   PointeeType.isNull() ? PointeeType : PointeeType.getCanonicalType();
   auto Res = NullPointerVals.try_emplace(CanonicalPointeeType, nullptr);
   if (Res.second) {
-auto  = getStableStorageLocation(CanonicalPointeeType);
+auto  = createStorageLocation(CanonicalPointeeType);
 Res.first->second =
 (std::make_unique(PointeeLoc));
   }
Index: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
@@ -90,12 +90,12 @@
 return *cast(Vals.back().get());
   }
 
-  /// Returns a stable storage location appropriate for `Type`.
+  /// Returns a new storage location appropriate for `Type`.
   ///
   /// Requirements:
   ///
   ///  `Type` must not be null.
-  StorageLocation (QualType Type);
+  StorageLocation (QualType Type);
 
   /// Returns a stable storage location for `D`.
   StorageLocation (const VarDecl );


Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -337,7 +337,7 @@
 }
 
 StorageLocation ::createStorageLocation(QualType Type) {
-  return DACtx->getStableStorageLocation(Type);
+  return DACtx->createStorageLocation(Type);
 }
 
 StorageLocation ::createStorageLocation(const VarDecl ) {
Index: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
@@ -24,15 +24,14 @@
 namespace clang {
 namespace dataflow {
 
-StorageLocation &
-DataflowAnalysisContext::getStableStorageLocation(QualType Type) {
+StorageLocation ::createStorageLocation(QualType Type) {
   if (!Type.isNull() &&
   (Type->isStructureOrClassType() || Type->isUnionType())) {
 // FIXME: Explore options to avoid eager initialization of fields as some of
 // them might not be needed for a particular analysis.
 llvm::DenseMap FieldLocs;
 for (const FieldDecl *Field : getObjectFields(Type))
-  FieldLocs.insert({Field, (Field->getType())});
+  FieldLocs.insert({Field, 

[PATCH] D131021: [clang][dataflow] Rename member to make it clear that it isn't stable

[Stanislav Gatev via Phabricator via cfe-commits]

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

Rename `DataflowAnalysisContext::getStableStorageLocation(QualType)`
to `createStorageLocation, to make it clear that it doesn't return a
stable storage location.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131021

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
  clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp


Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -337,7 +337,7 @@
 }
 
 StorageLocation ::createStorageLocation(QualType Type) {
-  return DACtx->getStableStorageLocation(Type);
+  return DACtx->createStorageLocation(Type);
 }
 
 StorageLocation ::createStorageLocation(const VarDecl ) {
Index: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
@@ -24,15 +24,14 @@
 namespace clang {
 namespace dataflow {
 
-StorageLocation &
-DataflowAnalysisContext::getStableStorageLocation(QualType Type) {
+StorageLocation ::createStorageLocation(QualType Type) 
{
   if (!Type.isNull() &&
   (Type->isStructureOrClassType() || Type->isUnionType())) {
 // FIXME: Explore options to avoid eager initialization of fields as some 
of
 // them might not be needed for a particular analysis.
 llvm::DenseMap FieldLocs;
 for (const FieldDecl *Field : getObjectFields(Type))
-  FieldLocs.insert({Field, (Field->getType())});
+  FieldLocs.insert({Field, (Field->getType())});
 return takeOwnership(
 std::make_unique(Type, 
std::move(FieldLocs)));
   }
@@ -43,7 +42,7 @@
 DataflowAnalysisContext::getStableStorageLocation(const VarDecl ) {
   if (auto *Loc = getStorageLocation(D))
 return *Loc;
-  auto  = getStableStorageLocation(D.getType());
+  auto  = createStorageLocation(D.getType());
   setStorageLocation(D, Loc);
   return Loc;
 }
@@ -52,7 +51,7 @@
 DataflowAnalysisContext::getStableStorageLocation(const Expr ) {
   if (auto *Loc = getStorageLocation(E))
 return *Loc;
-  auto  = getStableStorageLocation(E.getType());
+  auto  = createStorageLocation(E.getType());
   setStorageLocation(E, Loc);
   return Loc;
 }
@@ -63,7 +62,7 @@
   PointeeType.isNull() ? PointeeType : PointeeType.getCanonicalType();
   auto Res = NullPointerVals.try_emplace(CanonicalPointeeType, nullptr);
   if (Res.second) {
-auto  = getStableStorageLocation(CanonicalPointeeType);
+auto  = createStorageLocation(CanonicalPointeeType);
 Res.first->second =
 (std::make_unique(PointeeLoc));
   }
Index: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
@@ -90,12 +90,12 @@
 return *cast(Vals.back().get());
   }
 
-  /// Returns a stable storage location appropriate for `Type`.
+  /// Returns a new storage location appropriate for `Type`.
   ///
   /// Requirements:
   ///
   ///  `Type` must not be null.
-  StorageLocation (QualType Type);
+  StorageLocation (QualType Type);
 
   /// Returns a stable storage location for `D`.
   StorageLocation (const VarDecl );


Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -337,7 +337,7 @@
 }
 
 StorageLocation ::createStorageLocation(QualType Type) {
-  return DACtx->getStableStorageLocation(Type);
+  return DACtx->createStorageLocation(Type);
 }
 
 StorageLocation ::createStorageLocation(const VarDecl ) {
Index: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
===
--- clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
@@ -24,15 +24,14 @@
 namespace clang {
 namespace dataflow {
 
-StorageLocation &
-DataflowAnalysisContext::getStableStorageLocation(QualType Type) {
+StorageLocation ::createStorageLocation(QualType Type) {
   if (!Type.isNull() &&
   (Type->isStructureOrClassType() || Type->isUnionType())) {
 // FIXME: Explore options to avoid eager 

[PATCH] D129097: [clang][dataflow] Handle null pointers of type std::nullptr_t

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:96
   ///
   ///  `Type` must not be null.
   StorageLocation (QualType Type);

This is inconsistent with the change introduced by this patch.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:27
 DataflowAnalysisContext::getStableStorageLocation(QualType Type) {
-  assert(!Type.isNull());
-  if (Type->isStructureOrClassType() || Type->isUnionType()) {
+  if (!Type.isNull() &&
+  (Type->isStructureOrClassType() || Type->isUnionType())) {

What does that mean? We are analyzing an incomplete translation unit? Why would 
the type ever be null here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129097/new/

https://reviews.llvm.org/D129097

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131014: [clang][dataflow] Make the type of the post visit callback consistent

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 449398.
sgatev added a comment.

Update commit message


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131014/new/

https://reviews.llvm.org/D131014

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.h
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1245,9 +1245,10 @@
   return UncheckedOptionalAccessModel(Ctx, Options);
 },
 [, Diagnoser = UncheckedOptionalAccessDiagnoser(Options)](
-ASTContext , const Stmt *Stmt,
+ASTContext , const CFGStmt ,
 const TypeErasedDataflowAnalysisState ) mutable {
-  auto StmtDiagnostics = Diagnoser.diagnose(Ctx, Stmt, State.Env);
+  auto StmtDiagnostics =
+  Diagnoser.diagnose(Ctx, Stmt.getStmt(), State.Env);
   llvm::move(StmtDiagnostics, std::back_inserter(Diagnostics));
 },
 [](AnalysisData AnalysisData) {
Index: clang/unittests/Analysis/FlowSensitive/TestingSupport.h
===
--- clang/unittests/Analysis/FlowSensitive/TestingSupport.h
+++ clang/unittests/Analysis/FlowSensitive/TestingSupport.h
@@ -76,7 +76,7 @@
 llvm::StringRef Code,
 ast_matchers::internal::Matcher TargetFuncMatcher,
 std::function MakeAnalysis,
-std::function
 PostVisitStmt,
 std::function VerifyResults, ArrayRef Args,
@@ -112,11 +112,11 @@
   Environment Env(DACtx, *F);
   auto Analysis = MakeAnalysis(Context, Env);
 
-  std::function
+  std::function
   PostVisitStmtClosure = nullptr;
   if (PostVisitStmt != nullptr) {
 PostVisitStmtClosure = [, ](
-   const Stmt *Stmt,
+   const CFGStmt ,
const TypeErasedDataflowAnalysisState ) {
   PostVisitStmt(Context, Stmt, State);
 };
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -333,7 +333,8 @@
 runTypeErasedDataflowAnalysis(
 const ControlFlowContext , TypeErasedDataflowAnalysis ,
 const Environment ,
-std::function
+std::function
 PostVisitStmt) {
   PostOrderCFGView POV(());
   ForwardDataflowWorklist Worklist(CFCtx.getCFG(), );
@@ -398,12 +399,9 @@
   // Skip blocks that were not evaluated.
   if (!BlockStates[Block->getBlockID()])
 continue;
-  transferBlock(
-  CFCtx, BlockStates, *Block, InitEnv, Analysis,
-  [](const clang::CFGStmt ,
-   const TypeErasedDataflowAnalysisState ) {
-PostVisitStmt(Stmt.getStmt(), State);
-  });
+
+  transferBlock(CFCtx, BlockStates, *Block, InitEnv, Analysis,
+PostVisitStmt);
 }
   }
 
Index: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -138,7 +138,8 @@
 runTypeErasedDataflowAnalysis(
 const ControlFlowContext , TypeErasedDataflowAnalysis ,
 const Environment ,
-std::function
+std::function
 PostVisitStmt = nullptr);
 
 } // namespace dataflow
Index: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -125,14 +125,14 @@
 runDataflowAnalysis(
 const ControlFlowContext , AnalysisT ,
 const Environment ,
-std::function &)>
+std::function &)>
 PostVisitStmt = nullptr) {
-  std::function
+  std::function
   PostVisitStmtClosure = nullptr;
   if (PostVisitStmt != nullptr) {
 PostVisitStmtClosure = [](
-   const Stmt *Stmt,
+   const CFGStmt ,
const TypeErasedDataflowAnalysisState ) {
   auto *Lattice =
   llvm::any_cast();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D131014: [clang][dataflow] Make the type of the post visit callback consistent

[Stanislav Gatev via Phabricator via cfe-commits]

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

Make the type of the post visit callback consistent.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131014

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.h
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1245,9 +1245,10 @@
   return UncheckedOptionalAccessModel(Ctx, Options);
 },
 [, Diagnoser = UncheckedOptionalAccessDiagnoser(Options)](
-ASTContext , const Stmt *Stmt,
+ASTContext , const CFGStmt ,
 const TypeErasedDataflowAnalysisState ) mutable {
-  auto StmtDiagnostics = Diagnoser.diagnose(Ctx, Stmt, State.Env);
+  auto StmtDiagnostics =
+  Diagnoser.diagnose(Ctx, Stmt.getStmt(), State.Env);
   llvm::move(StmtDiagnostics, std::back_inserter(Diagnostics));
 },
 [](AnalysisData AnalysisData) {
Index: clang/unittests/Analysis/FlowSensitive/TestingSupport.h
===
--- clang/unittests/Analysis/FlowSensitive/TestingSupport.h
+++ clang/unittests/Analysis/FlowSensitive/TestingSupport.h
@@ -76,7 +76,7 @@
 llvm::StringRef Code,
 ast_matchers::internal::Matcher TargetFuncMatcher,
 std::function MakeAnalysis,
-std::function
 PostVisitStmt,
 std::function VerifyResults, ArrayRef Args,
@@ -112,11 +112,11 @@
   Environment Env(DACtx, *F);
   auto Analysis = MakeAnalysis(Context, Env);
 
-  std::function
+  std::function
   PostVisitStmtClosure = nullptr;
   if (PostVisitStmt != nullptr) {
 PostVisitStmtClosure = [, ](
-   const Stmt *Stmt,
+   const CFGStmt ,
const TypeErasedDataflowAnalysisState ) {
   PostVisitStmt(Context, Stmt, State);
 };
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -333,7 +333,8 @@
 runTypeErasedDataflowAnalysis(
 const ControlFlowContext , TypeErasedDataflowAnalysis ,
 const Environment ,
-std::function
+std::function
 PostVisitStmt) {
   PostOrderCFGView POV(());
   ForwardDataflowWorklist Worklist(CFCtx.getCFG(), );
@@ -398,12 +399,9 @@
   // Skip blocks that were not evaluated.
   if (!BlockStates[Block->getBlockID()])
 continue;
-  transferBlock(
-  CFCtx, BlockStates, *Block, InitEnv, Analysis,
-  [](const clang::CFGStmt ,
-   const TypeErasedDataflowAnalysisState ) {
-PostVisitStmt(Stmt.getStmt(), State);
-  });
+
+  transferBlock(CFCtx, BlockStates, *Block, InitEnv, Analysis,
+PostVisitStmt);
 }
   }
 
Index: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -138,7 +138,8 @@
 runTypeErasedDataflowAnalysis(
 const ControlFlowContext , TypeErasedDataflowAnalysis ,
 const Environment ,
-std::function
+std::function
 PostVisitStmt = nullptr);
 
 } // namespace dataflow
Index: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
===
--- clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -125,14 +125,14 @@
 runDataflowAnalysis(
 const ControlFlowContext , AnalysisT ,
 const Environment ,
-std::function &)>
+std::function &)>
 PostVisitStmt = nullptr) {
-  std::function
+  std::function
   PostVisitStmtClosure = nullptr;
   if (PostVisitStmt != nullptr) {
 PostVisitStmtClosure = [](
-   const Stmt *Stmt,
+   const CFGStmt ,
const 

[PATCH] D130270: [clang][dataflow] Use a dedicated bool to encode which branch was taken

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:103
   if (Value *MergedVal = MergedEnv.createValue(Type))
 if (Model.merge(Type, *Val1, Env1, *Val2, Env2, *MergedVal, MergedEnv))
   return MergedVal;

We should probably pass `TookSecondBranch` here so that models can also use it 
in the implementation of `merge`. Could be in a follow up, but let's leave a 
FIXME.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130270/new/

https://reviews.llvm.org/D130270

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130600: [clang][dataflow] Handle return statements

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:114
+  ///
+  ///  `return` must not be assigned a storage location.
+  void setReturnStorageLocation(StorageLocation ) {

li.zhe.hua wrote:
> samestep wrote:
> > li.zhe.hua wrote:
> > > Fix this as well? A reader shouldn't need to root around in private 
> > > implementation details to understand the requirements for calling a 
> > > function.
> > Could you clarify what you mean? I was simply copying the signature and 
> > docstring from `setThisPointeeStorageLocation`.
> You've marked `return` in backticks. There is no parameter named `return` and 
> it is unclear what `return` refers to. My best guess is that this is a typo 
> of `ReturnLoc`, which is a private data member. So this is a public interface 
> with a requirement that a private data member has some property. This should 
> instead reframe the requirement as properties from the external reader's 
> perspective.
That was my guess initially too, but my next best guess is that `return` in 
backticks stands for the keyword/AST node. In any case, let's make it less 
ambiguous and let's not add requirements based on implementation details. How 
about: `The return value must not be assigned a storage location.`?



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:338-339
+if (Loc == nullptr) {
+  // The outermost context does not set a storage location for `return`, so
+  // in that case we just ignore `return` statements.
+  return;

samestep wrote:
> sgatev wrote:
> > Let's make this a FIXME to set a storage location for the outermost context 
> > too.
> @sgatev I could add a `FIXME` for that, or I could just do it in this same 
> patch; do you have a preference between those two options?
Same patch works!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130600/new/

https://reviews.llvm.org/D130600

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130600: [clang][dataflow] Handle return statements

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:338-339
+if (Loc == nullptr) {
+  // The outermost context does not set a storage location for `return`, so
+  // in that case we just ignore `return` statements.
+  return;

Let's make this a FIXME to set a storage location for the outermost context too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130600/new/

https://reviews.llvm.org/D130600

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130519: [clang][dataflow] Add explicit "AST" nodes for implications and iff

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:40-41
 
 // Synthetic boolean values are either atomic values or composites that
 // represent conjunctions, disjunctions, and negations.
 AtomicBool,





Comment at: clang/lib/Analysis/FlowSensitive/DebugSupport.cpp:110
+  auto R = debugString(BV.getRightSubValue(), Depth + 1);
+  S = formatv("(=\n{0}\n{1})", L, R);
+  break;

I think `<=>` would be more natural.



Comment at: clang/unittests/Analysis/FlowSensitive/SolverTest.cpp:201
+
+  expectUnsatisfiable(solve({NotEquivalent}));
+}

Let's add a label: `!((X <=> Y) <=> ((X ^ Y) v (!X ^ !Y)))`



Comment at: clang/unittests/Analysis/FlowSensitive/SolverTest.cpp:303
+
+  expectUnsatisfiable(solve({NotEquivalent}));
+}

Let's add a label: `!((X => Y) <=> (!X v Y))`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130519/new/

https://reviews.llvm.org/D130519

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130522: [clang][dataflow] Fix SAT solver crashes on `X ^ X` and `X v X`

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/WatchedLiteralsSolver.cpp:273
+
+// Visit a sub-value of `Val` (pick any, they are identical).
+  } else {

Let's visit `C->getLeftSubValue()` here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130522/new/

https://reviews.llvm.org/D130522

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130305: [clang][dataflow] Refactor ApplyBuiltinTransfer field out into DataflowAnalysisOptions struct

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:66
   explicit DataflowAnalysis(ASTContext ) : Context(Context) {}
   explicit DataflowAnalysis(ASTContext , bool ApplyBuiltinTransfer)
   : TypeErasedDataflowAnalysis(ApplyBuiltinTransfer), Context(Context) {}

samestep wrote:
> gribozavr2 wrote:
> > Will you remove this bool overload in a later patch?
> I didn't have any plans to, but I'm fine with removing it in a later patch if 
> people want to.
I think it makes sense to remove it. Can you already document that it's 
deprecated in this patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130305/new/

https://reviews.llvm.org/D130305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129546: [clang][dataflow] Refactor boolean creation as a test utility.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:263
+private:
+  std::vector> Vals;
+};

`#include `


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129546/new/

https://reviews.llvm.org/D129546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129180: [clang][dataflow] Return a solution from the solver when `Constraints` are `Satisfiable`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Solver.h:78
+Status Status;
+std::optional> Solution;
   };

`#include "llvm/ADT/DenseMap.h"`



Comment at: clang/lib/Analysis/FlowSensitive/WatchedLiteralsSolver.cpp:200
 
   if (!SubValsToVar.try_emplace(Val, NextVar).second)
 continue;

Let's create a local `Var` variable initialized to `NextVar` and use that here 
and below, where `NextVar - 1` is currently used.



Comment at: clang/lib/Analysis/FlowSensitive/WatchedLiteralsSolver.cpp:471-472
 private:
+  // Returns a satisfying truth assignment to the atomic values in the boolean
+  // formula.
+  llvm::DenseMap




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129180/new/

https://reviews.llvm.org/D129180

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129180: [clang][dataflow] Return a solution from the solver when `Constraints` are `Satisfiable`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Solver.h:45
+enum class Assignment : int8_t {
+  Unassigned = -1,
+  AssignedFalse = 0,

A solution consists of true/false assignments for all variables. Having an 
`Unassigned` option seems confusing. I suggest defining only two values in this 
enum and translating `WatchedLiteralsSolver`'s internal representation to 
`Solution` right before returning it, in `buildValAssignment`.



Comment at: clang/include/clang/Analysis/FlowSensitive/Solver.h:50-57
+static Result Unsatisfiable() { return Result(Status::Unsatisfiable, {}); }
+
+static Result TimedOut() { return Result(Status::TimedOut, {}); }
+
+static Result
+Satisfiable(llvm::DenseMap Solution) {
+  return Result(Status::Satisfiable, std::move(Solution));

Let's document these members.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129180/new/

https://reviews.llvm.org/D129180

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128924: [clang][dataflow] Replace TEST_F with TEST where possible

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128924/new/

https://reviews.llvm.org/D128924

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128833: [clang][dataflow] Handle `for` statements without conditions

[Stanislav Gatev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8207c2a66030: [clang][dataflow] Handle `for` statements 
without conditions (authored by sgatev).
Herald added a reviewer: NoQ.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128833/new/

https://reviews.llvm.org/D128833

Files:
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3827,4 +3827,30 @@
   });
 }
 
+TEST_F(TransferTest, ForStmtBranchWithoutConditionDoesNotExtendFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  for (;;) {
+(void)0;
+// [[loop_body]]
+  }
+}
+  )";
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("loop_body", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_FALSE(LoopBodyEnv.flowConditionImplies(LoopBodyFooVal));
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -97,8 +97,8 @@
 
   void VisitForStmt(const ForStmt *S) {
 auto *Cond = S->getCond();
-assert(Cond != nullptr);
-extendFlowCondition(*Cond);
+if (Cond != nullptr)
+  extendFlowCondition(*Cond);
   }
 
   void VisitBinaryOperator(const BinaryOperator *S) {


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3827,4 +3827,30 @@
   });
 }
 
+TEST_F(TransferTest, ForStmtBranchWithoutConditionDoesNotExtendFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  for (;;) {
+(void)0;
+// [[loop_body]]
+  }
+}
+  )";
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("loop_body", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_FALSE(LoopBodyEnv.flowConditionImplies(LoopBodyFooVal));
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -97,8 +97,8 @@
 
   void VisitForStmt(const ForStmt *S) {
 auto *Cond = S->getCond();
-assert(Cond != nullptr);
-extendFlowCondition(*Cond);
+if (Cond != nullptr)
+  extendFlowCondition(*Cond);
   }
 
   void VisitBinaryOperator(const BinaryOperator *S) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128833: [clang][dataflow] Handle `for` statements without conditions

[Stanislav Gatev via Phabricator via cfe-commits]

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

Handle `for` statements without conditions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128833

Files:
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3827,4 +3827,30 @@
   });
 }
 
+TEST_F(TransferTest, ForStmtBranchWithoutConditionDoesNotExtendFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  for (;;) {
+(void)0;
+// [[loop_body]]
+  }
+}
+  )";
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("loop_body", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_FALSE(LoopBodyEnv.flowConditionImplies(LoopBodyFooVal));
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -97,8 +97,8 @@
 
   void VisitForStmt(const ForStmt *S) {
 auto *Cond = S->getCond();
-assert(Cond != nullptr);
-extendFlowCondition(*Cond);
+if (Cond != nullptr)
+  extendFlowCondition(*Cond);
   }
 
   void VisitBinaryOperator(const BinaryOperator *S) {


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3827,4 +3827,30 @@
   });
 }
 
+TEST_F(TransferTest, ForStmtBranchWithoutConditionDoesNotExtendFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  for (;;) {
+(void)0;
+// [[loop_body]]
+  }
+}
+  )";
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("loop_body", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_FALSE(LoopBodyEnv.flowConditionImplies(LoopBodyFooVal));
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -97,8 +97,8 @@
 
   void VisitForStmt(const ForStmt *S) {
 auto *Cond = S->getCond();
-assert(Cond != nullptr);
-extendFlowCondition(*Cond);
+if (Cond != nullptr)
+  extendFlowCondition(*Cond);
   }
 
   void VisitBinaryOperator(const BinaryOperator *S) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128352: [clang][dataflow] Use diagnosis API in optional checker

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:126-129
+auto *Lattice =
+llvm::any_cast();
+PostVisitStmt(Stmt, DataflowAnalysisState{
+*Lattice, State.Env});




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128352/new/

https://reviews.llvm.org/D128352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:401
+   const TypeErasedDataflowAnalysisState ) {
+PostVisitStmt(Stmt.getStmt(), State);
+  });

Let's harmonize the arguments to `transferBlock` and 
`runTypeErasedDataflowAnalysis`. I think in both cases we only need access to 
the internal `Stmt` so there's no need to pass `CFGStmt` to `transferBlock`. 
This way we can pass `PostVisitStmt` directly here without having to wrap it in 
a lambda.



Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:87-89
+std::function(ASTContext &)>
+MakePostVisitStmt,

Why is this a function that returns a function? Couldn't it be simply a void 
function, like `VerifyResults`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127898/new/

https://reviews.llvm.org/D127898

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128521: [clang][dataflow] Implement functionality to compare if two boolean values are equivalent.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:138
 
+bool DataflowAnalysisContext::equivalentBoolValues(BoolValue ,
+   BoolValue ) {

This seems unrelated to flow conditions (is this intentional?) and a bit too 
specific for `DataflowAnalysisContext`. Perhaps we should expose the solver and 
let user code use it the way it needs to.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128521/new/

https://reviews.llvm.org/D128521

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128519: [clang][dataflow] Move logic for creating implication and iff expressions into `DataflowAnalysisContext` from `DataflowEnvironment`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:170
+  /// will be a value that represents the true boolean literal.
+  BoolValue (BoolValue , BoolValue );
+

Let's call it `getOrCreateImplication` similar to the members above. Same for 
iff below.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:170
+  /// will be a value that represents the true boolean literal.
+  BoolValue (BoolValue , BoolValue );
+

sgatev wrote:
> Let's call it `getOrCreateImplication` similar to the members above. Same for 
> iff below.
Let's add tests for the new members, possibly moving existing tests from 
`DataflowEnvironmentTest.cpp` to `DataflowAnalysisContextTest.cpp`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128519/new/

https://reviews.llvm.org/D128519

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128359: [clang][dataflow] Move logic for `createStorageLocation` from `DataflowEnvironment` to `DataflowAnalysisContext`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:91
 
+  /// Creates a stable storage location appropriate for `Type`.
+  ///





Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:98
+
+  /// Creates a stable storage location for `D`.
+  StorageLocation (const VarDecl );





Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:101
+
+  /// Creates a stable storage location for `E`.
+  StorageLocation (const Expr );




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128359/new/

https://reviews.llvm.org/D128359

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:57
+  }
+  return std::move(Diags);
+}

Better to remove this and rely on NRVO? 
https://en.cppreference.com/w/cpp/language/copy_elision



Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:49
+template  struct DiagnoseState {
+  DiagnoseState(DiagsT , const Environment )
+  : Diags(Diags), Env(Env) {}

samestep wrote:
> sgatev wrote:
> > Move this to Diagnosis.h?
> I'd prefer to keep it in `MatchSwitch.h` alongside `TransferState`, unless we 
> plan to, e.g., move that type to `DataflowAnalysis.h`.
Fair enough, but generally I see `TransferState` and `DiagnoseState` as very 
specific to the dataflow analysis framework whereas `MatchSwitchBuilder` seems 
to be a bit more generic so I'd consider separating them.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h:76
 
+class UncheckedOptionalAccessDiagnosis {
+public:

samestep wrote:
> samestep wrote:
> > sgatev wrote:
> > > samestep wrote:
> > > > samestep wrote:
> > > > > sgatev wrote:
> > > > > > Move this to a new UncheckedOptionalAccessDiagnosis.(h,cpp)?
> > > > > OK! I'll do that next.
> > > > Actually, @sgatev would that go under `FlowSensitive/Models/` or just 
> > > > under `FlowSensitive/`?
> > > I suggest keeping it under `FlowSensitive/Models/` for now. We can change 
> > > that at a later point if there's a better alternative.
> > Where should I put the `UncheckedOptionalAccessModelOptions` type and 
> > `ignorableOptional` function that need to be shared between both the model 
> > and the diagnosis?
> Same question for the following other helper functions:
> 
> - `hasOptionalType`
> - `isOptionalMemberCallWithName`
> - `isOptionalOperatorCallWithName`
> 
> Given how much code is shared between the two, I'm really not sure whether 
> it's the best idea to move this into a separate file in this patch...
I suggest either declaring those in the header and using them from 
`UncheckedOptionalAccessDiagnosis.cpp` or moving them to a separate file and 
including them both in the model and in the diagnosis. In general I think it's 
fair if the diagnosis depends on the model, but not the other way around. It's 
perfectly fine if we do this refactoring in a follow up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127898/new/

https://reviews.llvm.org/D127898

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:164-166
+  /// Returns a pointer value that represents a null pointer. Calls
+  /// with `PointeeType` that are canonically equivalent will return the same
+  /// result.





Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:316-318
+  /// Returns a pointer value that represents a null pointer. Calls
+  /// with `PointeeType` that are canonically equivalent will return the same
+  /// result.





Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:319
+  /// result.
+  PointerValue (QualType PointeeType);
+

Let's move this below `getThisPointeeStorageLocation` so that it's closer to 
related members. Same for the cpp file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128056/new/

https://reviews.llvm.org/D128056

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128359: [clang][dataflow] Move logic for `createStorageLocation` from `DataflowEnvironment` to `DataflowAnalysisContext`.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:91-92
 
+  // FIXME: Rename `createOrGetStorageLocation` to 
`getOrCreateStorageLocation`,
+  // `getStableStorageLocation`, or something more appropriate.
+

Let's implement this for the new members you are adding and keep the existing 
members in `DataflowEnvironment` as they are. I suggest calling the new ones 
`getStableStorageLocation`.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:94-95
+
+  /// Creates a storage location appropriate for `Type`. Does not assign a 
value
+  /// to the returned storage location in the environment.
+  ///





Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:102-104
+  /// Creates a storage location for `D`. Does not assign the returned storage
+  /// location to `D` in the environment. Does not assign a value to the
+  /// returned storage location in the environment.





Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:107-109
+  /// Creates a storage location for `E`. Does not assign the returned storage
+  /// location to `E` in the environment. Does not assign a value to the
+  /// returned storage location in the environment.





Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:41-43
+  // Evaluated declarations are always assigned the same storage locations to
+  // ensure that the environment stabilizes across loop iterations. Storage
+  // locations for evaluated declarations are stored in the analysis context.

I think it makes sense to keep this comment in the wrapper in 
`DataflowEnvironment`. Same for the one below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128359/new/

https://reviews.llvm.org/D128359

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h:76
 
+class UncheckedOptionalAccessDiagnosis {
+public:

samestep wrote:
> samestep wrote:
> > sgatev wrote:
> > > Move this to a new UncheckedOptionalAccessDiagnosis.(h,cpp)?
> > OK! I'll do that next.
> Actually, @sgatev would that go under `FlowSensitive/Models/` or just under 
> `FlowSensitive/`?
I suggest keeping it under `FlowSensitive/Models/` for now. We can change that 
at a later point if there's a better alternative.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127898/new/

https://reviews.llvm.org/D127898

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:138-150
+  auto ConstraintsIt = FlowConditionConstraints.find();
+  if (ConstraintsIt == FlowConditionConstraints.end()) {
+Constraints.insert();
+  } else {
+// Bind flow condition token via `iff` to its set of constraints
+// FC <=> (C1 ^ C2 ^ ...), where Ci are constraints
+// = (FC v !(C1 ^ C2 ^ ...)) ^ (!FC v (C1 ^ C2 ^ ...))





Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:142-144
+// Bind flow condition token via `iff` to its set of constraints
+// FC <=> (C1 ^ C2 ^ ...), where Ci are constraints
+// = (FC v !(C1 ^ C2 ^ ...)) ^ (!FC v (C1 ^ C2 ^ ...))




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128357/new/

https://reviews.llvm.org/D128357

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128352: [clang][dataflow] Use diagnosis API in optional checker

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:58
   BlockToOutputState =
-  dataflow::runDataflowAnalysis(*Context, Analysis, Env);
+  dataflow::runTypeErasedDataflowAnalysis(*Context, Analysis, Env);
   if (!BlockToOutputState)

I think the typed API should still be preferred unless we make a call to drop 
it. The type-erased API is only used internally in the dataflow analysis 
framework. I suggest taking a `DataflowAnalysisState` in `diagnoseCFG` 
instead of `TypeErasedDataflowAnalysisState` for now. Would that work?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128352/new/

https://reviews.llvm.org/D128352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:26
+template 
+using Diagnosis =
+std::function;

Let's add some documentation for `Diagnosis` and `diagnoseCFG`.



Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:27
+using Diagnosis =
+std::function;
+

`#include `



Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:32
+const ControlFlowContext ,
+std::vector> ,
+const Environment , TypeErasedDataflowAnalysis ,

`#include ` and `#include "llvm/ADT/Optional.h"`



Comment at: clang/include/clang/Analysis/FlowSensitive/Diagnosis.h:49
+  }
+  return std::move(Diags);
+}

`#include `



Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:49
+template  struct DiagnoseState {
+  DiagnoseState(DiagsT , const Environment )
+  : Diags(Diags), Env(Env) {}

Move this to Diagnosis.h?



Comment at: 
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h:76
 
+class UncheckedOptionalAccessDiagnosis {
+public:

Move this to a new UncheckedOptionalAccessDiagnosis.(h,cpp)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127898/new/

https://reviews.llvm.org/D127898

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128183: [clang][dataflow] Extend flow condition in the body of a do/while loop

[Stanislav Gatev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sgatev marked an inline comment as done.
Closed by commit rGe363c5963dc3: [clang][dataflow] Extend flow condition in the 
body of a do/while loop (authored by sgatev).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128183/new/

https://reviews.llvm.org/D128183

Files:
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3628,25 +3628,81 @@
 void target(bool Foo) {
   while (Foo) {
 (void)0;
-// [[while_branch]]
+// [[loop_body]]
   }
+  (void)0;
+  // [[after_loop]]
 }
   )";
-  runDataflow(Code,
-  [](llvm::ArrayRef<
- std::pair>>
- Results,
- ASTContext ) {
-ASSERT_THAT(Results, ElementsAre(Pair("while_branch", _)));
-const Environment  = Results[0].second.Env;
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("after_loop", _), Pair("loop_body", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
 
-const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
-ASSERT_THAT(FooDecl, NotNull());
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
 
-BoolValue  =
-*cast(Env.getValue(*FooDecl, SkipPast::None));
-EXPECT_TRUE(Env.flowConditionImplies(FooVal));
-  });
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(LoopBodyEnv.flowConditionImplies(LoopBodyFooVal));
+
+BoolValue  =
+*cast(AfterLoopEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(AfterLoopEnv.flowConditionImplies(
+AfterLoopEnv.makeNot(AfterLoopFooVal)));
+  });
+}
+
+TEST_F(TransferTest, DoWhileStmtBranchExtendsFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  bool Bar = true;
+  do {
+(void)0;
+// [[loop_body]]
+Bar = false;
+  } while (Foo);
+  (void)0;
+  // [[after_loop]]
+}
+  )";
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("after_loop", _), Pair("loop_body", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
+ASSERT_THAT(BarDecl, NotNull());
+
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*FooDecl, SkipPast::None));
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*BarDecl, SkipPast::None));
+EXPECT_TRUE(LoopBodyEnv.flowConditionImplies(
+LoopBodyEnv.makeOr(LoopBodyBarVal, LoopBodyFooVal)));
+
+BoolValue  =
+*cast(AfterLoopEnv.getValue(*FooDecl, SkipPast::None));
+BoolValue  =
+*cast(AfterLoopEnv.getValue(*BarDecl, SkipPast::None));
+EXPECT_TRUE(AfterLoopEnv.flowConditionImplies(
+AfterLoopEnv.makeNot(AfterLoopFooVal)));
+EXPECT_TRUE(AfterLoopEnv.flowConditionImplies(
+AfterLoopEnv.makeNot(AfterLoopBarVal)));
+  });
 }
 
 TEST_F(TransferTest, ForStmtBranchExtendsFlowCondition) {
@@ -3654,25 +3710,34 @@
 void target(bool Foo) {
   for (; Foo;) {
 (void)0;
-// [[for_branch]]
+// [[loop_body]]
   }
+  (void)0;
+  // [[after_loop]]
 }
   )";
-  runDataflow(Code,
-  [](llvm::ArrayRef<
- std::pair>>
- Results,
- ASTContext ) {
-ASSERT_THAT(Results, ElementsAre(Pair("for_branch", _)));
-const Environment  = Results[0].second.Env;
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("after_loop", _), Pair("loop_body", _)));
+const Environment  = Results[1].second.Env;
+   

[PATCH] D128183: [clang][dataflow] Extend flow condition in the body of a do/while loop

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev marked an inline comment as done.
sgatev added inline comments.



Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:3700
+EXPECT_TRUE(AfterLoopEnv.flowConditionImplies(
+AfterLoopEnv.makeNot(AfterLoopFooVal)));
+  });

gribozavr2 wrote:
> Can we infer that after the loop bar is false?
Yes!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128183/new/

https://reviews.llvm.org/D128183

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128183: [clang][dataflow] Extend flow condition in the body of a do/while loop

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 438427.
sgatev added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128183/new/

https://reviews.llvm.org/D128183

Files:
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3628,25 +3628,81 @@
 void target(bool Foo) {
   while (Foo) {
 (void)0;
-// [[while_branch]]
+// [[loop_body]]
   }
+  (void)0;
+  // [[after_loop]]
 }
   )";
-  runDataflow(Code,
-  [](llvm::ArrayRef<
- std::pair>>
- Results,
- ASTContext ) {
-ASSERT_THAT(Results, ElementsAre(Pair("while_branch", _)));
-const Environment  = Results[0].second.Env;
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("after_loop", _), Pair("loop_body", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
 
-const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
-ASSERT_THAT(FooDecl, NotNull());
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
 
-BoolValue  =
-*cast(Env.getValue(*FooDecl, SkipPast::None));
-EXPECT_TRUE(Env.flowConditionImplies(FooVal));
-  });
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(LoopBodyEnv.flowConditionImplies(LoopBodyFooVal));
+
+BoolValue  =
+*cast(AfterLoopEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(AfterLoopEnv.flowConditionImplies(
+AfterLoopEnv.makeNot(AfterLoopFooVal)));
+  });
+}
+
+TEST_F(TransferTest, DoWhileStmtBranchExtendsFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  bool Bar = true;
+  do {
+(void)0;
+// [[loop_body]]
+Bar = false;
+  } while (Foo);
+  (void)0;
+  // [[after_loop]]
+}
+  )";
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("after_loop", _), Pair("loop_body", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
+ASSERT_THAT(BarDecl, NotNull());
+
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*FooDecl, SkipPast::None));
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*BarDecl, SkipPast::None));
+EXPECT_TRUE(LoopBodyEnv.flowConditionImplies(
+LoopBodyEnv.makeOr(LoopBodyBarVal, LoopBodyFooVal)));
+
+BoolValue  =
+*cast(AfterLoopEnv.getValue(*FooDecl, SkipPast::None));
+BoolValue  =
+*cast(AfterLoopEnv.getValue(*BarDecl, SkipPast::None));
+EXPECT_TRUE(AfterLoopEnv.flowConditionImplies(
+AfterLoopEnv.makeNot(AfterLoopFooVal)));
+EXPECT_TRUE(AfterLoopEnv.flowConditionImplies(
+AfterLoopEnv.makeNot(AfterLoopBarVal)));
+  });
 }
 
 TEST_F(TransferTest, ForStmtBranchExtendsFlowCondition) {
@@ -3654,25 +3710,34 @@
 void target(bool Foo) {
   for (; Foo;) {
 (void)0;
-// [[for_branch]]
+// [[loop_body]]
   }
+  (void)0;
+  // [[after_loop]]
 }
   )";
-  runDataflow(Code,
-  [](llvm::ArrayRef<
- std::pair>>
- Results,
- ASTContext ) {
-ASSERT_THAT(Results, ElementsAre(Pair("for_branch", _)));
-const Environment  = Results[0].second.Env;
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("after_loop", _), Pair("loop_body", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
 
-const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
-ASSERT_THAT(FooDecl, NotNull());
+const ValueDecl *FooDecl 

[PATCH] D128183: [clang][dataflow] Extend flow condition in the body of a do/while loop

[Stanislav Gatev via Phabricator via cfe-commits]

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

Extend flow condition in the body of a do/while loop.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128183

Files:
  clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3628,25 +3628,77 @@
 void target(bool Foo) {
   while (Foo) {
 (void)0;
-// [[while_branch]]
+// [[loop_body]]
   }
+  (void)0;
+  // [[after_loop]]
 }
   )";
-  runDataflow(Code,
-  [](llvm::ArrayRef<
- std::pair>>
- Results,
- ASTContext ) {
-ASSERT_THAT(Results, ElementsAre(Pair("while_branch", _)));
-const Environment  = Results[0].second.Env;
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("after_loop", _), Pair("loop_body", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
 
-const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
-ASSERT_THAT(FooDecl, NotNull());
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
 
-BoolValue  =
-*cast(Env.getValue(*FooDecl, SkipPast::None));
-EXPECT_TRUE(Env.flowConditionImplies(FooVal));
-  });
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(LoopBodyEnv.flowConditionImplies(LoopBodyFooVal));
+
+BoolValue  =
+*cast(AfterLoopEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(AfterLoopEnv.flowConditionImplies(
+AfterLoopEnv.makeNot(AfterLoopFooVal)));
+  });
+}
+
+TEST_F(TransferTest, DoWhileStmtBranchExtendsFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  bool Bar = true;
+  do {
+(void)0;
+// [[loop_body]]
+Bar = false;
+  } while (Foo);
+  (void)0;
+  // [[after_loop]]
+}
+  )";
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("after_loop", _), Pair("loop_body", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
+ASSERT_THAT(BarDecl, NotNull());
+
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*FooDecl, SkipPast::None));
+BoolValue  =
+*cast(LoopBodyEnv.getValue(*BarDecl, SkipPast::None));
+EXPECT_TRUE(LoopBodyEnv.flowConditionImplies(
+LoopBodyEnv.makeOr(LoopBodyBarVal, LoopBodyFooVal)));
+
+BoolValue  =
+*cast(AfterLoopEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(AfterLoopEnv.flowConditionImplies(
+AfterLoopEnv.makeNot(AfterLoopFooVal)));
+  });
 }
 
 TEST_F(TransferTest, ForStmtBranchExtendsFlowCondition) {
@@ -3654,25 +3706,34 @@
 void target(bool Foo) {
   for (; Foo;) {
 (void)0;
-// [[for_branch]]
+// [[loop_body]]
   }
+  (void)0;
+  // [[after_loop]]
 }
   )";
-  runDataflow(Code,
-  [](llvm::ArrayRef<
- std::pair>>
- Results,
- ASTContext ) {
-ASSERT_THAT(Results, ElementsAre(Pair("for_branch", _)));
-const Environment  = Results[0].second.Env;
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("after_loop", _), Pair("loop_body", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
 
-const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
-ASSERT_THAT(FooDecl, NotNull());
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+

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

[Stanislav Gatev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sgatev marked an inline comment as done.
Closed by commit rG83232099cb5e: [clang][dataflow] Extend flow condition in the 
body of a for loop (authored by sgatev).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128060/new/

https://reviews.llvm.org/D128060

Files:
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3588,4 +3588,91 @@
   });
 }
 
+TEST_F(TransferTest, IfStmtBranchExtendsFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  if (Foo) {
+(void)0;
+// [[if_then]]
+  } else {
+(void)0;
+// [[if_else]]
+  }
+}
+  )";
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("if_else", _), Pair("if_then", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(ThenEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(ThenEnv.flowConditionImplies(ThenFooVal));
+
+BoolValue  =
+*cast(ElseEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(ElseEnv.flowConditionImplies(ElseEnv.makeNot(ElseFooVal)));
+  });
+}
+
+TEST_F(TransferTest, WhileStmtBranchExtendsFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  while (Foo) {
+(void)0;
+// [[while_branch]]
+  }
+}
+  )";
+  runDataflow(Code,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("while_branch", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(Env.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+  });
+}
+
+TEST_F(TransferTest, ForStmtBranchExtendsFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  for (; Foo;) {
+(void)0;
+// [[for_branch]]
+  }
+}
+  )";
+  runDataflow(Code,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("for_branch", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(Env.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -89,6 +89,12 @@
 extendFlowCondition(*Cond);
   }
 
+  void VisitForStmt(const ForStmt *S) {
+auto *Cond = S->getCond();
+assert(Cond != nullptr);
+extendFlowCondition(*Cond);
+  }
+
   void VisitBinaryOperator(const BinaryOperator *S) {
 assert(S->getOpcode() == BO_LAnd || S->getOpcode() == BO_LOr);
 auto *LHS = S->getLHS();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev marked an inline comment as done.
sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:92
 
+  void VisitForStmt(const ForStmt *S) {
+auto *Cond = S->getCond();

xazax.hun wrote:
> Do we support `DoStmt` or is that missing?
Good question. I think it's worth adding support for `DoStmt`, but that won't 
be very useful at this point because the condition will not always be true 
within the body of the loop. I'd be happy to follow up with a patch for it, but 
I'll first need to figure out what test to write. Ideas are welcome. : )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128060/new/

https://reviews.llvm.org/D128060

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 438257.
sgatev added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128060/new/

https://reviews.llvm.org/D128060

Files:
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3588,4 +3588,91 @@
   });
 }
 
+TEST_F(TransferTest, IfStmtBranchExtendsFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  if (Foo) {
+(void)0;
+// [[if_then]]
+  } else {
+(void)0;
+// [[if_else]]
+  }
+}
+  )";
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("if_else", _), Pair("if_then", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(ThenEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(ThenEnv.flowConditionImplies(ThenFooVal));
+
+BoolValue  =
+*cast(ElseEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(ElseEnv.flowConditionImplies(ElseEnv.makeNot(ElseFooVal)));
+  });
+}
+
+TEST_F(TransferTest, WhileStmtBranchExtendsFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  while (Foo) {
+(void)0;
+// [[while_branch]]
+  }
+}
+  )";
+  runDataflow(Code,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("while_branch", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(Env.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+  });
+}
+
+TEST_F(TransferTest, ForStmtBranchExtendsFlowCondition) {
+  std::string Code = R"(
+void target(bool Foo) {
+  for (; Foo;) {
+(void)0;
+// [[for_branch]]
+  }
+}
+  )";
+  runDataflow(Code,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("for_branch", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(Env.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -89,6 +89,12 @@
 extendFlowCondition(*Cond);
   }
 
+  void VisitForStmt(const ForStmt *S) {
+auto *Cond = S->getCond();
+assert(Cond != nullptr);
+extendFlowCondition(*Cond);
+  }
+
   void VisitBinaryOperator(const BinaryOperator *S) {
 assert(S->getOpcode() == BO_LAnd || S->getOpcode() == BO_LOr);
 auto *LHS = S->getLHS();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128013: [clang][dataflow] Add support for comma binary operator

[Stanislav Gatev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGba53906ceff1: [clang][dataflow] Add support for comma binary 
operator (authored by sgatev).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128013/new/

https://reviews.llvm.org/D128013

Files:
  clang/lib/Analysis/FlowSensitive/Transfer.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3557,4 +3557,35 @@
   });
 }
 
+TEST_F(TransferTest, BinaryOperatorComma) {
+  std::string Code = R"(
+void target(int Foo, int Bar) {
+  int  = (Foo, Bar);
+  // [[p]]
+}
+  )";
+  runDataflow(Code,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
+ASSERT_THAT(BarDecl, NotNull());
+
+const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
+ASSERT_THAT(BazDecl, NotNull());
+
+const StorageLocation *BarLoc =
+Env.getStorageLocation(*BarDecl, SkipPast::Reference);
+ASSERT_THAT(BarLoc, NotNull());
+
+const StorageLocation *BazLoc =
+Env.getStorageLocation(*BazDecl, SkipPast::Reference);
+EXPECT_EQ(BazLoc, BarLoc);
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -95,6 +95,11 @@
 : Env.makeNot(LHSEqRHSValue));
   break;
 }
+case BO_Comma: {
+  if (auto *Loc = Env.getStorageLocation(*RHS, SkipPast::None))
+Env.setStorageLocation(*S, *Loc);
+  break;
+}
 default:
   break;
 }


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3557,4 +3557,35 @@
   });
 }
 
+TEST_F(TransferTest, BinaryOperatorComma) {
+  std::string Code = R"(
+void target(int Foo, int Bar) {
+  int  = (Foo, Bar);
+  // [[p]]
+}
+  )";
+  runDataflow(Code,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
+ASSERT_THAT(BarDecl, NotNull());
+
+const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
+ASSERT_THAT(BazDecl, NotNull());
+
+const StorageLocation *BarLoc =
+Env.getStorageLocation(*BarDecl, SkipPast::Reference);
+ASSERT_THAT(BarLoc, NotNull());
+
+const StorageLocation *BazLoc =
+Env.getStorageLocation(*BazDecl, SkipPast::Reference);
+EXPECT_EQ(BazLoc, BarLoc);
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -95,6 +95,11 @@
 : Env.makeNot(LHSEqRHSValue));
   break;
 }
+case BO_Comma: {
+  if (auto *Loc = Env.getStorageLocation(*RHS, SkipPast::None))
+Env.setStorageLocation(*S, *Loc);
+  break;
+}
 default:
   break;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 437918.
sgatev added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128060/new/

https://reviews.llvm.org/D128060

Files:
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3557,4 +3557,88 @@
   });
 }
 
+TEST_F(TransferTest, BranchesExtendFlowCondition) {
+  std::string IfBranchCode = R"(
+void target(bool Foo) {
+  if (Foo) {
+(void)0;
+// [[if_then]]
+  } else {
+(void)0;
+// [[if_else]]
+  }
+}
+  )";
+  runDataflow(
+  IfBranchCode,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results,
+ElementsAre(Pair("if_else", _), Pair("if_then", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(ThenEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(ThenEnv.flowConditionImplies(ThenFooVal));
+
+BoolValue  =
+*cast(ElseEnv.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(ElseEnv.flowConditionImplies(ElseEnv.makeNot(ElseFooVal)));
+  });
+
+  std::string WhileBranchCode = R"(
+void target(bool Foo) {
+  while (Foo) {
+(void)0;
+// [[while_branch]]
+  }
+}
+  )";
+  runDataflow(WhileBranchCode,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("while_branch", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(Env.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+  });
+
+  std::string ForBranchCode = R"(
+void target(bool Foo) {
+  for (; Foo;) {
+(void)0;
+// [[for_branch]]
+  }
+}
+  )";
+  runDataflow(ForBranchCode,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("for_branch", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(Env.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -89,6 +89,12 @@
 extendFlowCondition(*Cond);
   }
 
+  void VisitForStmt(const ForStmt *S) {
+auto *Cond = S->getCond();
+assert(Cond != nullptr);
+extendFlowCondition(*Cond);
+  }
+
   void VisitBinaryOperator(const BinaryOperator *S) {
 assert(S->getOpcode() == BO_LAnd || S->getOpcode() == BO_LOr);
 auto *LHS = S->getLHS();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

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

Extend flow condition in the body of a for loop.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128060

Files:
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3557,4 +3557,87 @@
   });
 }
 
+TEST_F(TransferTest, BranchesExtendFlowCondition) {
+  std::string IfBranchCode = R"(
+void target(bool Foo) {
+  if (Foo) {
+(void)0;
+// [[if_branch_0]]
+  } else {
+(void)0;
+// [[if_branch_1]]
+  }
+}
+  )";
+  runDataflow(IfBranchCode,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("if_branch_1", _),
+ Pair("if_branch_0", _)));
+const Environment  = Results[1].second.Env;
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(Env0.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(Env0.flowConditionImplies(FooVal0));
+
+BoolValue  =
+*cast(Env1.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(Env1.flowConditionImplies(Env1.makeNot(FooVal1)));
+  });
+
+  std::string WhileBranchCode = R"(
+void target(bool Foo) {
+  while (Foo) {
+(void)0;
+// [[while_branch]]
+  }
+}
+  )";
+  runDataflow(WhileBranchCode,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("while_branch", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(Env.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+  });
+
+  std::string ForBranchCode = R"(
+void target(bool Foo) {
+  for (; Foo;) {
+(void)0;
+// [[for_branch]]
+  }
+}
+  )";
+  runDataflow(ForBranchCode,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("for_branch", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ASSERT_THAT(FooDecl, NotNull());
+
+BoolValue  =
+*cast(Env.getValue(*FooDecl, SkipPast::None));
+EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -89,6 +89,12 @@
 extendFlowCondition(*Cond);
   }
 
+  void VisitForStmt(const ForStmt *S) {
+auto *Cond = S->getCond();
+assert(Cond != nullptr);
+extendFlowCondition(*Cond);
+  }
+
   void VisitBinaryOperator(const BinaryOperator *S) {
 assert(S->getOpcode() == BO_LAnd || S->getOpcode() == BO_LOr);
 auto *LHS = S->getLHS();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128013: [clang][dataflow] Add support for comma binary operator

[Stanislav Gatev via Phabricator via cfe-commits]

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

Add support for comma binary operator.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128013

Files:
  clang/lib/Analysis/FlowSensitive/Transfer.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3557,4 +3557,35 @@
   });
 }
 
+TEST_F(TransferTest, BinaryOperatorComma) {
+  std::string Code = R"(
+void target(int Foo, int Bar) {
+  int  = (Foo, Bar);
+  // [[p]]
+}
+  )";
+  runDataflow(Code,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
+ASSERT_THAT(BarDecl, NotNull());
+
+const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
+ASSERT_THAT(BazDecl, NotNull());
+
+const StorageLocation *BarLoc =
+Env.getStorageLocation(*BarDecl, SkipPast::Reference);
+ASSERT_THAT(BarLoc, NotNull());
+
+const StorageLocation *BazLoc =
+Env.getStorageLocation(*BazDecl, SkipPast::Reference);
+EXPECT_EQ(BazLoc, BarLoc);
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -95,6 +95,11 @@
 : Env.makeNot(LHSEqRHSValue));
   break;
 }
+case BO_Comma: {
+  if (auto *Loc = Env.getStorageLocation(*RHS, SkipPast::None))
+Env.setStorageLocation(*S, *Loc);
+  break;
+}
 default:
   break;
 }


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3557,4 +3557,35 @@
   });
 }
 
+TEST_F(TransferTest, BinaryOperatorComma) {
+  std::string Code = R"(
+void target(int Foo, int Bar) {
+  int  = (Foo, Bar);
+  // [[p]]
+}
+  )";
+  runDataflow(Code,
+  [](llvm::ArrayRef<
+ std::pair>>
+ Results,
+ ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
+ASSERT_THAT(BarDecl, NotNull());
+
+const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
+ASSERT_THAT(BazDecl, NotNull());
+
+const StorageLocation *BarLoc =
+Env.getStorageLocation(*BarDecl, SkipPast::Reference);
+ASSERT_THAT(BarLoc, NotNull());
+
+const StorageLocation *BazLoc =
+Env.getStorageLocation(*BazDecl, SkipPast::Reference);
+EXPECT_EQ(BazLoc, BarLoc);
+  });
+}
+
 } // namespace
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -95,6 +95,11 @@
 : Env.makeNot(LHSEqRHSValue));
   break;
 }
+case BO_Comma: {
+  if (auto *Loc = Env.getStorageLocation(*RHS, SkipPast::None))
+Env.setStorageLocation(*S, *Loc);
+  break;
+}
 default:
   break;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127865: [clang][dataflow] Make `Value` and `StorageLocation` non-copyable

[Stanislav Gatev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0c2edf27a22e: [clang][dataflow] Make `Value` and 
`StorageLocation` non-copyable (authored by sgatev).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127865/new/

https://reviews.llvm.org/D127865

Files:
  clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
  clang/include/clang/Analysis/FlowSensitive/Value.h


Index: clang/include/clang/Analysis/FlowSensitive/Value.h
===
--- clang/include/clang/Analysis/FlowSensitive/Value.h
+++ clang/include/clang/Analysis/FlowSensitive/Value.h
@@ -47,6 +47,12 @@
 
   explicit Value(Kind ValKind) : ValKind(ValKind) {}
 
+  // Non-copyable because addresses of values are used as their identities
+  // throughout framework and user code. The framework is responsible for
+  // construction and destruction of values.
+  Value(const Value &) = delete;
+  Value =(const Value &) = delete;
+
   virtual ~Value() = default;
 
   Kind getKind() const { return ValKind; }
Index: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
===
--- clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
+++ clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
@@ -31,6 +31,12 @@
 
   StorageLocation(Kind LocKind, QualType Type) : LocKind(LocKind), Type(Type) 
{}
 
+  // Non-copyable because addresses of storage locations are used as their
+  // identities throughout framework and user code. The framework is 
responsible
+  // for construction and destruction of storage locations.
+  StorageLocation(const StorageLocation &) = delete;
+  StorageLocation =(const StorageLocation &) = delete;
+
   virtual ~StorageLocation() = default;
 
   Kind getKind() const { return LocKind; }


Index: clang/include/clang/Analysis/FlowSensitive/Value.h
===
--- clang/include/clang/Analysis/FlowSensitive/Value.h
+++ clang/include/clang/Analysis/FlowSensitive/Value.h
@@ -47,6 +47,12 @@
 
   explicit Value(Kind ValKind) : ValKind(ValKind) {}
 
+  // Non-copyable because addresses of values are used as their identities
+  // throughout framework and user code. The framework is responsible for
+  // construction and destruction of values.
+  Value(const Value &) = delete;
+  Value =(const Value &) = delete;
+
   virtual ~Value() = default;
 
   Kind getKind() const { return ValKind; }
Index: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
===
--- clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
+++ clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
@@ -31,6 +31,12 @@
 
   StorageLocation(Kind LocKind, QualType Type) : LocKind(LocKind), Type(Type) {}
 
+  // Non-copyable because addresses of storage locations are used as their
+  // identities throughout framework and user code. The framework is responsible
+  // for construction and destruction of storage locations.
+  StorageLocation(const StorageLocation &) = delete;
+  StorageLocation =(const StorageLocation &) = delete;
+
   virtual ~StorageLocation() = default;
 
   Kind getKind() const { return LocKind; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127865: [clang][dataflow] Make `Value` and `StorageLocation` non-copyable

[Stanislav Gatev via Phabricator via cfe-commits]

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

This makes it harder to misuse APIs that return references by
accidentally copying the results which could happen when assigning the
them to variables declared as `auto`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127865

Files:
  clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
  clang/include/clang/Analysis/FlowSensitive/Value.h


Index: clang/include/clang/Analysis/FlowSensitive/Value.h
===
--- clang/include/clang/Analysis/FlowSensitive/Value.h
+++ clang/include/clang/Analysis/FlowSensitive/Value.h
@@ -47,6 +47,12 @@
 
   explicit Value(Kind ValKind) : ValKind(ValKind) {}
 
+  // Non-copyable because addresses of values are used as their identities
+  // throughout framework and user code. The framework is responsible for
+  // construction and destruction of values.
+  Value(const Value &) = delete;
+  Value =(const Value &) = delete;
+
   virtual ~Value() = default;
 
   Kind getKind() const { return ValKind; }
Index: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
===
--- clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
+++ clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
@@ -31,6 +31,12 @@
 
   StorageLocation(Kind LocKind, QualType Type) : LocKind(LocKind), Type(Type) 
{}
 
+  // Non-copyable because addresses of storage locations are used as their
+  // identities throughout framework and user code. The framework is 
responsible
+  // for construction and destruction of storage locations.
+  StorageLocation(const StorageLocation &) = delete;
+  StorageLocation =(const StorageLocation &) = delete;
+
   virtual ~StorageLocation() = default;
 
   Kind getKind() const { return LocKind; }


Index: clang/include/clang/Analysis/FlowSensitive/Value.h
===
--- clang/include/clang/Analysis/FlowSensitive/Value.h
+++ clang/include/clang/Analysis/FlowSensitive/Value.h
@@ -47,6 +47,12 @@
 
   explicit Value(Kind ValKind) : ValKind(ValKind) {}
 
+  // Non-copyable because addresses of values are used as their identities
+  // throughout framework and user code. The framework is responsible for
+  // construction and destruction of values.
+  Value(const Value &) = delete;
+  Value =(const Value &) = delete;
+
   virtual ~Value() = default;
 
   Kind getKind() const { return ValKind; }
Index: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
===
--- clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
+++ clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
@@ -31,6 +31,12 @@
 
   StorageLocation(Kind LocKind, QualType Type) : LocKind(LocKind), Type(Type) {}
 
+  // Non-copyable because addresses of storage locations are used as their
+  // identities throughout framework and user code. The framework is responsible
+  // for construction and destruction of storage locations.
+  StorageLocation(const StorageLocation &) = delete;
+  StorageLocation =(const StorageLocation &) = delete;
+
   virtual ~StorageLocation() = default;
 
   Kind getKind() const { return LocKind; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8fcdd625856b: [clang][dataflow] Add support for correlated 
branches to optional model (authored by sgatev).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125931/new/

https://reviews.llvm.org/D125931

Files:
  
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1861,7 +1861,26 @@
   )",
  UnorderedElementsAre(Pair("check", "safe")));
 
-  // FIXME: Add tests that call `emplace` in conditional branches.
+  // FIXME: Add tests that call `emplace` in conditional branches:
+  //  ExpectLatticeChecksFor(
+  //  R"(
+  //#include "unchecked_optional_access_test.h"
+  //
+  //void target($ns::$optional opt, bool b) {
+  //  if (b) {
+  //opt.emplace(0);
+  //  }
+  //  if (b) {
+  //opt.value();
+  ///*[[check-1]]*/
+  //  } else {
+  //opt.value();
+  ///*[[check-2]]*/
+  //  }
+  //}
+  //  )",
+  //  UnorderedElementsAre(Pair("check-1", "safe"),
+  //   Pair("check-2", "unsafe: input.cc:12:9")));
 }
 
 TEST_P(UncheckedOptionalAccessTest, Reset) {
@@ -1892,7 +1911,27 @@
   )",
   UnorderedElementsAre(Pair("check", "unsafe: input.cc:7:9")));
 
-  // FIXME: Add tests that call `reset` in conditional branches.
+  // FIXME: Add tests that call `reset` in conditional branches:
+  //  ExpectLatticeChecksFor(
+  //  R"(
+  //#include "unchecked_optional_access_test.h"
+  //
+  //void target(bool b) {
+  //  $ns::$optional opt = $ns::make_optional(0);
+  //  if (b) {
+  //opt.reset();
+  //  }
+  //  if (b) {
+  //opt.value();
+  ///*[[check-1]]*/
+  //  } else {
+  //opt.value();
+  ///*[[check-2]]*/
+  //  }
+  //}
+  //  )",
+  //  UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:10:9"),
+  //   Pair("check-2", "safe")));
 }
 
 TEST_P(UncheckedOptionalAccessTest, ValueAssignment) {
@@ -2347,6 +2386,284 @@
   UnorderedElementsAre(Pair("check", "unsafe: input.cc:12:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, CorrelatedBranches) {
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b || opt.has_value()) {
+if (!b) {
+  opt.value();
+  /*[[check-1]]*/
+}
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-1", "safe")));
+
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b && !opt.has_value()) return;
+  if (b) {
+opt.value();
+/*[[check-2]]*/
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-2", "safe")));
+
+  ExpectLatticeChecksFor(
+  R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value()) b = true;
+  if (b) {
+opt.value();
+/*[[check-3]]*/
+  }
+}
+  )code",
+  UnorderedElementsAre(Pair("check-3", "unsafe: input.cc:7:9")));
+
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b) return;
+  if (opt.has_value()) b = true;
+  if (b) {
+opt.value();
+/*[[check-4]]*/
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-4", "safe")));
+
+  ExpectLatticeChecksFor(R"(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value() == b) {
+if (b) {
+  opt.value();
+  /*[[check-5]]*/
+}
+  }
+}
+  )",
+ UnorderedElementsAre(Pair("check-5", "safe")));
+
+  ExpectLatticeChecksFor(R"(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value() != b) {
+if (!b) {
+  opt.value();
+  /*[[check-6]]*/
+}
+  }
+}
+  )",
+ UnorderedElementsAre(Pair("check-6", "safe")));
+
+  ExpectLatticeChecksFor(R"(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b) {
+  

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:288
 
+bool isEngagedOptionalValue(const Value , const Environment ) {
+  auto *HasValueVal =

xazax.hun wrote:
> I wonder whether `NonEmpty` is clearer than `Engaged`. I think `Engaged` can 
> also be OK, but we probably want to have a comment somewhere to explain the 
> terminology.
Sounds good. I went with `isEmptyOptional` and `isNonEmptyOptional`.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:689
+  }
+  setHasValue(MergedVal, HasValueVal);
+  return true;

xazax.hun wrote:
> Would it make sense to have the converese? I.e., when both optionals are 
> empty, create an empty optional here. While currently this modeling does not 
> distinguish between unchecked and empty optionals it might be handy in the 
> future. Although I am also happy with this as is, not adding any code that 
> does not have any utility as of today.
Good idea! I think this can be useful. I added the code and a test case based 
on dead code:

```
if (b) {
  opt = std::nullopt;
} else {
  opt = std::nullopt;
}
if (opt.has_value()) {
  // unreachable
}
```



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:174
+/// property of the optional value `OptionalVal`.
+void setHasValue(StructValue , BoolValue ) {
+  OptionalVal.setProperty("has_value", HasValueVal);

ymandel wrote:
> sgatev wrote:
> > ymandel wrote:
> > > I believe you can relax this to `Value` because `setProperty` is no 
> > > longer specific to `StructValue`.
> > I did that intentionally because I still think it makes sense to assert 
> > that an optional is modeled as a `StructValue`. I haven't thought about 
> > where and how it'd be best to assert that though so I'll happily remove the 
> > casts for now.
> Sounds good. I had the same thought. But, it occurs to me that at this point, 
> are we using the `StructValue` at all? If not, then the only reason we expect 
> it to be a `StructValue` is because we know the optional type is a record 
> type. But, that's not an assumption of our model. So, maybe we should be 
> agnostic to the underlying representation?
Agreed. Let's not assume a `StructValue` representation unless it's necessary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125931/new/

https://reviews.llvm.org/D125931

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 437088.
sgatev marked 3 inline comments as done.
sgatev added a comment.
Herald added a subscriber: martong.

Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125931/new/

https://reviews.llvm.org/D125931

Files:
  
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1861,7 +1861,26 @@
   )",
  UnorderedElementsAre(Pair("check", "safe")));
 
-  // FIXME: Add tests that call `emplace` in conditional branches.
+  // FIXME: Add tests that call `emplace` in conditional branches:
+  //  ExpectLatticeChecksFor(
+  //  R"(
+  //#include "unchecked_optional_access_test.h"
+  //
+  //void target($ns::$optional opt, bool b) {
+  //  if (b) {
+  //opt.emplace(0);
+  //  }
+  //  if (b) {
+  //opt.value();
+  ///*[[check-1]]*/
+  //  } else {
+  //opt.value();
+  ///*[[check-2]]*/
+  //  }
+  //}
+  //  )",
+  //  UnorderedElementsAre(Pair("check-1", "safe"),
+  //   Pair("check-2", "unsafe: input.cc:12:9")));
 }
 
 TEST_P(UncheckedOptionalAccessTest, Reset) {
@@ -1892,7 +1911,27 @@
   )",
   UnorderedElementsAre(Pair("check", "unsafe: input.cc:7:9")));
 
-  // FIXME: Add tests that call `reset` in conditional branches.
+  // FIXME: Add tests that call `reset` in conditional branches:
+  //  ExpectLatticeChecksFor(
+  //  R"(
+  //#include "unchecked_optional_access_test.h"
+  //
+  //void target(bool b) {
+  //  $ns::$optional opt = $ns::make_optional(0);
+  //  if (b) {
+  //opt.reset();
+  //  }
+  //  if (b) {
+  //opt.value();
+  ///*[[check-1]]*/
+  //  } else {
+  //opt.value();
+  ///*[[check-2]]*/
+  //  }
+  //}
+  //  )",
+  //  UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:10:9"),
+  //   Pair("check-2", "safe")));
 }
 
 TEST_P(UncheckedOptionalAccessTest, ValueAssignment) {
@@ -2347,6 +2386,284 @@
   UnorderedElementsAre(Pair("check", "unsafe: input.cc:12:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, CorrelatedBranches) {
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b || opt.has_value()) {
+if (!b) {
+  opt.value();
+  /*[[check-1]]*/
+}
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-1", "safe")));
+
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b && !opt.has_value()) return;
+  if (b) {
+opt.value();
+/*[[check-2]]*/
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-2", "safe")));
+
+  ExpectLatticeChecksFor(
+  R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value()) b = true;
+  if (b) {
+opt.value();
+/*[[check-3]]*/
+  }
+}
+  )code",
+  UnorderedElementsAre(Pair("check-3", "unsafe: input.cc:7:9")));
+
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b) return;
+  if (opt.has_value()) b = true;
+  if (b) {
+opt.value();
+/*[[check-4]]*/
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-4", "safe")));
+
+  ExpectLatticeChecksFor(R"(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value() == b) {
+if (b) {
+  opt.value();
+  /*[[check-5]]*/
+}
+  }
+}
+  )",
+ UnorderedElementsAre(Pair("check-5", "safe")));
+
+  ExpectLatticeChecksFor(R"(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value() != b) {
+if (!b) {
+  opt.value();
+  /*[[check-6]]*/
+}
+  }
+}
+  )",
+ UnorderedElementsAre(Pair("check-6", "safe")));
+
+  ExpectLatticeChecksFor(R"(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b) {
+  $ns::$optional opt1 = $ns::nullopt;
+  $ns::$optional opt2;
+  if (b) {
+opt2 = 

[PATCH] D127746: [clang][dataflow] Convert `PointeeLoc` of PointerValue from reference to pointer. This allows PointeeLoc to be empty in the case of `nullptr`

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:189
 public:
-  explicit PointerValue(StorageLocation )
+  explicit PointerValue(StorageLocation *PointeeLoc)
   : Value(Kind::Pointer), PointeeLoc(PointeeLoc) {}

Can you please document when this can be null?



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:266-267
 
-  auto  = Env.createStorageLocation(*S);
-  Env.setStorageLocation(*S, Loc);
-  Env.setValue(Loc, Env.takeOwnership(std::make_unique(
-SubExprVal->getPointeeLoc(;
+  // If PointeeLoc is null, then we are dereferencing a nullptr, skip
+  // creating a value for the dereference
+  if (auto *PointeeLoc = SubExprVal->getPointeeLoc()) {

Can you please add a test for this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127746/new/

https://reviews.llvm.org/D127746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127745: [clang][dataflow] Rename `getPointeeLoc` to `getReferencePointeeLoc` and `getPointerPointeeLoc` respectively for ReferenceValue and PointerValue.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added a comment.

On a high level, what's wrong with having `getPointeeLoc` members with 
different return types in `ReferenceValue` and `PointerValue`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127745/new/

https://reviews.llvm.org/D127745

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:174
+/// property of the optional value `OptionalVal`.
+void setHasValue(StructValue , BoolValue ) {
+  OptionalVal.setProperty("has_value", HasValueVal);

ymandel wrote:
> I believe you can relax this to `Value` because `setProperty` is no longer 
> specific to `StructValue`.
I did that intentionally because I still think it makes sense to assert that an 
optional is modeled as a `StructValue`. I haven't thought about where and how 
it'd be best to assert that though so I'll happily remove the casts for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125931/new/

https://reviews.llvm.org/D125931

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 436710.
sgatev marked 4 inline comments as done.
sgatev added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125931/new/

https://reviews.llvm.org/D125931

Files:
  
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1861,7 +1861,26 @@
   )",
  UnorderedElementsAre(Pair("check", "safe")));
 
-  // FIXME: Add tests that call `emplace` in conditional branches.
+  // FIXME: Add tests that call `emplace` in conditional branches:
+  //  ExpectLatticeChecksFor(
+  //  R"(
+  //#include "unchecked_optional_access_test.h"
+  //
+  //void target($ns::$optional opt, bool b) {
+  //  if (b) {
+  //opt.emplace(0);
+  //  }
+  //  if (b) {
+  //opt.value();
+  ///*[[check-1]]*/
+  //  } else {
+  //opt.value();
+  ///*[[check-2]]*/
+  //  }
+  //}
+  //  )",
+  //  UnorderedElementsAre(Pair("check-1", "safe"),
+  //   Pair("check-2", "unsafe: input.cc:12:9")));
 }
 
 TEST_P(UncheckedOptionalAccessTest, Reset) {
@@ -1892,7 +1911,27 @@
   )",
   UnorderedElementsAre(Pair("check", "unsafe: input.cc:7:9")));
 
-  // FIXME: Add tests that call `reset` in conditional branches.
+  // FIXME: Add tests that call `reset` in conditional branches:
+  //  ExpectLatticeChecksFor(
+  //  R"(
+  //#include "unchecked_optional_access_test.h"
+  //
+  //void target(bool b) {
+  //  $ns::$optional opt = $ns::make_optional(0);
+  //  if (b) {
+  //opt.reset();
+  //  }
+  //  if (b) {
+  //opt.value();
+  ///*[[check-1]]*/
+  //  } else {
+  //opt.value();
+  ///*[[check-2]]*/
+  //  }
+  //}
+  //  )",
+  //  UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:10:9"),
+  //   Pair("check-2", "safe")));
 }
 
 TEST_P(UncheckedOptionalAccessTest, ValueAssignment) {
@@ -2347,6 +2386,265 @@
   UnorderedElementsAre(Pair("check", "unsafe: input.cc:12:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, CorrelatedBranches) {
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b || opt.has_value()) {
+if (!b) {
+  opt.value();
+  /*[[check-1]]*/
+}
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-1", "safe")));
+
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b && !opt.has_value()) return;
+  if (b) {
+opt.value();
+/*[[check-2]]*/
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-2", "safe")));
+
+  ExpectLatticeChecksFor(
+  R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value()) b = true;
+  if (b) {
+opt.value();
+/*[[check-3]]*/
+  }
+}
+  )code",
+  UnorderedElementsAre(Pair("check-3", "unsafe: input.cc:7:9")));
+
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b) return;
+  if (opt.has_value()) b = true;
+  if (b) {
+opt.value();
+/*[[check-4]]*/
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-4", "safe")));
+
+  ExpectLatticeChecksFor(R"(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value() == b) {
+if (b) {
+  opt.value();
+  /*[[check-5]]*/
+}
+  }
+}
+  )",
+ UnorderedElementsAre(Pair("check-5", "safe")));
+
+  ExpectLatticeChecksFor(R"(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value() != b) {
+if (!b) {
+  opt.value();
+  /*[[check-6]]*/
+}
+  }
+}
+  )",
+ UnorderedElementsAre(Pair("check-6", "safe")));
+
+  // FIXME: Add support for operator==.
+  // ExpectLatticeChecksFor(R"(
+  //   #include "unchecked_optional_access_test.h"
+  //
+  //   void target($ns::$optional opt1, $ns::$optional opt2) {
+  // if (opt1 == opt2) {
+  //   if 

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev marked 3 inline comments as done.
sgatev added inline comments.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:596-598
+  MergedEnv.makeOr(
+  MergedEnv.makeAnd(Env1.getFlowConditionToken(), *HasValueVal1),
+  MergedEnv.makeAnd(Env2.getFlowConditionToken(), *HasValueVal2)));

ymandel wrote:
> xazax.hun wrote:
> > ymandel wrote:
> > > What's the plan for loops? This will increase the size of the constraint 
> > > on every iteration, preventing termination.
> > I have nothing to add, just a brain dump. In this particular case, the 
> > analysis domain has infinite chains, the size of the Boolean formulas can 
> > increase indefinitely (at least syntactically). The canonical solution to 
> > deal with infinite chains is to do a widening operation such that the 
> > widened state over approximates both the state of the old and the new 
> > iteration. A very simple way of doing this would be to simply erase all 
> > knowledge we accumulated about the Boolean value. On the other hand, I 
> > wonder if it is often possible to do something smarter. There might be 
> > cases when the value is syntactically larger in the new iteration but 
> > actually it has the exact same truth table. The solver might be able to 
> > show this. On the other hand, if the loop generates new Boolean symbols in 
> > each iteration, we have little hope fore that.
> > I have nothing to add, just a brain dump. In this particular case, the 
> > analysis domain has infinite chains, the size of the Boolean formulas can 
> > increase indefinitely (at least syntactically). The canonical solution to 
> > deal with infinite chains is to do a widening operation such that the 
> > widened state over approximates both the state of the old and the new 
> > iteration. A very simple way of doing this would be to simply erase all 
> > knowledge we accumulated about the Boolean value. On the other hand, I 
> > wonder if it is often possible to do something smarter. There might be 
> > cases when the value is syntactically larger in the new iteration but 
> > actually it has the exact same truth table. The solver might be able to 
> > show this. On the other hand, if the loop generates new Boolean symbols in 
> > each iteration, we have little hope fore that.
> 
> Agreed. In our internal implementation, we have something like this -- it 
> consults the solver to see if the two are logically equivalent.  That loses 
> the (subtle) support for correlated conditions. However, until we have more 
> precise support for widening (that is, separate from `merge`), I think this 
> is the only way to go to ensure that loop analysis terminates.
After discussing this, we decided to go for the internal implementation which 
is tested on a large codebase and commit to smarter merging that preserves flow 
condition information only after testing it extensively. I have updated the 
patch accordingly, please take a look.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125931/new/

https://reviews.llvm.org/D125931

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 436590.
sgatev added a comment.

Rebase main.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125931/new/

https://reviews.llvm.org/D125931

Files:
  
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1861,7 +1861,26 @@
   )",
  UnorderedElementsAre(Pair("check", "safe")));
 
-  // FIXME: Add tests that call `emplace` in conditional branches.
+  // FIXME: Add tests that call `emplace` in conditional branches:
+  //  ExpectLatticeChecksFor(
+  //  R"(
+  //#include "unchecked_optional_access_test.h"
+  //
+  //void target($ns::$optional opt, bool b) {
+  //  if (b) {
+  //opt.emplace(0);
+  //  }
+  //  if (b) {
+  //opt.value();
+  ///*[[check-1]]*/
+  //  } else {
+  //opt.value();
+  ///*[[check-2]]*/
+  //  }
+  //}
+  //  )",
+  //  UnorderedElementsAre(Pair("check-1", "safe"),
+  //   Pair("check-2", "unsafe: input.cc:12:9")));
 }
 
 TEST_P(UncheckedOptionalAccessTest, Reset) {
@@ -1892,7 +1911,27 @@
   )",
   UnorderedElementsAre(Pair("check", "unsafe: input.cc:7:9")));
 
-  // FIXME: Add tests that call `reset` in conditional branches.
+  // FIXME: Add tests that call `reset` in conditional branches:
+  //  ExpectLatticeChecksFor(
+  //  R"(
+  //#include "unchecked_optional_access_test.h"
+  //
+  //void target(bool b) {
+  //  $ns::$optional opt = $ns::make_optional(0);
+  //  if (b) {
+  //opt.reset();
+  //  }
+  //  if (b) {
+  //opt.value();
+  ///*[[check-1]]*/
+  //  } else {
+  //opt.value();
+  ///*[[check-2]]*/
+  //  }
+  //}
+  //  )",
+  //  UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:10:9"),
+  //   Pair("check-2", "safe")));
 }
 
 TEST_P(UncheckedOptionalAccessTest, ValueAssignment) {
@@ -2347,6 +2386,265 @@
   UnorderedElementsAre(Pair("check", "unsafe: input.cc:12:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, CorrelatedBranches) {
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b || opt.has_value()) {
+if (!b) {
+  opt.value();
+  /*[[check-1]]*/
+}
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-1", "safe")));
+
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b && !opt.has_value()) return;
+  if (b) {
+opt.value();
+/*[[check-2]]*/
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-2", "safe")));
+
+  ExpectLatticeChecksFor(
+  R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value()) b = true;
+  if (b) {
+opt.value();
+/*[[check-3]]*/
+  }
+}
+  )code",
+  UnorderedElementsAre(Pair("check-3", "unsafe: input.cc:7:9")));
+
+  ExpectLatticeChecksFor(R"code(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (b) return;
+  if (opt.has_value()) b = true;
+  if (b) {
+opt.value();
+/*[[check-4]]*/
+  }
+}
+  )code",
+ UnorderedElementsAre(Pair("check-4", "safe")));
+
+  ExpectLatticeChecksFor(R"(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value() == b) {
+if (b) {
+  opt.value();
+  /*[[check-5]]*/
+}
+  }
+}
+  )",
+ UnorderedElementsAre(Pair("check-5", "safe")));
+
+  ExpectLatticeChecksFor(R"(
+#include "unchecked_optional_access_test.h"
+
+void target(bool b, $ns::$optional opt) {
+  if (opt.has_value() != b) {
+if (!b) {
+  opt.value();
+  /*[[check-6]]*/
+}
+  }
+}
+  )",
+ UnorderedElementsAre(Pair("check-6", "safe")));
+
+  // FIXME: Add support for operator==.
+  // ExpectLatticeChecksFor(R"(
+  //   #include "unchecked_optional_access_test.h"
+  //
+  //   void target($ns::$optional opt1, $ns::$optional opt2) {
+  // if (opt1 == opt2) {
+  //   if (opt1.has_value()) {
+  // opt2.value();
+  // 

[PATCH] D127434: [clang][dataflow] In `optional` model, match call return via hasType

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added a comment.

I see that the declaration of `operator*` in `std::unique_ptr` is `typename 
add_lvalue_reference::type operator*() const;`. I managed to reproduce the 
crash with the following snippet:

  #include 
  
  namespace detail {
   
  template 
  struct type_identity { using type = T; };
  
  template 
  auto try_add_lvalue_reference(int) -> type_identity;
  
  template 
  auto try_add_lvalue_reference(...) -> type_identity;
  
  template 
  auto try_add_rvalue_reference(int) -> type_identity;
  
  template 
  auto try_add_rvalue_reference(...) -> type_identity;
  
  }  // namespace detail
  
  template 
  struct add_lvalue_reference : 
decltype(detail::try_add_lvalue_reference(0)) {};
  
  template 
  struct add_rvalue_reference : 
decltype(detail::try_add_rvalue_reference(0)) {};
  
  template 
  struct smart_ptr {
typename add_lvalue_reference::type operator*() &;
  };
  
  void foo() {
smart_ptr> x;
*x = std::nullopt;
  }

I suggest adding the definition of `add_lvalue_reference` to 
StdTypeTraitsHeader 

 and adding a test with the rest of the code above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127434/new/

https://reviews.llvm.org/D127434

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127196: [clang][dataflow] Enable use of synthetic properties on all Value instances.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:29
 /// Base class for all values computed by abstract interpretation.
+/// All Value instances should be separately allocated and stored by pointer
+/// for pointer stability.

I'm not sure I understand what is meant by "separately allocated" and "stored 
by pointer". Could you please clarify?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127196/new/

https://reviews.llvm.org/D127196

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126972: [clang][dataflow] Modify optional-checker to handle type aliases.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev accepted this revision.
sgatev added inline comments.
This revision is now accepted and ready to land.



Comment at: 
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:2217
 
+// Verifies that the check sees through aliases.
+TEST_P(UncheckedOptionalAccessTest, WithAlias) {

Let's replace "check"/"checker" with "model" here and in the commit message.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126972/new/

https://reviews.llvm.org/D126972

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sgatev marked an inline comment as done.
Closed by commit rG65e710c3fc03: [clang][dataflow] Model calls returning 
optionals (authored by sgatev).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126759/new/

https://reviews.llvm.org/D126759

Files:
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2150,6 +2150,70 @@
   UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, CallReturningOptional) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+$ns::$optional MakeOpt();
+
+void target() {
+  $ns::$optional opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-1]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:9:7")));
+
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+const $ns::$optional& MakeOpt();
+
+void target() {
+  $ns::$optional opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-2]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-2", "unsafe: input.cc:9:7")));
+
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using IntOpt = $ns::$optional;
+IntOpt MakeOpt();
+
+void target() {
+  IntOpt opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-3]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-3", "unsafe: input.cc:10:7")));
+
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using IntOpt = $ns::$optional;
+const IntOpt& MakeOpt();
+
+void target() {
+  IntOpt opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-4]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-4", "unsafe: input.cc:10:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -45,6 +45,11 @@
 
 auto hasOptionalType() { return hasType(optionalClass()); }
 
+auto hasOptionalOrAliasType() {
+  return hasUnqualifiedDesugaredType(
+  recordType(hasDeclaration(optionalClass(;
+}
+
 auto isOptionalMemberCallWithName(
 llvm::StringRef MemberName,
 llvm::Optional Ignorable = llvm::None) {
@@ -158,6 +163,12 @@
ComparesToSame(integerLiteral(equals(0);
 }
 
+auto isCallReturningOptional() {
+  return callExpr(callee(functionDecl(
+  returns(anyOf(hasOptionalOrAliasType(),
+referenceType(pointee(hasOptionalOrAliasType(;
+}
+
 /// Creates a symbolic value for an `optional` value using `HasValueVal` as the
 /// symbolic value of its "has_value" property.
 StructValue (Environment , BoolValue ) {
@@ -322,6 +333,18 @@
   });
 }
 
+void transferCallReturningOptional(const CallExpr *E,
+   const MatchFinder::MatchResult ,
+   LatticeTransferState ) {
+  if (State.Env.getStorageLocation(*E, SkipPast::None) != nullptr)
+return;
+
+  auto  = State.Env.createStorageLocation(*E);
+  State.Env.setStorageLocation(*E, Loc);
+  State.Env.setValue(
+  Loc, createOptionalValue(State.Env, State.Env.makeAtomicBoolValue()));
+}
+
 void assignOptionalValue(const Expr , LatticeTransferState ,
  BoolValue ) {
   if (auto *OptionalLoc =
@@ -547,6 +570,10 @@
   // opt.value_or(X) != X
   .CaseOf(isValueOrNotEqX(), transferValueOrNotEqX)
 
+  // returns optional
+  .CaseOf(isCallReturningOptional(),
+transferCallReturningOptional)
+
   .Build();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev marked an inline comment as done.
sgatev added inline comments.



Comment at: 
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:161
 
+auto possiblyAliasedOptionalType() {
+  return hasUnqualifiedDesugaredType(

xazax.hun wrote:
> Does alias refers to a type alias in this case? I first parsed this as if 
> this has something to do with pointers being aliased. I wonder if 
> `possiblyOptionalAliasType` would read better (considering the optional type 
> its own alias). Feel free to ignore. 
I opted for `hasOptionalOrAliasType`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126759/new/

https://reviews.llvm.org/D126759

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 434000.
sgatev added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126759/new/

https://reviews.llvm.org/D126759

Files:
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2150,6 +2150,70 @@
   UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, CallReturningOptional) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+$ns::$optional MakeOpt();
+
+void target() {
+  $ns::$optional opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-1]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:9:7")));
+
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+const $ns::$optional& MakeOpt();
+
+void target() {
+  $ns::$optional opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-2]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-2", "unsafe: input.cc:9:7")));
+
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using IntOpt = $ns::$optional;
+IntOpt MakeOpt();
+
+void target() {
+  IntOpt opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-3]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-3", "unsafe: input.cc:10:7")));
+
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using IntOpt = $ns::$optional;
+const IntOpt& MakeOpt();
+
+void target() {
+  IntOpt opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-4]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-4", "unsafe: input.cc:10:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -45,6 +45,11 @@
 
 auto hasOptionalType() { return hasType(optionalClass()); }
 
+auto hasOptionalOrAliasType() {
+  return hasUnqualifiedDesugaredType(
+  recordType(hasDeclaration(optionalClass(;
+}
+
 auto isOptionalMemberCallWithName(
 llvm::StringRef MemberName,
 llvm::Optional Ignorable = llvm::None) {
@@ -158,6 +163,12 @@
ComparesToSame(integerLiteral(equals(0);
 }
 
+auto isCallReturningOptional() {
+  return callExpr(callee(functionDecl(
+  returns(anyOf(hasOptionalOrAliasType(),
+referenceType(pointee(hasOptionalOrAliasType(;
+}
+
 /// Creates a symbolic value for an `optional` value using `HasValueVal` as the
 /// symbolic value of its "has_value" property.
 StructValue (Environment , BoolValue ) {
@@ -322,6 +333,18 @@
   });
 }
 
+void transferCallReturningOptional(const CallExpr *E,
+   const MatchFinder::MatchResult ,
+   LatticeTransferState ) {
+  if (State.Env.getStorageLocation(*E, SkipPast::None) != nullptr)
+return;
+
+  auto  = State.Env.createStorageLocation(*E);
+  State.Env.setStorageLocation(*E, Loc);
+  State.Env.setValue(
+  Loc, createOptionalValue(State.Env, State.Env.makeAtomicBoolValue()));
+}
+
 void assignOptionalValue(const Expr , LatticeTransferState ,
  BoolValue ) {
   if (auto *OptionalLoc =
@@ -547,6 +570,10 @@
   // opt.value_or(X) != X
   .CaseOf(isValueOrNotEqX(), transferValueOrNotEqX)
 
+  // returns optional
+  .CaseOf(isCallReturningOptional(),
+transferCallReturningOptional)
+
   .Build();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
sgatev marked an inline comment as done.
Closed by commit rG0e286b77cf7b: [clang][dataflow] Add transfer functions for 
structured bindings (authored by sgatev).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120495/new/

https://reviews.llvm.org/D120495

Files:
  clang/lib/Analysis/FlowSensitive/Transfer.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3369,4 +3369,192 @@
   LangStandard::lang_cxx17, /*ApplyBuiltinTransfer=*/true, "operator=");
 }
 
+TEST_F(TransferTest, StructuredBindingAssignFromStructIntMembersToRefs) {
+  std::string Code = R"(
+struct A {
+  int Foo;
+  int Bar;
+};
+
+void target() {
+  int Qux;
+  A Baz;
+  Baz.Foo = Qux;
+  auto  = Baz.Foo;
+  auto  = Baz.Bar;
+  auto &[BoundFooRef, BoundBarRef] = Baz;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooRefDecl = findValueDecl(ASTCtx, "FooRef");
+ASSERT_THAT(FooRefDecl, NotNull());
+
+const ValueDecl *BarRefDecl = findValueDecl(ASTCtx, "BarRef");
+ASSERT_THAT(BarRefDecl, NotNull());
+
+const ValueDecl *QuxDecl = findValueDecl(ASTCtx, "Qux");
+ASSERT_THAT(QuxDecl, NotNull());
+
+const ValueDecl *BoundFooRefDecl = findValueDecl(ASTCtx, "BoundFooRef");
+ASSERT_THAT(BoundFooRefDecl, NotNull());
+
+const ValueDecl *BoundBarRefDecl = findValueDecl(ASTCtx, "BoundBarRef");
+ASSERT_THAT(BoundBarRefDecl, NotNull());
+
+const StorageLocation *FooRefLoc =
+Env.getStorageLocation(*FooRefDecl, SkipPast::Reference);
+ASSERT_THAT(FooRefLoc, NotNull());
+
+const StorageLocation *BarRefLoc =
+Env.getStorageLocation(*BarRefDecl, SkipPast::Reference);
+ASSERT_THAT(BarRefLoc, NotNull());
+
+const Value *QuxVal = Env.getValue(*QuxDecl, SkipPast::None);
+ASSERT_THAT(QuxVal, NotNull());
+
+const StorageLocation *BoundFooRefLoc =
+Env.getStorageLocation(*BoundFooRefDecl, SkipPast::Reference);
+EXPECT_EQ(BoundFooRefLoc, FooRefLoc);
+
+const StorageLocation *BoundBarRefLoc =
+Env.getStorageLocation(*BoundBarRefDecl, SkipPast::Reference);
+EXPECT_EQ(BoundBarRefLoc, BarRefLoc);
+
+EXPECT_EQ(Env.getValue(*BoundFooRefDecl, SkipPast::Reference), QuxVal);
+  });
+}
+
+TEST_F(TransferTest, StructuredBindingAssignFromStructRefMembersToRefs) {
+  std::string Code = R"(
+struct A {
+  int 
+  int 
+};
+
+void target(A Baz) {
+  int Qux;
+  Baz.Foo = Qux;
+  auto  = Baz.Foo;
+  auto  = Baz.Bar;
+  auto &[BoundFooRef, BoundBarRef] = Baz;
+  // [[p]]
+}
+  )";
+  runDataflow(
+  Code, [](llvm::ArrayRef<
+   std::pair>>
+   Results,
+   ASTContext ) {
+ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+const Environment  = Results[0].second.Env;
+
+const ValueDecl *FooRefDecl = findValueDecl(ASTCtx, "FooRef");
+ASSERT_THAT(FooRefDecl, NotNull());
+
+const ValueDecl *BarRefDecl = findValueDecl(ASTCtx, "BarRef");
+ASSERT_THAT(BarRefDecl, NotNull());
+
+const ValueDecl *QuxDecl = findValueDecl(ASTCtx, "Qux");
+ASSERT_THAT(QuxDecl, NotNull());
+
+const ValueDecl *BoundFooRefDecl = findValueDecl(ASTCtx, "BoundFooRef");
+ASSERT_THAT(BoundFooRefDecl, NotNull());
+
+const ValueDecl *BoundBarRefDecl = findValueDecl(ASTCtx, "BoundBarRef");
+ASSERT_THAT(BoundBarRefDecl, NotNull());
+
+const StorageLocation *FooRefLoc =
+Env.getStorageLocation(*FooRefDecl, SkipPast::Reference);
+ASSERT_THAT(FooRefLoc, NotNull());
+
+const StorageLocation *BarRefLoc =
+Env.getStorageLocation(*BarRefDecl, SkipPast::Reference);
+ASSERT_THAT(BarRefLoc, NotNull());
+
+const Value *QuxVal = Env.getValue(*QuxDecl, SkipPast::None);
+ASSERT_THAT(QuxVal, NotNull());
+
+const StorageLocation *BoundFooRefLoc =
+Env.getStorageLocation(*BoundFooRefDecl, SkipPast::Reference);
+EXPECT_EQ(BoundFooRefLoc, FooRefLoc);
+
+const StorageLocation *BoundBarRefLoc =
+Env.getStorageLocation(*BoundBarRefDecl, SkipPast::Reference);
+EXPECT_EQ(BoundBarRefLoc, BarRefLoc);
+
+EXPECT_EQ(Env.getValue(*BoundFooRefDecl, 

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

[Stanislav Gatev via Phabricator via cfe-commits]

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

Model calls returning optionals


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126759

Files:
  clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
  clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2150,6 +2150,70 @@
   UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, CallReturningOptional) {
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+$ns::$optional MakeOpt();
+
+void target() {
+  $ns::$optional opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-1]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:9:7")));
+
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+const $ns::$optional& MakeOpt();
+
+void target() {
+  $ns::$optional opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-2]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-2", "unsafe: input.cc:9:7")));
+
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using IntOpt = $ns::$optional;
+IntOpt MakeOpt();
+
+void target() {
+  IntOpt opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-3]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-3", "unsafe: input.cc:10:7")));
+
+  ExpectLatticeChecksFor(
+  R"(
+#include "unchecked_optional_access_test.h"
+
+using IntOpt = $ns::$optional;
+const IntOpt& MakeOpt();
+
+void target() {
+  IntOpt opt = 0;
+  opt = MakeOpt();
+  opt.value();
+  /*[[check-4]]*/
+}
+  )",
+  UnorderedElementsAre(Pair("check-4", "unsafe: input.cc:10:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -158,6 +158,21 @@
ComparesToSame(integerLiteral(equals(0);
 }
 
+auto possiblyAliasedOptionalType() {
+  return hasUnqualifiedDesugaredType(
+  recordType(hasDeclaration(optionalClass(;
+}
+
+auto possiblyAliasedOrReferencedOptionalType() {
+  return anyOf(possiblyAliasedOptionalType(),
+   referenceType(pointee(possiblyAliasedOptionalType(;
+}
+
+auto isCallReturningOptional() {
+  return callExpr(
+  callee(functionDecl(returns(possiblyAliasedOrReferencedOptionalType();
+}
+
 /// Creates a symbolic value for an `optional` value using `HasValueVal` as the
 /// symbolic value of its "has_value" property.
 StructValue (Environment , BoolValue ) {
@@ -320,6 +335,18 @@
   });
 }
 
+void transferCallReturningOptional(const CallExpr *E,
+   const MatchFinder::MatchResult ,
+   LatticeTransferState ) {
+  if (State.Env.getStorageLocation(*E, SkipPast::None) != nullptr)
+return;
+
+  auto  = State.Env.createStorageLocation(*E);
+  State.Env.setStorageLocation(*E, Loc);
+  State.Env.setValue(
+  Loc, createOptionalValue(State.Env, State.Env.makeAtomicBoolValue()));
+}
+
 void assignOptionalValue(const Expr , LatticeTransferState ,
  BoolValue ) {
   if (auto *OptionalLoc =
@@ -545,6 +572,10 @@
   // opt.value_or(X) != X
   .CaseOf(isValueOrNotEqX(), transferValueOrNotEqX)
 
+  // returns optional
+  .CaseOf(isCallReturningOptional(),
+transferCallReturningOptional)
+
   .Build();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev marked 2 inline comments as done.
sgatev added inline comments.



Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:80
+///
+/// FIXME: Consider adding support for structured bindings to the CFG builder.
+class DecompositionVisitor : public ConstStmtVisitor {

xazax.hun wrote:
> sgatev wrote:
> > xazax.hun wrote:
> > > Did you look into how hard would it be to add structured bindings to the 
> > > CFG builder? If the effort is comparable to this patch (and not 
> > > significantly bigger), it might be better to do that work instead of 
> > > spending effort on some temporary workaround. What do you think?
> > Circling back to this after a while. I believe we explored changing the CFG 
> > briefly, but don't have a fully fleshed out proposal for it. I recently 
> > noticed 
> > https://discourse.llvm.org/t/implement-support-for-c-17-structured-bindings-in-the-clang-static-analyzer/60588.
> >  It seems that part of the project is exploring necessary changes to the 
> > CFG. What do you think about submitting this patch with local pattern 
> > matching and revisiting that once the GSoC project completes?
> Officially, there is no candidate accepted for that project yet, so there is 
> no guarantee that we will have someone working on this. That being said, the 
> CSA devs started to discuss what would be the best way to represent 
> structured bindings in the CFG, and it is possible that for many of the cases 
> we do not actually need to change the CFG, because the AST nodes have rich 
> information. But of course, none of this is final, but we do not need to 
> block this patch on that. So I'm fine adding this for now.
> 
> The AST for the supported case looks like:
> ```
>`-DeclStmt 0x5599ad9de6b0 
>   `-DecompositionDecl 0x5599ad9de310  col:13 used 'A &' 
> cinit
> |-DeclRefExpr 0x5599ad9de388  'A' lvalue Var 0x5599ad9ddb80 
> 'Baz' 'A'
> |-BindingDecl 0x5599ad9de280  col:14 BoundFooRef 'int'
> | `-MemberExpr 0x5599ad9de630  'int' lvalue .Foo 
> 0x5599ad9dd970
> |   `-DeclRefExpr 0x5599ad9de610  'A':'A' lvalue 
> Decomposition 0x5599ad9de310 '' 'A &'
> `-BindingDecl 0x5599ad9de2c8  col:27 BoundBarRef 'int'
>   `-MemberExpr 0x5599ad9de680  'int' lvalue .Bar 
> 0x5599ad9dd9d8
> `-DeclRefExpr 0x5599ad9de660  'A':'A' lvalue 
> Decomposition 0x5599ad9de310 '' 'A &'
> ``` 
> 
> So each `BindingDecl` is like:
> ```
> `-BindingDecl 0x5599ad9de2c8  col:27 BoundBarRef 'int'
>   `-MemberExpr 0x5599ad9de680  'int' lvalue .Bar 
> 0x5599ad9dd9d8
> `-DeclRefExpr 0x5599ad9de660  'A':'A' lvalue 
> Decomposition 0x5599ad9de310 '' 'A &'
> ```
> 
> Is there a case where the `BindingDecl` would have a different shape? If we 
> do not expect that happening, I feel like an `StmtVisitor` is a bit of an 
> overkill for this and we could handle the supported case more directly. What 
> do you think?
Agreed. I simplified the patch. Please take a look.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120495/new/

https://reviews.llvm.org/D120495

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits