PR32382: Emit complex DWARF expressions with the correct location description kind

[aprantl]

Summary

This fixes most of PR32382. It does not fix the DWARF 2 expressions yet; that part can be done as a separate, simpler follow-up commit.

The DWARF specification knows 3 kinds of non-empty simple location descriptions:

1. Register location descriptions
   • describe a variable in a register
   • consist of only a DW_OP_reg
2. Memory location descriptions
   • describe the address of a variable
3. Implicit location descriptions
   • describe the value of a variable
   • end with DW_OP_stack_value & friends

The existing DwarfExpression code is pretty much ignorant of these restrictions. This used to not matter because we only emitted very short expressions that we happened to get right by accident. This patch makes DwarfExpression aware of the rules defined by the DWARF standard and now chooses the right kind of location description for each expression being emitted.

There are two major changes in this patch:

• I had to fix the semantics of dbg.declare for describing function arguments. After this patch a dbg.declare always takes the *address* of a variable as the first argument, even if the argument is not an alloca.
• When lowering a DBG_VALUE, the decision of whether to emit a register location description or a memory location description depends on the MachineLocation — register machine locations may get promoted to memory locations based on their DIExpression. (Future) optimization passes that want to salvage implicit debug location for variables may do so by appending a DW_OP_stack_value. For example: DBG_VALUE, [RBP-8] --> DW_OP_fbreg -8 DBG_VALUE, RAX --> DW_OP_reg0 +0 DBG_VALUE, RAX, DIExpression(DW_OP_deref) --> DW_OP_reg0 +0

All testcases that were modified were regenerated from clang. I also added source-based testcases for each of these to the debuginfo-tests repository over the last week to make sure that no synchronized bugs slip in. The debuginfo-tests compile from source and run the debugger.

https://bugs.llvm.org/show_bug.cgi?id=32382
rdar://problem/31205000

[dblaikie]

There are a lot of small, careful changes in here - I might need a bit of a high level walkthrough of what the changes are and how they achieve the goal. More detail on how this changes behavior would be good. "makes LLVM aware of these DWARF restrictions" is a bit too high level.

lib/Bitcode/Reader/MetadataLoader.cpp

1536–1540

This seems a bit complicated - any way to simplify it/make it more obvious? (or at least add comments)

Some of it looks the same as the condition in the case 0 of this switch. Could it be factored into a common function wiht a name that might aid readability?

lib/Bitcode/Writer/BitcodeWriter.cpp

1759–1760

Where did the HasOpFragmentFlag go? Or it morphed into a version flag stored above the first bit?

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

177

So is this another "not quite DWARF expression" contract it DIExpression? (like the LLVM_fragment?)

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1517

Remove commented-out code.

[aprantl]

In D31439#713317, @dblaikie wrote:

There are a lot of small, careful changes in here - I might need a bit of a high level walkthrough of what the changes are and how they achieve the goal. More detail on how this changes behavior would be good. "makes LLVM aware of these DWARF restrictions" is a bit too high level.

This was unfortunately the smallest I could break the patch up into.
The observable behavior changes only for DIExpressions that have a DW_OP_deref operator *and* additional operators. The primary change is that the old code treated DW_OP_breg as if it where dereferencing the contents of the register. This happened to work if no other operations followed (because with no DW_OP_stack_value present, debuggers treat the expression as a memory location description, i.e.: deref the expression).

lib/Bitcode/Reader/MetadataLoader.cpp

1536–1540

It doesn't have enough in common to be factored out, but perhaps adding the comment

//[DW_OP_deref, ..., DW_OP_LLVM_fragment, x, y]
// -> [..., DW_OP_deref, DW_OP_LLVM_fragment, x, y]

makes it more obvious what is happening there.

lib/Bitcode/Writer/BitcodeWriter.cpp

1759–1760

It's rolled into the Version. I have a laundry list of future improvements to DIExpression, that will involve revving the version and are easier to handle as incremental upgrades rather than individual "features".

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

177

Yes, this is a bit obtuse. We could also add an addAddress() call to DwarfExpression analogous to addMachineReg() that then sets the location description kind inside of DwarfExpression. We could effectively move all the code inside if (Global) into DwarfExpression::addAddress().

[aprantl]

Add DIEDwarfExpression::addAddress() and DIEDwarfExpression::addThreadLocalAddress().

[dblaikie]

