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

Consider dropping support for <field initialize="true" /> #107379

Open
MichalStrehovsky opened this issue Oct 19, 2021 · 4 comments · May be fixed by #107380
Open

Consider dropping support for <field initialize="true" /> #107379

MichalStrehovsky opened this issue Oct 19, 2021 · 4 comments · May be fixed by #107380
Assignees
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@MichalStrehovsky
Copy link
Member

https://github.com/dotnet/linker/blob/main/docs/data-formats.md#override-static-field-value-with-a-constant

The implementation of this is to delete any stsfld in the cctor and inject a ststfld before the last ret in the method body but:

  • There's no law the last ret of the method body needs to be executed at all (there could be an if/else in the cctor that just terminates early, never executing the injected code, or an EH block, or anything else).
  • There's no law that the only way to initialize a static is through a stsfld in the cctor. It could be initialized from a method the cctor calls, it could be initialized through ldsflda/stind, etc.

I don't know what use case motivated initialize, but given the real complexity of implementing it properly is much higher than the simple thing IL Linker has, it should be put under scrutiny whether we need it in the first place. If we need it, the above are bugs.

@marek-safar
Copy link
Contributor

IIRC the motivation was to trim managed code for logic that depended on BitConverter.IsLittleEndian

@MichalStrehovsky
Copy link
Member Author

IIRC the motivation was to trim managed code for logic that depended on BitConverter.IsLittleEndian

Looks like we got away without needing the initialize part for that one so maybe we don't have a use case?

@sbomer
Copy link
Member

sbomer commented Jul 12, 2024

We should also consider dropping support for field substitutions, which don't behave predictably, see #82821 (comment)

@sbomer sbomer self-assigned this Sep 4, 2024
@sbomer sbomer transferred this issue from dotnet/linker Sep 4, 2024
@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 Sep 4, 2024
@sbomer sbomer added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 4, 2024
@sbomer sbomer added this to the 10.0.0 milestone Sep 4, 2024
Copy link
Contributor

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

@sbomer sbomer linked a pull request Sep 4, 2024 that will close this issue
@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 Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers in-pr There is an active PR which will close this issue when it is merged
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

3 participants