-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[clr-interp] Fix issue around compliance with the rules about lazy initialized basic block stack details #122253
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
[clr-interp] Fix issue around compliance with the rules about lazy initialized basic block stack details #122253
Conversation
…itialized basic block stack details - When we added support for basic blocks with undefined stacks to be initialized with a stack state via a backwards branch we missed the case of basic blocks which begin after a ret instruction This fixes the jit\Methodical\tailcall\test_implicit tests.
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
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.
Pull request overview
This PR fixes a bug in the CLR interpreter's basic block creation logic where basic blocks that begin after a ret instruction were not being properly created. This caused issues with lazy initialization of basic block stack details when such blocks were reached via backwards branches.
- Adds a call to
GetBBto create a basic block entry for any instruction that follows aretinstruction - Follows the same pattern already established for other control flow instructions (branches, throws, etc.)
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.
The fix is reasonable but if the code following the ret is the target of a branch, then it should already be created in CreateBasicBlocks. For CEE_RET instruction we set the linkBBlocks = false. I'm wondering if the issue is that the test in question has dead instructions following the ret opcode that should be ignored. I suspect an alternative fix could be done by bumping the IL pointer until we get to the start of a new bblock (if we reach the case at the start of instruction loop where pNewBB is NULL despite linkBBlocks being false).
janvorli
left a comment
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, thank you!
|
@BrzVlad this is a case with dead il instructions not referenced by anything. As you say, we could bump the il pointer forward, but this should also work, and matches how all the other cases like this work. |
This fixes the jit\Methodical\tailcall\test_implicit tests.