Couple of minor things. Not sure I fully understand everything here, but seems plausible.

include/llvm/IR/DebugInfoMetadata.h

2252–2257

linear algorithm for testing the last element? that doesn't seem ideal (getElements().back() instead?) (conditional on !getElements().empty())

lib/Bitcode/Reader/MetadataLoader.cpp

1536–1540

Maybe it'd be easier if it were a std::copy?

if (deref) {
  auto End = Elts.end();
  if (Elts.size() >= 3 && std::prev(End, 3) == fragment)
    std::prev(End, 3);
  std::move(std::next(Elts.begin()), End, Elts.begin());
  *std::prev(End) = fragment;
}

Something like that?

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

229–231

Should fixme be conditional on "when using DWARF N < M" (where M is whatever version that added stack_value). Would be good to mention that in the comment if that's the case.

[aprantl]

Thanks a lot for reviewing this, David! While I was preparing the next updated of the corresponding clang changes in https://reviews.llvm.org/D31440 I realized that there is one use-case that is not supported by this variant. To encode locations for by-ref captures clang wants to describe a location that is at an offset inside of a larger alloca. To support this use-case I need to remove the implicit DW_OP_deref that we are adding to dbg.declares everywhere and make it explicit instead.
This will be a good change:

• it will make a future patch that replaces all instances of dbg.declare with dbg.value very simple
• we won't need to rewrite DIExpressions when we lower dbg.declare -> dbg.value
• it will make the DW_OP_xderef use-case that I complain about in DwarfCompileUnit.cpp:570 appear less out-of-place.

I'll be back here with an updated (albeit somewhat longer) patch soon. Meanwhile I'm also extending the debuginfo-tests a bit to make sure I'm not breaking the integration between llvm and clang.

[aprantl]

Addressed David's review feedback for the DW_OP_deref bitcode upgrade.

[aprantl]

Added test coverage for SelectionDAG in sret.ll and ensured it matches the FastISel behavior.

[aprantl]

David, the review is still marked as accepted, but I did add a couple of changes since then, so I think I should be moving it back into "Request review".
Would you mind taking a second look?

As I mentioned in the description, all IR testcases that were modified were regenerated from clang. I also added source-based testcases for each of these to the debuginfo-tests repository over the last week to make sure that no synchronized bugs slip in. The debuginfo-tests compile from source and run the debugger and are exercised by the green dragon bots, so all these changes should be safe.
thanks!
adrian

[dblaikie]

Phab's not helping me see what the delta since the last time I accepted this are, but taking a very rough glance it seems ok - carry on :)

[aaboud]

This commit caused the following bug:
https://bugs.llvm.org/show_bug.cgi?id=33166

[aaboud]

In D31439#764342, @aaboud wrote:

This commit caused the following bug:
https://bugs.llvm.org/show_bug.cgi?id=33166

Seems like the issue was fixed in r301210. If that is true, then sorry for the noise.
Adrain, can you please confirm that?

Thanks

include/llvm/IR/DebugInfoMetadata.h

public:
   /// \pre \a isValid() gives \c true.
   /// @{
   expr_op_iterator expr_op_begin() const {
     return expr_op_iterator(elements_begin());
   }
   expr_op_iterator expr_op_end() const {
     return expr_op_iterator(elements_end());
   }
+  iterator_range<expr_op_iterator> expr_ops() const {
+    return {expr_op_begin(), expr_op_end()};
+  }
   /// @}
 
   bool isValid() const;
 
   static bool classof(const Metadata *MD) {
     return MD->getMetadataID() == DIExpressionKind;
   }
 
-  /// Is the first element a DW_OP_deref?.
+  /// Return whether the first element a DW_OP_deref.
   bool startsWithDeref() const {
     return getNumElements() > 0 && getElement(0) == dwarf::DW_OP_deref;
   }
 
