MapViewOfFile should generate an overload returning SafeMemoryMappedViewHandle

[sharwell]

Actual behavior

MapViewOfFile only generates an overload returning void*.

Expected behavior

A separate overload, e.g. a generated MapViewOfFile_SafeHandle method, should return SafeMemoryMappedViewHandle.

https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.MapViewOfFile.cs#L12-L18

https://referencesource.microsoft.com/#System.Core/Microsoft/Win32/SafeHandles/SafeMemoryMappedViewHandle.cs,fbe85260617a73ea

Repro steps

1. NativeMethods.txt content:

MapViewOfFile

2. NativeMethods.json content (if present):

N/A

3. Any of your own code that should be shared?

Context

• CsWin32 version: 0.1.506-beta
• Win32Metadata version (if explicitly set by project): N/A
• Target Framework: net45
• LangVersion (if explicitly set by project): 9

[AArnott]

We generate SafeHandles where a typedef struct with a release attribute is applied. This would have to come from the metadata.
After which, we may want to add this existing .NET SafeHandle type to our list of safe handles to reuse instead of regenerate. But maybe only on .NET 6, since the inaccessible SafeHandle.ctor before that makes it impossible to call from our generated friendly overload code.

[mikebattista]

What exactly do you want to see MapViewOfFile return instead of void *?

[sharwell]

@mikebattista I would expect it to return SafeMemoryMappedViewHandle, which is the same return type used by P/Invoke definitions for MapViewOfFile inside the .NET Framework.

[mikebattista]

That's how it would be projected, but I'm asking about the metadata.

[sharwell]

@AArnott would need to answer that part. I'm not sure how the metadata relates to the CsWin32 project, and I'm only aware of the output from the latter.

[mikebattista]

For example, CreateFile returns the below in the metadata which C#/Win32 translates into a .NET SafeHandle. Other projections handle it differently.

[RAIIFree("CloseHandle")]
[NativeTypedef]
public struct HANDLE
{
	public IntPtr Value;
}

@AArnott sounded like you wanted something similar here but could you clarify what definition you expect?

[AArnott]

Apparently the value returned from this method is supposed to be released with UnmapViewOfFile, so returning a typedef with an [RAIIReleaseAttribute("UnmapViewOfFile")] may be appropriate. Doing this in the metadata would result in a SafeHandle being generated in the C# projection. We could then reuse .NET's existing SafeHandle type to improve interop.

[mikebattista]

@sotteson1 @kennykerr

So something like this?

[RAIIFree("UnmapViewOfFile")]
[NativeTypedef]
public struct MemoryMappedViewHandle
{
	public IntPtr Value;
}

[AArnott]

Yes, that looks good.

[marler8997]

Somewhat related, the Zig projection would be able to make use of some way of knowing which types are "handles" and which aren't.

Reason being is that Zig distinguishes between handles that are "optional", allowing NULL or 0 to be assigned to them and disallowing this for non-optional handles. My current solution is I maintain a list of all the types in the metadata that are handles (https://github.com/marlersoft/zigwin32gen/blob/main/src/handletypes.zig#L7), but having this information in the metadata would mean other projections could utilize it as well. This could be done with an attribute like [IsHandle] but I'm not picky.

[kennykerr]

Seems ok. #423 (comment)

[AArnott]

FWIW CsWin32 figures out which structs are handle by convention, and a tiny hard-coded list of one: https://github.com/microsoft/CsWin32/blob/14443c08be185c26d91da5acac8c6acaccd37622/src/Microsoft.Windows.CsWin32/Generator.cs#L3114-L3132

[mikebattista]

There are several MapViewOfFile APIs. We need clarity on which Unmap function should be used for each and what to do about additional flags if anything.

• MapViewOfFile
• MapViewOfFile3
• MapViewOfFile3FromApp
• MapViewOfFileEx
• MapViewOfFileExNuma
• MapViewOfFileFromApp
• MapViewOfFileNuma2

Available Unmap functions:

• UnmapViewOfFile
• UnmapViewOfFile2
• UnmapViewOfFileEx

[KalleOlaviNiemitalo]

AFAICT, you can use any Unmap function with any Map function, but if you used a Map function that takes process handle parameter and specified a process other than the caller, then you must use UnmapViewOfFile2 so that you can specify the same process (although not necessarily the same process handle).

[AArnott]

Thanks, @KalleOlaviNiemitalo.
Hmmm... when a function returns a handle that may need to be released in a variety of ways depending on inputs, IMO that's grounds to say just return IntPtr and leave the user to manage it.

[mikebattista]

If we return a MemoryMappedViewHandle for all of these functions but without an RAIIFree, does that buy us anything? Right now they all return void*.

[AArnott]

That's an interesting idea, as it retains more discoverable types than IntPtr would.
Can we get that same typedef struct to be used in each of the release methods, even though [RAIIFree] isn't present on the struct?

[mikebattista]

Yes we could update Unmap* to take this new typedef struct.

[riverar]

Re-opening this issue as I don't think MapViewOfFile should be returning a MEMORYMAPPEDVIEW_HANDLE.

From the docs:

If the function succeeds, the return value is the starting address of the mapped view.

[riverar]

Looking at the .NET code in the original post, it appears this is not a real handle, is just a handle by name, and is just a RAII wrapper around the returned void* that calls UnmapViewOfFile during dispose.

So proposed actions include:

• Change to void*/IntPtr/etc. return type
• Add return:RAIIFree(UnmapViewOfFile) attribute if supported/desired (Optional)

[mikebattista]

We can keep the autotype but change it to void* rather than DECLARE_HANDLE.

[riverar]

@kennykerr Can you double-check me here? Believe this change will resolve microsoft/windows-rs#2436

[kennykerr]

Ideally, there would be a distinct attribute for these "invented" handle types, like MEMORYMAPPEDVIEW_HANDLE, to distinguish them from native types defs that are actually defined in the Windows SDK. Without this, languages like C++ and Rust that aim to preserve the original definitions more faithfully are now stuck not being able to distinguish between something like PSID which is actually defined by the Windows SDK and MEMORYMAPPEDVIEW_HANDLE which is not. I thought this was what the NativeTypeDef attribute was for but perhaps I misunderstood.

[riverar]

I see we still have MEMORYMAPPEDVIEW_HANDLE in there. I don't think that's right. This API returns a pointer to memory. There's no handle at all and metadata shouldn't have any reference to handles.

[riverar]

The following APIs deal with pointers, not handles

MapViewOfFile
MapViewOfFile3
MapViewOfFile3FromApp
MapViewOfFileEx
MapViewOfFileExNuma
MapViewOfFileFromApp
MapViewOfFileNuma2
UnmapViewOfFile
UnmapViewOfFile2
UnmapViewOfFileEx

.NET SafeMemoryMappedViewHandle is a SafeHandleZeroOrMinusOneIsInvalid specialization that overrides Release to call UnmapViewOfFile. Or in other words, just a .NET wrapper. The .NET code comments seem to indicate the developers were aware the API does not return handles and just invented one on the spot.

/*============================================================
**
** Class:  SafeMemoryMappedViewHandle
**
** Purpose: Safe handle wrapping a MMF view pointer
**
** Date:  February 7, 2007
**
===========================================================*/

So to remain language neutral, I argue the aforementioned APIs should return a void*/IntPtr/etc. type. I'm not opposed to additional metadata that would assist others in generating RAII wrappers (e.g. RAIIFree). (I recognize that work is not yet complete though.)

[mikebattista]

We have many cases like this where we add a typedef that is returned by one API and passed to another. Unless you're saying these APIs are unlike all the others where we have void* typedefs, I'm unclear on the problem.

[kennykerr]

Yes, these are unlike functions that return handles. These functions return addresses, just like say CoTaskMemAlloc and LockResource. This is why in Rust I treat all of the NativeTypeDef types as handles rather than just "typedefs" because virtually anything could be a C-style typedef. It's an important distinction.

[mikebattista]

I'm fine reverting the changes for now but that leaves the original feedback unaddressed.

[riverar]

Is something like this possible?

Source:

WINBASEAPI
...
LPVOID
WINAPI
MapViewOfFile(
  _In_ HANDLE hFileMappingObject,
  _In_ DWORD dwDesiredAccess,
  _In_ DWORD dwFileOffsetHigh,
  _In_ DWORD dwFileOffsetLow,
  _In_ SIZE_T dwNumberOfBytesToMap
);

Target:

/* ... */
[return: RAIIFree("UnmapViewOfFile")]
public static extern void* MapViewOfFile(
  [In] HANDLE hFileMappingObject,
  [In] FILE_MAP dwDesiredAccess,
  [In] uint dwFileOffsetHigh,
  [In] uint dwFileOffsetLow,
  [In] UIntPtr dwNumberOfBytesToMap
);

Folks can then choose to do something with the RAIIFree attribute if needed.

[riverar]

Or if you want/need to create a type, at least drop the handle suffix, e.g. MEMORY_MAPPED_VIEW. (This then segues into the question of how do I know what's real and what metadata created for convienence? covered by #1533.)

[mikebattista]

Was it the HANDLE suffix that was the problem? Or the fact that there was a NativeTypeDef at all? What you're suggesting is how we use typedefs in many places but everyone said that was problematic here.

[riverar]

Yeah the HANDLE suffix was bugging me since the API doesn't deal with handles and restarted handle provenance discussion in the Rust community 😂

There's a separate discussion to be had about NativeTypeDefs and whatnot, but am okay to punt that.

[mikebattista]

So if we add back a void* NativeTypeDef but remove the HANDLE suffix, that works for everybody?

[riverar]

I suggested [return: RAIIFree("UnmapViewOfFile")] first because we have an outstanding (admittedly long-standing) issue where projection authors are forced to emit these metadata-generated typedefs (e.g. MEMORYMAPPEDVIEW). They are currently indistinguishable from real typedefs (e.g. HFONT) and therefore cannot be conditionally unwrapped so to speak. I think this goes against the idea that metadata represents the true Windows API surface, with extra bits--like invalid handle values and min target version--available for folks that want it on an opt-in basis.

Ideally the resolution to #1533 would guide the fix here.

[mikebattista]

Let me know if anything looks wrong. Below is the MetadataTypedef definition that you can happily unwrap if you want.

[InvalidHandleValue(0L)]
[MetadataTypedef]
public struct MEMORY_MAPPED_VIEW_ADDRESS
{
	public unsafe void* Value;
}

[riverar]

That looks okay to me.

Am wondering if we can go an extra step and return the APIs to their roots to keep the core definitions separate from metadata sugar. Maybe something like:

[MetadataTypedef]
public struct MEMORY_MAPPED_VIEW_ADDRESS
{
   public unsafe void* Value;
}

[return: XxxReturns("MEMORY_MAPPED_VIEW_ADDRESS")]
public static extern void* MapViewOfFile(
  [In] HANDLE hFileMappingObject,
  [In] FILE_MAP dwDesiredAccess,
  [In] uint dwFileOffsetHigh,
  [In] uint dwFileOffsetLow,
  [In] UIntPtr dwNumberOfBytesToMap
);

I think we have a similar issue already being tracked, will poke around.

[kennykerr]

I took the MetadataTypedef attribute for a spin but it's useless as the metadata doesn't actually tell me what it replaced. I don't know for example that CreateCompatibleDC actually returns an HDC. Ideally the metadata would use the native types (e.g. HDC in this case) and use attributes to indicate alternatives that may optionally be used but as it stands, we can tell that something's changed but not what it changed from.

[mikebattista]

That shouldn't generally be true. In the MapViewOfFile case, you should unwrap the struct to the native type within.

CreateCompatibleDC is interesting because it returns a MetadataTypedef (CreatedHDC) that is AlsoUsableFor a NativeTypedef (HDC) and you'd want the function to return the NativeTypedef.

Is this an outlier? Or did you run into other examples of this case?

[kennykerr]

That shouldn't generally be true. In the MapViewOfFile case, you should unwrap the struct to the native type within.

I'm happy to do that but then I need the native type within CreatedHDC to be HDC, not IntPtr.

[kennykerr]

Is this an outlier? Or did you run into other examples of this case?

Since the metadata drops the original type information, I have no way to determine how widespread this problem is.

[mikebattista]

CreatedHDC does appear to be an outlier here. Ideally CreateCompatibleDC would just return HDC, but CreatedHDC was created to allow for a different RAIIFree function (DeleteDC). Given that HDCs need to be closed with different functions depending on the API, I'm wondering if we should just return HDC here and remove RAIIFree and leave it up to the caller to call the proper API for the scenario. This is the same reason we left off RAIIFree on the MapViewOfFile typedef since it depends on the scenario.

[kennykerr]

That sounds good to me - I was never able to get RAIIFree to work practically. I'd rather the metadata prioritize accurately describing the original APIs.

[mikebattista]

Will do.

I was never able to get RAIIFree to work practically. I'd rather the metadata prioritize accurately describing the original APIs.

For that, please take a look at #389 (comment) and let me know what you think.

[kennykerr]

I won't be using RAIIFree regardless, but I have to use AlsoUsableFor only because of these invented types. If the metadata accurately described the original APIs, then I wouldn't have to use AlsoUsableFor either. It's just a band aid to get back to the original types.

[mikebattista]

It's just a band aid to get back to the original types.

That's not true. In the latest builds, HINSTANCE and HMODULE both exist and are interchangeable. That relationship is defined by AlsoUsableFor. You're going to have problems if you don't support this.

[kennykerr]

That's only because the metadata has two types for the same thing. If that's all it's for, you should probably consider calling it a "type alias" so that they're interchangeable rather than having two distinct types that now have to point at each other and have languages have to track down all uses of one or the other and allow then to be used interchangeably. That's just way too contrived in practice. Just have one type say HINSTANCE and add an attribute that says something like TypeAlias("HMODULE").

[mikebattista]

They're both NativeTypedefs that exist in the headers. AlsoUsableFor means support implicit conversion between the types. C# can handle this just fine. I'm assuming Rust can as well.