-
Notifications
You must be signed in to change notification settings - Fork 142
Update crossarch list based on new component partitions #2124
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
riverar
left a comment
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.
The headers show:
typedef union
{
#ifdef _WIN64
QWORD IP6Qword[2];
#endif
DWORD IP6Dword[4];
WORD IP6Word[8];
BYTE IP6Byte[16];
#ifdef IN6_ADDR
IN6_ADDR In6;
#endif
}
IP6_ADDRESS, *PIP6_ADDRESS;
Metadata currently represents the IP6_ADDRESS as:
[StructLayout(LayoutKind.Explicit)]
public struct IP6_ADDRESS
{
[FieldOffset(0)]
public uint[] IP6Dword;
[FieldOffset(0)]
public ushort[] IP6Word;
[FieldOffset(0)]
public byte[] IP6Byte;
}
This structure exists to facilitate type punning of IPv6 addresses in C++. The union allows the same 16-byte IPv6 address to be accessed as different integer types without copying data.
I'm not sure this PR is worth the cost of downstream churn of multiple architecture-specific types. For example, folks may be using this in AnyCPU apps and will suddenly need to do a bunch of work for a QWORD accessor they likely don't care about.
I think a better path forward for improving IP6_ADDRESS could be:
- Keep the type architecture-neutral
- Add the missing QWORD member
- Add the missing IN6_ADDR member
This approach provides the missing functionality without breaking existing code or forcing architecture-specific handling.
|
@riverar It was actually a cswin32 unittest that caught this regression -- it used to be arch-specific in the 61.x version of win32metadata and became arch-neutral accidentally in 64.x. So the goal here was to restore the previous behavior. You're right that in theory this struct is completely useable without access to that field, but then I think if we're going to change it shouldn't we find a way to do it where the arch-specific compilations of the headers produce the same result? That way if someone comes and does what I did where I just compiled with ExcludeFromCrossArch removed, we'd want it to show that there are no arch differences. If we want to do this then is there an affordance in the tool to say "treat this specific type as arch-neutral even though it's not"? Or do we need to provide an alternate definition in a manually defined header somewhere? |
|
I'm concerned that we're thinking only about IP6_ADDRESS here. Isn't it the whole header file? Unless anyone has audited the file carefully, we should assume there may be other But as @jevansaks says, converting it to be arch-specific will hardly be a horrible blow to people, since it has been arch-specific for a very long time. |
In theory all headers could be looking at architecture-specific constants. I can do what @riverar suggests but I don't know the sustainable change to make. I don't want to hack the windnsdef.h header file since that comes from the SDK. Perhaps we #undef _WIN64 before including that header even on _WIN64? It's sketchy though because the header has this: My preference is to merge this in to restore behavior with this struct to where it was and then @riverar we can discuss how to fix this to be AnyCPU if you feel strongly about it. |
|
Agreed! When I realized it was a regression, it definitely changed the calculus here. |
|
@jevansaks ya, we definitely won't want to be messing with pack values. They need to be right for each architecture. |
|
Thanks @riverar! @mikebattista, can you approve & merge? |
IP6_ADDRESS used to be per-arch but was dropped when it moved to the Dns partition (which was set as ExcludeFromCrossArch). Remove Dns partition from ExcludeFromCrossArch so we can once again have per-arch IP6_ADDRESS.