-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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.
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
Conversation
Every unwind fragment should be non-zero sized. If we consider splitting on a boundary that would create a zero-sized fragment, don't report that one. This only occurs when setting the stress mode JitSplitFunctionSize to something small.
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsEvery unwind fragment should be non-zero sized. If we consider
|
@@ -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)); |
There was a problem hiding this comment.
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.
@@ -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 |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I actually forgot to ask you, but can you add a test case for this? |
Every unwind fragment should be non-zero sized. If we consider
splitting on a boundary that would create a zero-sized fragment,
don't report that one. This only occurs when setting the
stress mode JitSplitFunctionSize to something small.