[CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[StephenFan]

Summary

Fix https://github.com/llvm/llvm-project/issues/56356
For following test case:

extern int bar(char *A, int n);
void goto_bypass(void) {
  {
    char x;
  l1:
    bar(&x, 1);
  }
  goto l1;
}

And using clang -cc1 -O2 -S -emit-llvm to compile it.

In the past, due to the existence of bypassed label, the lifetime
intrinsic would not be generated. This was also the cause of pr56356.
In this patch, if the variable is bypassed, we do variable
allocation, emit lifetime-start intrinsic and record the lifetime-start
intrinsic in LexicalScope. Then When emitting the bypass label, we emit
the lifetime instrinsic again to make sure the lifetime of the bypassed
variable is start again. Address sanitizer will capture these lifetime
intrinsics and instrument poison and unpoison code. Finally pr56356 can
be resolved.

Here is the new llvm-ir of the test case above.

define dso_local void @goto_bypass() local_unnamed_addr #0 {
entry:
  %x = alloca i8, align 1
  call void @llvm.lifetime.start.p0(i64 1, ptr nonnull %x) #3
  br label %l1

l1:                                               ; preds = %l1, %entry
  call void @llvm.lifetime.start.p0(i64 1, ptr nonnull %x) #3
  %call = call i32 @bar(ptr noundef nonnull %x, i32 noundef 1) #3
  call void @llvm.lifetime.end.p0(i64 1, ptr nonnull %x) #3
  br label %l1
}

[vitalybuka]

looks like check-asan tests fail

[vitalybuka]

clang/lib/CodeGen/CodeGenFunction.h

935

LifetimeStartMarkers -> BypassedLifetimeStartMarkers
and below

if I read this correctly this is not any start marker

clang/test/CodeGen/lifetime2.c

41–42

It assume this will break Msan Transforms/Instrumentation/MemorySanitizer.cpp:1298 as it assume variable is not initialized on start

void goto_bypass(void) {
  {
    char x;
  l1:
    bar(&x, 1);
   if (x)
     goto l1
  }
  goto l1;
}

76–77

Please check for lifetime markers, I assume case 2 will have a new one

[vitalybuka]

clang/test/CodeGen/lifetime2.c

76–77

Please check for lifetime markers, I assume case 2 will have a new one

Please check for *all* lifetime markers

you can add use "FileCheck --implicit-check-not llvm.lifetime" so it will fail if something has no corresponding match

[StephenFan]

1. Record bypassed lifetime start markers in codegenfunction class instead of lexicalscope. Since bypassed variables may be defined when lexicalscope is nullptr. This can also fix test fail of use-after-scope-goto.cpp.
2. Check more lifetime.start or end intrinsics in lifetime2.c test file.

[StephenFan]

clang/test/CodeGen/lifetime2.c

41–42

Yes. I still need some time to see how to deal with it.

76–77

I have checked for all lifetime markers in Diff 444701. What's the point of adding --implicit-check-not llvm.lifetime?

[vitalybuka]

clang/test/CodeGen/lifetime2.c

76–77

Point is to have en error, if lifetime emitted, but we don't have a check for that.
Which is very reasonable for the test.

[vitalybuka]

clang/test/CodeGen/lifetime2.c

76–77

actually converting the test into --implicit-check-not in a separate patch could be nice, so we would be be confused by previously missed markers. https://reviews.llvm.org/D129789

[StephenFan]

1. Rebase.
2. Add a test case of goto_bypass.

[StephenFan]

clang/test/CodeGen/lifetime2.c

76–77

Thanks!

[StephenFan]

clang/test/CodeGen/lifetime2.c

41–42

Candidate solution: D129991

[vitalybuka]

I am not sure what to do with this patch. I really prefer to avoid it to minimize the risk of regressions.

We probably considered/prototyped to insert starts like in this solution then in 2016. But at the time of the solution was a comment by @rsmith https://reviews.llvm.org/D24693#559609
Even if it works for asan, I still don't know how this patch will affect miss-compiles in other places. Dropping intrinsics completely as is must not miscompile.

If the goal to fix asan, then it's extremely rare case, I measured it on llvm codebase and entire Google code. This is not worth of effort to me.
Also HWAsan and MTE, which can be considered asan successors, does not even bother to handle allocas with multiple lifetime start.

[StephenFan]

In D129448#3669296, @vitalybuka wrote:

I am not sure what to do with this patch. I really prefer to avoid it to minimize the risk of regressions.

We probably considered/prototyped to insert starts like in this solution then in 2016. But at the time of the solution was a comment by @rsmith https://reviews.llvm.org/D24693#559609
Even if it works for asan, I still don't know how this patch will affect miss-compiles in other places. Dropping intrinsics completely as is must not miscompile.

