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

LARGE_INTEGER/ULARGE_INTEGER should map to Int64/UInt64 #69

Closed
saul opened this issue Jan 24, 2021 · 7 comments · Fixed by #99
Closed

LARGE_INTEGER/ULARGE_INTEGER should map to Int64/UInt64 #69

saul opened this issue Jan 24, 2021 · 7 comments · Fixed by #99
Assignees

Comments

@saul
Copy link

saul commented Jan 24, 2021

These structs date back to a time where C compilers didn't natively support 64-bit integers. I think these should be represented as System.Int64 and System.UInt64 in the metadata.

I tried having a go at this myself by adding:

LARGE_INTEGER=Int64
_LARGE_INTEGER=Int64
ULARGE_INTEGER=UInt64
_ULARGE_INTEGER=UInt64

To generation/baseRemap.rsp, but to my surprise it just resulted in Microsoft.Windows.Sdk.Int64 and Microsoft.Windows.Sdk.UInt64 types being generated.

Do you think this is a reasonable change?

@MauriceKayser

This comment has been minimized.

@kennykerr
Copy link

FILETIME and ULARGE_INTEGER are not interchangeable because they have different alignment. FILETIME consists of two 32-bit fields whereas ULARGE_INTEGER consists of a union, one of whose fields is a single 64-bit value. Hence the alignment of ULARGE_INTEGER will be 8 bytes and the alignment of FILETIME will be 4 bytes.

So it is indeed safe to replace ULARGE_INTEGER and LARGE_INTEGER with uint64 and int64 but it is not safe to do so for FILETIME.

@DefaultRyan
Copy link
Member

I believe that doing this would break ABI, based on my experiences with microsoft/cppwin32#3.

The gist of what I found in that defect is that this:

int64 f1();

and this:

struct S
{
  int 64 value;
}
S f2();

are going to push different amounts of stuff onto the stack, causing memory corruption and crashes if we try to pretend one of them is the other.

We need to keep these structs as-is in the metadata. Projections are free to simplify them and project them idiomatically, but the structs need to exist in metadata so that we preserve ABI compatibility.

@kennykerr
Copy link

kennykerr commented Jan 26, 2021

Right, there is a peculiarity in the C++ ABI for the Visual C++ compiler that results in these being different when stdcall is involved. Rust for Windows already maps these to i64 and u64 because it doesn't suffer from this weirdness, but Visual C++ may not be able to handle that reliably.

If your language doesn't yet map this in a natural way, I suggest you open an issue with the language team.

@saul
Copy link
Author

saul commented Jan 26, 2021

Okay just so we're on the same page - should I open an issue on CsWin32 to have the struct projected to int64/uint64?

@kennykerr
Copy link

Yes, for C# it would be the CsWin32 repo.

@kennykerr kennykerr transferred this issue from microsoft/win32metadata Jan 26, 2021
@kennykerr kennykerr reopened this Jan 26, 2021
@kennykerr
Copy link

@saul - I've just transferred it here for you.

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 a pull request may close this issue.

5 participants