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][interp] Remove an assertion. #78596

Closed
wants to merge 1 commit into from

Conversation

vargaz
Copy link
Contributor

@vargaz vargaz commented Nov 19, 2022

The assertion can be hit in normal usage on wasm when calling a catch clause inside a finally clause from interp_run_clause_with_il_state ().

Test case is TCErrorConditionWriter:var_21 () in the xml test suite.

The assertion can be hit in normal usage on wasm when calling
a catch clause inside a finally clause from interp_run_clause_with_il_state ().

Test case is TCErrorConditionWriter:var_21 () in the xml test suite.
@ghost
Copy link

ghost commented Nov 19, 2022

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

Issue Details

The assertion can be hit in normal usage on wasm when calling a catch clause inside a finally clause from interp_run_clause_with_il_state ().

Test case is TCErrorConditionWriter:var_21 () in the xml test suite.

Author: vargaz
Assignees: vargaz
Labels:

area-Codegen-Interpreter-mono

Milestone: -

@vargaz
Copy link
Contributor Author

vargaz commented Nov 19, 2022

Re:
#78358

@vargaz
Copy link
Contributor Author

vargaz commented Nov 19, 2022

This will need a backport to 7.0.

@vargaz
Copy link
Contributor Author

vargaz commented Nov 19, 2022

The failure was only hit on 7.0 due to:
#78595

@vargaz
Copy link
Contributor Author

vargaz commented Nov 20, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BrzVlad
Copy link
Member

BrzVlad commented Nov 21, 2022

I don't understand the behavior with this change.

Let's say we have a method like this:

block1;
try {
    block2;
} finally {
    try {
        block3;
    } catch {
        block4;
    }
    block5;
}
block6;

So from your description, I assume that block1, block2, block3 are executed by AOT. Within block3 there is a throw so we then pass execution to interpreter. AOT expects interp_run_clause_with_il_state to run the method until the end and then just return to parent method were execution continues normally in AOT code.

Interpreter will start executing block4, and within block5 it encounters MINT_ENDFINALLY opcode. In interpreter, finally blocks are called by MINT_CALL_HANDLER which writes the leave address. Since block2 wasn't executed by interpreter, the endfinally leave address (which is block6) is not written (with current implementation sounds like it could even be junk). This means that we currently just silently return execution out of interpreter without ever executing block6.

@vargaz Is this correct ?

@jandupej
Copy link
Member

jandupej commented Nov 21, 2022

I don't understand the behavior with this change.

Let's say we have a method like this:

block1;
try {
    block2;
} finally {
    try {
        block3;
    } catch {
        block4;
    }
    block5;
}
block6;

So from your description, I assume that block1, block2, block3 are executed by AOT. Within block3 there is a throw so we then pass execution to interpreter. AOT expects interp_run_clause_with_il_state to run the method until the end and then just return to parent method were execution continues normally in AOT code.

Interpreter will start executing block4, and within block5 it encounters MINT_ENDFINALLY opcode. In interpreter, finally blocks are called by MINT_CALL_HANDLER which writes the leave address. Since block2 wasn't executed by interpreter, the endfinally leave address (which is block6) is not written (with current implementation sounds like it could even be junk). This means that we currently just silently return execution out of interpreter without ever executing block6.

@vargaz Is this correct ?

@BrzVlad @vargaz Doesn't block5 just branch to start of block6?

@vargaz
Copy link
Contributor Author

vargaz commented Nov 21, 2022

Yes, this might not be correct.

@vargaz vargaz added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 21, 2022
@BrzVlad
Copy link
Member

BrzVlad commented Nov 21, 2022

@jandupej No, because the behavior is dependent on who executes this block. The finally block can be invoked as part of the method execution, when the guarded code is finished, or independently from EH. In which case the endfinally opcode (which has to be at the end of all finally blocks) just returns out of interp_exec_method

@vargaz
Copy link
Contributor Author

vargaz commented Nov 21, 2022

Not the right fix, closing.

@vargaz vargaz closed this Nov 21, 2022
@vargaz vargaz deleted the fix-interp-regress branch November 21, 2022 16:05
@ghost ghost locked as resolved and limited conversation to collaborators Dec 21, 2022
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants