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

[mono][interpreter] Throw when basic blocks after branch are not created #79246

Closed
wants to merge 3 commits into from

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Dec 5, 2022

New blocks are created when branch/jump instructions are encountered (i.e. ret, br, throw). In such cases, if a new block is not created before the end of the instruction stream, IL code is invalid and InvalidProgramException should be thrown. Similar approach in Mono JIT:

if (start_new_bblock != 1)
UNVERIFIED;

Fix for #54396

@ghost
Copy link

ghost commented Dec 5, 2022

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

Issue Details

If last BB is dead, InvalidProgramException should be thrown. Similar approach in Mono JIT:

if (start_new_bblock != 1)
UNVERIFIED;

Fix for #54396

Author: kotlarmilos
Assignees: kotlarmilos
Labels:

area-Codegen-Interpreter-mono

Milestone: -

@kotlarmilos
Copy link
Member Author

@BrzVlad not sure if there is an existing attribute that could be used.

@kotlarmilos kotlarmilos changed the title Throw when last BB is dead [mono][interpreter] Throw when last BB is dead Dec 6, 2022
@kotlarmilos kotlarmilos added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Dec 6, 2022
@BrzVlad
Copy link
Member

BrzVlad commented Dec 6, 2022

There shouldn't be a problem if the last bblock is dead in a method. The problem in the test case seems to be that leave has an invalid offset which is caught in get_basic_blocks, but on interp we don't throw error.

@kotlarmilos
Copy link
Member Author

New blocks are created when branch/jump instructions are encountered (i.e. ret, br, throw). In such cases, if a new block is not created before the end of the instruction stream, IL code is invalid.

get_basic_blocks checks if opcode exists but can't check if an offset is invalid:

i = mono_opcode_value ((const guint8 **)&ip, end);
if (i < 0)
UNVERIFIED;

Var start_new_bblock in method-to-ir.c indicates if a new block should be created according to particular instructions (i.e. ret, br, throw). If instruction stream ends with start_new_bblock == 0 it means that a new BB should have been created and InvalidProgramException is thrown.

@kotlarmilos kotlarmilos changed the title [mono][interpreter] Throw when last BB is dead [mono][interpreter] Throw when basic blocks after branch are not created Dec 6, 2022
@BrzVlad
Copy link
Member

BrzVlad commented Dec 6, 2022

I don't understand what start_new_bblock is trying to achieve and how it is related to the test case you are trying to fix. Once again, the problem in the test case is that leave has an invalid offset which is caught in the jit in get_basic_blocks, but on interp we don't throw error.

Seems to me that you keep trying to fix a separate issue. I think what you are trying to do is done by CHECK_FALLTHRU.

@kotlarmilos
Copy link
Member Author

After offline sync with Vlad, we agreed to check how it is implemented in coreclr as the current implementation in mono jit doesn't cover all cases.

@kotlarmilos
Copy link
Member Author

The fix proposed in this PR doesn't cover all cases. Closing this PR in favour of #80136.

@kotlarmilos kotlarmilos closed this Jan 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 2, 2023
@kotlarmilos kotlarmilos deleted the dead-basic-blocks branch February 20, 2023 15:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Codegen-Interpreter-mono NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it
Projects
None yet
2 participants