Append CXXDefaultInitExpr's wrapped expression to the CFG when visiting a constructor initializer

[epertoso]

Summary

This patch is part of http://llvm-reviews.chandlerc.com/D2181.

In-class initializers are appended to the CFG when CFGBuilder::addInitializer is called.

[epertoso]

+cfe-commits

[jordan_rose]

This doesn't actually help the analyzer yet: the init expression will be evaluated, but then the result will be thrown away and the old constant-evaluation mechanism will be used instead (see `ExprEngine::Visit`). I'm okay with you not worrying about this for now.

I do approve of the comment change, though.

lib/StaticAnalyzer/Core/BugReporter.cpp

251–261 ↗

(On Diff #6000)

Usually we do this using ParentMap so that it's a linear search. We'd have to start adding the children of the CXXDefaultInitExpr to the ParentMap, though.

282–294 ↗

(On Diff #6000)

This definitely needs testing. The whole point of this section is to make Xcode arrows prettier, so without a test case that actually includes a plist it's kind of useless.

If you have Xcode, you can test the new arrows by overriding the analyzer binary with the not-so-secret setting CLANG_ANALYZER_EXEC. If not, don't worry about the arrows for now; I'll get to them soon.

[epertoso]

lib/StaticAnalyzer/Core/BugReporter.cpp

251–261 ↗

(On Diff #6000)

We don't actually need to know a statement's parent, I used a llvm::DenseSet instead. Unlike the ParentMap, it can be cleared.

282–294 ↗

(On Diff #6000)

test/Analysis/edges-new.mm is actually a plist based test and without changes to this section it was failing (a CXXDefaultInitExpr may now include the field initializer, so it may spawn more than a PathDiagnosticControlFlowPiece).

I modified it a little bit (inserting a conditional operator in edges-new.mm at line 580) to make sure that there's no arrow pointing to (or starting from) the actual init expression.

[jordan_rose]

lib/StaticAnalyzer/Core/BugReporter.cpp

251–261 ↗

(On Diff #6000)

I'm not sure why this is an improvement. You don't even get to short-circuit here, and you rebuild the set every time a path piece goes through this default init expr. What if the same constructor is called several times in the course of analyzing a top-level function?

282–294 ↗

(On Diff #6000)

What I mean is that we need some positive tests here, such as tests where an error happens within the default expr. If you change a plist test without the plist output changing, you're very likely not testing anything new.

[epertoso]

lib/StaticAnalyzer/Core/BugReporter.cpp

251–261 ↗

(On Diff #6000)

I'm using the ParentMap returned by AnalysisDeclContext::getParentMap() now. I think it makes more sense.
Do you see any problem with that?

282–294 ↗

(On Diff #6000)

I added a null-pointer dereference at line 594 in edges-new.mm. This dereference happens in the default initializer expression when the value of another variable is set in the constructor.

The static analyzer now correctly detects the null-pointer dereference on that line. I'm still not 100% happy because, in the presence of multiple constructors, it's not immediately clear which one of them leads to the warning, but at least it won't go unnoticed.

There were indeed edges to the in-class initializer: I'm not sure what the correct behaviour should be in this regard. I tried to be conservative and the current patch prevents them from appearing.

[jordan_rose]

Ah, thanks! But hm...that's not a very good path. We should definitely be allowing edges within the initializer (so we can see that the reason we choose the null pointer path is because a != 1.

If you don't want to deal with this right now, you can just add another configuration option to the CFG and have the analyzer currently not include these nodes. I'll take it back out once we've got the analyzer properly handling this as well.

[epertoso]

Hi,
sorry for my late reply.

Regarding to events appearing in an in-class initializer expression, we have now two simple approaches: to add an option for the CFG builder (as you suggested), or to let the code remove all the edges excluding those appearing within the expression wrapped by the CXXDefaultInitExpr.

Currently (at least until r199843), those events will not be reported (either in Xcode or in the command line). The first approach preserves this behaviour.
{F24874} (screenshot: http://llvm-reviews.chandlerc.com/file/data/lqyk5vypoxtfxvvzbj6m/PHID-FILE-3fxx2h44c3u33vhpjvtg/).

With the second approach, the output is ambiguous in the presence of multiple constructors.
{F24876} (screenshot: http://llvm-reviews.chandlerc.com/file/data/43y6wcdmgsstwxsoo7aq/PHID-FILE-wwyqyn2u2pfnot4f3gue/)

I still prefer to see that warning. If anything more complex than the second approach is required, I'd prefer to keep it separate from this patch and follow the first one. Any suggestions?

[jordan_rose]

Yeah, I'm not yet happy with the second approach. Notice how there are still no edges for the in-class initializer of Foo, even though that's clearly an important part of the path that leads to the null dereference. I don't want to block the main part of your patch (improving -Wuninitialized), so let's go with the option for now, and then do it right after.

[epertoso]

OK, this just became simpler.

[jordan_rose]

This is fine but it doesn't turn it on for anybody. Unless Richard has any objections, I think it's fine to turn this on for -Wuninitialized, just not for the analyzer. It looks like one good place to do this is in AnalysisManager, which is only used by the analyzer.

lib/Analysis/CFG.cpp

773–775

I would do this using nested ifs but otherwise this looks good.

[epertoso]

lib/Analysis/CFG.cpp

773–775

Done.

[jordan_rose]

This looks good to me. Do you have commit access, or do you need one of us to commit it for you?

[epertoso]

No, I don't have commit access.

Although the tests are passing, I'd still like to clarify the
unreachable code warnings:

The code in Richard's example doesn't generate a warning. Should it?

I could also try to spend another couple of days to see how the
unreachable code warnings are generated and make sure we're not doing
it when we shouldn't.

[epertoso]

Updated.

include/clang/Analysis/CFG.h

public:
 
     bool PruneTriviallyFalseEdges;
     bool AddEHEdges;
     bool AddInitializers;
     bool AddImplicitDtors;
     bool AddTemporaryDtors;
     bool AddStaticInitBranches;
     bool AddCXXNewAllocator;
+    bool AddCXXDefaultInitExprInCtors;
 
     bool alwaysAdd(const Stmt *stmt) const {
       return alwaysAddMask[stmt->getStmtClass()];
     }
 
     BuildOptions &setAlwaysAdd(Stmt::StmtClass stmtClass, bool val = true) {
       alwaysAddMask[stmtClass] = val;
       return *this;
     }
 
     BuildOptions &setAllAlwaysAdd() {
       alwaysAddMask.set();
       return *this;
     }
 
     BuildOptions()
     : forcedBlkExprs(0), PruneTriviallyFalseEdges(true)
       ,AddEHEdges(false)
       ,AddInitializers(false)
       ,AddImplicitDtors(false)
       ,AddTemporaryDtors(false)
       ,AddStaticInitBranches(false)
-      ,AddCXXNewAllocator(false) {}
+      ,AddCXXNewAllocator(false)
+      ,AddCXXDefaultInitExprInCtors(false) {}
   };
 
   /// \brief Provides a custom implementation of the iterator class to have the
   /// same interface as Function::iterator - iterator returns CFGBlock
   /// (not a pointer to CFGBlock).
   class graph_iterator {
   public:
     typedef const CFGBlock                  value_type;

lib/Analysis/CFG.cpp

CFGBlock *CFGBuilder::addInitializer(CXXCtorInitializer *I) {
   appendInitializer(Block, I);
 
   if (Init) {
     if (HasTemporaries) {
       // For expression with temporaries go directly to subexpression to omit
       // generating destructors for the second time.
       return Visit(cast<ExprWithCleanups>(Init)->getSubExpr());
     }
+    CXXDefaultInitExpr *Default;
+    if (BuildOpts.AddCXXDefaultInitExprInCtors &&
+        (Default = dyn_cast<CXXDefaultInitExpr>(Init))) {
+      // In general, appending the expression wrapped by a CXXDefaultInitExpr
+      // may cause the same Expr to appear more than once in the CFG. Doing it
+      // here is safe because there's only one initializer per field.
+      autoCreateBlock();
+      appendStmt(Block, Default);
+      if (Stmt *Child = Default->getExpr())
+        if (CFGBlock *R = Visit(Child))
+          Block = R;
+      return Block;
+    }
     return Visit(Init);
   }
 
   return Block;
 }
 
 /// \brief Retrieve the type of the temporary object whose lifetime was 
 /// extended by a local reference with the given initializer.