Align definition of DW_OP_plus with DWARF spec [3/3]

Summary:
This patch is part of 3 patches that together form a single patch, but must be introduced in stages in order not to break things.
 
The way that LLVM interprets DW_OP_plus in DIExpression nodes is basically that of the DW_OP_plus_uconst operator since LLVM expects an unsigned constant operand. This unnecessarily restricts the DW_OP_plus operator, preventing it from being used to describe the evaluation of runtime values on the expression stack. These patches try to align the semantics of DW_OP_plus and DW_OP_minus with that of the DWARF definition, which pops two elements off the expression stack, performs the operation and pushes the result back on the stack.
 
This is done in three stages:
• The first patch (LLVM) adds support for DW_OP_plus_uconst.
• The second patch (Clang) contains changes all its uses from DW_OP_plus to DW_OP_plus_uconst.
• The third patch (LLVM) changes the semantics of DW_OP_plus and DW_OP_minus to be in line with its DWARF meaning. This patch includes the bitcode upgrade from legacy DIExpressions.

Patch by Sander de Smalen.

Reviewers: echristo, pcc, aprantl

Reviewed By: aprantl

Subscribers: fhahn, javed.absar, aprantl, llvm-commits

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

llvm-svn: 305386

9 files changed, 133 insertions, 64 deletions

diff --git a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
index ee2fe2a0cc1..b1504a8034e 100644
--- a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
+++ b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
@@ -407,6 +407,11 @@ void PlaceholderQueue::flush(BitcodeReaderMetadataList &MetadataList) {
 
 } // anonynous namespace
 
+static Error error(const Twine &Message) {
+  return make_error<StringError>(
+      Message, make_error_code(BitcodeError::CorruptedBitcode));
+}
+
 class MetadataLoader::MetadataLoaderImpl {
   BitcodeReaderMetadataList MetadataList;
   BitcodeReaderValueList &ValueList;
@@ -533,6 +538,88 @@ class MetadataLoader::MetadataLoaderImpl {
             }
   }
 
+  /// Upgrade the expression from previous versions.
+  Error upgradeDIExpression(uint64_t FromVersion,
+                            MutableArrayRef<uint64_t> &Expr,
+                            SmallVectorImpl<uint64_t> &Buffer) {
+    auto N = Expr.size();
+    switch (FromVersion) {
+    default:
+      return error("Invalid record");
+    case 0:
+      if (N >= 3 && Expr[N - 3] == dwarf::DW_OP_bit_piece)
+        Expr[N - 3] = dwarf::DW_OP_LLVM_fragment;
+      LLVM_FALLTHROUGH;
+    case 1:
+      // Move DW_OP_deref to the end.
+      if (N && Expr[0] == dwarf::DW_OP_deref) {
+        auto End = Expr.end();
+        if (Expr.size() >= 3 &&
+            *std::prev(End, 3) == dwarf::DW_OP_LLVM_fragment)
+          End = std::prev(End, 3);
+        std::move(std::next(Expr.begin()), End, Expr.begin());
+        *std::prev(End) = dwarf::DW_OP_deref;
+      }
+      NeedDeclareExpressionUpgrade = true;
+      LLVM_FALLTHROUGH;
+    case 2: {
+      // Change DW_OP_plus to DW_OP_plus_uconst.
+      // Change DW_OP_minus to DW_OP_uconst, DW_OP_minus
+      auto SubExpr = ArrayRef<uint64_t>(Expr);
+      while (!SubExpr.empty()) {
+        // Skip past other operators with their operands
+        // for this version of the IR, obtained from
+        // from historic DIExpression::ExprOperand::getSize().
+        size_t HistoricSize;
+        switch (SubExpr.front()) {
+        default:
+          HistoricSize = 1;
+          break;
+        case dwarf::DW_OP_constu:
+        case dwarf::DW_OP_minus:
+        case dwarf::DW_OP_plus:
+          HistoricSize = 2;
+          break;
+        case dwarf::DW_OP_LLVM_fragment:
+          HistoricSize = 3;
+          break;
+        }
+
+        // If the expression is malformed, make sure we don't
+        // copy more elements than we should.
+        HistoricSize = std::min(SubExpr.size(), HistoricSize);
+        ArrayRef<uint64_t> Args = SubExpr.slice(1, HistoricSize-1);
+
+        switch (SubExpr.front()) {
+        case dwarf::DW_OP_plus:
+          Buffer.push_back(dwarf::DW_OP_plus_uconst);
+          Buffer.append(Args.begin(), Args.end());
+          break;
+        case dwarf::DW_OP_minus:
+          Buffer.push_back(dwarf::DW_OP_constu);
+          Buffer.append(Args.begin(), Args.end());
+          Buffer.push_back(dwarf::DW_OP_minus);
+          break;
+        default:
+          Buffer.push_back(*SubExpr.begin());
+          Buffer.append(Args.begin(), Args.end());
+          break;
+        }
+
+        // Continue with remaining elements.
+        SubExpr = SubExpr.slice(HistoricSize);
+      }
+      Expr = MutableArrayRef<uint64_t>(Buffer);
+      LLVM_FALLTHROUGH;
+    }
+    case 3:
+      // Up-to-date!
+      break;
+    }
+
+    return Error::success();
+  }
+
   void upgradeDebugInfo() {
     upgradeCUSubprograms();
     upgradeCUVariables();
@@ -590,11 +677,6 @@ public:
   void upgradeDebugIntrinsics(Function &F) { upgradeDeclareExpressions(F); }
 };
 
-static Error error(const Twine &Message) {
-  return make_error<StringError>(
-      Message, make_error_code(BitcodeError::CorruptedBitcode));
-}
-
 Expected<bool>
 MetadataLoader::MetadataLoaderImpl::lazyLoadModuleMetadataBlock() {
   IndexCursor = Stream;
@@ -1551,34 +1633,13 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata(
     IsDistinct = Record[0] & 1;
     uint64_t Version = Record[0] >> 1;
     auto Elts = MutableArrayRef<uint64_t>(Record).slice(1);
-    unsigned N = Elts.size();
-    // Perform various upgrades.
-    switch (Version) {
-    case 0:
-      if (N >= 3 && Elts[N - 3] == dwarf::DW_OP_bit_piece)
-        Elts[N - 3] = dwarf::DW_OP_LLVM_fragment;
-      LLVM_FALLTHROUGH;
-    case 1:
-      // Move DW_OP_deref to the end.
-      if (N && Elts[0] == dwarf::DW_OP_deref) {
-        auto End = Elts.end();
-        if (Elts.size() >= 3 && *std::prev(End, 3) == dwarf::DW_OP_LLVM_fragment)
-          End = std::prev(End, 3);
-        std::move(std::next(Elts.begin()), End, Elts.begin());
-        *std::prev(End) = dwarf::DW_OP_deref;
-      }
-      NeedDeclareExpressionUpgrade = true;
-      LLVM_FALLTHROUGH;
-    case 2:
-      // Up-to-date!
-      break;
-    default:
-      return error("Invalid record");
-    }
+
+    SmallVector<uint64_t, 6> Buffer;
+    if (Error Err = upgradeDIExpression(Version, Elts, Buffer))
+      return Err;
 
     MetadataList.assignValue(
-        GET_OR_DISTINCT(DIExpression, (Context, makeArrayRef(Record).slice(1))),
-        NextMetadataNo);
+        GET_OR_DISTINCT(DIExpression, (Context, Elts)), NextMetadataNo);
     NextMetadataNo++;
     break;
   }
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index d5879fec95c..e679424bf1f 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -1663,7 +1663,7 @@ void ModuleBitcodeWriter::writeDIExpression(const DIExpression *N,
                                             SmallVectorImpl<uint64_t> &Record,
                                             unsigned Abbrev) {
   Record.reserve(N->getElements().size() + 1);
-  const uint64_t Version = 2 << 1;
+  const uint64_t Version = 3 << 1;
   Record.push_back((uint64_t)N->isDistinct() | Version);
   Record.append(N->elements_begin(), N->elements_end());
 
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
index 04073b3aed6..dc39d1e6cb5 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
@@ -552,7 +552,7 @@ DIE *DwarfCompileUnit::constructVariableDIEImpl(const DbgVariable &DV,
     int Offset = TFI->getFrameIndexReference(*Asm->MF, Fragment.FI, FrameReg);
     DwarfExpr.addFragmentOffset(Expr);
     SmallVector<uint64_t, 8> Ops;
-    Ops.push_back(dwarf::DW_OP_plus);
+    Ops.push_back(dwarf::DW_OP_plus_uconst);
     Ops.push_back(Offset);
     Ops.append(Expr->elements_begin(), Expr->elements_end());
     DIExpressionCursor Cursor(Ops);
@@ -821,7 +821,7 @@ void DwarfCompileUnit::addAddress(DIE &Die, dwarf::Attribute Attribute,
 
   SmallVector<uint64_t, 8> Ops;
   if (Location.isIndirect() && Location.getOffset()) {
-    Ops.push_back(dwarf::DW_OP_plus);
+    Ops.push_back(dwarf::DW_OP_plus_uconst);
     Ops.push_back(Location.getOffset());
   }
   DIExpressionCursor Cursor(Ops);
@@ -850,7 +850,7 @@ void DwarfCompileUnit::addComplexAddress(const DbgVariable &DV, DIE &Die,
 
   SmallVector<uint64_t, 8> Ops;
   if (Location.isIndirect() && Location.getOffset()) {
-    Ops.push_back(dwarf::DW_OP_plus);
+    Ops.push_back(dwarf::DW_OP_plus_uconst);
     Ops.push_back(Location.getOffset());
   }
   Ops.append(DIExpr->elements_begin(), DIExpr->elements_end());
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index e3fd21a1fd7..75eb355bfb5 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -1511,7 +1511,7 @@ static void emitDebugLocValue(const AsmPrinter &AP, const DIBasicType *BT,
       DwarfExpr.setMemoryLocationKind();
     SmallVector<uint64_t, 8> Ops;
     if (Location.isIndirect() && Location.getOffset()) {
-      Ops.push_back(dwarf::DW_OP_plus);
+      Ops.push_back(dwarf::DW_OP_plus_uconst);
       Ops.push_back(Location.getOffset());
     }
     Ops.append(DIExpr->elements_begin(), DIExpr->elements_end());
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
index 73ce58e3ca5..fe38ee80568 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
@@ -254,15 +254,19 @@ bool DwarfExpression::addMachineRegExpression(const TargetRegisterInfo &TRI,
     ExprCursor.take();
   }
 
-  // [Reg, Offset, DW_OP_plus] --> [DW_OP_breg, Offset].
-  // [Reg, Offset, DW_OP_minus] --> [DW_OP_breg, -Offset].
+  // [Reg, DW_OP_constu, Offset, DW_OP_plus]  --> [DW_OP_breg, Offset]
+  // [Reg, DW_OP_constu, Offset, DW_OP_minus] --> [DW_OP_breg,-Offset]
   // If Reg is a subregister we need to mask it out before subtracting.
-  if (Op && ((Op->getOp() == dwarf::DW_OP_plus) ||
-             (Op->getOp() == dwarf::DW_OP_minus && !SubRegisterSizeInBits))) {
-    int Offset = Op->getArg(0);
-    SignedOffset = (Op->getOp() == dwarf::DW_OP_plus) ? Offset : -Offset;
-    ExprCursor.take();
+  if (Op && Op->getOp() == dwarf::DW_OP_constu) {
+    auto N = ExprCursor.peekNext();
+    if (N && (N->getOp() == dwarf::DW_OP_plus ||
+             (N->getOp() == dwarf::DW_OP_minus && !SubRegisterSizeInBits))) {
+      int Offset = Op->getArg(0);
+      SignedOffset = (N->getOp() == dwarf::DW_OP_minus) ? -Offset : Offset;
+      ExprCursor.consume(2);
+    }
   }
+
   if (FBReg)
     addFBReg(SignedOffset);
   else
@@ -326,18 +330,14 @@ void DwarfExpression::addExpression(DIExpressionCursor &&ExprCursor,
       LocationKind = Unknown;
       return;
     }
-    case dwarf::DW_OP_plus:
     case dwarf::DW_OP_plus_uconst:
       assert(LocationKind != Register);
       emitOp(dwarf::DW_OP_plus_uconst);
       emitUnsigned(Op->getArg(0));
       break;
+    case dwarf::DW_OP_plus:
     case dwarf::DW_OP_minus:
-      assert(LocationKind != Register);
-      // There is no DW_OP_minus_uconst.
-      emitOp(dwarf::DW_OP_constu);
-      emitUnsigned(Op->getArg(0));
-      emitOp(dwarf::DW_OP_minus);
+      emitOp(Op->getOp());
       break;
     case dwarf::DW_OP_deref: {
       assert(LocationKind != Register);
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
index de861320006..728f8ad9225 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
@@ -42,6 +42,9 @@ public:
   DIExpressionCursor(ArrayRef<uint64_t> Expr)
       : Start(Expr.begin()), End(Expr.end()) {}
 
+  DIExpressionCursor(const DIExpressionCursor &C)
+      : Start(C.Start), End(C.End) {}
+
   /// Consume one operation.
   Optional<DIExpression::ExprOperand> take() {
     if (Start == End)
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
index 7f7d3e650e0..708f5f7536f 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
@@ -475,7 +475,7 @@ void DwarfUnit::addBlockByrefAddress(const DbgVariable &DV, DIE &Die,
 
   SmallVector<uint64_t, 9> Ops;
   if (Location.isIndirect() && Location.getOffset()) {
-    Ops.push_back(dwarf::DW_OP_plus);
+    Ops.push_back(dwarf::DW_OP_plus_uconst);
     Ops.push_back(Location.getOffset());
   }
   // If we started with a pointer to the __Block_byref... struct, then
@@ -487,7 +487,7 @@ void DwarfUnit::addBlockByrefAddress(const DbgVariable &DV, DIE &Die,
   // DW_OP_plus_uconst ForwardingFieldOffset.  Note there's no point in
   // adding the offset if it's 0.
   if (forwardingFieldOffset > 0) {
-    Ops.push_back(dwarf::DW_OP_plus);
+    Ops.push_back(dwarf::DW_OP_plus_uconst);
     Ops.push_back(forwardingFieldOffset);
   }
 
@@ -499,7 +499,7 @@ void DwarfUnit::addBlockByrefAddress(const DbgVariable &DV, DIE &Die,
   // for the variable's field to get to the location of the actual variable:
   // DW_OP_plus_uconst varFieldOffset.  Again, don't add if it's 0.
   if (varFieldOffset > 0) {
-    Ops.push_back(dwarf::DW_OP_plus);
+    Ops.push_back(dwarf::DW_OP_plus_uconst);
     Ops.push_back(varFieldOffset);
   }
 
diff --git a/llvm/lib/IR/DebugInfoMetadata.cpp b/llvm/lib/IR/DebugInfoMetadata.cpp
index a34f8d9eac1..0bf68b4c53b 100644
--- a/llvm/lib/IR/DebugInfoMetadata.cpp
+++ b/llvm/lib/IR/DebugInfoMetadata.cpp
@@ -598,9 +598,7 @@ unsigned DIExpression::ExprOperand::getSize() const {
   case dwarf::DW_OP_LLVM_fragment:
     return 3;
   case dwarf::DW_OP_constu:
-  case dwarf::DW_OP_plus:
   case dwarf::DW_OP_plus_uconst:
-  case dwarf::DW_OP_minus:
     return 2;
   default:
     return 1;
@@ -666,11 +664,12 @@ DIExpression::getFragmentInfo(expr_op_iterator Start, expr_op_iterator End) {
 void DIExpression::appendOffset(SmallVectorImpl<uint64_t> &Ops,
                                 int64_t Offset) {
   if (Offset > 0) {
-    Ops.push_back(dwarf::DW_OP_plus);
+    Ops.push_back(dwarf::DW_OP_plus_uconst);
     Ops.push_back(Offset);
   } else if (Offset < 0) {
-    Ops.push_back(dwarf::DW_OP_minus);
+    Ops.push_back(dwarf::DW_OP_constu);
     Ops.push_back(-Offset);
+    Ops.push_back(dwarf::DW_OP_minus);
   }
 }
 
@@ -679,17 +678,23 @@ bool DIExpression::extractIfOffset(int64_t &Offset) const {
     Offset = 0;
     return true;
   }
-  if (getNumElements() != 2)
-    return false;
-  if (Elements[0] == dwarf::DW_OP_plus ||
-      Elements[0] == dwarf::DW_OP_plus_uconst) {
+
+  if (getNumElements() == 2 && Elements[0] == dwarf::DW_OP_plus_uconst) {
     Offset = Elements[1];
     return true;
   }
-  if (Elements[0] == dwarf::DW_OP_minus) {
-    Offset = -Elements[1];
-    return true;
+
+  if (getNumElements() == 3 && Elements[0] == dwarf::DW_OP_constu) {
+    if (Elements[2] == dwarf::DW_OP_plus) {
+      Offset = Elements[1];
+      return true;
+    }
+    if (Elements[2] == dwarf::DW_OP_minus) {
+      Offset = -Elements[1];
+      return true;
+    }
   }
+
   return false;
 }
 
diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp
index 0b1bc9a8c27..92e5798dcf2 100644
--- a/llvm/lib/IR/Metadata.cpp
+++ b/llvm/lib/IR/Metadata.cpp
@@ -1470,7 +1470,7 @@ void GlobalObject::copyMetadata(const GlobalObject *Other, unsigned Offset) {
       if (E)
         OrigElements = E->getElements();
       std::vector<uint64_t> Elements(OrigElements.size() + 2);
-      Elements[0] = dwarf::DW_OP_plus;
+      Elements[0] = dwarf::DW_OP_plus_uconst;
       Elements[1] = Offset;
       std::copy(OrigElements.begin(), OrigElements.end(), Elements.begin() + 2);
       E = DIExpression::get(getContext(), Elements);