[CFG] Fix an assertion failure with static initializers

[xazax.hun]

Summary

The branches protecting the static initializers have a DeclStmt as last Stmt. Since it is not an expression that will trigger an assertion failure in CFGBlock::getLastCondition.
This is triggered by the lifetime analysis that is currently in a fork. I am not sure what would be the best way to test this change here.

[Charusso]

clang/lib/Analysis/CFG.cpp

5923

What about the following?:

if (const auto *E = dyn_cast<Expr>(StmtElem->getStmt()))
  return E->IgnoreParens();
return nullptr;

[Szelethus]

I am not sure what would be the best way to test this change here.

I'll admit, I'm a bit out of touch with Clang and will unfortunately be for a while, but the short answer is that the unit tests we have for the CFG are kinda bad. The constructed CFG has a longer lifetime than the AST its based on (see: D63538), so we would need to rewrite the BuildCFG() function that creates it. I have some memories digging into it but always got distracted :)

Until then, could you please include the crash you mentioned through a regular lit test?

clang/lib/Analysis/CFG.cpp

5925–5926

I guess its time to update and/or move this comment a few lines up, maybe even add a descriptive assert.

[NoQ]

I am not sure what would be the best way to test this change here.

A unittest, if you're brave :)

clang/lib/Analysis/CFG.cpp

5923

Not fail-fast enough.

[NoQ]

Wait, i didn't real @Szelethus's reply :)

In any case, i'd love to see an example of such CFG and whether control dependency tracking can expose the crash.

[xazax.hun]

I decided to fix the unittests. Having CFGs full of dangling pointers to the AST does not look fun at all, so I think this change was long overdue.

[Szelethus]

In D71791#1795093, @xazax.hun wrote:

I decided to fix the unittests. Having CFGs full of dangling pointers to the AST does not look fun at all, so I think this change was long overdue.

Very much appreciated! LGTM!

[NoQ]

Woohoo tests!~~

clang/unittests/Analysis/CFGBuildResult.h

69

It should be slightly more visually appealing to construct the AST before the callback and pass it into the constructor of the callback.

clang/lib/Analysis/CFG.cpp

const Expr *CFGBlock::getLastCondition() const {
   if (size() == 0)
     return nullptr;
 
   auto StmtElem = rbegin()->getAs<CFGStmt>();
   if (!StmtElem)
     return nullptr;
 
   const Stmt *Cond = StmtElem->getStmt();
-  if (isa<ObjCForCollectionStmt>(Cond))
+  if (isa<ObjCForCollectionStmt>(Cond) || isa<DeclStmt>(Cond))
     return nullptr;
 
   // Only ObjCForCollectionStmt is known not to be a non-Expr terminator, hence
   // the cast<>.
   return cast<Expr>(Cond)->IgnoreParens();
 }
 
 Stmt *CFGBlock::getTerminatorCondition(bool StripParens) {
   Stmt *Terminator = getTerminatorStmt();
   if (!Terminator)
     return nullptr;
 

clang/unittests/Analysis/CFGBuildResult.h

enum Status {
     BuiltCFG,
   };
 
   BuildResult(Status S, std::unique_ptr<CFG> Cfg = nullptr)
       : S(S), Cfg(std::move(Cfg)) {}
 
   Status getStatus() const { return S; }
   CFG *getCFG() const { return Cfg.get(); }
+  ASTUnit *getAST() const { return AST.get(); }
+  void setAST(std::unique_ptr<ASTUnit> AST) { this->AST = std::move(AST); }
 
 private:
   Status S;
   std::unique_ptr<CFG> Cfg;
+  std::unique_ptr<ASTUnit> AST;
 };
 
 class CFGCallback : public ast_matchers::MatchFinder::MatchCallback {
 public:
   BuildResult TheBuildResult = BuildResult::ToolRan;
+  CFG::BuildOptions Options;
 
   void run(const ast_matchers::MatchFinder::MatchResult &Result) override {
     const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>("func");
     Stmt *Body = Func->getBody();
     if (!Body)
       return;
     TheBuildResult = BuildResult::SawFunctionBody;
-    CFG::BuildOptions Options;
     Options.AddImplicitDtors = true;
     if (std::unique_ptr<CFG> Cfg =
             CFG::buildCFG(nullptr, Body, Result.Context, Options))
       TheBuildResult = {BuildResult::BuiltCFG, std::move(Cfg)};
   }
 };
 
-inline BuildResult BuildCFG(const char *Code) {
+inline BuildResult BuildCFG(const char *Code, CFG::BuildOptions Options = {}) {
   CFGCallback Callback;
+  Callback.Options = Options;
 
   ast_matchers::MatchFinder Finder;
   Finder.addMatcher(ast_matchers::functionDecl().bind("func"), &Callback);
-  std::unique_ptr<tooling::FrontendActionFactory> Factory(
-      tooling::newFrontendActionFactory(&Finder));
   std::vector<std::string> Args = {"-std=c++11",
                                    "-fno-delayed-template-parsing"};
-  if (!tooling::runToolOnCodeWithArgs(Factory->create(), Code, Args))
+  std::unique_ptr<ASTUnit> AST = tooling::buildASTFromCodeWithArgs(Code, Args);
+  if (!AST)
     return BuildResult::ToolFailed;
+  Finder.matchAST(AST->getASTContext());
+  Callback.TheBuildResult.setAST(std::move(AST));
   return std::move(Callback.TheBuildResult);
 }
 
 } // namespace analysis
 } // namespace clang

clang/unittests/Analysis/CFGTest.cpp

const char *Code = "class Foo;\n"
                      "template <typename T>\n"
                      "void f(const T &Range) {\n"
                      "  for (const Foo *TheFoo : Range) {\n"
                      "  }\n"
                      "}\n";
   EXPECT_EQ(BuildResult::SawFunctionBody, BuildCFG(Code).getStatus());
 }
 
+TEST(CFG, StaticInitializerLastCondition) {
+  const char *Code = "void f() {\n"
+                     "  int i = 5 ;\n"
+                     "  static int j = 3 ;\n"
+                     "}\n";
+  CFG::BuildOptions Options;
+  Options.AddStaticInitBranches = true;
+  Options.setAllAlwaysAdd();
+  BuildResult B = BuildCFG(Code, Options);
+  EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus());
+  EXPECT_EQ(1, B.getCFG()->getEntry().succ_size());
+  CFGBlock *Block = *B.getCFG()->getEntry().succ_begin();
+  EXPECT_TRUE(isa<DeclStmt>(Block->getTerminatorStmt()));
+  EXPECT_EQ(nullptr, Block->getLastCondition());
+}
+
 // Constructing a CFG containing a delete expression on a dependent type should
 // not crash.
 TEST(CFG, DeleteExpressionOnDependentType) {
   const char *Code = "template<class T>\n"
                      "void f(T t) {\n"
                      "  delete t;\n"
                      "}\n";
   EXPECT_EQ(BuildResult::BuiltCFG, BuildCFG(Code).getStatus());