[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:230
+
+  virtual void releaseMemory() {
+PostDomTree.releaseMemory();

kuhar wrote:
> If the virtual function is introduced by ManagesAnalysis, isn't it better to 
> use override here?
I admit to have copy-pasted this from the class above, and, well, it isn't 
introduced by `ManagedAnalysis`. Which begs the question why it was made 
virtual at all.

Nice catch!



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:235
+  // Lazily retrieves the set of control dependencies to \p A.
+  const CFGBlockVector (CFGBlock *A) {
+auto It = ControlDepenencyMap.find(A);

kuhar wrote:
>  Can it be const?
IDFCalculator takes it's arguments by a non-const pointer. I guess I could fix 
that too on LLVM's side eventually. The method itself retrieves control 
dependencies lazily, and might modify the state of this class.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:252
+  /// Whether \p A is control dependent on \p B.
+  bool isControlDependent(CFGBlock *A, CFGBlock *B) {
+return llvm::is_contained(getControlDependencies(A), B);

kuhar wrote:
> Can it be const?
Same.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:257
+  // Dumps immediate control dependencies for each block.
+  void dump() {
+CFG *cfg = PostDomTree.getCFG();

NoQ wrote:
> kuhar wrote:
> > kuhar wrote:
> > > Can `dump` be const? 
> > In LLVM, `dump`s are usually annotated with the `LLVM_DUMP_METHOD` 
> > attribute and not compiled in release builds. Is the convention different 
> > in the static analyzer?
> `LLVM_DUMP_METHOD` is great. Hiding dump methods under `#ifndef NDEBUG` is 
> something i've seen very rarely. It's fairly annoying to me that exploded 
> graph dumps are unavailable in release builds, but apart from that i don't 
> have any immediate opinion, so this sounds like a global LLVM policy that 
> we're historically not paying much attention to, but i don't mind complying.
Cant be const for the same reasons.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62619



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

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Szelethus marked 11 inline comments as done.
Closed by commit rL365197: [analyzer][IDF] Add a control dependency calculator 
+ a new debug checker (authored by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62619?vs=205917=208157#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62619

Files:
  cfe/trunk/include/clang/Analysis/Analyses/Dominators.h
  cfe/trunk/include/clang/Analysis/CFG.h
  cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
  cfe/trunk/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  cfe/trunk/test/Analysis/domtest.c
  cfe/trunk/test/Analysis/domtest.cpp
  cfe/trunk/unittests/Analysis/CFGDominatorTree.cpp

Index: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1237,6 +1237,10 @@
   HelpText<"Print the post dominance tree for a given CFG">,
   Documentation;
 
+def ControlDependencyTreeDumper : Checker<"DumpControlDependencies">,
+  HelpText<"Print the post control dependency tree for a given CFG">,
+  Documentation;
+
 def LiveVariablesDumper : Checker<"DumpLiveVars">,
   HelpText<"Print results of live variable analysis">,
   Documentation;
Index: cfe/trunk/include/clang/Analysis/CFG.h
===
--- cfe/trunk/include/clang/Analysis/CFG.h
+++ cfe/trunk/include/clang/Analysis/CFG.h
@@ -1285,6 +1285,9 @@
   static ChildIteratorType child_end(NodeRef N) { return N->succ_end(); }
 };
 
+template <> struct GraphTraits
+: GraphTraits {};
+
 template <> struct GraphTraits< const ::clang::CFGBlock *> {
   using NodeRef = const ::clang::CFGBlock *;
   using ChildIteratorType = ::clang::CFGBlock::const_succ_iterator;
@@ -1294,6 +1297,9 @@
   static ChildIteratorType child_end(NodeRef N) { return N->succ_end(); }
 };
 
+template <> struct GraphTraits
+: GraphTraits {};
+
 template <> struct GraphTraits> {
   using NodeRef = ::clang::CFGBlock *;
   using ChildIteratorType = ::clang::CFGBlock::const_pred_iterator;
@@ -1306,6 +1312,9 @@
   static ChildIteratorType child_end(NodeRef N) { return N->pred_end(); }
 };
 
+template <> struct GraphTraits>
+: GraphTraits {};
+
 template <> struct GraphTraits> {
   using NodeRef = const ::clang::CFGBlock *;
   using ChildIteratorType = ::clang::CFGBlock::const_pred_iterator;
@@ -1318,6 +1327,9 @@
   static ChildIteratorType child_end(NodeRef N) { return N->pred_end(); }
 };
 
+template <> struct GraphTraits>
+: GraphTraits {};
+
 // Traits for: CFG
 
 template <> struct GraphTraits< ::clang::CFG* >
Index: cfe/trunk/include/clang/Analysis/Analyses/Dominators.h
===
--- cfe/trunk/include/clang/Analysis/Analyses/Dominators.h
+++ cfe/trunk/include/clang/Analysis/Analyses/Dominators.h
@@ -18,6 +18,7 @@
 #include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/ADT/GraphTraits.h"
 #include "llvm/ADT/iterator.h"
+#include "llvm/Support/GenericIteratedDominanceFrontier.h"
 #include "llvm/Support/GenericDomTree.h"
 #include "llvm/Support/GenericDomTreeConstruction.h"
 #include "llvm/Support/raw_ostream.h"
@@ -44,12 +45,17 @@
 public:
   using DominatorTreeBase = llvm::DominatorTreeBase;
 
-  DominatorTreeBase DT;
-
   CFGDominatorTreeImpl() = default;
+
+  CFGDominatorTreeImpl(CFG *cfg) {
+buildDominatorTree(cfg);
+  }
+
   ~CFGDominatorTreeImpl() override = default;
 
-  DominatorTreeBase& getBase() { return *DT; }
+  DominatorTreeBase () { return DT; }
+
+  CFG *getCFG() { return cfg; }
 
   /// \returns the root CFGBlock of the dominators tree.
   CFGBlock *getRoot() const {
@@ -172,11 +178,96 @@
 
 private:
   CFG *cfg;
+  DominatorTreeBase DT;
 };
 
 using CFGDomTree = CFGDominatorTreeImpl;
 using CFGPostDomTree = CFGDominatorTreeImpl;
 
+} // end of namespace clang
+
+namespace llvm {
+namespace IDFCalculatorDetail {
+
+/// Specialize ChildrenGetterTy to skip nullpointer successors.
+template 
+struct ChildrenGetterTy {
+  using NodeRef = typename GraphTraits::NodeRef;
+  using ChildrenTy = SmallVector;
+
+  ChildrenTy get(const NodeRef ) {
+using OrderedNodeTy =
+typename IDFCalculatorBase::OrderedNodeTy;
+
+auto Children = children(N);
+ChildrenTy Ret{Children.begin(), Children.end()};
+Ret.erase(std::remove(Ret.begin(), Ret.end(), nullptr), Ret.end());
+return Ret;
+  }
+};
+
+} // end of namespace IDFCalculatorDetail
+} // end of namespace llvm
+
+namespace clang {
+
+class ControlDependencyCalculator : public ManagedAnalysis {
+  using IDFCalculator = llvm::IDFCalculatorBase;
+  using CFGBlockVector = llvm::SmallVector;
+  using 

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Sure!


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

https://reviews.llvm.org/D62619



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

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Can you formally accept then? :) (I'll address inlines before commiting of 
course!)

In D62619#1566693 , @kuhar wrote:

> The non-static-analyzer bits look good to me, I added a few nits.


Thank you! This part of the project was fear of mine, and I'm super grateful 
for all the guidance you have given!


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

https://reviews.llvm.org/D62619



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

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Static Analyzer bits look great to me as well!




Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:257
+  // Dumps immediate control dependencies for each block.
+  void dump() {
+CFG *cfg = PostDomTree.getCFG();

kuhar wrote:
> kuhar wrote:
> > Can `dump` be const? 
> In LLVM, `dump`s are usually annotated with the `LLVM_DUMP_METHOD` attribute 
> and not compiled in release builds. Is the convention different in the static 
> analyzer?
`LLVM_DUMP_METHOD` is great. Hiding dump methods under `#ifndef NDEBUG` is 
something i've seen very rarely. It's fairly annoying to me that exploded graph 
dumps are unavailable in release builds, but apart from that i don't have any 
immediate opinion, so this sounds like a global LLVM policy that we're 
historically not paying much attention to, but i don't mind complying.



Comment at: clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp:90
+if (AnalysisDeclContext *AC = mgr.getAnalysisDeclContext(D)) {
+  ControlDependencyCalculator dom(AC->getCFG());
+  dom.dump();

CaPiTaLiZe VaRiAbLeS.

(*doesn't really care*)


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

https://reviews.llvm.org/D62619



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

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

The non-static-analyzer bits look good to me, I added a few nits.




Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:230
+
+  virtual void releaseMemory() {
+PostDomTree.releaseMemory();

If the virtual function is introduced by ManagesAnalysis, isn't it better to 
use override here?



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:235
+  // Lazily retrieves the set of control dependencies to \p A.
+  const CFGBlockVector (CFGBlock *A) {
+auto It = ControlDepenencyMap.find(A);

 Can it be const?



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:252
+  /// Whether \p A is control dependent on \p B.
+  bool isControlDependent(CFGBlock *A, CFGBlock *B) {
+return llvm::is_contained(getControlDependencies(A), B);

Can it be const?



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:257
+  // Dumps immediate control dependencies for each block.
+  void dump() {
+CFG *cfg = PostDomTree.getCFG();

Can `dump` be const? 



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:257
+  // Dumps immediate control dependencies for each block.
+  void dump() {
+CFG *cfg = PostDomTree.getCFG();

kuhar wrote:
> Can `dump` be const? 
In LLVM, `dump`s are usually annotated with the `LLVM_DUMP_METHOD` attribute 
and not compiled in release builds. Is the convention different in the static 
analyzer?


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

https://reviews.llvm.org/D62619



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

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Gentle ping :^)


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

https://reviews.llvm.org/D62619



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

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 205917.
Szelethus added a comment.

Rebase on previous patches.


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

https://reviews.llvm.org/D62619

Files:
  clang/include/clang/Analysis/Analyses/Dominators.h
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  clang/test/Analysis/domtest.c
  clang/test/Analysis/domtest.cpp
  clang/unittests/Analysis/CFGDominatorTree.cpp

Index: clang/unittests/Analysis/CFGDominatorTree.cpp
===
--- clang/unittests/Analysis/CFGDominatorTree.cpp
+++ clang/unittests/Analysis/CFGDominatorTree.cpp
@@ -117,6 +117,99 @@
   EXPECT_TRUE(PostDom.dominates(nullptr, ExitBlock));
 }
 
+TEST(CFGDominatorTree, ControlDependency) {
+  const char *Code = R"(bool coin();
+
+void funcWithBranch() {
+  int x = 0;
+  if (coin()) {
+if (coin()) {
+  x = 5;
+}
+int j = 10 / x;
+(void)j;
+  }
+};)";
+  BuildResult Result = BuildCFG(Code);
+  EXPECT_EQ(BuildResult::BuiltCFG, Result.getStatus());
+
+  //  1st if  2nd if
+  //  [B5 (ENTRY)]  -> [B4] -> [B3] -> [B2] -> [B1] -> [B0 (EXIT)]
+  //\\  / /
+  // \-> /
+  //  -->
+
+  CFG *cfg = Result.getCFG();
+
+  // Sanity checks.
+  EXPECT_EQ(cfg->size(), 6u);
+
+  CFGBlock *ExitBlock = *cfg->begin();
+  EXPECT_EQ(ExitBlock, >getExit());
+
+  CFGBlock *NullDerefBlock = *(cfg->begin() + 1);
+
+  CFGBlock *SecondThenBlock = *(cfg->begin() + 2);
+
+  CFGBlock *SecondIfBlock = *(cfg->begin() + 3);
+  EXPECT_TRUE(hasStmtType(SecondIfBlock));
+
+  CFGBlock *FirstIfBlock = *(cfg->begin() + 4);
+  EXPECT_TRUE(hasStmtType(FirstIfBlock));
+
+  CFGBlock *EntryBlock = *(cfg->begin() + 5);
+  EXPECT_EQ(EntryBlock, >getEntry());
+
+  ControlDependencyCalculator Control(cfg);
+
+  EXPECT_TRUE(Control.isControlDependent(SecondThenBlock, SecondIfBlock));
+  EXPECT_TRUE(Control.isControlDependent(SecondIfBlock, FirstIfBlock));
+  EXPECT_FALSE(Control.isControlDependent(NullDerefBlock, SecondIfBlock));
+}
+
+TEST(CFGDominatorTree, ControlDependencyWithLoops) {
+  const char *Code = R"(int test3() {
+  int x,y,z;
+
+  x = y = z = 1;
+  if (x > 0) {
+while (x >= 0){
+  while (y >= x) {
+x = x-1;
+y = y/2;
+  }
+}
+  }
+  z = y;
+
+  return 0;
+})";
+  BuildResult Result = BuildCFG(Code);
+  EXPECT_EQ(BuildResult::BuiltCFG, Result.getStatus());
+
+  //   <- [B2] <-
+  //  /  \
+  // [B8 (ENTRY)] -> [B7] -> [B6] ---> [B5] -> [B4] -> [B3]
+  //   \   | \  /
+  //\  |  <-
+  // \  \
+  //  > [B1] -> [B0 (EXIT)]
+
+  CFG *cfg = Result.getCFG();
+
+  ControlDependencyCalculator Control(cfg);
+
+  auto GetBlock = [cfg] (unsigned Index) -> CFGBlock * {
+assert(Index < cfg->size());
+return *(cfg->begin() + Index);
+  };
+
+  // While not immediately obvious, the second block in fact post dominates the
+  // fifth, hence B5 is not a control dependency of 2.
+  EXPECT_FALSE(Control.isControlDependent(GetBlock(5), GetBlock(2)));
+}
+
+
 } // namespace
 } // namespace analysis
 } // namespace clang
Index: clang/test/Analysis/domtest.cpp
===
--- clang/test/Analysis/domtest.cpp
+++ clang/test/Analysis/domtest.cpp
@@ -1,6 +1,7 @@
 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -analyzer-checker=debug.DumpDominators \
 // RUN:   -analyzer-checker=debug.DumpPostDominators \
+// RUN:   -analyzer-checker=debug.DumpControlDependencies \
 // RUN:   2>&1 | FileCheck %s
 
 bool coin();
@@ -20,7 +21,8 @@
 
 //  [B3 (ENTRY)]  -> [B1] -> [B2] -> [B0 (EXIT)]
 
-// CHECK:  Immediate dominance tree (Node#,IDom#):
+// CHECK:  Control dependencies (Node#,Dependency#):
+// CHECK-NEXT: Immediate dominance tree (Node#,IDom#):
 // CHECK-NEXT: (0,2)
 // CHECK-NEXT: (1,3)
 // CHECK-NEXT: (2,1)
@@ -42,13 +44,18 @@
   }
 }
 
-//> [B2] >
-//   /\
-// [B5 (ENTRY)] -> [B4] -> [B3] ---> [B1]
-//   

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:51
+  CFGDominatorTreeImpl(CFG *cfg) {
+buildDominatorTree(cfg);
+  }

kuhar wrote:
> DomTree has a constructor that runs the builder -- why not use it directly?
That is correct, but `DomTreeBase` does not. :/



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:160
   /// changes.
   void changeImmediateDominator(CFGBlock *N, CFGBlock *NewIDom) {
 DT.changeImmediateDominator(N, NewIDom);

kuhar wrote:
> Do you need it at all? I understand it's a wrapper around dominators, but 
> this API is virtually impossible to use safely.
I agree 100%, I'll get rid of this in another patch.


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

https://reviews.llvm.org/D62619



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

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 205109.
Szelethus marked 7 inline comments as done.
Szelethus added a comment.

Fixes according to reviewer comments.


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

https://reviews.llvm.org/D62619

Files:
  clang/include/clang/Analysis/Analyses/Dominators.h
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  clang/test/Analysis/domtest.c
  clang/test/Analysis/domtest.cpp
  clang/unittests/Analysis/CFGDominatorTree.cpp

Index: clang/unittests/Analysis/CFGDominatorTree.cpp
===
--- clang/unittests/Analysis/CFGDominatorTree.cpp
+++ clang/unittests/Analysis/CFGDominatorTree.cpp
@@ -117,6 +117,99 @@
   EXPECT_TRUE(PostDom.dominates(nullptr, ExitBlock));
 }
 
+TEST(CFGDominatorTree, ControlDependency) {
+  const char *Code = R"(bool coin();
+
+void funcWithBranch() {
+  int x = 0;
+  if (coin()) {
+if (coin()) {
+  x = 5;
+}
+int j = 10 / x;
+(void)j;
+  }
+};)";
+  BuildResult Result = BuildCFG(Code);
+  EXPECT_EQ(BuildResult::BuiltCFG, Result.getStatus());
+
+  //  1st if  2nd if
+  //  [B5 (ENTRY)]  -> [B4] -> [B3] -> [B2] -> [B1] -> [B0 (EXIT)]
+  //\\  / /
+  // \-> /
+  //  -->
+
+  CFG *cfg = Result.getCFG();
+
+  // Sanity checks.
+  EXPECT_EQ(cfg->size(), 6u);
+
+  CFGBlock *ExitBlock = *cfg->begin();
+  EXPECT_EQ(ExitBlock, >getExit());
+
+  CFGBlock *NullDerefBlock = *(cfg->begin() + 1);
+
+  CFGBlock *SecondThenBlock = *(cfg->begin() + 2);
+
+  CFGBlock *SecondIfBlock = *(cfg->begin() + 3);
+  EXPECT_TRUE(hasStmtType(SecondIfBlock));
+
+  CFGBlock *FirstIfBlock = *(cfg->begin() + 4);
+  EXPECT_TRUE(hasStmtType(FirstIfBlock));
+
+  CFGBlock *EntryBlock = *(cfg->begin() + 5);
+  EXPECT_EQ(EntryBlock, >getEntry());
+
+  ControlDependencyCalculator Control(cfg);
+
+  EXPECT_TRUE(Control.isControlDependent(SecondThenBlock, SecondIfBlock));
+  EXPECT_TRUE(Control.isControlDependent(SecondIfBlock, FirstIfBlock));
+  EXPECT_FALSE(Control.isControlDependent(NullDerefBlock, SecondIfBlock));
+}
+
+TEST(CFGDominatorTree, ControlDependencyWithLoops) {
+  const char *Code = R"(int test3() {
+  int x,y,z;
+
+  x = y = z = 1;
+  if (x > 0) {
+while (x >= 0){
+  while (y >= x) {
+x = x-1;
+y = y/2;
+  }
+}
+  }
+  z = y;
+
+  return 0;
+})";
+  BuildResult Result = BuildCFG(Code);
+  EXPECT_EQ(BuildResult::BuiltCFG, Result.getStatus());
+
+  //   <- [B2] <-
+  //  /  \
+  // [B8 (ENTRY)] -> [B7] -> [B6] ---> [B5] -> [B4] -> [B3]
+  //   \   | \  /
+  //\  |  <-
+  // \  \
+  //  > [B1] -> [B0 (EXIT)]
+
+  CFG *cfg = Result.getCFG();
+
+  ControlDependencyCalculator Control(cfg);
+
+  auto GetBlock = [cfg] (unsigned Index) -> CFGBlock * {
+assert(Index < cfg->size());
+return *(cfg->begin() + Index);
+  };
+
+  // While not immediately obvious, the second block in fact post dominates the
+  // fifth, hence B5 is not a control dependency of 2.
+  EXPECT_FALSE(Control.isControlDependent(GetBlock(5), GetBlock(2)));
+}
+
+
 } // namespace
 } // namespace analysis
 } // namespace clang
