-
Notifications
You must be signed in to change notification settings - Fork 5.2k
readonly-ify fields based on whole program view
#92923
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
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsMarking fields
In this PR, I'm adding infrastructure to collect information about field writes during whole program analysis. This collects information about fields that are effectively read-only. Then during compilation, we use this data instead of the This is also a correctness fix - the compiler would previously assume preinitializing instances of classes with all-readonly instance fields is safe, but one can set them with reflection and this would lead to a crash in the write barrier. Not that anyone should be reflection-setting readonly fields outside of deserialization scenarios. Cc @dotnet/ilc-contrib
|
Per discord conversation.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Marking fields
readonlyhas a couple benefits:class A { public static int X = 123 } class B { static int Y = A.X }-Acan be preinitialized at compile time, butBcannot. However:class A { public static readonly int X = 123 } class B { static int Y = A.X }bothAandBcan preinitialize at compile time.In this PR, I'm adding infrastructure to collect information about field writes during whole program analysis. This collects information about fields that are effectively read-only. Then during compilation, we use this data instead of the
IsInitOnlymetadata. The big advantage of this approach (as opposed to e.g. a Roslyn analyzer) is that we look at the app after trimming - some fields might be writable, but if we discarded the code that writes them, they're read only.This is also a correctness fix - the compiler would previously assume preinitializing instances of classes with all-readonly instance fields is safe, but one can set them with reflection and this would lead to a crash in the write barrier. Not that anyone should be reflection-setting readonly fields outside of deserialization scenarios.
Cc @dotnet/ilc-contrib