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

[API Proposal]: Allow setting explicit packing for 32 bit platforms only. #95802

Closed
JeremyKuhne opened this issue Dec 8, 2023 · 5 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Interop-coreclr

Comments

@JeremyKuhne
Copy link
Member

JeremyKuhne commented Dec 8, 2023

Background and motivation

In Win32 there are a number of headers that are set for single byte packing on 32 bit (notably in the shell). The 64 bit definitions use default packing in all of the cases I know of. This creates a problem for defining structs for interop when building for AnyCPU (as WinForms does). We either have to define twice or, in some cases, just ignore the packing as things align on the same boundaries they would with default 32 bit packing (which has risk if one nests struct definitions).

This is blocking us from directly utilizing the published Win32 metadata (through CsWin32) in these cases. If we could specify the packing for 32 bit we would be able to safely generate Win32 interop methods.

I believe this would require a change to the CLI and the runtime / JIT. My naive idea is that if the first byte of PackingSize is specified it would be the 32 bit specific packing and the second byte would be "normal" packing (as the spec limits the packing to a max of 128).

API Proposal

namespace System.Runtime.InteropServices;

public sealed class StructLayoutAttribute : Attribute
{
    // Added (value is shifted into the high byte in metadata)
    public int Pack32BitPlatform;
}

API Usage

[StructLayout(LayoutKind.Sequential, Pack32BitPlatform = 1)]
public partial struct PRINTDLGW
{
   // ...
}

Alternative Designs

Perhaps introducing another attribute? StructLayout32Attribute?

Risks

No response

@JeremyKuhne JeremyKuhne added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 8, 2023
@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 Dec 8, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 8, 2023
@danmoseley danmoseley added area-Interop-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 8, 2023
@tannergooding
Copy link
Member

Perhaps introducing another attribute? StructLayout32Attribute?

I expect this would be required, StructLayout is a "magic" attribute that has special metadata encoding.

I expect that just StructLayout32 would also not be sufficient. Win32 has some ARM specific packing, some 32-bit specific packing, some 64-bit specific packing, etc. So I would push us towards ensuring any "new" thing could sufficiently handle the nuances of this and allow valid CPU/OS combinations as needed.

@MichalPetryka
Copy link
Contributor

MichalPetryka commented Dec 8, 2023

I'd think that some PlatformLayout(System.Runtime.Intrinsics.Architecture) would make the most sense and would allow overriding every element of layout (I guess for explicit layouts we'd need PlatformOffset(System.Runtime.Intrinsics.Architecture) too).

The issue would be that checking this custom attribute could be expensive for the type loader (StructLayout is encoded as magic metadata AFAIK), but I guess it could be worked around by using the currently free LayoutKind value of 1 to be encoded in custom attributes which would make the VM only check them in such rare cases. In such case "transparent layout` could be also encoded as a custom attribute if there'd be a decision to add it.

@jkotas
Copy link
Member

jkotas commented Dec 9, 2023

Duplicate of #60573

@jkotas jkotas marked this as a duplicate of #60573 Dec 9, 2023
@am11
Copy link
Member

am11 commented Dec 9, 2023

We either have to define twice

Or as many times as needed. Statically generating all the required variants of struct at build-time is laborious (can be source generated) but less intense than the full Reflection.Emit alternative. Conditions based on APIs like OperatingSystem.IsWindows(), RuntimeInformation.ProcessArchitecture == Architecture.X86 and Environment.Is64BitProcess are pretty cheap, AOT friendly and can be used to avoid the reflection stack.

@AaronRobinsonMSFT
Copy link
Member

Closing as duplicate.

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 2, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Interop-coreclr
Projects
None yet
Development

No branches or pull requests

7 participants