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

UnsafeAccessorAttribute fails to access volatile field #109665

Closed
filipnavara opened this issue Nov 9, 2024 · 7 comments · Fixed by #109694
Closed

UnsafeAccessorAttribute fails to access volatile field #109665

filipnavara opened this issue Nov 9, 2024 · 7 comments · Fixed by #109694
Labels
area-System.Runtime.CompilerServices in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@filipnavara
Copy link
Member

Description

When trying to access volatile string private field through UnsafeAccessorAttribute, MissingFieldException is thrown.

Reproduction Steps

using System;
using System.Runtime.CompilerServices;

var a = new A();
Console.WriteLine(GetSetFoo(a));

[UnsafeAccessor(UnsafeAccessorKind.Field, Name = "foo")]
extern static ref string GetSetFoo(A args);

class A { private volatile string? foo = "Bar"; }

Expected behavior

The program output is Bar.

Actual behavior

Unhandled exception. System.MissingFieldException: Field not found: 'A.foo'.
   at Program.<<Main>$>g__GetSetFoo|0_0(A args)
   at Program.<Main>$(String[] args) in D:\unsafeaccessor\Program.cs:line 5

Regression?

No response

Known Workarounds

No response

Configuration

This seems to be broken both in .NET 8 and .NET 9 with CoreCLR runtime. Tested on Windows and macOS.

Other information

The question of modreqs/modopts came up in the original feature proposal and this was the answer (#81741 (comment)):

And how would this interact with modreqs and modopts that can't be specified in C# (e.g. the IsExternalInit modifier on init property setters / volatile fields etc)?

Yeah, modreqs and modopts would be complicated. I think we will skip modreqs and modopts for the signature match.

This originally came up in KirillOsenkov/MSBuildStructuredLog#834.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 9, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Nov 9, 2024
@filipnavara filipnavara added area-System.Runtime.CompilerServices and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 9, 2024
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.

@filipnavara
Copy link
Member Author

Additionally, this does work in both .NET 8 and .NET 9 with NativeAOT publishing.

@am11
Copy link
Member

am11 commented Nov 10, 2024

This seems to be broken both in .NET 8 and .NET 9 with CoreCLR runtime.

I can repro it on osx-arm64 with .NET 9.0.0 and .NET 10.0.0-alpha.1.24553.1 (dotnet run) but not with .NET 8.0.10. Also, it doesn't repro on sharplab net8.0.

@jkotas
Copy link
Member

jkotas commented Nov 10, 2024

cc @AaronRobinsonMSFT

@AaronRobinsonMSFT
Copy link
Member

This is likely because of

state.IgnoreCustomModifiers = false;

It might be the case we should always ignore custom modifiers for fields.

@AaronRobinsonMSFT
Copy link
Member

Yep, setting IgnoreCustomModifiers to true makes this work, but the generated accessor isn't going to be correct since we don't currently emit the .volatile prefix for the ldsfld/ldfld instructions. I can see an argument that this is "unsafe" but skipping the .volatile really seems wrong. Let me try and special case volatile.

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Nov 11, 2024
@AaronRobinsonMSFT
Copy link
Member

Looks like there isn't any need to update the generated getters. See #109694.

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Nov 11, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 10.0.0 milestone Nov 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.CompilerServices in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants