[safestack] Fixup llvm.dbg.value when rewriting unsafe allocas.

[eugenis]

Summary

When moving unsafe allocas to the unsafe stack, dbg.declare intrinsics are
updated to refer to the new location.

This change does the same to dbg.value intrinsics.

[eugenis]

ping

[pcc]

Can you also add a test case for dynamic alloca?

It looks like the same thing could also be done for ASan (but that can be a separate change).

lib/Transforms/Utils/Local.cpp

1276

This won't terminate if there are any non-llvm.dbg.value uses of the value (or if replaceOneDbgValueForAlloca gives up).

[eugenis]

In D20986#457042, @pcc wrote:

Can you also add a test case for dynamic alloca?

Done. I've noticed that this call with Offset=0 does not do anything so I removed it.
I also could not make clang emit a dbg.value for a dynamic alloca because dbg.declare-to-dbg.value optimization is done only for scalar allocas, so I hacked the IR a bit for the test.

lib/Transforms/Utils/Local.cpp

1276

fixed

[pcc]

lib/Transforms/Utils/Local.cpp

1276

Please add negative tests covering both cases.

test/Transforms/SafeStack/debug-loc-dynamic.ll

3

I was confused about what you were trying to test here (given that you removed the call to replaceDbgValueForAlloca for dynamic allocas) until I realized that you were just testing that the IR is unchanged. Can you please explicitly mention this?

test/Transforms/SafeStack/debug-loc2.ll

19

Do you want to match %[[USP]] here?

23

Same here

[eugenis]

debug-loc2.ll test extended to cover the two negative cases.

[pcc]

LGTM

[eugenis]

r272968.
Thanks for the review!

include/llvm/Transforms/Utils/Local.h

 /// Replaces llvm.dbg.declare instruction when the alloca it describes
 /// is replaced with a new value. If Deref is true, an additional DW_OP_deref is
 /// prepended to the expression. If Offset is non-zero, a constant displacement
 /// is added to the expression (after the optional Deref). Offset can be
 /// negative. New llvm.dbg.declare is inserted immediately before AI.
 bool replaceDbgDeclareForAlloca(AllocaInst *AI, Value *NewAllocaAddress,
                                 DIBuilder &Builder, bool Deref, int Offset = 0);
 
+/// Replaces multiple llvm.dbg.value instructions when the alloca it describes
+/// is replaced with a new value. If Offset is non-zero, a constant displacement
+/// is added to the expression (after the mandatory Deref). Offset can be
+/// negative. New llvm.dbg.value instructions are inserted at the locations of
+/// the instructions they replace.
+void replaceDbgValueForAlloca(AllocaInst *AI, Value *NewAllocaAddress,
+                              DIBuilder &Builder, int Offset = 0);
+
 /// Remove all instructions from a basic block other than it's terminator
 /// and any present EH pad instructions.
 unsigned removeAllNonTerminatorAndEHPadInstructions(BasicBlock *BB);
 
 /// Insert an unreachable instruction before the specified
 /// instruction, making it and the rest of the code in the block dead.
 unsigned changeToUnreachable(Instruction *I, bool UseLLVMTrap);
 

lib/CodeGen/SafeStack.cpp

for (AllocaInst *AI : StaticAllocas) {
     Value *Off = IRB.CreateGEP(BasePointer, // BasePointer is i8*
                                ConstantInt::get(Int32Ty, -StaticOffset));
     Value *NewAI = IRB.CreateBitCast(Off, AI->getType(), AI->getName());
     if (AI->hasName() && isa<Instruction>(NewAI))
       cast<Instruction>(NewAI)->takeName(AI);
 
     // Replace alloc with the new location.
     replaceDbgDeclareForAlloca(AI, BasePointer, DIB, /*Deref=*/true, -StaticOffset);
+    replaceDbgValueForAlloca(AI, BasePointer, DIB, -StaticOffset);
     AI->replaceAllUsesWith(NewAI);
     AI->eraseFromParent();
   }
 
   // Re-align BasePointer so that our callees would see it aligned as
   // expected.
   // FIXME: no need to update BasePointer in leaf functions.
   StaticOffset = alignTo(StaticOffset, StackAlignment);

lib/Transforms/Utils/Local.cpp

