Skip to content

[mono][interp] Fix an issue with deopt and interpreter tiering. - #76743

Merged
lewing merged 2 commits into
dotnet:mainfrom
vargaz:il-state-tier
Oct 14, 2022
Merged

[mono][interp] Fix an issue with deopt and interpreter tiering.#76743
lewing merged 2 commits into
dotnet:mainfrom
vargaz:il-state-tier

Conversation

@vargaz

Copy link
Copy Markdown
Contributor

If a method is tiered while being run from interp_run_clause_with_il_state (), the clause_args argument to interp_exec_method () still contains the old IL offsets confusing the EH code, i.e. this line:

if (clause_args && frame == clause_args->exec_frame && context->handler_ip >= clause_args->end_at_ip)

Clear out clause_args at the beginning to avoid this.

Hopefully fixes
#76134
#74302

@ghost

ghost commented Oct 7, 2022

Copy link
Copy Markdown

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

Issue Details

If a method is tiered while being run from interp_run_clause_with_il_state (), the clause_args argument to interp_exec_method () still contains the old IL offsets confusing the EH code, i.e. this line:

if (clause_args && frame == clause_args->exec_frame && context->handler_ip >= clause_args->end_at_ip)

Clear out clause_args at the beginning to avoid this.

Hopefully fixes
#76134
#74302

Author:vargaz
Assignees:vargaz
Labels:

area-Codegen-Interpreter-mono

Milestone:-

@vargaz

Copy link
Copy Markdown
ContributorAuthor

@BrzVlad this might also affect the other cases where clause_args is set i.e. interp_run_finally ()/interp_run_filter ().

@vargaz

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
vargaz marked this pull request as draft October 7, 2022 17:01
@BrzVlad

Copy link
Copy Markdown
Member

We should probably update the native offset in clause args end_at_ip when tiering

@vargaz

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vargaz

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vargaz

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vargaz

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
vargaz marked this pull request as ready for review October 10, 2022 10:52
If a method is tiered while being run from interp_run_clause_with_il_state (),
the clause_args argument to interp_exec_method () still contains the old IL
offsets confusing the EH code, i.e. this line:
```
if (clause_args && frame == clause_args->exec_frame && context->handler_ip >= clause_args->end_at_ip)
```
Clear out clause_args at the beginning to avoid this.
Hopefully fixesdotnet#76134dotnet#74302
@build-analysisbuild-analysisBot mentioned this pull request Oct 11, 2022
2 tasks
The IL offsets in the clause_args argument become out-of-date after tiering up.
@lewinglewing added this to the 7.0.x milestone Oct 14, 2022
@lewing
lewing merged commit a071887 into dotnet:mainOct 14, 2022
@lewing

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3252554294

@BrzVlad

Copy link
Copy Markdown
Member

@vargaz From what I understand the problem with these issues was that the tiered ip was not updated in the clause_args. Your change disables tiering while running from EH. So what's the point of the rest of the change involving run_until_end, I also don't understand what that is supposed to do

@vargaz

Copy link
Copy Markdown
ContributorAuthor

The first commit fixed the original issue when called from with_il_state, the second one fixes the potential issue when called from run_finally ()/run_filter (). Maybe the second one fixes the first issue as well, but clearing out clause_args seems cleaner.

@BrzVlad

Copy link
Copy Markdown
Member

I understand now that clearing out the clause_args might make sense when running catch clauses of aot methods, since we run with interp all the way until the method end, but doing this also for filter clauses seems bogus.

@vargaz

Copy link
Copy Markdown
ContributorAuthor

clause_args is only really used when running finally/filter clauses for 'real' interpreted methods, we just reuse the argument from with_il_state () to avoid having to add another argument. So this is why we clear it. Maybe we should clear it for catch clauses as well.

@ghostghost locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@vargaz@BrzVlad@lewing@lambdageek