Skip to content

Handle zero-sized unwind fragment candidates #62931

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

Merged
BruceForstall merged 1 commit into from
Dec 17, 2021
Merged

Handle zero-sized unwind fragment candidates #62931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 19 additions & 3 deletions src/coreclr/jit/emit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2736,13 +2736,29 @@ void emitter::emitSplit(emitLocation* startLoc,
reportCandidate = false;
}

// Don't report a zero-size candidate. This will only occur in a stress mode with JitSplitFunctionSize
Copy link
Contributor

@kunalspathak kunalspathak Dec 16, 2021

Choose a reason for hiding this comment

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

Can we also add an assert in Alloc() for

assert(this->ufiEmitLoc->GetIG()->igSize > 0);

Copy link
Contributor Author

@BruceForstall BruceForstall Dec 16, 2021

Choose a reason for hiding this comment

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

Which Alloc are you referring to?

Copy link
Contributor

@kunalspathak kunalspathak Dec 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

sorry, I meant Allocate() method, somewhere around here:

runtime/src/coreclr/jit/unwindarm.cpp

Line 1631 in d5f5950

assert(endOffset > startOffset);

Copy link
Contributor Author

@BruceForstall BruceForstall Dec 16, 2021

Choose a reason for hiding this comment

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

We can't have that assert, or any assert about the size of a particular IG, because the IG could be zero sized, as long as there are other IGs in the fragment that have code. That's what the endOffset > startOffset is asserting.

Copy link
Contributor

@kunalspathak kunalspathak Dec 16, 2021

Choose a reason for hiding this comment

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

Hhm, that's true. There could be other non-zero IGs in the fragment.

// set to something small, and a zero-sized IG (possibly inserted for use by the alignment code). Normally,
// the split size will be much larger than the maximum size of an instruction group. The invariant we want
// to maintain is that each fragment contains a non-zero amount of code.
if (reportCandidate && (candidateSize == 0))
{
#ifdef DEBUG
if (EMITVERBOSE)
printf("emitSplit: can't split at IG%02u; zero-sized candidate\n", igLastCandidate->igNum);
#endif
reportCandidate = false;
}

// Report it!
if (reportCandidate)
{
#ifdef DEBUG
if (EMITVERBOSE && (candidateSize >= maxSplitSize))
printf("emitSplit: split at IG%02u is size %d, larger than requested maximum size of %d\n",
igLastCandidate->igNum, candidateSize, maxSplitSize);
if (EMITVERBOSE)
{
printf("emitSplit: split at IG%02u is size %d, %s than requested maximum size of %d\n",
igLastCandidate->igNum, candidateSize, (candidateSize >= maxSplitSize) ? "larger" : "less",
maxSplitSize);
}
#endif

// hand memory ownership to the callback function
Expand Down
7 changes: 6 additions & 1 deletion src/coreclr/jit/unwindarm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1693,7 +1693,12 @@ void UnwindFragmentInfo::Dump(int indent)
printf("%*sUnwindFragmentInfo #%d, @0x%08p, size:%d:\n", indent, "", ufiNum, dspPtr(this), sizeof(*this));
printf("%*s uwiComp: 0x%08p\n", indent, "", dspPtr(uwiComp));
printf("%*s ufiNext: 0x%08p\n", indent, "", dspPtr(ufiNext));
printf("%*s ufiEmitLoc: 0x%08p\n", indent, "", dspPtr(ufiEmitLoc));
printf("%*s ufiEmitLoc: 0x%08p ", indent, "", dspPtr(ufiEmitLoc));
Copy link
Contributor Author

@BruceForstall BruceForstall Dec 16, 2021

Choose a reason for hiding this comment

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

This was a useful change when I was debugging.

if (ufiEmitLoc != nullptr)
{
ufiEmitLoc->Print(uwiComp->compMethodID);
}
printf("\n");
printf("%*s ufiHasPhantomProlog: %s\n", indent, "", dspBool(ufiHasPhantomProlog));
printf("%*s %d epilog%s\n", indent, "", count, (count != 1) ? "s" : "");
printf("%*s ufiEpilogList: 0x%08p\n", indent, "", dspPtr(ufiEpilogList));
Expand Down