Index: clang/test/Analysis/domtest.cpp
===
--- clang/test/Analysis/domtest.cpp
+++ clang/test/Analysis/domtest.cpp
@@ -1,6 +1,7 @@
 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -analyzer-checker=debug.DumpDominators \
 // RUN:   -analyzer-checker=debug.DumpPostDominators \
+// RUN:   -analyzer-checker=debug.DumpControlDependencies \
 // RUN:   2>&1 | FileCheck %s
 
 bool coin();
@@ -20,7 +21,8 @@
 
 //  [B3 (ENTRY)]  -> [B1] -> [B2] -> [B0 (EXIT)]
 
-// CHECK:  Immediate dominance tree (Node#,IDom#):
+// CHECK:  Control dependencies (Node#,Dependency#):
+// CHECK-NEXT: Immediate dominance tree (Node#,IDom#):
 // CHECK-NEXT: (0,2)
 // CHECK-NEXT: (1,3)
 // CHECK-NEXT: (2,1)
@@ -42,13 +44,18 @@
   }
 }
 
-//> [B2] >
-//   /\
-// [B5 (ENTRY)] -> [B4] -> [B3] ---> 

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:51
+  CFGDominatorTreeImpl(CFG *cfg) {
+buildDominatorTree(cfg);
+  }

DomTree has a constructor that runs the builder -- why not use it directly?



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:160
   /// changes.
   void changeImmediateDominator(CFGBlock *N, CFGBlock *NewIDom) {
 DT.changeImmediateDominator(N, NewIDom);

Do you need it at all? I understand it's a wrapper around dominators, but this 
API is virtually impossible to use safely.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:225
+
+  /// Whether \p A is control dependent of \p B.
+  bool isControlDependent(CFGBlock *A, CFGBlock *B) {

I thinks it should be `A is control dependent on B`


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

https://reviews.llvm.org/D62619



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