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

Postpone marking fields on types with layout #1309

Merged
merged 1 commit into from
Jul 6, 2020

Conversation

MichalStrehovsky
Copy link
Member

We can postpone marking fields on types with layout until we're really sure they're needed. They're needed if they're visible to reflection, or considered instantiated (valuetypes are implicitly considered instantiated).

I'm also allowing stripping fields on Auto layout structs. This is a rare sight, but there's no reason that I know of to force them to have all their fields kept.

We can postpone marking fields on types with layout until we're really sure they're needed. They're needed if they're visible to reflection, or considered instantiated (valuetypes are implicitly considered instantiated).

I'm also allowing stripping fields on Auto layout structs. This is a rare sight, but there's no reason that I know of to force them to have all their fields kept.
@MichalStrehovsky MichalStrehovsky merged commit da2cc0f into dotnet:master Jul 6, 2020
@MichalStrehovsky MichalStrehovsky deleted the layout branch July 6, 2020 12:47
@marek-safar
Copy link
Contributor

This is causing regression in linker bump dotnet/runtime#38880. The problem seems to be trigger by this code https://github.com/dotnet/runtime/blob/master/src/mono/netcore/System.Private.CoreLib/src/System/Array.Mono.cs#L57 which uses StructLayout type as type argument and RawData struct no longer has all fields marked

@MichalStrehovsky could please look into that or revert this change

akoeplinger added a commit to dotnet/runtime that referenced this pull request Jul 8, 2020
@MichalStrehovsky
Copy link
Member Author

Right, linker optimizations are only valid as long as type safety is ensured (e.g. if we were to put instance methods on RawData and start calling them, we would quickly find out that the "unreachable bodies" optimization stubbed them out).

The fix would be to put DynamicDependency on RawData that makes RawData depend on it's ctor, but we don't support that option.

The only options I see are an ILLinkTrim entry for RawData::.ctor, or reverting this change. Do you have a preference?

@MichalStrehovsky
Copy link
Member Author

Brainstorming some more, this particular problem could also be fixed by keeping all fields on explicit/sequential types if any field is kept. It's weird, but it would work and probably still let us optimize out junk.

tkapin pushed a commit to tkapin/runtime that referenced this pull request Jan 31, 2023
We can postpone marking fields on types with layout until we're really sure they're needed. They're needed if they're visible to reflection, or considered instantiated (valuetypes are implicitly considered instantiated).

I'm also allowing stripping fields on Auto layout structs. This is a rare sight, but there's no reason that I know of to force them to have all their fields kept.

Commit migrated from dotnet/linker@da2cc0f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants