Skip to content

Commit ffc498d

Browse files
fhahnfhahn
committedJun 14, 2017
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
1 parent 3f25038 commit ffc498d

33 files changed

+200
-114
lines changed
 

Diff for: ‎llvm/docs/LangRef.rst

+7-3
Original file line numberDiff line numberDiff line change
@@ -4404,7 +4404,11 @@ referenced LLVM variable relates to the source language variable.
44044404
The current supported vocabulary is limited:
44054405

44064406
- ``DW_OP_deref`` dereferences the top of the expression stack.
4407-
- ``DW_OP_plus, 93`` adds ``93`` to the working expression.
4407+
- ``DW_OP_plus`` pops the last two entries from the expression stack, adds
4408+
them together and appends the result to the expression stack.
4409+
- ``DW_OP_minus`` pops the last two entries from the expression stack, subtracts
4410+
the last entry from the second last entry and appends the result to the
4411+
expression stack.
44084412
- ``DW_OP_plus_uconst, 93`` adds ``93`` to the working expression.
44094413
- ``DW_OP_LLVM_fragment, 16, 8`` specifies the offset and size (``16`` and ``8``
44104414
here, respectively) of the variable fragment from the working expression. Note
@@ -4427,10 +4431,10 @@ combined with a concrete location.
44274431
.. code-block:: llvm
44284432
44294433
!0 = !DIExpression(DW_OP_deref)
4430-
!1 = !DIExpression(DW_OP_plus, 3)
44314434
!1 = !DIExpression(DW_OP_plus_uconst, 3)
4435+
!1 = !DIExpression(DW_OP_constu, 3, DW_OP_plus)
44324436
!2 = !DIExpression(DW_OP_bit_piece, 3, 7)
4433-
!3 = !DIExpression(DW_OP_deref, DW_OP_plus, 3, DW_OP_LLVM_fragment, 3, 7)
4437+
!3 = !DIExpression(DW_OP_deref, DW_OP_constu, 3, DW_OP_plus, DW_OP_LLVM_fragment, 3, 7)
44344438
!4 = !DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef)
44354439
!5 = !DIExpression(DW_OP_constu, 42, DW_OP_stack_value)
44364440

Diff for: ‎llvm/include/llvm/IR/DebugInfoMetadata.h

