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

For static extern methods, C# should support passing null to any out or ref parameters marked with [Optional]. #13119

Closed
tannergooding opened this issue Aug 12, 2016 · 11 comments

Comments

@tannergooding
Copy link
Member

tannergooding commented Aug 12, 2016

Issue
There are a number of interop scenarios where a static extern method takes an out parameter and allows it to be null.

For Example

HRESULT WINAPI D3D12CreateDevice(
  _In_opt_  IUnknown          *pAdapter,
            D3D_FEATURE_LEVEL MinimumFeatureLevel,
  _In_      REFIID            riid,
  _Out_opt_ void              **ppDevice
);

might translate to

[DllImport("D3D12.dll", BestFitMapping = false, CallingConvention = CallingConvention.Winapi, CharSet = CharSet.Unicode, EntryPoint = "D3D12CreateDevice", ExactSpelling = true, PreserveSig = true, SetLastError = false, ThrowOnUnmappableChar = false)]
public static extern int CreateDevice(
    [In, Optional] IntPtr pAdapter,
    [In] uint MinimumFeatureLevel,
    [In] ref Guid riid,
    [Out, Optional] out IntPtr ppDevice
);

However, C# does not currently support passing null to the optional fourth parameter.

Workaround
Use unsafe code and remove the out keyword so that the user can actually pass null.

For example, the above becomes:

[DllImport("D3D12.dll", BestFitMapping = false, CallingConvention = CallingConvention.Winapi, CharSet = CharSet.Unicode, EntryPoint = "D3D12CreateDevice", ExactSpelling = true, PreserveSig = true, SetLastError = false, ThrowOnUnmappableChar = false)]
unsafe public static extern int CreateDevice(
    [In, Optional] IntPtr pAdapter,
    [In] uint MinimumFeatureLevel,
    [In] ref Guid riid,
    [Out, Optional] IntPtr* ppDevice
);

Proposal
For static extern methods, C# should support passing null to any out or ref parameter marked with Optional.

This would allow you to write code such as:

[DllImport("D3D12.dll", BestFitMapping = false, CallingConvention = CallingConvention.Winapi, CharSet = CharSet.Unicode, EntryPoint = "D3D12CreateDevice", ExactSpelling = true, PreserveSig = true, SetLastError = false, ThrowOnUnmappableChar = false)]
public static extern int CreateDevice(
    [In, Optional] IntPtr pAdapter,
    [In] uint MinimumFeatureLevel,
    [In] ref Guid riid,
    [Out, Optional] out IntPtr ppDevice
);

public static int CreateDevice(IntPtr pAdapter, uint MinimumFeatureLevel, ref Guid riid)
{
    return CreateDevice(pAdapter, MinimumFeatureLevel, ref riid, null); // or possibly: out null
}

Other Thoughts
It would be immensely useful if this was supported in other scenarios as well, such as for COM Interop.

For example, one might modify the above function to take a strongly type interface:

