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

Make sure not to strip endfilter #86374

Merged
merged 1 commit into from
May 17, 2023

Conversation

MichalStrehovsky
Copy link
Member

Fixes the issue described in #86304 (but doesn't fix the issue because that's for ILLink). Found by Pri0 tests.

Cc @dotnet/ilc-contrib

Fixes the issue described in dotnet#86304 (but doesn't fix the issue because that's for ILLink). Found by Pri0 tests.
@ghost
Copy link

ghost commented May 17, 2023

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

Issue Details

Fixes the issue described in #86304 (but doesn't fix the issue because that's for ILLink). Found by Pri0 tests.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@vitek-karas
Copy link
Member

I have the ILLink bug fixed here: https://github.com/vitek-karas/runtime/tree/UnreachableFilterEnd
I was looking at NativeAOT now - but got stuck on how to test this correctly.

Do we just rely on the CoreCLR runtime test which uncovered this for NativeAOT coverage?

@MichalStrehovsky
Copy link
Member Author

Do we just rely on the CoreCLR runtime test which uncovered this for NativeAOT coverage?

Yup, I didn't see a reason do duplicate things. The test is specifically testing unreachable things after a throw in IL.

&& methodBytes[expectedEndfilterLocation + 1] == unchecked((byte)ILOpcode.endfilter);
if (isValidFilter)
{
flags[expectedEndfilterLocation] |= OpcodeFlags.VisibleBasicBlockStart | OpcodeFlags.Mark;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean we will keep the endfilter always (and thus the filter block), or does this preserve the ability to remove the entire filter block?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code runs when we're looking at EH regions associated with a marked instruction. So if the block covered by the filter got trimmed, the filter will be trimmed too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finally wrapped my head around how this works in the linker, but the implementation in NativeAOT is different... I'll have to spend some time trying to understand it as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general approach is the same as in ILLinker - seek backwards in the instruction stream and try to come up with a constant value. It doesn't try to reformat the IL after the fact (just nop things out) because RyuJIT is very efficient at getting rid of nops. We don't have to adjust EH regions or recompute branch offsets as a result.

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

I'll see if we can leverage the illink test I wrote for this. The ilc will report an invalid IL message, so maybe the test can detect that as a way to verify that it at least partially "worked".

@MichalStrehovsky MichalStrehovsky merged commit 71bd3ac into dotnet:main May 17, 2023
@MichalStrehovsky MichalStrehovsky deleted the endfilter branch May 17, 2023 21:53
@ghost ghost locked as resolved and limited conversation to collaborators Jun 17, 2023
Sign up for free to 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.

2 participants