Fix SafeStack debug locations

[eugenis]

Summary

This change tweaks debug locations for the local variables moved onto the unsafe stack so that they don't get "optimized out".

Instead of describing these locations via an IR temp that is likely to be optimized out (even at -O0!), we base the locations off the unsafe stack pointer which is always present. This required a few tweaks in other parts of LLVM:

1. The location expression changed from *tmp to *tmp - offset, hence the need to support DW_OP_minus in the AsmPrinter.
2. SDAG plainly refuses to emit debug locations on anything other that an alloca or a function parameter. I could not find any adverse effects from allowing those on any IR temps, and it works perfectly in our case, when the temp is a load from some thread-local variable. FastISel already supports that.

With this, debug locations for the unsafe allocas seem to reliably survive -O2.

[eugenis]

This change depends on http://reviews.llvm.org/D13230.

[eugenis]

Rebased on top of D13230.
Verified that this CL passes the gdb test suite.
PTAL

[samsonov]

Nice!

include/llvm/Transforms/Utils/Local.h

276

Please elaborate what's actually going on - whether you rewrite expr to be (*orig + offset), or *(orig + offset) - it's not entirely clear from comment

lib/IR/DebugInfoMetadata.cpp

519

This looks weird to me: why don't we use std::advance(I, I->getSize()) instead?

[samsonov]

LGTM

include/llvm/Transforms/Utils/Local.h

276

Please disregard this, I probably misread the new comment.

lib/IR/DebugInfoMetadata.cpp

519

That's fine: expr_op iterator will handle this.

[eugenis]

Committed as r248933

include/llvm/Transforms/Utils/Local.h

 /// of llvm.dbg.value intrinsics.
 bool LowerDbgDeclare(Function &F);
 
 /// FindAllocaDbgDeclare - Finds the llvm.dbg.declare intrinsic corresponding to
 /// an alloca, if any.
 DbgDeclareInst *FindAllocaDbgDeclare(Value *V);
 
 /// \brief Replaces llvm.dbg.declare instruction when an alloca is replaced with
-/// a new value.  If Deref is true, tan additional DW_OP_deref is prepended to
-/// the expression.
+/// 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.
 bool replaceDbgDeclareForAlloca(AllocaInst *AI, Value *NewAllocaAddress,
-                                DIBuilder &Builder, bool Deref);
+                                DIBuilder &Builder, bool Deref, int Offset = 0);
 
 /// Replace 'BB's terminator with one that does not have an unwind successor
 /// block.  Rewrites `invoke` to `call`, `catchendpad unwind label %foo` to
 /// `catchendpad unwind to caller`, etc.  Updates any PHIs in unwind successor.
 ///
 /// \param BB  Block whose terminator will be replaced.  Its terminator must
 ///            have an unwind successor.
 void removeUnwindEdge(BasicBlock *BB);

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