[Guid("2411E7E1-12AC-4CCF-BD14-9798E8534DC0"), InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
unsafe public interface IDXGIAdapter : IDXGIObject
{
    // Method Declarations
}

[Guid("189819F1-1DB6-4B57-BE54-1821339B85F7"), InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
unsafe public interface ID3D12Device : ID3D12Object
{
    // Method Declarations

    [PreserveSig]
    HRESULT CreateHeap(
        [In] ref D3D12_HEAP_DESC pDesc,
        [In] ref Guid riid,
        [Out, Optional] out ID3D12Heap ppvHeap
    );

    // More Method Declarations
}

[DllImport("D3D12.dll", BestFitMapping = false, CallingConvention = CallingConvention.Winapi, CharSet = CharSet.Unicode, EntryPoint = "D3D12CreateDevice", ExactSpelling = true, PreserveSig = true, SetLastError = false, ThrowOnUnmappableChar = false)]
unsafe public static extern int CreateDevice(
    [In, Optional] IDXGIAdapter pAdapter,
    [In] uint MinimumFeatureLevel,
    [In] ref Guid riid,
    [Out, Optional] out ID3D12Device ppDevice
);

The proposal would work for the static extern method, but it would not work for the ID3D12Device.CreateHeap method.

Support for the static extern method is much simpler, because the code is guaranteed to not exist in managed code.

However, support for the latter would require some extension to the language. Such an extension would either indicate the interface/delegate/method exist in native code (and can therefore handle a null out parameter) or it would allow the language itself to work with null out parameters (currently there is no way to determine that the out is null, only that the object out points to is null).

It should be noted that [ComImport] does provide some of the functionality desired for optional out parameters on interfaces. However, applying the ComImport attribute may provide functionality that is undesirable in some COM interop scenarios.

@jnm2
Copy link
Contributor

jnm2 commented Aug 12, 2016

I would love this. It would save maintaining a plethora of alternate signatures with IntPtr in place of ref/out just so I can pass IntPtr.Zero.

@svick
Copy link
Contributor

svick commented Aug 12, 2016

I agree that it would be great if this specific scenario was improved, but I don't like that this proposal adds special behavior that only applies to extern methods. Especially since ignoring an out parameter is useful for non-extern methods too (see e.g. #13104).

What if passing null worked for all methods? For extern methods, it would actually pass the null pointer and for managed methods, it would pass a reference to an otherwise inaccessible variable (which makes it kind of similar to out var #6183).


Also, how would this proposal look like at IL level? Since calling an extern method with out parameter looks the same as calling a normal method with a ref/& parameter in IL, you can't do something like ldnull call (at least not in verifiable code). Would there be some special static field recognized by the compiler and the CLR, used as a placeholder for null? Or am I missing a more reasonable implementation?

@tannergooding
Copy link
Member Author

@svick, I attempted to address your first thought (about working everywhere) in the Other Thoughts section.

I agree working with all methods would be great, but as you mentioned, there would need to be language (and possibly CLR support) for getting it to work everywhere.

I don't think using a special field will work either (without CLR support), as the native code called into wouldn't see the variable as null, it would see it as addressof special field, which could potentially lead to memory leaks.

I think II.14.4.1 and II.14.4.2 cover the majority of the issues that would arise from doing something like this.

Namely:

Unverified code can pass an unmanaged pointer to a method that expects a managed pointer. This is safe only if one of the following is true:
a. The unmanaged pointer refers to memory that is not in memory used by the CLI for storing instances of objects (“garbage-collected memory” or “managed memory”).
b. The unmanaged pointer contains the address of a field within an object.
c. The unmanaged pointer contains the address of an element within an array.
d. The unmanaged pointer contains the address where the element following the last element in an array would be located.

and

Managed pointers cannot be null, and they shall be reported to the garbage collector even if they do not point to managed memory.

@jnm2
Copy link
Contributor

jnm2 commented Aug 12, 2016

It would be just as useful in non-extern methods, I agree.

Would the value in the temporary out variable be disposed if it implements IDisposable?

@tannergooding
Copy link
Member Author

tannergooding commented Aug 12, 2016

@jnm2, the temporary out variable would need to be aware of the exact semantics of the unmanaged allocation (meaning, is it COM, is it Heap Allocated, is it CoTaskMem, was it allocated by a call to malloc or new, etc...). Otherwise, given an arbitrary pointer, how do you know how it was allocated and therefore how to clean it up?

Edit I may have misread your comment. If the compiler generated a temporary variable (as it does with out var) and the type of said variable implemented IDisposable, then it would be disposed when the GC finally got around to it, yes (the compiler would need to explicitly support disposing it immediately otherwise).

In either case, having a temporary variable that is immediately or indirectly disposed introduces some overhead that is undesirable. The primary goal is to keep interop code easy to write and to allow the user to prevent the allocation entirely when a method supports it.

@gafter
Copy link
Member

gafter commented Aug 17, 2016

The new "out variable declaration" feature improves this use case, and in the future we expect to support out var * to not even create a variable.

@svick
Copy link
Contributor

svick commented Aug 17, 2016

@gafter How is out var or out var * useful in the PInvoke case, where you want the called method to actually receive a null pointer?

@tannergooding
Copy link
Member Author

@gafter, I agree with @svick.

The new out var feature, does improve the functionality somewhat (in that you don't have to create a variable and can do everything inline). However, it really misses the core point of this issue.

A lot of unmanaged functions that allow the out parameter to be null will execute a very different code path when the parameter is actually set to null. That is:

HRESULT WINAPI D3D12CreateDevice(
  _In_opt_  IUnknown          *pAdapter,
            D3D_FEATURE_LEVEL MinimumFeatureLevel,
  _In_      REFIID            riid,
  _Out_opt_ void              **ppDevice
);

// This code validates that the default adapter supports Direct3D12, but does not
// actually do the expensive operation of creating the device
D3D12CreateDevice(nullptr, D3D_FEATURE_LEVEL_12_0, __uuidof(ID3D12Device), nullptr);

// This code validates that the default adapter supports Direct3D12 and also
// creates the device if it is supported.
ID3D12Device* pD3DDevice = nullptr;
D3D12CreateDevice(nullptr, D3D_FEATURE_LEVEL_12_0, __uuidof(ID3D12Device), &pD3DDevice); 

Using just the out var declaration support, the second code path (where the expensive operation of creating a device is done) is executed. This can lead to memory bloat, additional cleanup costs, pre-mature resource initialization, etc...

This issue is proposing that, for an external method in the format of:

public enum D3D_FEATURE_LEVEL
{
    // Enumeration Values
}

[StructLayout(LayoutKind.Sequential, Pack = 8, Size = 4)]
public struct HRESULT
{
     private int _value;
}

[Guid("2411E7E1-12AC-4CCF-BD14-9798E8534DC0"), InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
unsafe public interface IDXGIAdapter : IDXGIObject
{
    // Method Declarations
}

[Guid("189819F1-1DB6-4B57-BE54-1821339B85F7"), InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
unsafe public interface ID3D12Device : ID3D12Object
{
    // Method Declarations
}

[DllImport("D3D12.dll", BestFitMapping = false, CallingConvention = CallingConvention.Winapi, CharSet = CharSet.Unicode, EntryPoint = "D3D12CreateDevice", ExactSpelling = true, PreserveSig = true, SetLastError = false, ThrowOnUnmappableChar = false)]
public static extern HRESULT D3D12CreateDevice(
    [In, Optional, MarshalAs(UnmanagedType.Interface)] IDXGIAdapter pAdapter,
    [In] D3D_FEATURE_LEVEL MinimumFeatureLevel,
    [In] ref Guid riid,
    [Out, Optional, MarshalAs(UnmanagedType.Interface)] out ID3D12Device ppDevice
);

The compiler should support a calling D3D12CreateDevice in a manner similar to the following

var riid = typeof(ID3D12Device).GUID;
D3D12CreateDevice(null, D3D_FEATURE_LEVEL._12_0, ref riid, out null);

This would cause the first code path to execute (where the expensive operation is not performed) because the compiler passes 'null' to the final parameter.

As mentioned in the OP, there are several workarounds (such as using IntPtr or void** and calling the appropriate Marshal.* methods to convert between the RCW and an unsafe pointer so that either IntPtr.Zero or null can be passed in). However, it requires some overhead that may be undesirable in these scenarios.

@int19h
Copy link

int19h commented Sep 10, 2016

I believe you'd need P/Invoke marshaling support (i.e. adding some code in CLR) to do this right. Even when the compiler knows that the method is extern, it can't circumvent CLR type safety rules to pass an actual null address to it - there's no way to do so on IL level that wouldn't involve unsafe code. So you do need a dummy value, and then you need the P/Invoke layer to detect those dummy values.

However, instead of generating them, P/Invoke can just offer a public generic class generating such wrappers - i.e. somewhere in System.Runtime.InteropServices, there would be:

public static class NullPointer<T> {
  public T Value;
}

and then, whenever P/Invoke marshaler sees that the parameter is out or ref, and the corresponding managed pointer points to NullPointer<T>.Value, it translates that to null.

In fact, with this addition, there's no need for language support at all. The only thing that compiler would be doing is desugaring null into NullPointer<T>.Value, and it's probably not worthwhile just for P/Invoke.

@int19h
Copy link

int19h commented Sep 10, 2016

Another possibility is to reuse StrongBox<T> for P/Invoke contracts to mean "nullable pointer to something". So instead of e.g. ref int x, you'd declare the method with StrongBox<T> x. Since StrongBox itself is a reference type, one can pass null. But, again, this requires special support in P/Invoke marshaler.

@tannergooding
Copy link
Member Author

Ported to dotnet/csharplang#79

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants