[PATCH] D27680: [CodeGen] Move lifetime.start of a variable when goto jumps back past its declaration

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: lib/CodeGen/CGDecl.cpp:917
+      if (!InsertPt)
+        Builder.SetInsertPoint(BB, BB->begin());
+      // If InsertPt is a terminator, insert it before InsertPt.
----------------
ahatanak wrote:
> rjmccall wrote:
> > BB->begin() is not necessarily a legal place to insert a call.  This could happen if e.g. a scope was unconditionally entered after a statement including a ternary expression.
> > 
> > Also, I think lexical scopes don't necessarily have an active basic block upon entry, because their entry can be unreachable.  This happens most importantly with e.g. switch statements, but can also happen with goto or simply with unreachable code (which it's still important to not crash on).
> I'm not sure why BB->begin() wouldn't be the right place to insert a call when a scope is entered after a ternary expression. Is it because EmitConditionalOperatorLValue inserts a phi at the beginning of block "cond.end"?
> 
> Also, I'm not sure when goto or unreachable code might be mis-compiled. Could you elaborate further on that? I can see now why lifetime.start can be inserted at a wrong location when lowering switch statements because EmitSwitchStmt creates and inserts a SwitchInst before a lexical scope is entered, but I'm not sure about the other two cases you mentioned.
Also, I realized this patch doesn't always insert lifetime.start at the beginning of the block the variable is associated with. For example, when the following code is compiled, lifetime.start for "i" is inserted after the call to foo2, but it should be inserted before the call (at the beginning of the function):

```
void foo1(int a) {
  foo2();
  for (int i = 0; i < 10; ++i)
    foo3();
}
```

I'll fix this in my next patch.


https://reviews.llvm.org/D27680