[PATCH] D15031: CFG: Add CFGElement for automatic variables that leave the scope

[Richard Smith via cfe-commits]

rsmith added a comment.

Please add a test that builds and dumps a CFG and checks it has the right shape. It's generally not sufficient for the only tests for one LLVM project to exist in a different LLVM project. See test/Analysis/cfg.cpp for an example of how to do this.


================
Comment at: include/clang/Analysis/CFG.h:170
@@ -168,1 +169,3 @@
 
+class CFGAutomaticObjLeavesScope : public CFGElement {
+public:
----------------
What is this intended to model? There seem to be a few options here:

 1) The point at which the destructor would run if the object had a non-trivial destructor
 2) The point at which the storage duration for the underlying storage ends
 3) The point at which the lifetime of the object ends

Note that these are all different -- for an object with a non-trivial destructor, (1) and (3) are the same and for any other object (2) and (3) are the same; (2) occurs after all destructors for the scope have run.

This matters for cases like:

  void f() {
    struct A { int *p; ~A() { *p = 0; } } a;
    int n;
    a.p = &n;
  }

Here, the lifetime of `n` would end before the lifetime of `a` if `n` had a non-trivial destructor. If `n`s lifetime actually ended there, this code would have undefined behavior. But because the destruction of `n` is trivial, the lifetime of `n` instead ends when its storage duration ends, which is *after* `A`'s destructor runs, so the code is valid.

Please add a comment to this class to describe what it means.

================
Comment at: include/clang/Analysis/CFG.h:728
@@ +727,3 @@
+
+  // Scope leaving must be performed in reversed order. So insertion is in two
+  // steps. First we prepare space for some number of elements, then we insert
----------------
Why must reverse order be used? It's not possible for the effects of these operations to interact with each other, is it? At least according to the C++ standard, the "end of storage duration" effects all occur simultaneously.

================
Comment at: lib/Analysis/CFG.cpp:1233
@@ +1232,3 @@
+
+/// addAutomaticObjLeavesScope - Add to current block automatic objects thats leave the scope.
+void CFGBuilder::addAutomaticObjLeavesScope(LocalScope::const_iterator B,
----------------
thats -> that

================
Comment at: lib/Analysis/CFG.cpp:1447
@@ -1389,1 +1446,3 @@
+      }
+    return Scope;
   }
----------------
This will sometimes bail out without adding a scope; if both `AddAutomaticObjLeavesScope` and `AddImplicitDtors` are set, this seems like the wrong behavior. You should check for `AddAutomaticObjLeavesScope` first and unconditionally ensure there is a scope for that case.

================
Comment at: lib/Analysis/CFG.cpp:1490-1491
@@ +1489,4 @@
+/// destructors call site.
+/// FIXME: This mechanism for adding automatic destructors doesn't handle
+/// no-return destructors properly.
+void CFGBuilder::prependAutomaticObjLeavesScopeWithTerminator(CFGBlock *Blk,
----------------
This is not a mechanism for adding automatic destructors. This FIXME seems unnecessary.

================
Comment at: lib/Analysis/CFG.cpp:1501
@@ +1500,3 @@
+    InsertPos = Blk->insertAutomaticObjLeavesScope(InsertPos, *I,
+                                            Blk->getTerminator());
+}
----------------
Bad indentation


http://reviews.llvm.org/D15031