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.

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

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

BruceForstall
Copy link
Member

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

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
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 13, 2024
@ghost ghost assigned BruceForstall Jan 13, 2024
@ghost
Copy link

ghost commented Jan 13, 2024

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

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

Author: BruceForstall
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

Copy link
Member

@jakobbotsch jakobbotsch left a 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?

@AndyAyersMS
Copy link
Member

What was doing the splitting?

@BruceForstall
Copy link
Member Author

This kind of thing seems very fragile.

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.

Is anything preventing us from inserting the PInvoke epilog into all BBJ_RETURN blocks in lowering?

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.

What was doing the splitting?

Compiler::fgExpandStaticInit()

@BruceForstall
Copy link
Member Author

@jakobbotsch PTAL
cc @dotnet/jit-contrib

@BruceForstall
Copy link
Member Author

No diffs

@jakobbotsch
Copy link
Member

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.

Tail calls do get the pinvoke epilog inserted, but it is inserted inside LowerFastTailCall. However, note that we are inserting the regular epilog as part of GT_RETURN handling. While tailcall blocks are BBJ_RETURN blocks, they are not represented with any GT_RETURN node (they just end with the GT_CALL). So I think the check against genReturnBB can just be removed and we can avoid trying to maintain it after morph.

@BruceForstall
Copy link
Member Author

I think the check against genReturnBB can just be removed and we can avoid trying to maintain it after morph.

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.

Copy link
Member

@EgorBo EgorBo left a 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.

@jakobbotsch
Copy link
Member

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.

Sounds good to me.

@BruceForstall BruceForstall merged commit 02a3fcb into dotnet:main Jan 18, 2024
129 checks passed
@BruceForstall BruceForstall deleted the FixOneReturnBlockSplitting branch January 18, 2024 17:22
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
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
@github-actions github-actions bot locked and limited conversation to collaborators Feb 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure: ComInterfaceGenerator.Tests
4 participants