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

[sdesmalen]

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.

[aprantl]

Thanks, this is great! The only thing that is missing here is a bitcode upgrade that transforms the old use of DW_OP_plus to DW_OP_plus_uconst. I recommend incrementing the "Version number" in operand 0 of DIExpression to distinguish between the old and new format. You can look at r300522 for an example of how to do this.

[sdesmalen]

Incremented the expression's Version operand and added backwards compatibility with older bitcode files by constructing a DW_OP_plus_uconst from a DW_OP_plus.

[sdesmalen]

In D33892#772876, @aprantl wrote:

Thanks, this is great! The only thing that is missing here is a bitcode upgrade that transforms the old use of DW_OP_plus to DW_OP_plus_uconst. I recommend incrementing the "Version number" in operand 0 of DIExpression to distinguish between the old and new format. You can look at r300522 for an example of how to do this.

Thanks for pointing this out! I hope my changes to the patch reflect what you suggested.

Initially the diff also contained a binary diff for the changed .bc files, but these are not needed now as the old .bc files check backward compatibility as implemented in the 'upgradeMetadataExpression()' function. If I want to add binary changes to my patch though, how could I add these? (Phabricator failed to accept my initial diff that I created using git diff --binary)

[aprantl]

lib/Bitcode/Reader/MetadataLoader.cpp

536 ↗