if (auto *L = LocalAsMetadata::getIfExists(V))
     if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L))
       for (User *U : MDV->users())
         if (DbgDeclareInst *DDI = dyn_cast<DbgDeclareInst>(U))
           return DDI;
 
   return nullptr;
 }
 
+static void DIExprAddDeref(SmallVectorImpl<uint64_t> &Expr) {
+  Expr.push_back(dwarf::DW_OP_deref);
+}
+
+static void DIExprAddOffset(SmallVectorImpl<uint64_t> &Expr, int Offset) {
+  if (Offset > 0) {
+    Expr.push_back(dwarf::DW_OP_plus);
+    Expr.push_back(Offset);
+  } else if (Offset < 0) {
+    Expr.push_back(dwarf::DW_OP_minus);
+    Expr.push_back(-Offset);
+  }
+}
+
+static DIExpression *BuildReplacementDIExpr(DIBuilder &Builder,
+                                            DIExpression *DIExpr, bool Deref,
+                                            int Offset) {
+  if (!Deref && !Offset)
+    return DIExpr;
+  // Create a copy of the original DIDescriptor for user variable, prepending
+  // "deref" operation to a list of address elements, as new llvm.dbg.declare
+  // will take a value storing address of the memory for variable, not
+  // alloca itself.
+  SmallVector<uint64_t, 4> NewDIExpr;
+  if (Deref)
+    DIExprAddDeref(NewDIExpr);
+  DIExprAddOffset(NewDIExpr, Offset);
+  if (DIExpr)
+    NewDIExpr.append(DIExpr->elements_begin(), DIExpr->elements_end());
+  return Builder.createExpression(NewDIExpr);
+}
+
 bool llvm::replaceDbgDeclare(Value *Address, Value *NewAddress,
                              Instruction *InsertBefore, DIBuilder &Builder,
                              bool Deref, int Offset) {
   DbgDeclareInst *DDI = FindAllocaDbgDeclare(Address);
   if (!DDI)
     return false;
   DebugLoc Loc = DDI->getDebugLoc();
   auto *DIVar = DDI->getVariable();
   auto *DIExpr = DDI->getExpression();
   assert(DIVar && "Missing variable");
 
-  if (Deref || Offset) {
-    // Create a copy of the original DIDescriptor for user variable, prepending
-    // "deref" operation to a list of address elements, as new llvm.dbg.declare
-    // will take a value storing address of the memory for variable, not
-    // alloca itself.
-    SmallVector<uint64_t, 4> NewDIExpr;
-    if (Deref)
-      NewDIExpr.push_back(dwarf::DW_OP_deref);
-    if (Offset > 0) {
-      NewDIExpr.push_back(dwarf::DW_OP_plus);
-      NewDIExpr.push_back(Offset);
-    } else if (Offset < 0) {
-      NewDIExpr.push_back(dwarf::DW_OP_minus);
-      NewDIExpr.push_back(-Offset);
-    }
-    if (DIExpr)
-      NewDIExpr.append(DIExpr->elements_begin(), DIExpr->elements_end());
-    DIExpr = Builder.createExpression(NewDIExpr);
-  }
+  DIExpr = BuildReplacementDIExpr(Builder, DIExpr, Deref, Offset);
 
   // Insert llvm.dbg.declare immediately after the original alloca, and remove
   // old llvm.dbg.declare.
   Builder.insertDeclare(NewAddress, DIVar, DIExpr, Loc, InsertBefore);
   DDI->eraseFromParent();
   return true;
 }
 
 bool llvm::replaceDbgDeclareForAlloca(AllocaInst *AI, Value *NewAllocaAddress,
                                       DIBuilder &Builder, bool Deref, int Offset) {
   return replaceDbgDeclare(AI, NewAllocaAddress, AI->getNextNode(), Builder,
                            Deref, Offset);
 }
 