-3
Original file line numberDiff line numberDiff line change
@@ -2117,9 +2117,6 @@ class DIVariable : public DINode {
21172117
/// variable, or the location of a single piece of a variable, or (when using
21182118
/// DW_OP_stack_value) is the constant variable value.
21192119
///
2120-
/// FIXME: Instead of DW_OP_plus taking an argument, this should use DW_OP_const
2121-
/// and have DW_OP_plus consume the topmost elements on the stack.
2122-
///
21232120
/// TODO: Co-allocate the expression elements.
21242121
/// TODO: Separate from MDNode, or otherwise drop Distinct and Temporary
21252122
/// storage types.

Diff for: ‎llvm/lib/Bitcode/Reader/MetadataLoader.cpp

+92-31
Original file line numberDiff line numberDiff line change
@@ -407,6 +407,11 @@ void PlaceholderQueue::flush(BitcodeReaderMetadataList &MetadataList) {
407407

408408
} // anonynous namespace
409409

410+
static Error error(const Twine &Message) {
411+
return make_error<StringError>(
412+
Message, make_error_code(BitcodeError::CorruptedBitcode));
413+
}
414+
410415
class MetadataLoader::MetadataLoaderImpl {
411416
BitcodeReaderMetadataList MetadataList;
412417
BitcodeReaderValueList &ValueList;
@@ -533,6 +538,88 @@ class MetadataLoader::MetadataLoaderImpl {
533538
}
534539
}
535540

541+
/// Upgrade the expression from previous versions.
542+
Error upgradeDIExpression(uint64_t FromVersion,
543+
MutableArrayRef<uint64_t> &Expr,
544+
SmallVectorImpl<uint64_t> &Buffer) {
545+
auto N = Expr.size();
546+
switch (FromVersion) {
547+
default:
548+
return error("Invalid record");
549+
case 0:
550+
if (N >= 3 && Expr[N - 3] == dwarf::DW_OP_bit_piece)
551+
Expr[N - 3] = dwarf::DW_OP_LLVM_fragment;
552+
LLVM_FALLTHROUGH;
553+
case 1:
554+
// Move DW_OP_deref to the end.
555+
if (N && Expr[0] == dwarf::DW_OP_deref) {
556+
auto End = Expr.end();
557+
if (Expr.size() >= 3 &&
558+
*std::prev(End, 3) == dwarf::DW_OP_LLVM_fragment)
559+
End = std::prev(End, 3);
560+
std::move(std::next(Expr.begin()), End, Expr.begin());
561+
*std::prev(End) = dwarf::DW_OP_deref;
562+
}
563+
NeedDeclareExpressionUpgrade = true;
564+
LLVM_FALLTHROUGH;
565+
case 2: {
566+
// Change DW_OP_plus to DW_OP_plus_uconst.
567+
// Change DW_OP_minus to DW_OP_uconst, DW_OP_minus
568+
auto SubExpr = ArrayRef<uint64_t>(Expr);
569+
while (!SubExpr.empty()) {
570+
// Skip past other operators with their operands
571+
// for this version of the IR, obtained from
572+
// from historic DIExpression::ExprOperand::getSize().
573+
size_t HistoricSize;
574+
switch (SubExpr.front()) {
575+
default:
576+
HistoricSize = 1;
577+
break;
578+
case dwarf::DW_OP_constu:
579+
case dwarf::DW_OP_minus:
580+
case dwarf::DW_OP_plus:
581+
HistoricSize = 2;
582+
break;
583+
case dwarf::DW_OP_LLVM_fragment:
584+
HistoricSize = 3;
585+
break;
586+
}
587+
588+
// If the expression is malformed, make sure we don't
589+
// copy more elements than we should.
590+
HistoricSize = std::min(SubExpr.size(), HistoricSize);
591+
ArrayRef<uint64_t> Args = SubExpr.slice(1, HistoricSize-1);
592+
593+
switch (SubExpr.front()) {
594+
case dwarf::DW_OP_plus:
595+
Buffer.push_back(dwarf::DW_OP_plus_uconst);
596+
Buffer.append(Args.begin(), Args.end());
597+
break;
598+
case dwarf::DW_OP_minus:
599+
Buffer.push_back(dwarf::DW_OP_constu);
600+
Buffer.append(Args.begin(), Args.end());
601+
Buffer.push_back(dwarf::DW_OP_minus);
602+
break;
603+
default:
604+
Buffer.push_back(*SubExpr.begin());
605+
Buffer.append(Args.begin(), Args.end());
606+
break;
607+
}
608+
609+
// Continue with remaining elements.
610+
SubExpr = SubExpr.slice(HistoricSize);
611+
}
612+
Expr = MutableArrayRef<uint64_t>(Buffer);
613+
LLVM_FALLTHROUGH;
614+
}
615+
case 3:
616+
// Up-to-date!
617+
break;
618+
}
619+
620+
return Error::success();
621+
}
622+
536623
void upgradeDebugInfo() {
537624
upgradeCUSubprograms();
538625
upgradeCUVariables();
@@ -590,11 +677,6 @@ class MetadataLoader::MetadataLoaderImpl {
590677
void upgradeDebugIntrinsics(Function &F) { upgradeDeclareExpressions(F); }
591678
};
592679

593-
static Error error(const Twine &Message) {
594-
return make_error<StringError>(
595-
Message, make_error_code(BitcodeError::CorruptedBitcode));
596-
}
597-
598680
Expected<bool>
599681
MetadataLoader::MetadataLoaderImpl::lazyLoadModuleMetadataBlock() {
600682
IndexCursor = Stream;
@@ -1551,34 +1633,13 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata(
15511633
IsDistinct = Record[0] & 1;
15521634
uint64_t Version = Record[0] >> 1;
15531635
auto Elts = MutableArrayRef<uint64_t>(Record).slice(1);
1554-
unsigned N = Elts.size();
1555-
// Perform various upgrades.
1556-
switch (Version) {
1557-
case 0:
1558-
if (N >= 3 && Elts[N - 3] == dwarf::DW_OP_bit_piece)
1559-
Elts[N - 3] = dwarf::DW_OP_LLVM_fragment;
1560-
LLVM_FALLTHROUGH;
1561-
case 1:
1562-
// Move DW_OP_deref to the end.
1563-
if (N && Elts[0] == dwarf::DW_OP_deref) {
1564-
auto End = Elts.end();
1565-
if (Elts.size() >= 3 && *std::prev(End, 3) == dwarf::DW_OP_LLVM_fragment)
1566-
End = std::prev(End, 3);
1567-
std::move(std::next(Elts.begin()), End, Elts.begin());
1568-
*std::prev(End) = dwarf::DW_OP_deref;
1569-
}
1570-
NeedDeclareExpressionUpgrade = true;
1571-
LLVM_FALLTHROUGH;
1572-
case 2:
1573-
// Up-to-date!
1574-
break;
1575-
default:
1576-
return error("Invalid record");
1577-
}
1636+
1637+
SmallVector<uint64_t, 6> Buffer;
1638+
if (Error Err = upgradeDIExpression(Version, Elts, Buffer))
1639+
return Err;
15781640

15791641
MetadataList.assignValue(
1580-
GET_OR_DISTINCT(DIExpression, (Context, makeArrayRef(Record).slice(1))),
1581-
NextMetadataNo);
1642+
GET_OR_DISTINCT(DIExpression, (Context, Elts)), NextMetadataNo);
15821643
NextMetadataNo++;
15831644
break;
15841645
}

Diff for: ‎llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -1663,7 +1663,7 @@ void ModuleBitcodeWriter::writeDIExpression(const DIExpression *N,
16631663
SmallVectorImpl<uint64_t> &Record,
16641664
unsigned Abbrev) {
16651665
Record.reserve(N->getElements().size() + 1);
1666-
const uint64_t Version = 2 << 1;
1666+
const uint64_t Version = 3 << 1;
16671667
Record.push_back((uint64_t)N->isDistinct() | Version);
16681668
Record.append(N->elements_begin(), N->elements_end());
16691669

Diff for: ‎llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -552,7 +552,7 @@ DIE *DwarfCompileUnit::constructVariableDIEImpl(const DbgVariable &DV,
552552
int Offset = TFI->getFrameIndexReference(*Asm->MF, Fragment.FI, FrameReg);
553553
DwarfExpr.addFragmentOffset(Expr);
554554
SmallVector<uint64_t, 8> Ops;
555-
Ops.push_back(dwarf::DW_OP_plus);
555+
Ops.push_back(dwarf::DW_OP_plus_uconst);
556556
Ops.push_back(Offset);
557557
Ops.append(Expr->elements_begin(), Expr->elements_end());
558558
DIExpressionCursor Cursor(Ops);
@@ -821,7 +821,7 @@ void DwarfCompileUnit::addAddress(DIE &Die, dwarf::Attribute Attribute,
821821

822822
SmallVector<uint64_t, 8> Ops;
823823
if (Location.isIndirect() && Location.getOffset()) {
824-
Ops.push_back(dwarf::DW_OP_plus);
824+
Ops.push_back(dwarf::DW_OP_plus_uconst);
825825
Ops.push_back(Location.getOffset());
826826
}
827827
DIExpressionCursor Cursor(Ops);
@@ -850,7 +850,7 @@ void DwarfCompileUnit::addComplexAddress(const DbgVariable &DV, DIE &Die,
850850

851851
SmallVector<uint64_t, 8> Ops;
852852
if (Location.isIndirect() && Location.getOffset()) {
853-
Ops.push_back(dwarf::DW_OP_plus);
853+
Ops.push_back(dwarf::DW_OP_plus_uconst);
854854
Ops.push_back(Location.getOffset());
855855
}
856856
Ops.append(DIExpr->elements_begin(), DIExpr->elements_end());

Diff for: ‎llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -1511,7 +1511,7 @@ static void emitDebugLocValue(const AsmPrinter &AP, const DIBasicType *BT,
15111511
DwarfExpr.setMemoryLocationKind();
15121512
SmallVector<uint64_t, 8> Ops;
15131513
if (Location.isIndirect() && Location.getOffset()) {
1514-
Ops.push_back(dwarf::DW_OP_plus);
1514+
Ops.push_back(dwarf::DW_OP_plus_uconst);
15151515
Ops.push_back(Location.getOffset());
15161516
}
15171517
Ops.append(DIExpr->elements_begin(), DIExpr->elements_end());

Diff for: ‎llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

+13-13
Original file line numberDiff line numberDiff line change
@@ -254,15 +254,19 @@ bool DwarfExpression::addMachineRegExpression(const TargetRegisterInfo &TRI,
254254
ExprCursor.take();
255255
}
256256

257-
// [Reg, Offset, DW_OP_plus] --> [DW_OP_breg, Offset].
258-
// [Reg, Offset, DW_OP_minus] --> [DW_OP_breg, -Offset].
257+
// [Reg, DW_OP_constu, Offset, DW_OP_plus] --> [DW_OP_breg, Offset]
258+
// [Reg, DW_OP_constu, Offset, DW_OP_minus] --> [DW_OP_breg,-Offset]
259259
// If Reg is a subregister we need to mask it out before subtracting.
260-
if (Op && ((Op->getOp() == dwarf::DW_OP_plus) ||
261-
(Op->getOp() == dwarf::DW_OP_minus && !SubRegisterSizeInBits))) {
262-
int Offset = Op->getArg(0);
263-
SignedOffset = (Op->getOp() == dwarf::DW_OP_plus) ? Offset : -Offset;
264-
ExprCursor.take();
260+
if (Op && Op->getOp() == dwarf::DW_OP_constu) {
261+
auto N = ExprCursor.peekNext();
262+
if (N && (N->getOp() == dwarf::DW_OP_plus ||
263+
(N->getOp() == dwarf::DW_OP_minus && !SubRegisterSizeInBits))) {
264+
int Offset = Op->getArg(0);
265+
SignedOffset = (N->getOp() == dwarf::DW_OP_minus) ? -Offset : Offset;
266+
ExprCursor.consume(2);
267+
}
265268
}
269+
266270
if (FBReg)
267271
addFBReg(SignedOffset);
268272
else
@@ -326,18 +330,14 @@ void DwarfExpression::addExpression(DIExpressionCursor &&ExprCursor,
326330
LocationKind = Unknown;
327331
return;
328332
}
329-
case dwarf::DW_OP_plus:
330333
case dwarf::DW_OP_plus_uconst:
331334
assert(LocationKind != Register);
332335
emitOp(dwarf::DW_OP_plus_uconst);
333336
emitUnsigned(Op->getArg(0));
334337
break;
338+
case dwarf::DW_OP_plus:
335339
case dwarf::DW_OP_minus:
336-
assert(LocationKind != Register);
337-
// There is no DW_OP_minus_uconst.
338-
emitOp(dwarf::DW_OP_constu);
339-
emitUnsigned(Op->getArg(0));
340-
emitOp(dwarf::DW_OP_minus);
340+
emitOp(Op->getOp());
341341
break;
342342
case dwarf::DW_OP_deref: {
343343
assert(LocationKind != Register);

Diff for: ‎llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h

+3
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,9 @@ class DIExpressionCursor {
4242
DIExpressionCursor(ArrayRef<uint64_t> Expr)
4343
: Start(Expr.begin()), End(Expr.end()) {}
4444

45+
DIExpressionCursor(const DIExpressionCursor &C)
46+
: Start(C.Start), End(C.End) {}
47+
4548
/// Consume one operation.
4649
Optional<DIExpression::ExprOperand> take() {
4750
if (Start == End)

Diff for: ‎llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,7 @@ void DwarfUnit::addBlockByrefAddress(const DbgVariable &DV, DIE &Die,
475475

476476
SmallVector<uint64_t, 9> Ops;
477477
if (Location.isIndirect() && Location.getOffset()) {
478-
Ops.push_back(dwarf::DW_OP_plus);
478+
Ops.push_back(dwarf::DW_OP_plus_uconst);
479479
Ops.push_back(Location.getOffset());
480480
}
481481
// If we started with a pointer to the __Block_byref... struct, then
@@ -487,7 +487,7 @@ void DwarfUnit::addBlockByrefAddress(const DbgVariable &DV, DIE &Die,
487487
// DW_OP_plus_uconst ForwardingFieldOffset. Note there's no point in
488488
// adding the offset if it's 0.
489489
if (forwardingFieldOffset > 0) {
490-
Ops.push_back(dwarf::DW_OP_plus);
490+
Ops.push_back(dwarf::DW_OP_plus_uconst);
491491
Ops.push_back(forwardingFieldOffset);
492492
}
493493

@@ -499,7 +499,7 @@ void DwarfUnit::addBlockByrefAddress(const DbgVariable &DV, DIE &Die,
499499
// for the variable's field to get to the location of the actual variable:
500500
// DW_OP_plus_uconst varFieldOffset. Again, don't add if it's 0.
501501
if (varFieldOffset > 0) {
502-
Ops.push_back(dwarf::DW_OP_plus);
502+
Ops.push_back(dwarf::DW_OP_plus_uconst);
503503
Ops.push_back(varFieldOffset);
504504
}
505505

Diff for: ‎llvm/lib/IR/DebugInfoMetadata.cpp

+16-11
Original file line numberDiff line numberDiff line change
@@ -598,9 +598,7 @@ unsigned DIExpression::ExprOperand::getSize() const {
598598
case dwarf::DW_OP_LLVM_fragment:
599599
return 3;
600600
case dwarf::DW_OP_constu:
601-
case dwarf::DW_OP_plus:
602601
case dwarf::DW_OP_plus_uconst:
603-
case dwarf::DW_OP_minus:
604602
return 2;
605603
default:
606604
return 1;
@@ -666,11 +664,12 @@ DIExpression::getFragmentInfo(expr_op_iterator Start, expr_op_iterator End) {
666664
void DIExpression::appendOffset(SmallVectorImpl<uint64_t> &Ops,
667665
int64_t Offset) {
668666
if (Offset > 0) {
669-
Ops.push_back(dwarf::DW_OP_plus);
667+
Ops.push_back(dwarf::DW_OP_plus_uconst);
670668
Ops.push_back(Offset);
671669
} else if (Offset < 0) {
672-
Ops.push_back(dwarf::DW_OP_minus);
670+
Ops.push_back(dwarf::DW_OP_constu);
673671
Ops.push_back(-Offset);
672+
Ops.push_back(dwarf::DW_OP_minus);
674673
}
675674
}
676675

@@ -679,17 +678,23 @@ bool DIExpression::extractIfOffset(int64_t &Offset) const {
679678
Offset = 0;
680679
return true;
681680
}
682-
if (getNumElements() != 2)
683-
return false;
684-
if (Elements[0] == dwarf::DW_OP_plus ||
685-
Elements[0] == dwarf::DW_OP_plus_uconst) {
681+
682+
if (getNumElements() == 2 && Elements[0] == dwarf::DW_OP_plus_uconst) {
686683
Offset = Elements[1];
687684
return true;
688685
}
689-
if (Elements[0] == dwarf::DW_OP_minus) {
690-
Offset = -Elements[1];
691-
return true;
686+
687+
if (getNumElements() == 3 && Elements[0] == dwarf::DW_OP_constu) {
688+
if (Elements[2] == dwarf::DW_OP_plus) {
689+
Offset = Elements[1];
690+
return true;
691+
}
692+
if (Elements[2] == dwarf::DW_OP_minus) {
693+
Offset = -Elements[1];
694+
return true;
695+
}
692696
}
697+
693698
return false;
694699
}
695700

Diff for: ‎llvm/lib/IR/Metadata.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -1470,7 +1470,7 @@ void GlobalObject::copyMetadata(const GlobalObject *Other, unsigned Offset) {
14701470
if (E)
14711471
OrigElements = E->getElements();
14721472
std::vector<uint64_t> Elements(OrigElements.size() + 2);
1473-
Elements[0] = dwarf::DW_OP_plus;
1473+
Elements[0] = dwarf::DW_OP_plus_uconst;
14741474
Elements[1] = Offset;
14751475
std::copy(OrigElements.begin(), OrigElements.end(), Elements.begin() + 2);
14761476
E = DIExpression::get(getContext(), Elements);

0 commit comments

Comments
 (0)
Please sign in to comment.