Skip to content

Commit 941fa75

Browse files
adrian-prantladrian-prantl
committed
[DIExpression] Introduce a dedicated DW_OP_LLVM_fragment operation
so we can stop using DW_OP_bit_piece with the wrong semantics. The entire back story can be found here: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20161114/405934.html The gist is that in LLVM we've been misinterpreting DW_OP_bit_piece's offset field to mean the offset into the source variable rather than the offset into the location at the top the DWARF expression stack. In order to be able to fix this in a subsequent patch, this patch introduces a dedicated DW_OP_LLVM_fragment operation with the semantics that we used to apply to DW_OP_bit_piece, which is what we actually need while inside of LLVM. This patch is complete with a bitcode upgrade for expressions using the old format. It does not yet fix the DWARF backend to use DW_OP_bit_piece correctly. Implementation note: We discussed several options for implementing this, including reserving a dedicated field in DIExpression for the fragment size and offset, but using an custom operator at the end of the expression works just fine and is more efficient because we then only pay for it when we need it. Differential Revision: https://reviews.llvm.org/D27361 rdar://problem/29335809 llvm-svn: 288683
1 parent fc78d7c commit 941fa75

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

52 files changed

+319
-276
lines changed

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -517,7 +517,7 @@ namespace llvm {
517517
///
518518
/// \param OffsetInBits Offset of the piece in bits.
519519
/// \param SizeInBits Size of the piece in bits.
520-
DIExpression *createBitPieceExpression(unsigned OffsetInBits,
520+
DIExpression *createFragmentExpression(unsigned OffsetInBits,
521521
unsigned SizeInBits);
522522

523523
/// Create an expression for a variable that does not have an address, but

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

+5-5
Original file line numberDiff line numberDiff line change
@@ -1955,13 +1955,13 @@ class DIExpression : public MDNode {
19551955
}
19561956

19571957
/// Return whether this is a piece of an aggregate variable.
1958-
bool isBitPiece() const;
1958+
bool isFragment() const;
19591959

1960-
/// Return the offset of this piece in bits.
1961-
uint64_t getBitPieceOffset() const;
1960+
/// Return the offset of this fragment in bits.
1961+
uint64_t getFragmentOffsetInBits() const;
19621962

1963-
/// Return the size of this piece in bits.
1964-
uint64_t getBitPieceSize() const;
1963+
/// Return the size of this fragment in bits.
1964+
uint64_t getFragmentSizeInBits() const;
19651965

19661966
typedef ArrayRef<uint64_t>::iterator element_iterator;
19671967
element_iterator elements_begin() const { return getElements().begin(); }

Diff for: ‎llvm/include/llvm/Support/Dwarf.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,8 @@ enum LocationAtom {
108108
#define HANDLE_DW_OP(ID, NAME) DW_OP_##NAME = ID,
109109
#include "llvm/Support/Dwarf.def"
110110
DW_OP_lo_user = 0xe0,
111-
DW_OP_hi_user = 0xff
111+
DW_OP_hi_user = 0xff,
112+
DW_OP_LLVM_fragment = 0x1000 ///< Only used in LLVM metadata.
112113
};
113114

114115
enum TypeKind {

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

+8-1
Original file line numberDiff line numberDiff line change
@@ -3000,7 +3000,14 @@ Error BitcodeReader::parseMetadata(bool ModuleLevel) {
30003000
if (Record.size() < 1)
30013001
return error("Invalid record");
30023002

3003-
IsDistinct = Record[0];
3003+
IsDistinct = Record[0] & 1;
3004+
bool HasOpFragment = Record[0] & 2;
3005+
auto Elts = MutableArrayRef<uint64_t>(Record).slice(1);
3006+
if (!HasOpFragment)
3007+
if (unsigned N = Elts.size())
3008+
if (N >= 3 && Elts[N - 3] == dwarf::DW_OP_bit_piece)
3009+
Elts[N-3] = dwarf::DW_OP_LLVM_fragment;
3010+
30043011
MetadataList.assignValue(
30053012
GET_OR_DISTINCT(DIExpression,
30063013
(Context, makeArrayRef(Record).slice(1))),

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -1727,7 +1727,8 @@ void ModuleBitcodeWriter::writeDIExpression(const DIExpression *N,
17271727
unsigned Abbrev) {
17281728
Record.reserve(N->getElements().size() + 1);
17291729

1730-
Record.push_back(N->isDistinct());
1730+
const uint64_t HasOpFragmentFlag = 1 << 1;
1731+
Record.push_back(N->isDistinct() | HasOpFragmentFlag);
17311732
Record.append(N->elements_begin(), N->elements_end());
17321733

17331734
Stream.EmitRecord(bitc::METADATA_EXPRESSION, Record, Abbrev);

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

+4-4
Original file line numberDiff line numberDiff line change
@@ -713,9 +713,9 @@ static bool emitDebugValueComment(const MachineInstr *MI, AsmPrinter &AP) {
713713
OS << V->getName();
714714

715715
const DIExpression *Expr = MI->getDebugExpression();
716-
if (Expr->isBitPiece())
717-
OS << " [bit_piece offset=" << Expr->getBitPieceOffset()
718-
<< " size=" << Expr->getBitPieceSize() << "]";
716+
if (Expr->isFragment())
717+
OS << " [fragment offset=" << Expr->getFragmentOffsetInBits()
718+
<< " size=" << Expr->getFragmentSizeInBits() << "]";
719719
OS << " <- ";
720720

721721
// The second operand is only an offset if it's an immediate.
@@ -724,7 +724,7 @@ static bool emitDebugValueComment(const MachineInstr *MI, AsmPrinter &AP) {
724724

725725
for (unsigned i = 0; i < Expr->getNumElements(); ++i) {
726726
uint64_t Op = Expr->getElement(i);
727-
if (Op == dwarf::DW_OP_bit_piece) {
727+
if (Op == dwarf::DW_OP_LLVM_fragment) {
728728
// There can't be any operands after this in a valid expression
729729
break;
730730
} else if (Deref) {

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -191,8 +191,8 @@ void AsmPrinter::EmitDwarfRegOp(ByteStreamer &Streamer,
191191
"nop (could not find a dwarf register number)");
192192

193193
// Attempt to find a valid super- or sub-register.
194-
if (!Expr.AddMachineRegPiece(*MF->getSubtarget().getRegisterInfo(),
195-
MLoc.getReg()))
194+
if (!Expr.AddMachineRegFragment(*MF->getSubtarget().getRegisterInfo(),
195+
MLoc.getReg()))
196196
Expr.EmitOp(dwarf::DW_OP_nop,
197197
"nop (could not find a dwarf register number)");
198198
return;

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

+5-4
Original file line numberDiff line numberDiff line change
@@ -943,10 +943,10 @@ void CodeViewDebug::collectVariableInfo(const DISubprogram *SP) {
943943
bool IsSubfield = false;
944944
unsigned StructOffset = 0;
945945

946-
// Handle bitpieces.
947-
if (DIExpr && DIExpr->isBitPiece()) {
946+
// Handle fragments.
947+
if (DIExpr && DIExpr->isFragment()) {
948948
IsSubfield = true;
949-
StructOffset = DIExpr->getBitPieceOffset() / 8;
949+
StructOffset = DIExpr->getFragmentOffsetInBits() / 8;
950950
} else if (DIExpr && DIExpr->getNumElements() > 0) {
951951
continue; // Ignore unrecognized exprs.
952952
}
@@ -985,7 +985,8 @@ void CodeViewDebug::collectVariableInfo(const DISubprogram *SP) {
985985
// This range is valid until the next overlapping bitpiece. In the
986986
// common case, ranges will not be bitpieces, so they will overlap.
987987
auto J = std::next(I);
988-
while (J != E && !piecesOverlap(DIExpr, J->first->getDebugExpression()))
988+
while (J != E &&
989+
!fragmentsOverlap(DIExpr, J->first->getDebugExpression()))
989990
++J;
990991
if (J != E)
991992
End = getLabelBeforeInsn(J->first);

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

+16-17
Original file line numberDiff line numberDiff line change
@@ -63,14 +63,12 @@ MCSymbol *DebugHandlerBase::getLabelAfterInsn(const MachineInstr *MI) {
6363
return LabelsAfterInsn.lookup(MI);
6464
}
6565

66-
// Determine the relative position of the pieces described by P1 and P2.
67-
// Returns -1 if P1 is entirely before P2, 0 if P1 and P2 overlap,
68-
// 1 if P1 is entirely after P2.
69-
int DebugHandlerBase::pieceCmp(const DIExpression *P1, const DIExpression *P2) {
70-
unsigned l1 = P1->getBitPieceOffset();
71-
unsigned l2 = P2->getBitPieceOffset();
72-
unsigned r1 = l1 + P1->getBitPieceSize();
73-
unsigned r2 = l2 + P2->getBitPieceSize();
66+
int DebugHandlerBase::fragmentCmp(const DIExpression *P1,
67+
const DIExpression *P2) {
68+
unsigned l1 = P1->getFragmentOffsetInBits();
69+
unsigned l2 = P2->getFragmentOffsetInBits();
70+
unsigned r1 = l1 + P1->getFragmentSizeInBits();
71+
unsigned r2 = l2 + P2->getFragmentSizeInBits();
7472
if (r1 <= l2)
7573
return -1;
7674
else if (r2 <= l1)
@@ -79,11 +77,11 @@ int DebugHandlerBase::pieceCmp(const DIExpression *P1, const DIExpression *P2) {
7977
return 0;
8078
}
8179

82-
/// Determine whether two variable pieces overlap.
83-
bool DebugHandlerBase::piecesOverlap(const DIExpression *P1, const DIExpression *P2) {
84-
if (!P1->isBitPiece() || !P2->isBitPiece())
80+
bool DebugHandlerBase::fragmentsOverlap(const DIExpression *P1,
81+
const DIExpression *P2) {
82+
if (!P1->isFragment() || !P2->isFragment())
8583
return true;
86-
return pieceCmp(P1, P2) == 0;
84+
return fragmentCmp(P1, P2) == 0;
8785
}
8886

8987
/// If this type is derived from a base type then return base type size.
@@ -142,14 +140,15 @@ void DebugHandlerBase::beginFunction(const MachineFunction *MF) {
142140
if (DIVar->isParameter() &&
143141
getDISubprogram(DIVar->getScope())->describes(MF->getFunction())) {
144142
LabelsBeforeInsn[Ranges.front().first] = Asm->getFunctionBegin();
145-
if (Ranges.front().first->getDebugExpression()->isBitPiece()) {
146-
// Mark all non-overlapping initial pieces.
143+
if (Ranges.front().first->getDebugExpression()->isFragment()) {
144+
// Mark all non-overlapping initial fragments.
147145
for (auto I = Ranges.begin(); I != Ranges.end(); ++I) {
148-
const DIExpression *Piece = I->first->getDebugExpression();
146+
const DIExpression *Fragment = I->first->getDebugExpression();
149147
if (std::all_of(Ranges.begin(), I,
150148
[&](DbgValueHistoryMap::InstrRange Pred) {
151-
return !piecesOverlap(Piece, Pred.first->getDebugExpression());
152-
}))
149+
return !fragmentsOverlap(
150+
Fragment, Pred.first->getDebugExpression());
151+
}))
153152
LabelsBeforeInsn[I->first] = Asm->getFunctionBegin();
154153
else
155154
break;

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

+6-6
Original file line numberDiff line numberDiff line change
@@ -92,13 +92,13 @@ class DebugHandlerBase : public AsmPrinterHandler {
9292
/// Return Label immediately following the instruction.
9393
MCSymbol *getLabelAfterInsn(const MachineInstr *MI);
9494

95-
/// Determine the relative position of the pieces described by P1 and P2.
96-
/// Returns -1 if P1 is entirely before P2, 0 if P1 and P2 overlap,
97-
/// 1 if P1 is entirely after P2.
98-
static int pieceCmp(const DIExpression *P1, const DIExpression *P2);
95+
/// Determine the relative position of the fragments described by P1 and P2.
96+
/// Returns -1 if P1 is entirely before P2, 0 if P1 and P2 overlap, 1 if P1 is
97+
/// entirely after P2.
98+
static int fragmentCmp(const DIExpression *P1, const DIExpression *P2);
9999

100-
/// Determine whether two variable pieces overlap.
101-
static bool piecesOverlap(const DIExpression *P1, const DIExpression *P2);
100+
/// Determine whether two variable fragments overlap.
101+
static bool fragmentsOverlap(const DIExpression *P1, const DIExpression *P2);
102102

103103
/// If this type is derived from a base type then return base type size.
104104
static uint64_t getBaseTypeSize(const DITypeRef TyRef);

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

+5-5
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ class DebugLocEntry {
7272
const ConstantFP *getConstantFP() const { return Constant.CFP; }
7373
const ConstantInt *getConstantInt() const { return Constant.CIP; }
7474
MachineLocation getLoc() const { return Loc; }
75-
bool isBitPiece() const { return getExpression()->isBitPiece(); }
75+
bool isFragment() const { return getExpression()->isFragment(); }
7676
const DIExpression *getExpression() const { return Expression; }
7777
friend bool operator==(const Value &, const Value &);
7878
friend bool operator<(const Value &, const Value &);
@@ -129,7 +129,7 @@ class DebugLocEntry {
129129
Values.append(Vals.begin(), Vals.end());
130130
sortUniqueValues();
131131
assert(all_of(Values, [](DebugLocEntry::Value V) {
132-
return V.isBitPiece();
132+
return V.isFragment();
133133
}) && "value must be a piece");
134134
}
135135

@@ -172,11 +172,11 @@ inline bool operator==(const DebugLocEntry::Value &A,
172172
llvm_unreachable("unhandled EntryKind");
173173
}
174174

175-
/// \brief Compare two pieces based on their offset.
175+
/// Compare two fragments based on their offset.
176176
inline bool operator<(const DebugLocEntry::Value &A,
177177
const DebugLocEntry::Value &B) {
178-
return A.getExpression()->getBitPieceOffset() <
179-
B.getExpression()->getBitPieceOffset();
178+
return A.getExpression()->getFragmentOffsetInBits() <
179+
B.getExpression()->getFragmentOffsetInBits();
180180
}
181181

182182
}

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -730,7 +730,7 @@ void DwarfCompileUnit::addAddress(DIE &Die, dwarf::Attribute Attribute,
730730

731731
bool validReg;
732732
if (Location.isReg())
733-
validReg = addRegisterOpPiece(*Loc, Location.getReg());
733+
validReg = addRegisterFragment(*Loc, Location.getReg());
734734
else
735735
validReg = addRegisterOffset(*Loc, Location.getReg(), Location.getOffset());
736736

0 commit comments

Comments
 (0)