bool DwarfExpression::AddMachineRegExpression(const DIExpression *Expr,
   switch (I->getOp()) {
   case dwarf::DW_OP_bit_piece: {
     unsigned OffsetInBits = I->getArg(0);
     unsigned SizeInBits   = I->getArg(1);
     // Piece always comes at the end of the expression.
     return AddMachineRegPiece(MachineReg, SizeInBits,
                getOffsetOrZero(OffsetInBits, PieceOffsetInBits));
   }
-  case dwarf::DW_OP_plus: {
-    // [DW_OP_reg,Offset,DW_OP_plus,DW_OP_deref] --> [DW_OP_breg,Offset].
+  case dwarf::DW_OP_plus:
+  case dwarf::DW_OP_minus: {
+    // [DW_OP_reg,Offset,DW_OP_plus, DW_OP_deref] --> [DW_OP_breg, Offset].
+    // [DW_OP_reg,Offset,DW_OP_minus,DW_OP_deref] --> [DW_OP_breg,-Offset].
     auto N = I.getNext();
     if (N != E && N->getOp() == dwarf::DW_OP_deref) {
       unsigned Offset = I->getArg(0);
-      ValidReg = AddMachineRegIndirect(MachineReg, Offset);
+      ValidReg = AddMachineRegIndirect(
+          MachineReg, I->getOp() == dwarf::DW_OP_plus ? Offset : -Offset);
       std::advance(I, 2);
       break;
     } else
       ValidReg = AddMachineRegPiece(MachineReg);
   }
   case dwarf::DW_OP_deref: {
       // [DW_OP_reg,DW_OP_deref] --> [DW_OP_breg].
       ValidReg = AddMachineRegIndirect(MachineReg);
case dwarf::DW_OP_bit_piece: {
       unsigned SizeInBits   = I->getArg(1);
       AddOpPiece(SizeInBits, getOffsetOrZero(OffsetInBits, PieceOffsetInBits));
       break;
     }
     case dwarf::DW_OP_plus:
       EmitOp(dwarf::DW_OP_plus_uconst);
       EmitUnsigned(I->getArg(0));
       break;
+    case dwarf::DW_OP_minus:
+      // There is no OP_minus_uconst.
+      EmitOp(dwarf::DW_OP_constu);
+      EmitUnsigned(I->getArg(0));
+      EmitOp(dwarf::DW_OP_minus);
+      break;
     case dwarf::DW_OP_deref:
       EmitOp(dwarf::DW_OP_deref);
       break;
     default:
       llvm_unreachable("unhandled opcode found in expression");
     }
   }
 }

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

if (N.getNode()) {
               Variable, Expression, FINode->getIndex(), 0, dl, SDNodeOrder);
         else {
           // Address is an argument, so try to emit its dbg value using
           // virtual register info from the FuncInfo.ValueMap.
           EmitFuncArgumentDbgValue(Address, Variable, Expression, dl, 0, false,
                                    N);
           return nullptr;
         }
-      } else if (AI)
+      } else {
         SDV = DAG.getDbgValue(Variable, Expression, N.getNode(), N.getResNo(),
                               true, 0, dl, SDNodeOrder);
-      else {
-        // Can't do anything with other non-AI cases yet.
-        DEBUG(dbgs() << "Dropping debug info for " << DI << "\n");
-        DEBUG(dbgs() << "non-AllocaInst issue for Address: \n\t");
-        DEBUG(Address->dump());
-        return nullptr;
       }
       DAG.AddDbgValue(SDV, N.getNode(), isParameter);
     } else {
       // If Address is an argument then try to emit its dbg value using
       // virtual register info from the FuncInfo.ValueMap.
       if (!EmitFuncArgumentDbgValue(Address, Variable, Expression, dl, 0, false,
                                     N)) {
         // If variable is pinned by a alloca in dominating bb then

lib/IR/DebugInfoMetadata.cpp

DIExpression *DIExpression::getImpl(LLVMContext &Context,
   DEFINE_GETIMPL_STORE_NO_OPS(DIExpression, (Elements));
 }
 
 unsigned DIExpression::ExprOperand::getSize() const {
   switch (getOp()) {
   case dwarf::DW_OP_bit_piece:
     return 3;
   case dwarf::DW_OP_plus:
+  case dwarf::DW_OP_minus:
     return 2;
   default:
     return 1;
   }
 }
 
 bool DIExpression::isValid() const {
   for (auto I = expr_op_begin(), E = expr_op_end(); I != E; ++I) {
     // Check that there's space for the operand.
     if (I->get() + I->getSize() > E->get())
       return false;
 
     // Check that the operand is valid.
     switch (I->getOp()) {
     default:
       return false;
     case dwarf::DW_OP_bit_piece:
       // Piece expressions must be at the end.
       return I->get() + I->getSize() == E->get();
     case dwarf::DW_OP_plus:
+    case dwarf::DW_OP_minus:
     case dwarf::DW_OP_deref:
       break;
     }
   }
   return true;
 }
 
 bool DIExpression::isBitPiece() const {

lib/Transforms/Instrumentation/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, NewAI, DIB, /*Deref=*/true);
+    replaceDbgDeclareForAlloca(AI, BasePointer, DIB, /*Deref=*/true, -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 = RoundUpToAlignment(StaticOffset, StackAlignment);

lib/Transforms/Utils/Local.cpp

if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L))
       for (User *U : MDV->users())
         if (DbgDeclareInst *DDI = dyn_cast<DbgDeclareInst>(U))
           return DDI;
 
   return nullptr;
 }
 
 bool llvm::replaceDbgDeclareForAlloca(AllocaInst *AI, Value *NewAllocaAddress,
-                                      DIBuilder &Builder, bool Deref) {
+                                      DIBuilder &Builder, bool Deref, int Offset) {
   DbgDeclareInst *DDI = FindAllocaDbgDeclare(AI);
   if (!DDI)
     return false;
   DebugLoc Loc = DDI->getDebugLoc();
   auto *DIVar = DDI->getVariable();
   auto *DIExpr = DDI->getExpression();
   assert(DIVar && "Missing variable");
 
-  if (Deref) {
+  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);
   }
 
   // Insert llvm.dbg.declare immediately after the original alloca, and remove
   // old llvm.dbg.declare.
   Builder.insertDeclare(NewAllocaAddress, DIVar, DIExpr, Loc,

test/DebugInfo/Generic/dw_op_minus.ll

+; Test dwarf codegen of DW_OP_minus.
+; RUN: llc -O0 -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck %s
+
+; This was built by compiling the following source with SafeStack and
+; simplifying the result a little.
+; extern "C" {
+; void Capture(int *);
+; void f() {
+;   int buf[100];
+;   Capture(buf);
+; }
+; }
+; The interesting part is !DIExpression(DW_OP_deref, DW_OP_minus, 400)
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@__safestack_unsafe_stack_ptr = external thread_local(initialexec) global i8*
+
+define void @f() {
+entry:
+  %unsafe_stack_ptr = load i8*, i8** @__safestack_unsafe_stack_ptr
+  %unsafe_stack_static_top = getelementptr i8, i8* %unsafe_stack_ptr, i32 -400
+  store i8* %unsafe_stack_static_top, i8** @__safestack_unsafe_stack_ptr
+  %0 = getelementptr i8, i8* %unsafe_stack_ptr, i32 -400
+  %buf = bitcast i8* %0 to [100 x i32]*
+  %1 = bitcast [100 x i32]* %buf to i8*, !dbg !16
+  call void @llvm.dbg.declare(metadata i8* %unsafe_stack_ptr, metadata !8, metadata !17), !dbg !18
+  %arraydecay = getelementptr inbounds [100 x i32], [100 x i32]* %buf, i64 0, i64 0, !dbg !19
+  call void @Capture(i32* %arraydecay), !dbg !20
+  store i8* %unsafe_stack_ptr, i8** @__safestack_unsafe_stack_ptr, !dbg !21
+  ret void, !dbg !21
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) nounwind readnone
+
+declare void @Capture(i32*)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!13, !14}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 3.8.0 (trunk 248518) (llvm/trunk 248512)", isOptimized: true, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3)
+!1 = !DIFile(filename: "1.cc", directory: "/tmp")
+!2 = !{}
+!3 = !{!4}
+!4 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 4, type: !5, isLocal: false, isDefinition: true, scopeLine: 4, flags: DIFlagPrototyped, isOptimized: true, function: void ()* @f, variables: !7)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null}
+!7 = !{!8}
+!8 = !DILocalVariable(name: "buf", scope: !4, file: !1, line: 5, type: !9)
+!9 = !DICompositeType(tag: DW_TAG_array_type, baseType: !10, size: 3200, align: 32, elements: !11)
+!10 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!11 = !{!12}
+!12 = !DISubrange(count: 100)
+!13 = !{i32 2, !"Dwarf Version", i32 4}
+!14 = !{i32 2, !"Debug Info Version", i32 3}
+!15 = !{!"clang version 3.8.0 (trunk 248518) (llvm/trunk 248512)"}
+!16 = !DILocation(line: 5, column: 3, scope: !4)
+!17 = !DIExpression(DW_OP_deref, DW_OP_minus, 400)
+!18 = !DILocation(line: 5, column: 7, scope: !4)
+!19 = !DILocation(line: 6, column: 11, scope: !4)
+!20 = !DILocation(line: 6, column: 3, scope: !4)
+!21 = !DILocation(line: 7, column: 1, scope: !4)
+
+; RCX - 400
+; CHECK:      .short  6                       # Loc expr size
+; CHECK-NEXT: .byte 114                     # DW_OP_breg2
+; CHECK-NEXT: .byte 0                       # 0
+; CHECK-NEXT: .byte 16                      # DW_OP_constu
+; CHECK-NEXT: .byte 144                     # 400
+; CHECK-NEXT: .byte 3                       # DW_OP_minus
+; CHECK-NEXT: .byte 28
+
+; RCX is clobbered in call @Capture, but there is a spilled copy.
+; *(RSP + 8) - 400
+; CHECK:      .short  7                       # Loc expr size
+; CHECK-NEXT: .byte 119                     # DW_OP_breg7
+; CHECK-NEXT: .byte 8                       # 8
+; CHECK-NEXT: .byte 6                       # DW_OP_deref
+; CHECK-NEXT: .byte 16                      # DW_OP_constu
+; CHECK-NEXT: .byte 144                     # 400
+; CHECK-NEXT: .byte 3                       # DW_OP_minus
+; CHECK-NEXT: .byte 28

