-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix maintenance of genReturnBB
pointer
#96935
Fix maintenance of genReturnBB
pointer
#96935
Conversation
If the `genReturnBB` block is split, the pointer needs to be updated. Without this, we ended up with a situation where the `genReturnBB` did not point to the return block, leading to omitting the code to remove the PInvoke frame from the thread's Frame list. Fixes dotnet#96409
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsIf the Without this, we ended up with a situation where the Fixes #96409
|
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 kind of thing seems very fragile. Is anything preventing us from inserting the PInvoke epilog into all BBJ_RETURN
blocks in lowering?
What was doing the splitting? |
Agreed that it seems fragile. I was thinking maybe the "one return" should be a block flag instead of pointer, so it needs to be maintained like other such flags. The assert I added should help. For example, I'm not sure there's anything preventing us from compacting a BBJ_RETURN block that is genReturnBB, and I don't know how it gets maintained then.
The comment says "// Method doing PInvokes has exactly one return block unless it has tail calls.". Does that mean the tailcall returns are also BBJ_RETURN and those should not get PInvoke epilogs inserted (because they have alternative treatment)? That was my reading of the comment.
|
@jakobbotsch PTAL |
Tail calls do get the pinvoke epilog inserted, but it is inserted inside |
This is a nice observation. I've implemented that with #97130 which is implemented on top of this PR. I would like to propose (1) merging this PR as-is, because it is no-diff and fixes the bug, then (2) merge #97130 which has diffs. |
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. helpercallexpansion.cpp
has several phases which do this sort of split and may run into the same issue. I've checked that they all end up splitting blocks using fgSplitBlockAtEnd
.
Sounds good to me. |
If the `genReturnBB` block is split, the pointer needs to be updated. Without this, we ended up with a situation where the `genReturnBB` did not point to the return block, leading to omitting the code to remove the PInvoke frame from the thread's Frame list. Fixes dotnet#96409
If the
genReturnBB
block is split, the pointer needs to be updated.Without this, we ended up with a situation where the
genReturnBB
did not point to the return block, leading to omitting the code to remove the PInvoke frame from the thread's Frame list.Fixes #96409