microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
1.99k stars 84 forks source link

SafeHandle overload should be generated for functions with `out` parameters #1183

Open AArnott opened 1 month ago

AArnott commented 1 month ago

What I don't see here is an actual SafeHandle-derived type being emitted by CsWin32 even when the allocating InitializeProcThreadAttributeList function is generated. This is unexpected, and is probably due to InitializeProcThreadAttributeList using an output parameter to provide the handle to its caller instead of its return value. CsWin32 may not be prepared to trigger its SafeHandle overload and type for allocating functions except by return value.

Originally posted by @AArnott in https://github.com/microsoft/CsWin32/issues/1180#issuecomment-2108583372

From the metadata:

public unsafe static extern BOOL InitializeProcThreadAttributeList([Optional][Out][MemorySize(BytesParamIndex = 3)] LPPROC_THREAD_ATTRIBUTE_LIST lpAttributeList, [In] uint dwAttributeCount, [Optional][Reserved] uint dwFlags, [In][Out] UIntPtr* lpSize);

[RAIIFree("DeleteProcThreadAttributeList")]
[NativeTypedef]
public struct LPPROC_THREAD_ATTRIBUTE_LIST
{
    public unsafe void* Value;
}

That should be enough to produce an overload like this:

internal static unsafe winmdroot.Foundation.BOOL InitializeProcThreadAttributeList(out DeleteProcThreadAttributeListSafeHandle lpAttributeList, uint dwAttributeCount, uint dwFlags, ref nuint lpSize);

Now, I also see that the out parameter is optional, yet C# doesn't allow for optional out parameters, and the friendly overload couldn't tell whether the caller used out _, so that may also complicate fixing this.

waylaa commented 1 month ago

Shouldn't every method that includes out parameters generate SafeHandle derived types?

AArnott commented 1 month ago

Probably. But I'm not sure I understand the question. Already our SafeHandle overloads return derived types and accept the base type. The out parameter signatures should follow the return type precedent by producing the derived type.

waylaa commented 1 month ago

One method coming from the top of my head that does not contain SafeHandle derivative types is GetBuffer from IAudioCaptureClient, with the method being like this:

unsafe void GetBuffer(byte** ppData, out uint pNumFramesToRead, out uint pdwFlags, [Optional] ulong* pu64DevicePosition, [Optional] ulong* pu64QPCPosition);

and its extension method:

internal static unsafe void GetBuffer(this winmdroot.Media.Audio.IAudioCaptureClient @this, out byte* ppData, out uint pNumFramesToRead, out uint pdwFlags, ulong* pu64DevicePosition, ulong* pu64QPCPosition)
{
    fixed (byte** ppDataLocal = &ppData)
    {
        @this.GetBuffer(ppDataLocal, out pNumFramesToRead, out pdwFlags, pu64DevicePosition, pu64QPCPosition);
    }
}

Couldn't the extension method be like this instead? (pu64DevicePosition and pu64QPCPosition can stay as pointers too)

internal static void GetBuffer(this winmdroot.Media.Audio.IAudioCaptureClient @this, out SafeHandle ppData, out uint pNumFramesToRead, out uint pdwFlags, in ulong pu64DevicePosition, in ulong pu64QPCPosition)
AArnott commented 1 month ago

No.

I'm afraid we can't turn byte** parameter types into SafeHandle, as that requires a strongly-typed struct (or typedef in C) so that the metadata can describe what method can be used to release the native resource.

I also don't understand why you would define the last two parameters as in ulong instead of out ulong, considering the direction of data flow as documented for the method.

waylaa commented 1 month ago

I understand. Although it is unfortunate not being able to have SafeHandle there.

I also don't understand why you would define the last two parameters as in ulong instead of out ulong, considering the direction of data flow as documented for the method.

Oops! My bad. I wrote that comment late at evening before going to sleep haha.