If the goal to fix asan, then it's extremely rare case, I measured it on llvm codebase and entire Google code. This is not worth of effort to me.
Also HWAsan and MTE, which can be considered asan successors, does not even bother to handle allocas with multiple lifetime start.

Ok. I'm just fixing this out of interest, and it doesn't look like it's a rigid demand to fix it. If this patch may cause potential regression, I respect your decision.

[vitalybuka]

Abandon?

clang/lib/CodeGen/CGDecl.cpp

void CodeGenFunction::EmitAutoVarDecl(const VarDecl &D) {
   EmitAutoVarInit(emission);
   EmitAutoVarCleanups(emission);
 }
 
 /// Emit a lifetime.begin marker if some criteria are satisfied.
 /// \return a pointer to the temporary size Value if a marker was emitted, null
 /// otherwise
 llvm::Value *CodeGenFunction::EmitLifetimeStart(llvm::TypeSize Size,
-                                                llvm::Value *Addr) {
+                                                llvm::Value *Addr,
+                                                bool isBypassed) {
   if (!ShouldEmitLifetimeMarkers)
     return nullptr;
 
   assert(Addr->getType()->getPointerAddressSpace() ==
              CGM.getDataLayout().getAllocaAddrSpace() &&
          "Pointer should be in alloca address space");
   llvm::Value *SizeV = llvm::ConstantInt::get(
       Int64Ty, Size.isScalable() ? -1 : Size.getFixedValue());
   Addr = Builder.CreateBitCast(Addr, AllocaInt8PtrTy);
   llvm::CallInst *C =
       Builder.CreateCall(CGM.getLLVMLifetimeStartFn(), {SizeV, Addr});
+  if (isBypassed)
+    BypassedLifetimeStartMarkers[CurLexicalScope].push_back(C);
   C->setDoesNotThrow();
   return SizeV;
 }
 
 void CodeGenFunction::EmitLifetimeEnd(llvm::Value *Size, llvm::Value *Addr) {
   assert(Addr->getType()->getPointerAddressSpace() ==
              CGM.getDataLayout().getAllocaAddrSpace() &&
          "Pointer should be in alloca address space");
if (NRVO) {
       // the catch parameter starts in the catchpad instruction, and we can't
       // insert code in those basic blocks.
       bool IsMSCatchParam =
           D.isExceptionVariable() && getTarget().getCXXABI().isMicrosoft();
 
       // 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) {
-        // 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())) {
+        if (!(!getLangOpts().CPlusPlus && hasLabelBeenSeenInCurrentScope())) {
           llvm::TypeSize Size = CGM.getDataLayout().getTypeAllocSize(allocaTy);
-          emission.SizeForLifetimeMarkers =
-              EmitLifetimeStart(Size, AllocaAddr.getPointer());
+          emission.SizeForLifetimeMarkers = EmitLifetimeStart(
+              Size, AllocaAddr.getPointer(), Bypasses.IsBypassed(&D));
         }
       } else {
         assert(!emission.useLifetimeMarkers());
       }
     }
   } else {
     EnsureInsertPoint();
 

clang/lib/CodeGen/CGStmt.cpp

void CodeGenFunction::EmitLabel(const LabelDecl *D) {
   // it from the branch-fixups list.
   } else {
     assert(!Dest.getScopeDepth().isValid() && "already emitted label!");
     Dest.setScopeDepth(EHStack.stable_begin());
     ResolveBranchFixups(Dest.getBlock());
   }
 
   EmitBlock(Dest.getBlock());
+  restartBypassedVariable();
 
   // Emit debug info for labels.
   if (CGDebugInfo *DI = getDebugInfo()) {
     if (CGM.getCodeGenOpts().hasReducedDebugInfo()) {
       DI->setLocation(D->getLocation());
       DI->EmitLabel(D, Builder);
     }
   }

clang/lib/CodeGen/CodeGenFunction.h

 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/OpenMPKinds.h"
 #include "clang/Basic/TargetInfo.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Utils/SanitizerStats.h"
 
 namespace llvm {
 class BasicBlock;
 class LLVMContext;
 class MDNode;
public:
 
   // Cleanup stack depth of the RunCleanupsScope that was pushed most recently.
   EHScopeStack::stable_iterator CurrentCleanupScopeDepth =
       EHScopeStack::stable_end();
 
   class LexicalScope : public RunCleanupsScope {
     SourceRange Range;
     SmallVector<const LabelDecl*, 4> Labels;
     LexicalScope *ParentScope;
 
     LexicalScope(const LexicalScope &) = delete;
     void operator=(const LexicalScope &) = delete;
 
   public:
     /// Enter a new cleanup scope.
     explicit LexicalScope(CodeGenFunction &CGF, SourceRange Range)
       : RunCleanupsScope(CGF), Range(Range), ParentScope(CGF.CurLexicalScope) {
private:
   /// OutermostConditional - Points to the outermost active
   /// conditional control.  This is used so that we know if a
   /// temporary should be destroyed conditionally.
   ConditionalEvaluation *OutermostConditional = nullptr;
 
   /// The current lexical scope.
   LexicalScope *CurLexicalScope = nullptr;
 
+  /// Map from lexical socpe to vector of bypassed lifetime start markers.
+  /// NOTE: If a lifetime start marker isn't defined in any lexical scope(
+  /// defining while CurLexicalScope is nullptr), its key is nullptr;
+  std::map<const LexicalScope *, llvm::SmallVector<llvm::CallInst *, 8>>
+      BypassedLifetimeStartMarkers;
+
+  void restartBypassedVariable();
+
   /// The current source location that should be used for exception
   /// handling code.
   SourceLocation CurEHLocation;
 
   /// BlockByrefInfos - For each __block variable, contains
   /// information about the layout of the variable.
   llvm::DenseMap<const ValueDecl *, BlockByrefInfo> BlockByrefInfos;
 
public:
   void EmitCXXTemporary(const CXXTemporary *Temporary, QualType TempType,
                         Address Ptr);
 
   void EmitSehCppScopeBegin();
   void EmitSehCppScopeEnd();
   void EmitSehTryScopeBegin();
   void EmitSehTryScopeEnd();
 
-  llvm::Value *EmitLifetimeStart(llvm::TypeSize Size, llvm::Value *Addr);
+  llvm::Value *EmitLifetimeStart(llvm::TypeSize Size, llvm::Value *Addr,
+                                 bool isBypassed = false);
   void EmitLifetimeEnd(llvm::Value *Size, llvm::Value *Addr);
 
   llvm::Value *EmitCXXNewExpr(const CXXNewExpr *E);
   void EmitCXXDeleteExpr(const CXXDeleteExpr *E);
 
   void EmitDeleteCall(const FunctionDecl *DeleteFD, llvm::Value *Ptr,
                       QualType DeleteTy, llvm::Value *NumElements = nullptr,
                       CharUnits CookieSize = CharUnits());

clang/lib/CodeGen/CodeGenFunction.cpp

void CodeGenFunction::EmitBlockWithFallThrough(llvm::BasicBlock *BB,
   if (HaveInsertPoint() && CGM.getCodeGenOpts().hasProfileClangInstr()) {
     // When instrumenting for profiling, the fallthrough to certain
     // statements needs to skip over the instrumentation code so that we
     // get an accurate count.
     SkipCountBB = createBasicBlock("skipcount");
     EmitBranch(SkipCountBB);
   }
   EmitBlock(BB);
+  restartBypassedVariable();
+
   uint64_t CurrentCount = getCurrentProfileCount();
   incrementProfileCounter(S);
   setCurrentProfileCount(getCurrentProfileCount() + CurrentCount);
   if (SkipCountBB)
     EmitBlock(SkipCountBB);
 }
 
 /// Tries to mark the given function nounwind based on the
QualType CodeGenFunction::BuildFunctionArgList(GlobalDecl GD,
   }
 
   if (MD && (isa<CXXConstructorDecl>(MD) || isa<CXXDestructorDecl>(MD)))
     CGM.getCXXABI().addImplicitStructorParams(*this, ResTy, Args);
 
   return ResTy;
 }
 
+void CodeGenFunction::restartBypassedVariable() {
+  if (BypassedLifetimeStartMarkers.count(CurLexicalScope))
+    llvm::for_each(BypassedLifetimeStartMarkers[CurLexicalScope],
+                   [this](const llvm::CallInst *LifetimeStartMarker) {
+                     Builder.Insert(LifetimeStartMarker->clone());
+                   });
+}
+
 void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
                                    const CGFunctionInfo &FnInfo) {
   assert(Fn && "generating code for null Function");
   const FunctionDecl *FD = cast<FunctionDecl>(GD.getDecl());
   CurGD = GD;
 
   FunctionArgList Args;
   QualType ResTy = BuildFunctionArgList(GD, Args);
void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
 
   // Emit the standard function epilogue.
   FinishFunction(BodyRange.getEnd());
 
   // If we haven't marked the function nothrow through other means, do
   // a quick pass now to see if we can.
   if (!CurFn->doesNotThrow())
     TryMarkNoThrow(CurFn);
+
+  BypassedLifetimeStartMarkers.clear();
 }
 
 /// ContainsLabel - Return true if the statement contains a label in it.  If
 /// this statement is not executed normally, it not containing a label means
 /// that we can just remove the code.
 bool CodeGenFunction::ContainsLabel(const Stmt *S, bool IgnoreCaseStmts) {
   // Null statement, not a label!
   if (!S) return false;

clang/test/CodeGen/lifetime2.c

l1:
   goto l1;
   // Infinite loop
 }
 
 // CHECK-LABEL: @goto_bypass
 void goto_bypass(void) {
   {
     char x;
+    // O2: @llvm.lifetime.start.p0i8(i64 1
   l1:
+    // O2: @llvm.lifetime.start.p0i8(i64 1
     bar(&x, 1);
+    // O2: @llvm.lifetime.end.p0i8(i64 1
   }
   goto l1;
 }
 
 // CHECK-LABEL: @no_switch_bypass
 void no_switch_bypass(int n) {
   switch (n) {
   case 1: {

 }
 
 // CHECK-LABEL: @switch_bypass
 void switch_bypass(int n) {
   switch (n) {
   case 1:
     n = n;
     char x;
+    // O2: @llvm.lifetime.start.p0i8(i64 1
     bar(&x, 1);
     break;
   case 2:
+    // O2: @llvm.lifetime.start.p0i8(i64 1
     bar(&x, 1);
     break;
+    // O2: @llvm.lifetime.end.p0i8(i64 1
   }
 }
 
 // CHECK-LABEL: @indirect_jump
 void indirect_jump(int n) {
   char x;
+  // O2: @llvm.lifetime.start.p0i8(i64 1
   void *T[] = {&&L};
+  // O2: @llvm.lifetime.start.p0i8(i64 8
   goto *T[n];
 L:
+  // O2: @llvm.lifetime.start.p0i8(i64 1
+  // O2: @llvm.lifetime.start.p0i8(i64 8
   bar(&x, 1);
+  // O2: @llvm.lifetime.end.p0i8(i64 8
+  // O2: @llvm.lifetime.end.p0i8(i64 1
 }
 
 extern void foo2(int p);
 
 // O2-LABEL: @jump_backward_over_declaration(
 int jump_backward_over_declaration(int a) {
   int *p = 0;
 // O2: call void @llvm.lifetime.start.p0i8(

llvm/test/Instrumentation/MemorySanitizer/alloca.ll

 ; CHECK: call void @llvm.lifetime.end
 ; CHECK: call void @llvm.lifetime.start
 ; INLINE-NOT: call void @llvm.memset.p0i8.i64(i8* align 4 {{.*}}, i8 -1, i64 4, i1 false)
 ; CALL-NOT: call void @__msan_poison_stack(i8* {{.*}}, i64 4)
 ; ORIGIN-NOT: call void @__msan_set_alloca_origin4(i8* {{.*}}, i64 4,
 ; KMSAN-NOT: call void @__msan_poison_alloca(i8* {{.*}}, i64 4,
 ; CHECK: call void @llvm.lifetime.end
 
+define void @goto_bypass() sanitize_memory {
+entry:
+  %x = alloca i8, align 1
+  call void @llvm.lifetime.start.p0i8(i64 1, i8* %x)
+  br label %l1
+
+l1:
+  call void @llvm.lifetime.start.p0i8(i64 1, i8* %x)
+  call void @bar(i8* noundef %x, i32 noundef 1)
+  %0 = load i8, i8* %x
+  %tobool = icmp ne i8 %0, 0
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:
+  br label %l1
 
+if.end:
+  call void @llvm.lifetime.end.p0i8(i64 1, i8* %x)
+  br label %l1
+}
+
+; CHECK-LABEL: define void @goto_bypass(
+; CHECK-LABEL: entry:
+; CHECK: %x = alloca i8
+; INLINE: call void @llvm.memset.p0i8.i64(i8* align 1 {{.*}}, i8 -1, i64 1, i1 false)
+; CALL: call void @__msan_poison_stack(i8* {{.*}}, i64 1)
+; ORIGIN: call void @__msan_set_alloca_origin4(i8* {{.*}}, i64 1,
+; KMSAN: call void @__msan_poison_alloca(i8* {{.*}}, i64 1,
+
+; FIXME: It is invalid to instrument here Since variable x may be initialized by callee bar.
+; CHECK-LABEL: l1:
+; INLINE: call void @llvm.memset.p0i8.i64(i8* align 1 {{.*}}, i8 -1, i64 1, i1 false)
+; CALL: call void @__msan_poison_stack(i8* {{.*}}, i64 1)
+; ORIGIN: call void @__msan_set_alloca_origin4(i8* {{.*}}, i64 1,
+; KMSAN: call void @__msan_poison_alloca(i8* {{.*}}, i64 1,
 
+declare void @bar(i8* noundef, i32 noundef)
 declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)