(On Diff #101558)

Let's rename this to upgradeDIExpression.

536 ↗

(On Diff #101558)

Also, FromVersion is unused.

550 ↗

(On Diff #101558)

you could break here, since DW_OP_LLVM_fragment must be the last operator in the expression.

559 ↗

(On Diff #101558)

clang-format please

568 ↗

(On Diff #101558)

LLVM coding standard wants a . at the end of each comment.

569 ↗

(On Diff #101558)

Copying the expression here seems to be unnecessary, but I assume the reason is that the DW_OP_minus upgrade cannot be done in place.

1612 ↗

(On Diff #101558)

I would move the entire switch statement into the upgradeMetadataExpression helper.

[aprantl]

lib/Bitcode/Reader/MetadataLoader.cpp

536 ↗

(On Diff #101558)

Let's rename this to upgradeDIExpression.

test/Bitcode/DIExpression-deref.ll

15 ↗

(On Diff #101558)

This seems wrong. This file is supposed to be the disassembled version of the .bc file, so it should contain the old DIExpression to test the bitcode upgrade. (The CHECK is fine though).

[sdesmalen]

Refactoring of upgradeDIExpression()

[sdesmalen]

lib/Bitcode/Reader/MetadataLoader.cpp

549 ↗

(On Diff #101713)

You'll notice that for up-to-date expressions it does an unnecessary copy to NewExpr.
I did this to make the interface to upgradeDIExpression() more clear, i.e. the new expression is always in NewExpr and the old one is left alone. Otherwise the caller would need to figure out where the result is, either in the first or the second argument.

553 ↗

(On Diff #101713)

I'm starting with upgrading from version 2 to 0 (rather than in reverse order), so that we only need to do the 'copy' to NewExpr once.

536 ↗

(On Diff #101558)

This will be used now that I've moved the whole switch statement to upgradeDIExpression().

569 ↗

(On Diff #101558)

correct.

test/Bitcode/DIExpression-deref.ll

15 ↗

(On Diff #101558)

Well spotted! Thanks.
I did the same for DIGlobalVariableExpression.ll (that has a corresponding .bc file too).

[aprantl]

lib/Bitcode/Reader/MetadataLoader.cpp

553 ↗

(On Diff #101713)

I'm not sure I'm comfortable with reversing the order. In the original implementation we incrementally upgrade the expression from version N-1 to version N. Some of the upgrades (including the one that is added in this patch!) are changing how to iterate over the elements, so it is not generally safe to apply the upgrades in reverse order. If we are upgrading in chronological order then a new upgrade doesn't have to care about the old upgrades, but in the reverse order they have to inspect all other upgrades if they are still correct.

I recommend to leave the order as in the original implementation. I wouldn't worry too much about performance — the upgrade path is not the normal use-case.

[sdesmalen]

lib/Bitcode/Reader/MetadataLoader.cpp

553 ↗

(On Diff #101713)

Valid point about changing the order of applying updates!
I've changed it back to the original order.

[aprantl]

lib/Bitcode/Reader/MetadataLoader.cpp

586 ↗

(On Diff #101922)

This will crash if the expression is malformed and ends after the DW_OP_plus.
It would be better to just push a DW_OP_plus_uconst to the buffer and then fall through.

589 ↗

(On Diff #101922)

This will crash if the expression is malformed and ends after the operator.
Can you make sure not copy more elements than are in SubExpr here?

[aprantl]

Almost there! Couple more comments inline.

thanks,
adrian

lib/Bitcode/Reader/MetadataLoader.cpp

579 ↗

(On Diff #101922)

This no longer handling DW_OP_LLVM_fragment?

[sdesmalen]

Make sure not to read more elements than are in the array.

[sdesmalen]

lib/Bitcode/Reader/MetadataLoader.cpp

579 ↗

(On Diff #101922)

Added again.

[aprantl]

Thanks! I noticed that in the remainder of the patch there are still references to DW_OP_plus left, but they are completely untested. I would recommend removing all referenced to DW_OP_plus in this patch and then adding support for the "real" DW_OP_plus back in a separate commit, together with testcases.

lib/Bitcode/Reader/MetadataLoader.cpp

555 ↗

(On Diff #102026)

remove extra newline :-)

575 ↗

(On Diff #102026)

For readability, let's move the HistoricSize = 1 initialization here.

590 ↗

(On Diff #102026)

This could be expressed more compactly as:

auto Op = SubExpr.front();
Buffer.push_back(Op == DW_OP_plus ? dwarf::DW_OP_plus_uconst : Op);
Buffer.append(Args.begin(), Args.end());

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

329–330

Either we handle DW_OP_plus correctly here, or it should be removed.

lib/IR/DebugInfoMetadata.cpp

601–602

This must be removed.

646

This must be removed.

[sdesmalen]

In D33892#776908, @aprantl wrote:

Thanks! I noticed that in the remainder of the patch there are still references to DW_OP_plus left, but they are completely untested. I would recommend removing all referenced to DW_OP_plus in this patch and then adding support for the "real" DW_OP_plus back in a separate commit, together with testcases.

The reason for not yet removing support for DW_OP_plus is because Clang still emits DW_OP_plus, and removing support for DW_OP_plus would break things.

This patch removes a number of test-cases for DW_OP_plus, but some tests remain in MetadataTest.cpp. The plan was to first migrate all (LLVM) uses of DW_OP_plus to DW_OP_plus_uconst in patch [1/3], then to change all Clang uses of DW_OP_plus to DW_OP_plus_uconst in patch [2/3], and then change the meaning of DW_OP_plus in patch [3/3].

[sdesmalen]

Patch [1/3], now just adds support for DW_OP_plus_uconst.
Patch [3/3] changes meaning 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.

[sdesmalen]

lib/Bitcode/Reader/MetadataLoader.cpp

555 ↗

(On Diff #102026)

Perhaps Phabricator doesn't display it properly, but I don't see an extra newline here.
I did remove the extra newline between 'auto N = ..' and the switch statement.

590 ↗

(On Diff #102026)

Now that I have reshuffled the functionality within patches, I kept the less compact notation because the code also supports DW_OP_minus, which needs to be distinguished as a separate case.

[aprantl]

Technically this is missing a DWARF backend test, but since the next review will rewrite all DW_OP_plus to DW_OP_plus_uconst anyway, this is fine.

Thanks!

[aprantl]

It looks like this broke the stage2 build:

http://green.lab.llvm.org/green/job/clang-stage2-configure-Rlto_build/18448/consoleFull#-1569690971a1ca8a51-895e-46c6-af87-ce24fa4cd561

Could you please revert/investigate?

[aprantl]

Hm.. I think the other commit in the blamelist looks far more promising.

[fhahn]

I think the self-hosting builds should be fixed by https://reviews.llvm.org/rL305313

docs/LangRef.rst

 expression language. They are used in :ref:`debug intrinsics<dbg_intrinsics>`
 (such as ``llvm.dbg.declare`` and ``llvm.dbg.value``) to describe how the
 referenced LLVM variable relates to the source language variable.
 
 The current supported vocabulary is limited:
 
 - ``DW_OP_deref`` dereferences the top of the expression stack.
 - ``DW_OP_plus, 93`` adds ``93`` to the working expression.
+- ``DW_OP_plus_uconst, 93`` adds ``93`` to the working expression.
 - ``DW_OP_LLVM_fragment, 16, 8`` specifies the offset and size (``16`` and ``8``
   here, respectively) of the variable fragment from the working expression. Note
   that contrary to DW_OP_bit_piece, the offset is describing the the location
   within the described source variable.
 - ``DW_OP_swap`` swaps top two stack entries.
 - ``DW_OP_xderef`` provides extended dereference mechanism. The entry at the top
   of the stack is treated as an address. The second stack entry is treated as an
   address space identifier.
 - ``DW_OP_stack_value`` marks a constant value.
 
 DWARF specifies three kinds of simple location descriptions: Register, memory,
 and implicit location descriptions. Register and memory location descriptions
 describe the *location* of a source variable (in the sense that a debugger might
 modify its value), whereas implicit locations describe merely the *value* of a
 source variable. DIExpressions also follow this model: A DIExpression that
 doesn't have a trailing ``DW_OP_stack_value`` will describe an *address* when
 combined with a concrete location.
 
 .. code-block:: llvm
 
     !0 = !DIExpression(DW_OP_deref)
     !1 = !DIExpression(DW_OP_plus, 3)
+    !1 = !DIExpression(DW_OP_plus_uconst, 3)
     !2 = !DIExpression(DW_OP_bit_piece, 3, 7)
     !3 = !DIExpression(DW_OP_deref, DW_OP_plus, 3, DW_OP_LLVM_fragment, 3, 7)
     !4 = !DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef)
     !5 = !DIExpression(DW_OP_constu, 42, DW_OP_stack_value)
 
 DIObjCProperty
 """"""""""""""
 

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

bool DwarfExpression::addMachineRegExpression(const TargetRegisterInfo &TRI,
 
   assert(DwarfRegs.size() == 1);
   auto Reg = DwarfRegs[0];
   bool FBReg = isFrameRegister(TRI, MachineReg);
   int SignedOffset = 0;
   assert(Reg.Size == 0 && "subregister has same size as superregister");
 
   // Pattern-match combinations for which more efficient representations exist.
+  // [Reg, DW_OP_plus_uconst, Offset] --> [DW_OP_breg, Offset].
+  if (Op && (Op->getOp() == dwarf::DW_OP_plus_uconst)) {
+    SignedOffset = Op->getArg(0);
+    ExprCursor.take();
+  }
+
   // [Reg, Offset, DW_OP_plus] --> [DW_OP_breg, Offset].
   // [Reg, 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();
case dwarf::DW_OP_LLVM_fragment: {
 
       // Emit the DW_OP_piece.
       addOpPiece(SizeInBits, SubRegisterOffsetInBits);
       setSubRegisterPiece(0, 0);
       // Reset the location description kind.
       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_minus:
       assert(LocationKind != Register);
       // There is no DW_OP_minus_uconst.
       emitOp(dwarf::DW_OP_constu);

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_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;
   }
 }
 
 bool DIExpression::isValid() const {
case dwarf::DW_OP_swap: {
       // DW_LLVM_OP_implicit_location as a placeholder for the location this
       // DIExpression is attached to, or else pass the number of implicit stack
       // elements into isValid.
       if (getNumElements() == 1)
         return false;
       break;
     }
     case dwarf::DW_OP_constu:
+    case dwarf::DW_OP_plus_uconst:
     case dwarf::DW_OP_plus:
     case dwarf::DW_OP_minus:
     case dwarf::DW_OP_deref:
     case dwarf::DW_OP_xderef:
       break;
     }
   }
   return true;
 }

 
 bool DIExpression::extractIfOffset(int64_t &Offset) const {
   if (getNumElements() == 0) {
     Offset = 0;
     return true;
   }
   if (getNumElements() != 2)
     return false;
-  if (Elements[0] == dwarf::DW_OP_plus) {
+  if (Elements[0] == dwarf::DW_OP_plus ||
+      Elements[0] == dwarf::DW_OP_plus_uconst) {
     Offset = Elements[1];
     return true;
   }
   if (Elements[0] == dwarf::DW_OP_minus) {
     Offset = -Elements[1];
     return true;
   }
   return false;

test/Assembler/diexpression.ll

 ; RUN: llvm-as < %s | llvm-dis | llvm-as | llvm-dis | FileCheck %s
 ; RUN: verify-uselistorder %s
 
-; CHECK: !named = !{!0, !1, !2, !3, !4, !5}
-!named = !{!0, !1, !2, !3, !4, !5}
+; CHECK: !named = !{!0, !1, !2, !3, !4, !5, !6}
+!named = !{!0, !1, !2, !3, !4, !5, !6}
 
 ; CHECK:      !0 = !DIExpression()
 ; CHECK-NEXT: !1 = !DIExpression(DW_OP_deref)
 ; CHECK-NEXT: !2 = !DIExpression(DW_OP_plus, 3)
 ; CHECK-NEXT: !3 = !DIExpression(DW_OP_LLVM_fragment, 3, 7)
 ; CHECK-NEXT: !4 = !DIExpression(DW_OP_deref, DW_OP_plus, 3, DW_OP_LLVM_fragment, 3, 7)
 ; CHECK-NEXT: !5 = !DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef)
+; CHECK-NEXT: !6 = !DIExpression(DW_OP_plus_uconst, 3)
 !0 = !DIExpression()
 !1 = !DIExpression(DW_OP_deref)
 !2 = !DIExpression(DW_OP_plus, 3)
 !3 = !DIExpression(DW_OP_LLVM_fragment, 3, 7)
 !4 = !DIExpression(DW_OP_deref, DW_OP_plus, 3, DW_OP_LLVM_fragment, 3, 7)
 !5 = !DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef)
+!6 = !DIExpression(DW_OP_plus_uconst, 3)

unittests/IR/MetadataTest.cpp

do {                                                                         \
     EXPECT_FALSE(DIExpression::get(Context, Elements)->isValid());             \
   } while (false)
 
   // Empty expression should be valid.
   EXPECT_TRUE(DIExpression::get(Context, None));
 
   // Valid constructions.
   EXPECT_VALID(dwarf::DW_OP_plus, 6);
+  EXPECT_VALID(dwarf::DW_OP_plus_uconst, 6);
   EXPECT_VALID(dwarf::DW_OP_deref);
   EXPECT_VALID(dwarf::DW_OP_LLVM_fragment, 3, 7);
   EXPECT_VALID(dwarf::DW_OP_plus, 6, dwarf::DW_OP_deref);
   EXPECT_VALID(dwarf::DW_OP_deref, dwarf::DW_OP_plus, 6);
   EXPECT_VALID(dwarf::DW_OP_deref, dwarf::DW_OP_LLVM_fragment, 3, 7);
   EXPECT_VALID(dwarf::DW_OP_deref, dwarf::DW_OP_plus, 6,
                dwarf::DW_OP_LLVM_fragment, 3, 7);
 
   // Invalid constructions.
   EXPECT_INVALID(~0u);
   EXPECT_INVALID(dwarf::DW_OP_plus);
+  EXPECT_INVALID(dwarf::DW_OP_plus_uconst);
   EXPECT_INVALID(dwarf::DW_OP_LLVM_fragment);
   EXPECT_INVALID(dwarf::DW_OP_LLVM_fragment, 3);
   EXPECT_INVALID(dwarf::DW_OP_LLVM_fragment, 3, 7, dwarf::DW_OP_plus, 3);
   EXPECT_INVALID(dwarf::DW_OP_LLVM_fragment, 3, 7, dwarf::DW_OP_deref);
 
 #undef EXPECT_VALID
 #undef EXPECT_INVALID
 }