Skip to content

[release/9.0-staging] Fix UnsafeAccessor scenario for modopts/modreqs when comparing field sigs. #111675

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

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Jan 21, 2025

Backport of #111648 to release/9.0-staging

Customer Impact

  • Customer reported
  • Found internally

Consume custom modifiers and ByRef in RetType signature prior to comparing field signature. Specifically signatures have contain modopts/modreqs. This is follow-up for #109694, which was also a .NET 9 servicing for a regression, but missed a specific scenario involving readonly and was testing using a UnsafeAccessor declaration that did not properly validate the scenario.

Fixes #111647

Regression

  • Yes
  • No

Issue #109665 was fixed with #109694 and assumed to also address issues with readonly and so tests were added with in #109944 - these tests do pass.

The issue is best showcased with the following:

static class Accessors
{
    [UnsafeAccessor(UnsafeAccessorKind.Field, Name = "<obj>P")]
    public static extern ref readonly object MEMBER(ref readonly Foo value); 
    
    public static void A()
    {
        [UnsafeAccessor(UnsafeAccessorKind.Field, Name = "<obj>P")]
        static extern ref readonly object LOCAL(ref readonly Foo value);
    }
}

The MEMBER and LOCAL are the same definition, but MEMBER uses an modreq when LOCAL doesn't. For testing, the LOCAL scenario was used for validation and this created a false sense of correctness in the tests. The result is the modopt/modreq issues can still occur for fields depending on how the UnsafeAccessor is defined. This means we need to service .NET 9 again, see #109709, and make the correct fix in .NET 10 along with updating the testing so we validate both types of definition.

Testing

New tests have been added and the existing tests have been updated to cover this scenario.

Risk

Medium. This is likely "Low" from a regression standpoint, but increases the Risk since this is a second attempt at a fix. There is much more confidence in this fix after uncovering the metadata differences in signature declaration.

…d sigs. (dotnet#111648)

* Consume custom modifiers and ByRef in RetType signature
prior to comparing field signature.
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 4 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • src/coreclr/vm/prestub.cpp: Language not supported
  • src/coreclr/vm/siginfo.cpp: Language not supported
  • src/coreclr/vm/siginfo.hpp: Language not supported

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.

lgtm. please get a code review. we will take for consideration in 9.0.x

@rbhanda rbhanda modified the milestones: 9.0.x, 9.0.3 Jan 23, 2025
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 23, 2025
@AaronRobinsonMSFT AaronRobinsonMSFT merged commit eaea271 into dotnet:release/9.0-staging Jan 24, 2025
98 of 104 checks passed
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the runtime_90_111647 branch January 24, 2025 01:17
@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2025
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.

4 participants