Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some more precise debug info improvements #61419

Merged
merged 3 commits into from
Nov 11, 2021
Merged

Some more precise debug info improvements #61419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64db0c9
Some more precise debug info improvements
jakobbotsch Nov 10, 2021
ea04c11
Run jit-format
jakobbotsch Nov 10, 2021
3e53210
Add a space before dumping debug info in LIR
jakobbotsch Nov 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/coreclr/jit/codegenlinear.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,10 +432,14 @@ void CodeGen::genCodeForBBlist()
if (node->OperGet() == GT_IL_OFFSET)
{
GenTreeILOffset* ilOffset = node->AsILOffset();
genEnsureCodeEmitted(currentDI);
currentDI = ilOffset->gtStmtDI;
genIPmappingAdd(IPmappingDscKind::Normal, currentDI, firstMapping);
firstMapping = false;
DebugInfo rootDI = ilOffset->gtStmtDI.GetRoot();
if (rootDI.IsValid())
{
genEnsureCodeEmitted(currentDI);
currentDI = rootDI;
genIPmappingAdd(IPmappingDscKind::Normal, currentDI, firstMapping);
firstMapping = false;
}
#ifdef DEBUG
assert(ilOffset->gtStmtLastILoffs <= compiler->info.compILCodeSize ||
ilOffset->gtStmtLastILoffs == BAD_IL_OFFSET);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/debuginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class ILLocation
return m_isStackEmpty;
}

// Is this a call instruction? Used for managed return values.
bool IsCall() const
{
return m_isCall;
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/jit/fgbasic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4240,10 +4240,10 @@ BasicBlock* Compiler::fgSplitBlockAfterNode(BasicBlock* curr, GenTree* node)
if ((*riter)->gtOper == GT_IL_OFFSET)
{
GenTreeILOffset* ilOffset = (*riter)->AsILOffset();
if (ilOffset->gtStmtDI.IsValid())
DebugInfo rootDI = ilOffset->gtStmtDI.GetRoot();
if (rootDI.IsValid())
{
assert(ilOffset->gtStmtDI.GetInlineContext()->IsRoot());
splitPointILOffset = ilOffset->gtStmtDI.GetLocation().GetOffset();
splitPointILOffset = rootDI.GetLocation().GetOffset();
break;
}
}
Expand Down
11 changes: 2 additions & 9 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11478,15 +11478,8 @@ void Compiler::gtDispLeaf(GenTree* tree, IndentStack* indentStack)
break;

case GT_IL_OFFSET:
printf(" IL offset: ");
if (!tree->AsILOffset()->gtStmtDI.IsValid())
{
printf("???");
}
else
{
printf("0x%x", tree->AsILOffset()->gtStmtDI.GetLocation().GetOffset());
}
printf(" ");
tree->AsILOffset()->gtStmtDI.Dump(true);
break;

case GT_JCC:
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/gtlist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ GTNODE(MADD , GenTreeOp ,0, GTK_BINOP) // Ge
GTNODE(JMPTABLE , GenTree ,0, (GTK_LEAF|GTK_NOCONTAIN)) // Generates the jump table for switches
GTNODE(SWITCH_TABLE , GenTreeOp ,0, (GTK_BINOP|GTK_NOVALUE)) // Jump Table based switch construct
#ifdef TARGET_ARM64
GTNODE(BFIZ, GenTreeBfiz ,0, GTK_BINOP) // Bitfield Insert in Zero
GTNODE(BFIZ , GenTreeOp ,0, GTK_BINOP) // Bitfield Insert in Zero
#endif

//-----------------------------------------------------------------------------
Expand Down
156 changes: 74 additions & 82 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ inline void Compiler::impAppendStmt(Statement* stmt, unsigned chkLevel)
// Once we set the current offset as debug info in an appended tree, we are
// ready to report the following offsets. Note that we need to compare
// offsets here instead of debug info, since we do not set the "is call"
// debug in impCurStmtDI.
// bit in impCurStmtDI.

if (impLastStmt->GetDebugInfo().GetLocation().GetOffset() == impCurStmtDI.GetLocation().GetOffset())
{
Expand Down Expand Up @@ -3021,11 +3021,6 @@ unsigned Compiler::impInitBlockLineInfo()
impCurStmtOffsSet(blockOffs);
}

if (false && (info.compStmtOffsetsImplicit & ICorDebugInfo::CALL_SITE_BOUNDARIES))
{
impCurStmtOffsSet(blockOffs);
}

/* Always report IL offset 0 or some tests get confused.
Probably a good idea anyways */

Expand Down Expand Up @@ -11721,111 +11716,108 @@ void Compiler::impImportBlockCode(BasicBlock* block)
if (opts.compDbgInfo)
#endif
{
if (!compIsForInlining())
{
nxtStmtOffs =
(nxtStmtIndex < info.compStmtOffsetsCount) ? info.compStmtOffsets[nxtStmtIndex] : BAD_IL_OFFSET;
nxtStmtOffs =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I've always though the logic in this bit of code was convoluted. But it looks like all that's happened here is that you removed the exclusion for inlinees?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the only change here (it's easier to see if you check "Hide whitespace" under the review settings).

(nxtStmtIndex < info.compStmtOffsetsCount) ? info.compStmtOffsets[nxtStmtIndex] : BAD_IL_OFFSET;

/* Have we reached the next stmt boundary ? */

/* Have we reached the next stmt boundary ? */
if (nxtStmtOffs != BAD_IL_OFFSET && opcodeOffs >= nxtStmtOffs)
{
assert(nxtStmtOffs == info.compStmtOffsets[nxtStmtIndex]);

if (nxtStmtOffs != BAD_IL_OFFSET && opcodeOffs >= nxtStmtOffs)
if (verCurrentState.esStackDepth != 0 && opts.compDbgCode)
{
assert(nxtStmtOffs == info.compStmtOffsets[nxtStmtIndex]);
/* We need to provide accurate IP-mapping at this point.
So spill anything on the stack so that it will form
gtStmts with the correct stmt offset noted */

if (verCurrentState.esStackDepth != 0 && opts.compDbgCode)
{
/* We need to provide accurate IP-mapping at this point.
So spill anything on the stack so that it will form
gtStmts with the correct stmt offset noted */
impSpillStackEnsure(true);
}

impSpillStackEnsure(true);
}
// Have we reported debug info for any tree?

// Have we reported debug info for any tree?
if (impCurStmtDI.IsValid() && opts.compDbgCode)
{
GenTree* placeHolder = new (this, GT_NO_OP) GenTree(GT_NO_OP, TYP_VOID);
impAppendTree(placeHolder, (unsigned)CHECK_SPILL_NONE, impCurStmtDI);

if (impCurStmtDI.IsValid() && opts.compDbgCode)
{
GenTree* placeHolder = new (this, GT_NO_OP) GenTree(GT_NO_OP, TYP_VOID);
impAppendTree(placeHolder, (unsigned)CHECK_SPILL_NONE, impCurStmtDI);
assert(!impCurStmtDI.IsValid());
}

assert(!impCurStmtDI.IsValid());
}
if (!impCurStmtDI.IsValid())
{
/* Make sure that nxtStmtIndex is in sync with opcodeOffs.
If opcodeOffs has gone past nxtStmtIndex, catch up */

if (!impCurStmtDI.IsValid())
while ((nxtStmtIndex + 1) < info.compStmtOffsetsCount &&
info.compStmtOffsets[nxtStmtIndex + 1] <= opcodeOffs)
{
/* Make sure that nxtStmtIndex is in sync with opcodeOffs.
If opcodeOffs has gone past nxtStmtIndex, catch up */

while ((nxtStmtIndex + 1) < info.compStmtOffsetsCount &&
info.compStmtOffsets[nxtStmtIndex + 1] <= opcodeOffs)
{
nxtStmtIndex++;
}
nxtStmtIndex++;
}

/* Go to the new stmt */
/* Go to the new stmt */

impCurStmtOffsSet(info.compStmtOffsets[nxtStmtIndex]);
impCurStmtOffsSet(info.compStmtOffsets[nxtStmtIndex]);

/* Update the stmt boundary index */
/* Update the stmt boundary index */

nxtStmtIndex++;
assert(nxtStmtIndex <= info.compStmtOffsetsCount);
nxtStmtIndex++;
assert(nxtStmtIndex <= info.compStmtOffsetsCount);

/* Are there any more line# entries after this one? */
/* Are there any more line# entries after this one? */

if (nxtStmtIndex < info.compStmtOffsetsCount)
{
/* Remember where the next line# starts */
if (nxtStmtIndex < info.compStmtOffsetsCount)
{
/* Remember where the next line# starts */

nxtStmtOffs = info.compStmtOffsets[nxtStmtIndex];
}
else
{
/* No more line# entries */
nxtStmtOffs = info.compStmtOffsets[nxtStmtIndex];
}
else
{
/* No more line# entries */

nxtStmtOffs = BAD_IL_OFFSET;
}
nxtStmtOffs = BAD_IL_OFFSET;
}
}
else if ((info.compStmtOffsetsImplicit & ICorDebugInfo::STACK_EMPTY_BOUNDARIES) &&
(verCurrentState.esStackDepth == 0))
{
/* At stack-empty locations, we have already added the tree to
the stmt list with the last offset. We just need to update
impCurStmtDI
*/
}
else if ((info.compStmtOffsetsImplicit & ICorDebugInfo::STACK_EMPTY_BOUNDARIES) &&
(verCurrentState.esStackDepth == 0))
{
/* At stack-empty locations, we have already added the tree to
the stmt list with the last offset. We just need to update
impCurStmtDI
*/

impCurStmtOffsSet(opcodeOffs);
}
else if ((info.compStmtOffsetsImplicit & ICorDebugInfo::CALL_SITE_BOUNDARIES) &&
impOpcodeIsCallSiteBoundary(prevOpcode))
{
/* Make sure we have a type cached */
assert(callTyp != TYP_COUNT);

if (callTyp == TYP_VOID)
{
impCurStmtOffsSet(opcodeOffs);
}
else if ((info.compStmtOffsetsImplicit & ICorDebugInfo::CALL_SITE_BOUNDARIES) &&
impOpcodeIsCallSiteBoundary(prevOpcode))
else if (opts.compDbgCode)
{
/* Make sure we have a type cached */
assert(callTyp != TYP_COUNT);

if (callTyp == TYP_VOID)
{
impCurStmtOffsSet(opcodeOffs);
}
else if (opts.compDbgCode)
{
impSpillStackEnsure(true);
impCurStmtOffsSet(opcodeOffs);
}
impSpillStackEnsure(true);
impCurStmtOffsSet(opcodeOffs);
}
else if ((info.compStmtOffsetsImplicit & ICorDebugInfo::NOP_BOUNDARIES) && (prevOpcode == CEE_NOP))
}
else if ((info.compStmtOffsetsImplicit & ICorDebugInfo::NOP_BOUNDARIES) && (prevOpcode == CEE_NOP))
{
if (opts.compDbgCode)
{
if (opts.compDbgCode)
{
impSpillStackEnsure(true);
}

impCurStmtOffsSet(opcodeOffs);
impSpillStackEnsure(true);
}

assert(!impCurStmtDI.IsValid() || (nxtStmtOffs == BAD_IL_OFFSET) ||
(impCurStmtDI.GetLocation().GetOffset() <= nxtStmtOffs));
impCurStmtOffsSet(opcodeOffs);
}

assert(!impCurStmtDI.IsValid() || (nxtStmtOffs == BAD_IL_OFFSET) ||
(impCurStmtDI.GetLocation().GetOffset() <= nxtStmtOffs));
}

CORINFO_CLASS_HANDLE clsHnd = DUMMY_INIT(NULL);
Expand Down
15 changes: 9 additions & 6 deletions src/coreclr/jit/rationalize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,12 +958,15 @@ PhaseStatus Rationalizer::DoPhase()
BlockRange().InsertAtEnd(LIR::Range(statement->GetTreeList(), statement->GetRootNode()));

// If this statement has correct debug information, change it
// into a debug info node and insert it into the LIR. Currently
// we do not support describing IL offsets in inlinees in the
// emitter, so we normalize all debug info to be in the inline
// root here.
DebugInfo di = statement->GetDebugInfo().GetRoot();
if (di.IsValid())
// into a debug info node and insert it into the LIR. Note that
// we are currently reporting root info only back to the EE, so
// if the leaf debug info is invalid we still attach it.
// Note that we would like to have the invariant di.IsValid()
// => parent.IsValid() but it is currently not the case for
// NEWOBJ IL instructions where the debug info ends up attached
// to the allocation instead of the constructor call.
DebugInfo di = statement->GetDebugInfo();
if (di.IsValid() || di.GetRoot().IsValid())
{
GenTreeILOffset* ilOffset =
new (comp, GT_IL_OFFSET) GenTreeILOffset(di DEBUGARG(statement->GetLastILOffset()));
Expand Down