test/Transforms/SafeStack/debug-loc.ll

+; RUN: opt -safe-stack -S -mtriple=i386-pc-linux-gnu < %s -o - | FileCheck %s
+
+; Test debug location for the local variables moved onto the unsafe stack.
+; CHECK: define void @f
+; CHECK: %[[USP:.*]] = load i8*, i8** @__safestack_unsafe_stack_ptr
+
+; dbg.declare for %buf is gone; replaced with dbg.declare based off the unsafe stack pointer
+; CHECK-NOT: @llvm.dbg.declare.*%buf
+; CHECK: call void @llvm.dbg.declare(metadata i8* %[[USP]], metadata ![[VAR:.*]], metadata ![[EXPR:.*]])
+
+; dbg.declare appears before the first use of %buf
+; CHECK: getelementptr{{.*}}%buf
+; CHECK: call{{.*}}@Capture
+; CHECK: ret void
+
+; dbg.declare describes "buf"...
+; CHECK: ![[VAR]] = !DILocalVariable(name: "buf"
+
+; ... as an offset from the unsafe stack pointer
+; CHECK: ![[EXPR]] = !DIExpression(DW_OP_deref, DW_OP_minus, 400)
+
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: safestack uwtable
+define void @f() #0 {
+entry:
+  %buf = alloca [100 x i32], align 16
+  %0 = bitcast [100 x i32]* %buf to i8*, !dbg !16
+  call void @llvm.lifetime.start(i64 400, i8* %0) #4, !dbg !16
+  tail call void @llvm.dbg.declare(metadata [100 x i32]* %buf, metadata !8, metadata !17), !dbg !18
+
+
+  %arraydecay = getelementptr inbounds [100 x i32], [100 x i32]* %buf, i64 0, i64 0, !dbg !19
+  call void @Capture(i32* %arraydecay), !dbg !20
+  call void @llvm.lifetime.end(i64 400, i8* %0) #4, !dbg !21
+  ret void, !dbg !21
+}
+
+; Function Attrs: nounwind argmemonly
+declare void @llvm.lifetime.start(i64, i8* nocapture) #1
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #2
+
+declare void @Capture(i32*) #3
+
+; Function Attrs: nounwind argmemonly
+declare void @llvm.lifetime.end(i64, i8* nocapture) #1
+
+attributes #0 = { safestack uwtable "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"="+sse,+sse2" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind argmemonly }
+attributes #2 = { nounwind readnone }
+attributes #3 = { "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"="+sse,+sse2" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #4 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!13, !14}
+!llvm.ident = !{!15}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 3.8.0 (trunk 248518) (llvm/trunk 248512)", isOptimized: true, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3)
+!1 = !DIFile(filename: "1.cc", directory: "/tmp")
+!2 = !{}
+!3 = !{!4}
+!4 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 4, type: !5, isLocal: false, isDefinition: true, scopeLine: 4, flags: DIFlagPrototyped, isOptimized: true, function: void ()* @f, variables: !7)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null}
+!7 = !{!8}
+!8 = !DILocalVariable(name: "buf", scope: !4, file: !1, line: 5, type: !9)
+!9 = !DICompositeType(tag: DW_TAG_array_type, baseType: !10, size: 3200, align: 32, elements: !11)
+!10 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!11 = !{!12}
+!12 = !DISubrange(count: 100)
+!13 = !{i32 2, !"Dwarf Version", i32 4}
+!14 = !{i32 2, !"Debug Info Version", i32 3}
+!15 = !{!"clang version 3.8.0 (trunk 248518) (llvm/trunk 248512)"}
+!16 = !DILocation(line: 5, column: 3, scope: !4)
+!17 = !DIExpression()
+!18 = !DILocation(line: 5, column: 7, scope: !4)
+!19 = !DILocation(line: 6, column: 11, scope: !4)
+!20 = !DILocation(line: 6, column: 3, scope: !4)
+!21 = !DILocation(line: 7, column: 1, scope: !4)