+static void replaceOneDbgValueForAlloca(DbgValueInst *DVI, Value *NewAddress,
+                                        DIBuilder &Builder, int Offset) {
+  DebugLoc Loc = DVI->getDebugLoc();
+  auto *DIVar = DVI->getVariable();
+  auto *DIExpr = DVI->getExpression();
+  assert(DIVar && "Missing variable");
+
+  // This is an alloca-based llvm.dbg.value. The first thing it should do with
+  // the alloca pointer is dereference it. Otherwise we don't know how to handle
+  // it and give up.
+  if (!DIExpr || DIExpr->getNumElements() < 1 ||
+      DIExpr->getElement(0) != dwarf::DW_OP_deref)
+    return;
+
+  // Insert the offset immediately after the first deref.
+  // We could just change the offset argument of dbg.value, but it's unsigned...
+  if (Offset) {
+    SmallVector<uint64_t, 4> NewDIExpr;
+    DIExprAddDeref(NewDIExpr);
+    DIExprAddOffset(NewDIExpr, Offset);
+    NewDIExpr.append(DIExpr->elements_begin() + 1, DIExpr->elements_end());
+    DIExpr = Builder.createExpression(NewDIExpr);
+  }
+
+  Builder.insertDbgValueIntrinsic(NewAddress, DVI->getOffset(), DIVar, DIExpr,
+                                  Loc, DVI);
+  DVI->eraseFromParent();
+}
+
+void llvm::replaceDbgValueForAlloca(AllocaInst *AI, Value *NewAllocaAddress,
+                                    DIBuilder &Builder, int Offset) {
+  if (auto *L = LocalAsMetadata::getIfExists(AI))
+    if (auto *MDV = MetadataAsValue::getIfExists(AI->getContext(), L))
+      for (auto UI = MDV->use_begin(), UE = MDV->use_end(); UI != UE;) {
+        Use &U = *UI++;
+        if (auto *DVI = dyn_cast<DbgValueInst>(U.getUser()))
+          replaceOneDbgValueForAlloca(DVI, NewAllocaAddress, Builder, Offset);
+      }
+}
+
 unsigned llvm::removeAllNonTerminatorAndEHPadInstructions(BasicBlock *BB) {
   unsigned NumDeadInst = 0;
   // Delete the instructions backwards, as it has a reduced likelihood of
   // having to update as many def-use and use-def chains.
   Instruction *EndInst = BB->getTerminator(); // Last not to be deleted.
   while (EndInst != &BB->front()) {
     // Delete the next to last instruction.
     Instruction *Inst = &*--EndInst->getIterator();

test/Transforms/SafeStack/debug-loc-dynamic.ll

+; RUN: opt -safe-stack -S -mtriple=i386-pc-linux-gnu < %s -o - | FileCheck %s
+
+; Test llvm.dbg.value for dynamic allocas moved onto the unsafe stack.
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @f(i32 %n) safestack !dbg !6 {
+entry:
+  tail call void @llvm.dbg.value(metadata i32 %n, i64 0, metadata !11, metadata !14), !dbg !15
+  %0 = zext i32 %n to i64, !dbg !16
+
+; CHECK:  store i8* %[[VLA:.*]], i8** @__safestack_unsafe_stack_ptr
+; CHECK:  tail call void @llvm.dbg.value(metadata i8* %[[VLA]], i64 0, metadata ![[TYPE:.*]], metadata ![[EXPR:.*]])
+; CHECK:  call void @capture({{.*}} %[[VLA]])
+
+  %vla = alloca i8, i64 %0, align 16, !dbg !16
+  tail call void @llvm.dbg.value(metadata i8* %vla, i64 0, metadata !12, metadata !17), !dbg !18
+  call void @capture(i8* nonnull %vla), !dbg !19
+  ret void, !dbg !20
+}
+
+declare void @capture(i8*)
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 3.9.0 (trunk 272832) (llvm/trunk 272831)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "../llvm/1.cc", directory: "/code/build-llvm")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{!"clang version 3.9.0 (trunk 272832) (llvm/trunk 272831)"}
+!6 = distinct !DISubprogram(name: "f", linkageName: "_Z1fi", scope: !1, file: !1, line: 2, type: !7, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !10)
+!7 = !DISubroutineType(types: !8)
+!8 = !{null, !9}
+!9 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!10 = !{!11, !12}
+!11 = !DILocalVariable(name: "n", arg: 1, scope: !6, file: !1, line: 2, type: !9)
+
+; CHECK-DAG: ![[TYPE]] = !DILocalVariable(name: "x",
+!12 = !DILocalVariable(name: "x", scope: !6, file: !1, line: 3, type: !13)
+!13 = !DIBasicType(name: "char", size: 8, align: 8, encoding: DW_ATE_signed_char)
+!14 = !DIExpression()
+!15 = !DILocation(line: 2, column: 12, scope: !6)
+!16 = !DILocation(line: 3, column: 3, scope: !6)
+
+; CHECK-DAG: ![[EXPR]] = !DIExpression(DW_OP_deref)
+!17 = !DIExpression(DW_OP_deref)
+!18 = !DILocation(line: 3, column: 8, scope: !6)
+!19 = !DILocation(line: 4, column: 3, scope: !6)
+!20 = !DILocation(line: 5, column: 1, scope: !6)

test/Transforms/SafeStack/debug-loc2.ll

+; RUN: opt -safe-stack -S -mtriple=i386-pc-linux-gnu < %s -o - | FileCheck %s
+
+; Test llvm.dbg.value for the local variables moved onto the unsafe stack.
+; SafeStack rewrites them relative to the unsafe stack pointer (base address of
+; the unsafe stack frame).
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline safestack uwtable
+define void @f() #0 !dbg !6 {
+entry:
+; CHECK:   %[[USP:.*]] = load i8*, i8** @__safestack_unsafe_stack_ptr
+  %x1 = alloca i32, align 4
+  %x2 = alloca i32, align 4
+  %0 = bitcast i32* %x1 to i8*, !dbg !13
+  %1 = bitcast i32* %x2 to i8*, !dbg !14
+
+; CHECK: call void @llvm.dbg.value(metadata i8* %unsafe_stack_ptr, i64 0, metadata ![[X1:.*]], metadata ![[X1_EXPR:.*]])
+  tail call void @llvm.dbg.value(metadata i32* %x1, i64 0, metadata !10, metadata !15), !dbg !16
+  call void @capture(i32* nonnull %x1), !dbg !17
+
+; CHECK: call void @llvm.dbg.value(metadata i8* %unsafe_stack_ptr, i64 0, metadata ![[X2:.*]], metadata ![[X2_EXPR:.*]])
+  call void @llvm.dbg.value(metadata i32* %x2, i64 0, metadata !12, metadata !15), !dbg !18
+  call void @capture(i32* nonnull %x2), !dbg !19
+  ret void, !dbg !20
+}
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.lifetime.start(i64, i8* nocapture) #1
+
+declare void @capture(i32*) #2
+
+; Function Attrs: argmemonly nounwind
+declare void @llvm.lifetime.end(i64, i8* nocapture) #1
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #3
+
+attributes #0 = { noinline safestack uwtable "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { argmemonly nounwind }
+attributes #2 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #3 = { nounwind readnone }
+attributes #4 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 3.9.0 (trunk 271022) (llvm/trunk 271027)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "../llvm/2.cc", directory: "/code/build-llvm")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{!"clang version 3.9.0 (trunk 271022) (llvm/trunk 271027)"}
+!6 = distinct !DISubprogram(name: "f", linkageName: "_Z1fv", scope: !1, file: !1, line: 4, type: !7, isLocal: false, isDefinition: true, scopeLine: 4, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !9)
+!7 = !DISubroutineType(types: !8)
+!8 = !{null}
+!9 = !{!10, !12}
+
+; CHECK-DAG: ![[X1]] = !DILocalVariable(name: "x1",
+!10 = !DILocalVariable(name: "x1", scope: !6, file: !1, line: 5, type: !11)
+!11 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+
+; CHECK-DAG: ![[X2]] = !DILocalVariable(name: "x2",
+!12 = !DILocalVariable(name: "x2", scope: !6, file: !1, line: 6, type: !11)
+!13 = !DILocation(line: 5, column: 3, scope: !6)
+!14 = !DILocation(line: 6, column: 3, scope: !6)
+
+; CHECK-DAG: ![[X1_EXPR]] = !DIExpression(DW_OP_deref, DW_OP_minus, 4)
+; CHECK-DAG: ![[X2_EXPR]] = !DIExpression(DW_OP_deref, DW_OP_minus, 8)
+!15 = !DIExpression(DW_OP_deref)
+!16 = !DILocation(line: 5, column: 7, scope: !6)
+!17 = !DILocation(line: 8, column: 3, scope: !6)
+!18 = !DILocation(line: 6, column: 7, scope: !6)
+!19 = !DILocation(line: 9, column: 3, scope: !6)
+!20 = !DILocation(line: 10, column: 1, scope: !6)
+!21 = !DILocation(line: 10, column: 1, scope: !22)
+!22 = !DILexicalBlockFile(scope: !6, file: !1, discriminator: 1)
+
+