r293106 - [CodeGen] Suppress emission of lifetime markers if a label has been seen

[Akira Hatanaka via cfe-commits]

Author: ahatanak
Date: Wed Jan 25 16:55:13 2017
New Revision: 293106

URL: http://llvm.org/viewvc/llvm-project?rev=293106&view=rev
Log:
[CodeGen] Suppress emission of lifetime markers if a label has been seen
in the current lexical scope.

clang currently emits the lifetime.start marker of a variable when the
variable comes into scope even though a variable's lifetime starts at
the entry of the block with which it is associated, according to the C
standard. This normally doesn't cause any problems, but in the rare case
where a goto jumps backwards past the variable declaration to an earlier
point in the block (see the test case added to lifetime2.c), it can
cause mis-compilation.

To prevent such mis-compiles, this commit conservatively disables
emitting lifetime variables when a label has been seen in the current
block.

This problem was discussed on cfe-dev here: 
http://lists.llvm.org/pipermail/cfe-dev/2016-July/050066.html

rdar://problem/30153946

Differential Revision: https://reviews.llvm.org/D27680

Modified:
    cfe/trunk/lib/CodeGen/CGDecl.cpp
    cfe/trunk/lib/CodeGen/CodeGenFunction.h
    cfe/trunk/test/CodeGen/lifetime2.c

Modified: cfe/trunk/lib/CodeGen/CGDecl.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDecl.cpp?rev=293106&r1=293105&r2=293106&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CGDecl.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGDecl.cpp Wed Jan 25 16:55:13 2017
@@ -1022,11 +1022,21 @@ CodeGenFunction::EmitAutoVarAlloca(const
       // Emit a lifetime intrinsic if meaningful. There's no point in doing this
       // if we don't have a valid insertion point (?).
       if (HaveInsertPoint() && !IsMSCatchParam) {
-        // goto or switch-case statements can break lifetime into several
-        // regions which need more efforts to handle them correctly. PR28267
-        // This is rare case, but it's better just omit intrinsics than have
-        // them incorrectly placed.
-        if (!Bypasses.IsBypassed(&D)) {
+        // If there's a jump into the lifetime of this variable, its lifetime
+        // gets broken up into several regions in IR, which requires more work
+        // to handle correctly. For now, just omit the intrinsics; this is a
+        // rare case, and it's better to just be conservatively correct.
+        // PR28267.
+        //
+        // We have to do this in all language modes if there's a jump past the
+        // declaration. We also have to do it in C if there's a jump to an
+        // earlier point in the current block because non-VLA lifetimes begin as
+        // soon as the containing block is entered, not when its variables
+        // actually come into scope; suppressing the lifetime annotations
+        // completely in this case is unnecessarily pessimistic, but again, this
+        // is rare.
+        if (!Bypasses.IsBypassed(&D) &&
+            !(!getLangOpts().CPlusPlus && hasLabelBeenSeenInCurrentScope())) {
           uint64_t size = CGM.getDataLayout().getTypeAllocSize(allocaTy);
           emission.SizeForLifetimeMarkers =
               EmitLifetimeStart(size, address.getPointer());

Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=293106&r1=293105&r2=293106&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original)
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Wed Jan 25 16:55:13 2017
@@ -212,6 +212,13 @@ public:
   /// value. This is invalid iff the function has no return value.
   Address ReturnValue;
 
+  /// Return true if a label was seen in the current scope.
+  bool hasLabelBeenSeenInCurrentScope() const {
+    if (CurLexicalScope)
+      return CurLexicalScope->hasLabels();
+    return !LabelMap.empty();
+  }
+
   /// AllocaInsertPoint - This is an instruction in the entry block before which
   /// we prefer to insert allocas.
   llvm::AssertingVH<llvm::Instruction> AllocaInsertPt;
@@ -620,6 +627,10 @@ public:
         rescopeLabels();
     }
 
+    bool hasLabels() const {
+      return !Labels.empty();
+    }
+
     void rescopeLabels();
   };
 

Modified: cfe/trunk/test/CodeGen/lifetime2.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/lifetime2.c?rev=293106&r1=293105&r2=293106&view=diff
==============================================================================
--- cfe/trunk/test/CodeGen/lifetime2.c (original)
+++ cfe/trunk/test/CodeGen/lifetime2.c Wed Jan 25 16:55:13 2017
@@ -1,7 +1,7 @@
-// RUN: %clang -S -emit-llvm -o - -O2 %s | FileCheck %s -check-prefixes=CHECK,O2
-// RUN: %clang -S -emit-llvm -o - -O2 -Xclang -disable-lifetime-markers %s \
+// RUN: %clang_cc1 -S -emit-llvm -o - -O2 -disable-llvm-passes %s | FileCheck %s -check-prefixes=CHECK,O2
+// RUN: %clang_cc1 -S -emit-llvm -o - -O2 -disable-lifetime-markers %s \
 // RUN:       | FileCheck %s -check-prefixes=CHECK,O0
-// RUN: %clang -S -emit-llvm -o - -O0 %s | FileCheck %s -check-prefixes=CHECK,O0
+// RUN: %clang_cc1 -S -emit-llvm -o - -O0 %s | FileCheck %s -check-prefixes=CHECK,O0
 
 extern int bar(char *A, int n);
 
@@ -25,13 +25,11 @@ void no_goto_bypass() {
   char x;
 l1:
   bar(&x, 1);
-  // O2: @llvm.lifetime.start(i64 5
-  // O2: @llvm.lifetime.end(i64 5
   char y[5];
   bar(y, 5);
   goto l1;
   // Infinite loop
-  // O2-NOT: @llvm.lifetime.end(i64 1
+  // O2-NOT: @llvm.lifetime.end(
 }
 
 // CHECK-LABEL: @goto_bypass
@@ -91,3 +89,24 @@ void indirect_jump(int n) {
 L:
   bar(&x, 1);
 }
+
+// O2-LABEL: define i32 @jump_backward_over_declaration(
+// O2-NOT: call void @llvm.lifetime.{{.*}}(i64 4,
+
+extern void foo2(int p);
+
+int jump_backward_over_declaration(int a) {
+  int *p = 0;
+label1:
+  if (p) {
+    foo2(*p);
+    return 0;
+  }
+
+  int i = 999;
+  if (a != 2) {
+    p = &i;
+    goto label1;
+  }
+  return -1;
+}