+  /// Return whether the last element a DW_OP_xderef.
+  bool endsWithXDeref() const {
+    unsigned Last = 0;
+    for (auto &Op : expr_ops())
+      Last = Op.getOp();
+    return Last == dwarf::DW_OP_xderef;
+  }
+
   /// Holds the characteristics of one fragment of a larger variable.
   struct FragmentInfo {
     uint64_t SizeInBits;
     uint64_t OffsetInBits;
   };
 
   /// Retrieve the details of this fragment expression.
   static Optional<FragmentInfo> getFragmentInfo(expr_op_iterator Start,

lib/Bitcode/Reader/MetadataLoader.cpp

case bitc::METADATA_LOCAL_VAR: {
     NextMetadataNo++;
     break;
   }
   case bitc::METADATA_EXPRESSION: {
     if (Record.size() < 1)
       return error("Invalid record");
 
     IsDistinct = Record[0] & 1;
-    bool HasOpFragment = Record[0] & 2;
+    uint64_t Version = Record[0] >> 1;
     auto Elts = MutableArrayRef<uint64_t>(Record).slice(1);
-    if (!HasOpFragment)
-      if (unsigned N = Elts.size())
-        if (N >= 3 && Elts[N - 3] == dwarf::DW_OP_bit_piece)
-          Elts[N - 3] = dwarf::DW_OP_LLVM_fragment;
+    unsigned N = Elts.size();
+    // Perform various upgrades.
+    switch (Version) {
+    case 0:
+      LLVM_FALLTHROUGH;
+    case 1:
+      // Move DW_OP_deref to the end.
+      if (N && Elts[0] == dwarf::DW_OP_deref) {
+        unsigned I;
+        for (I = 0; I < N - 1; ++I) {
+          Elts[I] = Elts[I + 1];
+          if (N > 3 && I == N - 4 && Elts[I + 1] == dwarf::DW_OP_LLVM_fragment)
+            break;
+        }
+        Elts[I] = dwarf::DW_OP_deref;
+      }
+    case 2:
+      // Up-to-date!
+      break;
+    default:
+      return error("Invalid record");
+    }
 
     MetadataList.assignValue(
         GET_OR_DISTINCT(DIExpression, (Context, makeArrayRef(Record).slice(1))),
         NextMetadataNo);
     NextMetadataNo++;
     break;
   }
   case bitc::METADATA_GLOBAL_VAR_EXPR: {

lib/Bitcode/Writer/BitcodeWriter.cpp

void ModuleBitcodeWriter::writeDILocalVariable(
   Stream.EmitRecord(bitc::METADATA_LOCAL_VAR, Record, Abbrev);
   Record.clear();
 }
 
 void ModuleBitcodeWriter::writeDIExpression(const DIExpression *N,
                                             SmallVectorImpl<uint64_t> &Record,
                                             unsigned Abbrev) {
   Record.reserve(N->getElements().size() + 1);
-
-  const uint64_t HasOpFragmentFlag = 1 << 1;
-  Record.push_back((uint64_t)N->isDistinct() | HasOpFragmentFlag);
+  const uint64_t Version = 2 << 1;
+  Record.push_back((uint64_t)N->isDistinct() | Version);
   Record.append(N->elements_begin(), N->elements_end());
 
   Stream.EmitRecord(bitc::METADATA_EXPRESSION, Record, Abbrev);
   Record.clear();
 }
 
 void ModuleBitcodeWriter::writeDIGlobalVariableExpression(
     const DIGlobalVariableExpression *N, SmallVectorImpl<uint64_t> &Record,

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

if (!Global && (!Expr || !Expr->isConstant()))
       continue;
 
     if (!Loc) {
       addToAccelTable = true;
       Loc = new (DIEValueAllocator) DIELoc;
       DwarfExpr = llvm::make_unique<DIEDwarfExpression>(*Asm, *this, *Loc);
     }
 
+    SmallVector<uint64_t, 8> Ops;
     if (Global) {
+      // The location of a global is an address; the deref tells DwarfExpression
+      // to emit a memory location description.
+      Ops.push_back(dwarf::DW_OP_deref);
       const MCSymbol *Sym = Asm->getSymbol(Global);
       if (Global->isThreadLocal()) {
         if (Asm->TM.Options.EmulatedTLS) {
           // TODO: add debug info for emulated thread local mode.
         } else {
           // FIXME: Make this work with -gsplit-dwarf.
           unsigned PointerSize = Asm->getDataLayout().getPointerSize();
           assert((PointerSize == 4 || PointerSize == 8) &&
if (Global) {
           addUInt(*Loc, dwarf::DW_FORM_data1,
                   DD->useGNUTLSOpcode() ? dwarf::DW_OP_GNU_push_tls_address
                                         : dwarf::DW_OP_form_tls_address);
         }
       } else {
         DD->addArangeLabel(SymbolCU(this, Sym));
         addOpAddress(*Loc, Sym);
       }
-    }
-    if (Expr) {
-      DwarfExpr->addFragmentOffset(Expr);
-      DwarfExpr->addExpression(Expr);
-    }
+    } 
+
+    if (Expr)
+      Ops.append(Expr->elements_begin(), Expr->elements_end());
+    DIExpressionCursor Cursor(Ops);
+    DwarfExpr->addExpression(std::move(Cursor));
   }
   if (Loc)
     addBlock(*VariableDIE, dwarf::DW_AT_location, DwarfExpr->finalize());
 
   if (DD->useAllLinkageNames())
     addLinkageName(*VariableDIE, GV->getLinkageName());
 
   if (addToAccelTable) {
DIE *DwarfCompileUnit::constructVariableDIEImpl(const DbgVariable &DV,
   // .. else use frame index.
   if (!DV.hasFrameIndexExprs())
     return VariableDie;
 
   DIELoc *Loc = new (DIEValueAllocator) DIELoc;
   DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
   for (auto &Fragment : DV.getFrameIndexExprs()) {
     unsigned FrameReg = 0;
+    const DIExpression *Expr = Fragment.Expr;
     const TargetFrameLowering *TFI = Asm->MF->getSubtarget().getFrameLowering();
     int Offset = TFI->getFrameIndexReference(*Asm->MF, Fragment.FI, FrameReg);
-    DwarfExpr.addFragmentOffset(Fragment.Expr);
+    DwarfExpr.addFragmentOffset(Expr);
     SmallVector<uint64_t, 8> Ops;
     Ops.push_back(dwarf::DW_OP_plus);
     Ops.push_back(Offset);
+    // FIXME: This is a hack. We should come up with a less ambiguous encoding.
+    if (!Expr->endsWithXDeref())
       Ops.push_back(dwarf::DW_OP_deref);
-    Ops.append(Fragment.Expr->elements_begin(), Fragment.Expr->elements_end());
-    DIExpressionCursor Expr(Ops);
+    Ops.append(Expr->elements_begin(), Expr->elements_end());
+    DIExpressionCursor Cursor(Ops);
     DwarfExpr.addMachineRegExpression(
-        *Asm->MF->getSubtarget().getRegisterInfo(), Expr, FrameReg);
-    DwarfExpr.addExpression(std::move(Expr));
+        *Asm->MF->getSubtarget().getRegisterInfo(), Cursor, FrameReg);
+    DwarfExpr.addExpression(std::move(Cursor));
   }
   addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
 
   return VariableDie;
 }
 
 DIE *DwarfCompileUnit::constructVariableDIE(DbgVariable &DV,
                                             const LexicalScope &Scope,

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

if (Value.isInt()) {
     else
       DwarfExpr.addUnsignedConstant(Value.getInt());
   } else if (Value.isLocation()) {
     MachineLocation Location = Value.getLoc();
 
     SmallVector<uint64_t, 8> Ops;
     // FIXME: Should this condition be Location.isIndirect() instead?
     if (Location.getOffset()) {
+    //if (Location.isIndirect()) {
       Ops.push_back(dwarf::DW_OP_plus);
       Ops.push_back(Location.getOffset());
       Ops.push_back(dwarf::DW_OP_deref);
     }
     Ops.append(DIExpr->elements_begin(), DIExpr->elements_end());
     DIExpressionCursor Cursor(Ops);
     const TargetRegisterInfo &TRI = *AP.MF->getSubtarget().getRegisterInfo();
     if (!DwarfExpr.addMachineRegExpression(TRI, Cursor, Location.getReg()))

lib/CodeGen/AsmPrinter/DwarfExpression.h

protected:
   /// Current Fragment Offset in Bits.
   uint64_t OffsetInBits = 0;
   unsigned DwarfVersion;
 
   /// Sometimes we need to add a DW_OP_bit_piece to describe a subregister. 
   unsigned SubRegisterSizeInBits = 0;
   unsigned SubRegisterOffsetInBits = 0;
 
+  /// The kind of location description being produced.
+  enum { Unknown = 0, Register, Memory, Implicit } LocationKind = Unknown;
+
   /// Push a DW_OP_piece / DW_OP_bit_piece for emitting later, if one is needed
   /// to represent a subregister.
   void setSubRegisterPiece(unsigned SizeInBits, unsigned OffsetInBits) {
     SubRegisterSizeInBits = SizeInBits;
     SubRegisterOffsetInBits = OffsetInBits;
   }
 
   /// Add masking operations to stencil out a subregister.
   void maskSubRegister();
 
   /// Output a dwarf operand and an optional assembler comment.
   virtual void emitOp(uint8_t Op, const char *Comment = nullptr) = 0;
   /// Emit a raw signed value.
   virtual void emitSigned(int64_t Value) = 0;
   /// Emit a raw unsigned value.
   virtual void emitUnsigned(uint64_t Value) = 0;
   /// Return whether the given machine register is the frame register in the
   /// current function.
   virtual bool isFrameRegister(const TargetRegisterInfo &TRI, unsigned MachineReg) = 0;
 
-  /// Emit a DW_OP_reg operation.
+  /// Emit a DW_OP_reg operation. Note that this is only legal inside a DWARF
+  /// register location description.
   void addReg(int DwarfReg, const char *Comment = nullptr);
   /// Emit a DW_OP_breg operation.
   void addBReg(int DwarfReg, int Offset);
   /// Emit DW_OP_fbreg <Offset>.
   void addFBReg(int Offset);
 
   /// Emit a partial DWARF register operation.
   ///

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

 #include "llvm/Support/Dwarf.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetRegisterInfo.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
 
 using namespace llvm;
 
 void DwarfExpression::addReg(int DwarfReg, const char *Comment) {
-  assert(DwarfReg >= 0 && "invalid negative dwarf register number");
-  if (DwarfReg < 32) {
-    emitOp(dwarf::DW_OP_reg0 + DwarfReg, Comment);
+ assert((LocationKind == Unknown || LocationKind == Register) &&
+        "location description already locked down");
+ LocationKind = Register;
   } else {
     emitOp(dwarf::DW_OP_regx, Comment);
     emitUnsigned(DwarfReg);
   }
 }
 
 void DwarfExpression::addBReg(int DwarfReg, int Offset) {
   assert(DwarfReg >= 0 && "invalid negative dwarf register number");
+  assert(LocationKind != Register && "location description already locked down");
   if (DwarfReg < 32) {
     emitOp(dwarf::DW_OP_breg0 + DwarfReg);
   } else {
     emitOp(dwarf::DW_OP_bregx);
     emitUnsigned(DwarfReg);
   }
   emitSigned(Offset);
 }

 }
 
 void DwarfExpression::addStackValue() {
   if (DwarfVersion >= 4)
     emitOp(dwarf::DW_OP_stack_value);
 }
 
 void DwarfExpression::addSignedConstant(int64_t Value) {
+  assert(LocationKind == Implicit || LocationKind == Unknown);
+  LocationKind = Implicit;
   emitOp(dwarf::DW_OP_consts);
   emitSigned(Value);
-  addStackValue();
 }
 
 void DwarfExpression::addUnsignedConstant(uint64_t Value) {
+  assert(LocationKind == Implicit || LocationKind == Unknown);
+  LocationKind = Implicit;
   emitOp(dwarf::DW_OP_constu);
   emitUnsigned(Value);
-  addStackValue();
 }
 
 void DwarfExpression::addUnsignedConstant(const APInt &Value) {
+  assert(LocationKind == Implicit || LocationKind == Unknown);
+  LocationKind = Implicit;
+
   unsigned Size = Value.getBitWidth();
   const uint64_t *Data = Value.getRawData();
 
   // Chop it up into 64-bit pieces, because that's the maximum that
   // addUnsignedConstant takes.
   unsigned Offset = 0;
   while (Offset < Size) {
     addUnsignedConstant(*Data++);
     if (Offset == 0 && Size <= 64)
       break;
-    addOpPiece(std::min(Size-Offset, 64u), Offset);
+    addStackValue();
+    addOpPiece(std::min(Size - Offset, 64u), Offset);
     Offset += 64;
   }
 }
 
 bool DwarfExpression::addMachineRegExpression(const TargetRegisterInfo &TRI,
                                               DIExpressionCursor &ExprCursor,
                                               unsigned MachineReg,
                                               unsigned FragmentOffsetInBits) {
for (auto &Reg : DwarfRegs) {
       if (Reg.DwarfRegNo >= 0)
         addReg(Reg.DwarfRegNo, Reg.Comment);
       addOpPiece(Reg.Size);
     }
     DwarfRegs.clear();
     return true;
   }
 
+  // FIXME:
+  // Don't emit locations that cannot be expressed without DW_OP_stack_value.
+
   assert(DwarfRegs.size() == 1);
   auto Reg = DwarfRegs[0];
   bool FBReg = isFrameRegister(TRI, MachineReg); 
   assert(Reg.Size == 0 && "subregister has same size as superregister");
 
   // Pattern-match combinations for which more efficient representations exist.
   switch (Op->getOp()) {
-  default: {
-    if (FBReg)
-      addFBReg(0);
-    else
-      addReg(Reg.DwarfRegNo, 0);
-    break;
-  }
   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 = ExprCursor.peekNext();
-    if (N && N->getOp() == dwarf::DW_OP_deref) {
-      int Offset = Op->getArg(0);
-      int SignedOffset = (Op->getOp() == dwarf::DW_OP_plus) ? Offset : -Offset;
-      if (FBReg)
-        addFBReg(SignedOffset);
-      else
-        addBReg(Reg.DwarfRegNo, SignedOffset);
+    // [DW_OP_reg,Offset,DW_OP_plus] --> [DW_OP_breg, Offset].
+    // [DW_OP_reg,Offset,DW_OP_minus] --> [DW_OP_breg,-Offset].
 
-      ExprCursor.consume(2);
-      break;
-    }
-    addReg(Reg.DwarfRegNo, 0);
+    ExprCursor.take();
     break;
   }
-  case dwarf::DW_OP_deref:
-    // [DW_OP_reg,DW_OP_deref] --> [DW_OP_breg].
+  default: {
     if (FBReg)
       addFBReg(0);
     else
       addBReg(Reg.DwarfRegNo, 0);
-    ExprCursor.take();
     break;
   }
+  }
   DwarfRegs.clear();
   return true;
 }
 
+/// Assuming a well-formed expression, match "DW_OP_deref* DW_OP_LLVM_fragment?".
+static bool isMemoryLocation(DIExpressionCursor ExprCursor) {
+  while (ExprCursor) {
+    auto Op = ExprCursor.take();
+    switch (Op->getOp()) {
+    case dwarf::DW_OP_deref:
+    case dwarf::DW_OP_LLVM_fragment:
+      break;
+    default:
+      return false;
+    }
+  }
+  return true;
+}
+
 void DwarfExpression::addExpression(DIExpressionCursor &&ExprCursor,
                                     unsigned FragmentOffsetInBits) {
+  auto N = ExprCursor.peek();
   while (ExprCursor) {
     auto Op = ExprCursor.take();
 
     // If we need to mask out a subregister, do it now, unless the next
     // operation would emit an OpPiece anyway.
     if (SubRegisterSizeInBits && Op->getOp() != dwarf::DW_OP_LLVM_fragment)
       maskSubRegister();
 
case dwarf::DW_OP_LLVM_fragment: {
       // of the pieces that was already emitted.
       SizeInBits -= OffsetInBits - FragmentOffset;
 
       // If \a addMachineReg requested a DW_OP_bit_piece to stencil out a
       // sub-register that is smaller than the current fragment's size, use it.
       if (SubRegisterSizeInBits)
         SizeInBits = std::min<unsigned>(SizeInBits, SubRegisterSizeInBits);
       
+      if (LocationKind != Register && LocationKind != Memory)
+        // Turn this into an implicit location description.
+        addStackValue();
+
+      // Emit the DW_OP_piece.
       addOpPiece(SizeInBits, SubRegisterOffsetInBits);
       setSubRegisterPiece(0, 0);
-      break;
+      // Reset the location description kind.
+      LocationKind = Unknown;
+      return;
     }
     case dwarf::DW_OP_plus:
+      assert(LocationKind != Register && LocationKind != Memory);
       emitOp(dwarf::DW_OP_plus_uconst);
       emitUnsigned(Op->getArg(0));
       break;
     case dwarf::DW_OP_minus:
-      // There is no OP_minus_uconst.
+      assert(LocationKind != Register && LocationKind != Memory);
+      // There is no DW_OP_minus_uconst.
       emitOp(dwarf::DW_OP_constu);
       emitUnsigned(Op->getArg(0));
       emitOp(dwarf::DW_OP_minus);
       break;
-    case dwarf::DW_OP_deref:
+    case dwarf::DW_OP_deref: {
+      assert(LocationKind != Register);
+      if (LocationKind != Memory && isMemoryLocation(ExprCursor))
+        // Turning this into a memory location description makes the deref
+        // implicit.
+        LocationKind = Memory;
+      else
         emitOp(dwarf::DW_OP_deref);
       break;
+    }
     case dwarf::DW_OP_constu:
+      assert(LocationKind != Register && LocationKind != Memory);
       emitOp(dwarf::DW_OP_constu);
       emitUnsigned(Op->getArg(0));
       break;
     case dwarf::DW_OP_stack_value:
-      addStackValue();
+      assert(LocationKind != Register && LocationKind != Memory);
       break;
     case dwarf::DW_OP_swap:
+      assert(LocationKind != Register && LocationKind != Memory);
       emitOp(dwarf::DW_OP_swap);
       break;
     case dwarf::DW_OP_xderef:
+      assert(LocationKind != Register && LocationKind != Memory);
       emitOp(dwarf::DW_OP_xderef);
       break;
     default:
       llvm_unreachable("unhandled opcode found in expression");
     }
   }
+
+  if (LocationKind == Register || LocationKind == Memory)
+    return;
+
+  // Turn this into an implicit location description.
+  addStackValue();
 }
 
 /// add masking operations to stencil out a subregister.
 void DwarfExpression::maskSubRegister() {
   assert(SubRegisterSizeInBits && "no subregister was registered");
   if (SubRegisterOffsetInBits > 0)
     addShr(SubRegisterOffsetInBits);
   uint64_t Mask = (1ULL << (uint64_t)SubRegisterSizeInBits) - 1ULL;

test/Bitcode/DIExpression-deref.ll

+; RUN: llvm-dis -o - %s.bc | FileCheck %s
+
+!llvm.dbg.cu = !{!1}
+!llvm.module.flags = !{!20, !21}
+
+!0 = distinct !DIGlobalVariable(name: "g", scope: !1, file: !2, line: 1, type: !5, isLocal: false, isDefinition: true)
+!1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, producer: "clang (llvm/trunk 288154)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !3, globals: !4)
+!2 = !DIFile(filename: "a.c", directory: "/")
+!3 = !{}
+!4 = !{!10, !11, !12, !13}
+!5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+; DW_OP_deref should be moved to the back of the expression.
+;
+; CHECK: !DIExpression(DW_OP_plus, 0, DW_OP_deref, DW_OP_LLVM_fragment, 8, 32)
+!6 = !DIExpression(DW_OP_deref, DW_OP_plus, 0, DW_OP_LLVM_fragment, 8, 32)
+; CHECK: !DIExpression(DW_OP_plus, 0, DW_OP_deref)
+!7 = !DIExpression(DW_OP_deref, DW_OP_plus, 0)
+; CHECK: !DIExpression(DW_OP_plus, 1, DW_OP_deref)
+!8 = !DIExpression(DW_OP_plus, 1, DW_OP_deref)
+; CHECK: !DIExpression(DW_OP_deref)
+!9 = !DIExpression(DW_OP_deref)
+!10 = !DIGlobalVariableExpression(var: !0, expr: !6)
+!11 = !DIGlobalVariableExpression(var: !0, expr: !7)
+!12 = !DIGlobalVariableExpression(var: !0, expr: !8)
+!13 = !DIGlobalVariableExpression(var: !0, expr: !9)
+!20 = !{i32 2, !"Dwarf Version", i32 4}
+!21 = !{i32 2, !"Debug Info Version", i32 3}

test/DebugInfo/X86/block-capture.ll

 !61 = !DIDerivedType(tag: DW_TAG_member, name: "Size", line: 2, size: 64, align: 64, offset: 64, file: !1, scope: !58, baseType: !41)
 !62 = !DIDerivedType(tag: DW_TAG_member, name: "CopyFuncPtr", line: 2, size: 64, align: 64, offset: 128, file: !1, scope: !58, baseType: !63)
 !63 = !DIDerivedType(tag: DW_TAG_pointer_type, size: 64, align: 64, baseType: !11)
 !64 = !DIDerivedType(tag: DW_TAG_member, name: "DestroyFuncPtr", line: 2, size: 64, align: 64, offset: 192, file: !1, scope: !58, baseType: !63)
 !65 = !DIDerivedType(tag: DW_TAG_member, name: "block", line: 2, size: 64, align: 64, offset: 256, flags: DIFlagPublic, file: !1, scope: !5, baseType: !25)
 !66 = !DILocation(line: 2, column: 20, scope: !8)
 !67 = !DILocation(line: 2, column: 21, scope: !8)
 !68 = !DILocalVariable(name: "block", line: 2, scope: !8, file: !5, type: !25)
-!69 = !DIExpression(DW_OP_deref, DW_OP_plus, 32)
+!69 = !DIExpression(DW_OP_plus, 32, DW_OP_deref)
 !70 = !DILocation(line: 2, column: 9, scope: !8)
 !71 = !DILocation(line: 2, column: 23, scope: !72)
 !72 = distinct !DILexicalBlock(line: 2, column: 21, file: !1, scope: !8)
 !73 = !DILocation(line: 2, column: 32, scope: !8)

test/DebugInfo/X86/debug-info-block-captured-self.ll

 !84 = !DILocation(line: 33, scope: !38)
 !86 = !DILocalVariable(name: "self", line: 41, scope: !38, file: !1, type: !34)
 !87 = !DILocation(line: 41, scope: !38)
 !103 = !DILocation(line: 35, scope: !42)
 !105 = !DILocalVariable(name: "self", line: 40, scope: !42, file: !1, type: !34)
 !106 = !DILocation(line: 40, scope: !42)
 !107 = !DIFile(filename: "llvm/tools/clang/test/CodeGenObjC/debug-info-block-captured-self.m", directory: "")
 !108 = !{i32 1, !"Debug Info Version", i32 3}
-!109 = !DIExpression(DW_OP_deref, DW_OP_plus, 32)
-!110 = !DIExpression(DW_OP_deref, DW_OP_plus, 32)
+!109 = !DIExpression(DW_OP_plus, 32, DW_OP_deref)
+!110 = !DIExpression(DW_OP_plus, 32, DW_OP_deref)

test/DebugInfo/X86/debug-info-blocks.ll

 !103 = !DILocation(line: 52, scope: !31)
 !104 = !DILocalVariable(name: "", line: 52, arg: 2, flags: DIFlagArtificial, scope: !31, file: !32, type: !30)
 !105 = !DILocalVariable(name: "", line: 52, arg: 1, flags: DIFlagArtificial | DIFlagObjectPointer, scope: !35, file: !32, type: !30)
 !106 = !DILocation(line: 52, scope: !35)
 !107 = !DILocalVariable(name: "a", line: 61, scope: !36, file: !6, type: !61)
 !108 = !DILocation(line: 61, scope: !36)
 !109 = !DILocation(line: 62, scope: !36)
 !110 = !{i32 1, !"Debug Info Version", i32 3}
-!111 = !DIExpression(DW_OP_deref, DW_OP_plus, 32)
+!111 = !DIExpression(DW_OP_plus, 32, DW_OP_deref)

test/DebugInfo/X86/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)
+; The interesting part is !DIExpression(DW_OP_minus, 400, DW_OP_deref)
 
 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() !dbg !4 {
 entry:

 !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)
+!17 = !DIExpression(DW_OP_minus, 400, DW_OP_deref)
 !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:      .short  3                       # 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
+; CHECK-NEXT: .byte 240                     # -400
+; CHECK-NEXT: .byte 124
 
 ; 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/DebugInfo/X86/dw_op_minus_direct.ll

 ; Test dwarf codegen of DW_OP_minus.
 ; RUN: llc -filetype=obj < %s | llvm-dwarfdump - | FileCheck %s
 
 ; This was derived manually from:
 ; int inc(int i) {
 ;  return i+1;
 ; }
 
 ; CHECK: Beginning address offset: 0x0000000000000000
 ; CHECK:    Ending address offset: 0x0000000000000004
-; CHECK:     Location description: 50 10 ff ff ff ff 0f 1a 10 01 1c
-;                                  rax, constu 0xffffffff, and, constu 0x00000001, minus
+; CHECK:     Location description: 70 7f 9f
+;                                  breg0 -1, stack-value
 source_filename = "minus.c"
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.12.0"
 
 define i32 @inc(i32 %i) local_unnamed_addr #1 !dbg !7 {
 entry:
   %add = add nsw i32 %i, 1, !dbg !15
   tail call void @llvm.dbg.value(metadata i32 %add, i64 0, metadata !12, metadata !13), !dbg !14