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/8.0] [mono] Handle enum return type when inlining CreateInstance #91061

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 24, 2023

Backport of #91008 to release/8.0

/cc @fanyang-mono @uweigand

Customer Impact

Mono has the logic to inline the call to System.Activator.CreateInstance(). Prior to this change, Mono always assumed T as non-primitive type. However, for the customer reported situation, T was a enum type. Treating it as non-primitive type led to incorrect generated code. That triggered an assertion during runtime.

Testing

The customer confirmed that with this change, the assertion went away and the generated code was correct. All CI lanes were passed as well.

Risk

Adding a new scenario that Mono wasn't considering before. Thus, the risk of breaking existing functionality is very low.

Use underlying base type when deciding how to inline a
CreateInstance invocation in mini_emit_inst_for_method.

Fixes #90292
(Mono abort causing .NET 8 msbuild regression).
@fanyang-mono fanyang-mono added this to the 8.0.0 milestone Aug 24, 2023
@fanyang-mono fanyang-mono added the Servicing-consider Issue for next servicing release review label Aug 24, 2023
@carlossanlop
Copy link
Member

@SamMonoRT @marek-safar can we get your blessing for this change?

@fanyang-mono
Copy link
Member

We are waiting for @lambdageek to review when he's back next Monday.

@SamMonoRT SamMonoRT added the blocked Issue/PR is blocked on something - see comments label Aug 24, 2023
@SamMonoRT
Copy link
Member

Adding blocked label - we need a further approval.

@lambdageek lambdageek removed the blocked Issue/PR is blocked on something - see comments label Aug 28, 2023
@SamMonoRT
Copy link
Member

@marek-safar @jeffschwMSFT - needs an approval for backport. It is validated.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. this can be merged when ready

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

/cc @carlossanlop - this is ready to be merged. Thank you.

@carlossanlop carlossanlop merged commit fe64d25 into release/8.0 Aug 28, 2023
@carlossanlop carlossanlop deleted the backport/pr-91008-to-release/8.0 branch August 28, 2023 16:10
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Codegen-JIT-mono Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants