Page MenuHomePhabricator

[CFG] [analyzer] NFC: Allow more complicated construction contexts.
ClosedPublic

Authored by NoQ on Feb 16 2018, 7:26 PM.

Details

Summary

ConstructionContexts introduced in D42672 are an additional piece of information included with CFGConstructor elements that help the client of the CFG (specifically, the static analyzer - it's off for other clients) to understand where the newly constructed object is stored. For now ConstructionContext was only capable of containing a single statement, such as a DeclStmt (if it's a variable's constructor) or a CXXNewExpr (if it's a heap object) or a CXXCtorInitializer (if it's a field or a base-class instance - this isn't a statement however) or a CXXBindTemporaryExpr (if it's a temporary that requires a destructor call).

This patch continues the effort as planned. In order to handle more complex constructions, richer construction contexts are required. This patch adds two new possibilities (without immediately using any of them):

  • to nest construction contexts into each other, as a linked list that is constructed gradually during AST traversal;
  • to maintain construction contexts for multiple constructors simultaneously.

I'd give an example of how it's planned to be used here, and post a separate revision with actually using them that way. Consider the ternary operator example from http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html:

1    class C {
2    public:
3      C(int) {}
4      ~C() {}
5    };
6
7    void foo(int coin) {
8      const C &c = coin ? C(1) : C(2);
9    }
DeclStmt 0x7fc1e20023e0 <line:8:3, col:34>
`-VarDecl 0x7fc1e2001dc8 <col:3, col:33> col:12 c 'const C &' cinit
  `-ExprWithCleanups 0x7fc1e2002370 <col:16, col:33> 'const C' lvalue
    `-MaterializeTemporaryExpr 0x7fc1e2002340 <col:16, col:33> 'const C' lvalue extended by Var 0x7fc1e2001dc8 'c' 'const C &'
      `-ImplicitCastExpr 0x7fc1e2002328 <col:16, col:33> 'const C' <NoOp>
        `-ConditionalOperator 0x7fc1e20022f8 <col:16, col:33> 'C'
          |-ImplicitCastExpr 0x7fc1e2002170 <col:16> 'bool' <IntegralToBoolean>
          | `-ImplicitCastExpr 0x7fc1e2002158 <col:16> 'int' <LValueToRValue>
          |   `-DeclRefExpr 0x7fc1e2001e28 <col:16> 'int' lvalue ParmVar 0x7fc1e2001c18 'coin' 'int'
          |-CXXBindTemporaryExpr 0x7fc1e2002248 <col:23, col:26> 'C' (CXXTemporary 0x7fc1e2002240)
          | `-CXXConstructExpr 0x7fc1e2002208 <col:23, col:26> 'C' 'void (const C &) noexcept' elidable
          |   `-MaterializeTemporaryExpr 0x7fc1e20021a0 <col:23, col:26> 'const C' lvalue
          |     `-ImplicitCastExpr 0x7fc1e2002188 <col:23, col:26> 'const C' <NoOp>
          |       `-CXXFunctionalCastExpr 0x7fc1e2002078 <col:23, col:26> 'C' functional cast to class C <ConstructorConversion>
          |         `-CXXBindTemporaryExpr 0x7fc1e2002058 <col:23, col:26> 'C' (CXXTemporary 0x7fc1e2002050)
          |           `-CXXConstructExpr 0x7fc1e2002018 <col:23, col:26> 'C' 'void (int)'
          |             `-IntegerLiteral 0x7fc1e2001e60 <col:25> 'int' 1
          `-CXXBindTemporaryExpr 0x7fc1e20022d8 <col:30, col:33> 'C' (CXXTemporary 0x7fc1e20022d0)
            `-CXXConstructExpr 0x7fc1e2002298 <col:30, col:33> 'C' 'void (const C &) noexcept' elidable
              `-MaterializeTemporaryExpr 0x7fc1e2002280 <col:30, col:33> 'const C' lvalue
                `-ImplicitCastExpr 0x7fc1e2002268 <col:30, col:33> 'const C' <NoOp>
                  `-CXXFunctionalCastExpr 0x7fc1e2002130 <col:30, col:33> 'C' functional cast to class C <ConstructorConversion>
                    `-CXXBindTemporaryExpr 0x7fc1e2002110 <col:30, col:33> 'C' (CXXTemporary 0x7fc1e2002108)
                      `-CXXConstructExpr 0x7fc1e20020d0 <col:30, col:33> 'C' 'void (int)'
                        `-IntegerLiteral 0x7fc1e20020b0 <col:32> 'int' 2

Look at the two elidable copy-constructors. As explained in the aforementioned mail, if we want the CFG client to understand how lifetime extension works, we need to include both the surrounding CXXBindTemporaryExpr (0x7fc1e2002248 for the first constructor and 0x7fc1e20022d8 for the second constructor) and the surrounding MaterializeTemporaryExpr (0x7fc1e2002340 for both constructors) in the construction context.

While traversing the AST in findConstructionContexts() (formerly known as EnterConstructionContextIfNecessary()), we'll first encounter MaterializeTemporaryExpr 0x7fc1e2002340. We'll create a ConstructionContext for MaterializeTemporaryExpr and set it as the current ContextSoFar. Then we'll encounter the first CXXBindTemporaryExpr and change ContextSoFar to the new context that has CXXBindTemporaryExpr as the trigger statement and the old context as its parent. Then we'll see the constructor within the CXXBindTemporaryExpr and realize that we've just built its construction context. Then we'd pop out of the CXXBindTemporaryExpr and restore the old MaterializeTemporaryExpr-based context and go to another branch of our ConditionalOperator. Here we'd see the other CXXBindTemporaryExpr, for which we'd do the same thing. With that, both constructors would have their contexts set (simultaneously!), and their contexts would be triggered by their respective CXXBindTemporaryExpr and "previously triggered" (through the parent context) by their common MaterializeTemporaryExpr. Then during normal visit, we'd encounter both constructors and add their CFG elements with their respective construction contexts.

Note that later when we visit these CXXBindTemporaryExprs during normal visit, we'd have to try to see if they form construction contexts for any of their children, which would mean that we'd try to create a construction context for the same constructor twice. This makes it a bit tricky to assert that the algorithm is working correctly; previously we could assert that the construction context is not being set twice. Now we make a weaker assertion, but still a powerful one: when we're trying to set the construction context twice, on our second attempt we should be having a construction context that is a sub-context of the context that we already have. In this case we keep the old context because it provides more informantion.

I also added a new assertion that says that all construction contexts were fully consumed when CFG is fully built.

A similar process is planned to be used for aggregate constructors, where the context may be identified via a chain of multiple InitListExprs (which may also contain multiple constructors).

Because the ConstructionContext class is becoming increasingly complex, i strongly believe i should split it up into sub-classes enumerated by kinds.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Feb 16 2018, 7:26 PM
NoQ updated this revision to Diff 134785.Feb 16 2018, 7:28 PM

Fix comment.

NoQ retitled this revision from [CFG] NFC: Allow more complicated construction contexts. to [CFG] [analyzer] NFC: Allow more complicated construction contexts..Feb 16 2018, 7:32 PM
NoQ added a reviewer: szepet.Feb 16 2018, 7:32 PM
NoQ removed a subscriber: szepet.
dcoughlin accepted this revision.Feb 20 2018, 8:39 PM

It seems kind of sketchy to me that we're recursing over an expression to find construction contexts and then later doing it again for sub-expressions. I guess there is precedent here with VisitForTemporaryDtors(), but we should think about whether there is a better way to do this.

lib/Analysis/CFG.cpp
1160 ↗(On Diff #134785)

This is kind of scary since it means we'll be recursing over subexpressions repeatedly. I guess VisitForTemporaryDtors() does this too, but it sounds like we're wasting a lot of computation visiting the subtrees over and over again.

This revision is now accepted and ready to land.Feb 20 2018, 8:39 PM
This revision was automatically updated to reflect the committed changes.