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

[release/7.0] [Mono] Fix function pointer check #80927

Merged
merged 5 commits into from
Feb 8, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jan 20, 2023

Backport of #80855 to release/7.0
Backport of #81286 to release/7.0
Backport of #81122 to release/7.0

/cc @fanyang-mono

Customer Impact

This is a fix to a MAUI Android customer reported issue (#80095). In general, customers who use function pointer with Mono JIT or interpreter mode can be impacted by this bug. It also includes backport of the PR #81122 with fix for Crossgen2 bug uncovered by the new unit test introduced as part of the PR #80855.

Testing

Validated using customer provided Maui Android app. The reported exception is no longer happening with this change. On top of that, I added new runtime tests to lockdown this behavior. They passed when testing with main.

Risk

No known risk.

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

@lambdageek lambdageek added the Servicing-consider Issue for next servicing release review label Jan 20, 2023
@lambdageek lambdageek added this to the 7.0.x milestone Jan 20, 2023
@fanyang-mono
Copy link
Member

CI failures on Libraries Test Run release mono Linux arm64 Debug are not related to this PR.

@SamMonoRT SamMonoRT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 24, 2023
@SamMonoRT
Copy link
Member

/cc @carlossanlop - this is approved via Tactics email and failures are not related. This is ready to be merged.

@carlossanlop
Copy link
Member

Thanks for the ping, @SamMonoRT. I will merge it once the branches open again after branding.

@SamMonoRT
Copy link
Member

@carlossanlop -- the newly added test for the fix has exposed a CoreCLR limitation in the runtime-coreclr-outerloop lanes. @trylek is working on the fix via #81122 and should issue a request to backport the fix to 7.0 too to avoid lanes broken in servicing runs.

Please don't merge this PR till we have the fix approved for backport. I'll let you know when it's ready.

/cc @mangod9 @JulieLeeMSFT

@fanyang-mono
Copy link
Member

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@fanyang-mono
Copy link
Member

CI test failures are not related to the change in this PR.

@trylek
Copy link
Member

trylek commented Jan 31, 2023

I have merged the fix into runtime main but I have yet to figure out how to properly backport it:

#81122

The automated backport naturally failed as my fix should remove the issues.targets exclusion that Fan has added but that is not yet in the 7.0 branch as PRs are not merged into servicing branches right away, they only get there once the shiproom opens the appropriate servicing window. @jeffschwMSFT, could you please advise me how to proceed in this case? Should Fan remove the issues.targets exclusion from her backport PR with the rationale that it's going to be approved and merged in along with mine?

Thanks a lot

Tomas

@jeffschwMSFT
Copy link
Member

@trylek is it possible to hand port the change? It seems small enough. It is rare that we run into conflicts like this, and as you mention there are a few ways forward. I would pick the one with the lowest cost and best outcome.

@trylek
Copy link
Member

trylek commented Jan 31, 2023

@jeffschwMSFT - the actual change by itself is trivial, the tricky part is the "state machine" that this change constitutes in combination with @fanyang-mono's #80927, we are both requesting backports and our changes are intertwined in a way, we need to figure out how to resolve that or whether we should somehow combine them in a single backporting request.

@fanyang-mono
Copy link
Member

@trylek @jeffschwMSFT I suggest that we merge this PR first. Then Tomas's PR should be able to be backported using the bot. That way we avoid doing things manually. And everything should be able to get in smoothly.

Based on tactical discussion with Fan and Sam I have backported
the PR #81122 including the removal of the issues.targets entry
for function pointer tests as an extra commit on top of
the backport of Fan's PR #80855.

Thanks

Tomas
@trylek trylek removed the Servicing-approved Approved for servicing release label Feb 2, 2023
@trylek
Copy link
Member

trylek commented Feb 2, 2023

Based on tactical discussion with @SamMonoRT and @fanyang-mono we agreed that the easiest way to resolve the "state machine" I described above will be to backport my Crossgen2 fix along with Fan's original PR. I have pushed an extra commit with manual backport of the Crossgen2 fix per #81122 to this branch, I have deleted the issues.targets exclusion and I have reset the "Servicing approved" flag as we're going to ask the shiproom for new approval of this combined backport. I have tested locally that the functionpointer test works in Crossgen2 mode in this updated branch.

Thanks

Tomas

@SamMonoRT SamMonoRT added the Servicing-consider Issue for next servicing release review label Feb 2, 2023
@SamMonoRT
Copy link
Member

@trylek @fanyang-mono : Please confirm CI failures are not related to the changes.

@fanyang-mono
Copy link
Member

Kicked off a re-run. Will check the ones failed after re-run.

@fanyang-mono
Copy link
Member

fanyang-mono commented Feb 6, 2023

CI failures are not related to the change in this PR. Steve has approved this via Tactics email. This is ready to be merged.

/cc @carlossanlop

@SamMonoRT SamMonoRT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 6, 2023
@carlossanlop carlossanlop modified the milestones: 7.0.x, 7.0.4 Feb 8, 2023
@carlossanlop
Copy link
Member

Approved by Tactics via email for 7.0.4.
Signed off by area owner.
No OOB changes needed for this PR.
The ARM64 failures are all known, infra-related, and fixes have been merged to the branch: #81807, #81712
There's also a Roslyn Generator failure which is unrelated: #81123. There's also a chance it was related to the ongoing ARM64 infra problems.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 8502110 into release/7.0 Feb 8, 2023
@carlossanlop carlossanlop deleted the backport/pr-80855-to-release/7.0 branch February 8, 2023 22:59
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-meta-mono Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants