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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/CodeGen/CodeGenFunction.h:217
+  /// statements.
+  llvm::SmallVector<bool, 4> LabelSeenStack;
+
----------------
ahatanak wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > Shouldn't this be maintained by some existing scoping structure like LexicalScope?
> > I think I need a structure that gets pushed and popped when we enter and exit a compound statement, but I couldn't find one in CodeGenFunction.
> > 
> > Adding a flag to LexicalScope would fail to disable lifetime markers in some cases. For example, in the code below (which I think is valid), the lifetime markers of "i" wouldn't get disabled because a LexicalScope is pushed at the beginning of CodeGenFunction::EmitForStmt:
> > 
> > ```
> > void foo2(int *);
> > int c;
> > 
> > void foo1(int n) {
> >   int *p = 0;
> > 
> > label1:
> >   foo2(p);
> >   for (int i = 0; i < n; ++i)
> >     if (i == 5)
> >       p = &i;
> > 
> >   if (c)
> >     goto label1;
> > }
> > ```
> > 
> > If we want to avoid introducing new structures to track labels, perhaps we can just check whether a label was seen so far in the current function rather than the current compound statement, but I'm not sure how much impact that would have on performance.
> Alternatively, add a flag to LexicalScope that indicates it was created in EmitCompoundStmtWithoutScope. lifetime markers would be disabled if the flag of the enclosing CompoundStmt's LexicalScope were set.
The C language rule is not tied specifically to compound statements; it is tied to "blocks".  Selection (if/switch) and iteration (for/do/while) statements are also blocks.  So the code is not valid because the lifetime of 'i' is tied to the block of the for statement.  This is basically what we currently track with LexicalScope.

The C++ language rule is similar but stricter.


================
Comment at: lib/CodeGen/CodeGenFunction.h:236
+    LabelSeenStack.back() = true;
+  }
+
----------------
ahatanak wrote:
> rjmccall wrote:
> > A label in a nested scope acts like a label in outer scopes, too.
> I think you are suggesting setting all the flags on the stack rather than just the top (or maybe not?), but I'm not sure why that's necessary. Could you elaborate on your comment a little more and show me a simple example when this code would fail to disable lifetime markers when it should?
> 
> The boolean flag set to true is propagated to the outer scope's flag when we have completed processing the compound statement and LabelSeenRAII's destructor is called. So, for example, if I enclose label1 in function jump_backward_over_declaration of the test case, 
> IRGen disables lifetime markers for "i" even though "i" is declared in the outer compound statement of label1.
Okay.  And since it's not possible to query a non-active scope's flag, this works.


https://reviews.llvm.org/D27680