Marshalling generator pipeline should unmarshal / marshal parameters into local variables and assign just before return

[jtschuster]

See https://github.com/dotnet/runtime/issues/88111 for how the current generated code doesn't follow the rules.

Motivation:

To follow the ownership and lifetime expectations for COM, the generator pipeline should only assign to parameters and return values at the end of the method after unmarshalling / marshalling all parameters has succeeded. The generally recommended way to achieve this is to avoid modifying any parameters until the last basic block that returns a successful HRESULT. For our generator pipeline, this could look like an additional stage where local variables holding the final values of parameters are assigned to the parameters.

Proposal:

The new list of stages would be:

1. Setup
2. Marshal
3. Pin
4. PinnedMarshal
5. Invoke
6. NotifyForSuccessfulInvoke
7. UnmarshalCapture
8. Unmarshal
9. GuaranteedUnmarshal
10. Cleanup
11. AssignOut

<< Variable Declarations >>
<< Setup >>
try
{
    << Marshal >>
    << Pin >> (fixed)
    {
        << Pinned Marshal >>
        << Invoke >>
    }
    << Notify For Successful Invoke >>
    << Unmarshal Capture >>
    << Unmarshal >>
}
finally
{
    << GuaranteedUnmarshal >>
    << Cleanup >>
}
<< AssignOut >>
return <returnValue>;

Issues:

This causes slightly unexpected behavior for arrays of blittable elements that are pinned in the ManagedToUnmanaged stubs. These values could be modified if the pInvoke succeeds, but unmarshalling throws an exception. However, this may be more of a minor issue since this doesn't lead to a memory leak or double free, and can be fixed by allocating and copying all the values to a new temporary array to pass to the Unmanaged COM method.

Examples:

Assign out would look like the following for the following scenarios for ManagedToUnmanaged.

• struct, blittable type / primitive: Not applicable. These values cannot pass ownership back to the caller.
• reference to struct, blittable type / primitive (ref StructType parameter): \<parameter> = \<UnmarshalledValue>
  • ref StructType parameter:

    // Unmarshal
    StructType _parameter_managed = StructTypeMarshaller.ConvertToManaged((*_parameter_native));
    ...
    // Assign out
    parameter = _parameterManaged;

• Arrays: \<SpanOfUnmarshalledValues>.CopyTo(parameter)
  • ClassType[] array:

    // Unmarshal
    Span<ClassType> _array_managed = ...;
    ...
    // Assign Out
    _array_managed.CopyTo(array);

• References to reference type: = \<UnmarshalledValue>
  • ref ClassType parameter:

    // Unmarshal
    ClassType _parameter_managed = ClassTypeMarshaller.ConvertToManaged((*_parameter_native));
    ...
    // Assign out
    parameter = _parameterManaged;

And like the following for UnmanagedToManaged:

• struct, blittable type / primitive: Not applicable. These values cannot pass ownership back to the caller.
• reference to struct, blittable type / primitive (StructType* parameter): (*\<parameter>) = \<MarshalledValue>
  • StructType* value

    // Marshal
    StructType _value_native = StructTypeMarshaller.ConvertToUnmanaged(_value_managed);
    ...
    // Assign out
    (*value) = _value_native;

• Arrays: \<SpanOfMarshalledValues>.CopyTo(parameterUnmanagedValuesDestination)
  • int* array, int length:

    // Marshal
    Span<int> _array_native_nativeSpan = ...;
    ...
    // Assign out
    Span<int> _array_native_parameterSpan = ArrayMarshaller<int, int>.GetUnmanagedValuesDestination(array, length);
    _array_native_nativeSpan.CopyTo(_array_native_parameterSpan);

• References to references: (*) =
  • int[]* array / int[]* array

    // Marshal
    int* array_native_nativeSpan = ...;
    ...
    // Assign out
    (*array) = _array_native_nativeSpan;`

• SafeHandles
  • nint* handle

    // Marshal
    nint _handle = _handleMarshaller.ToUnmanaged(_handle_managed);
    ...
    // Assign out
    (*handle) = _handle_native;

Drawbacks:

This could be a significant amount of work and may only be necessary for a few edge cases that are left.

Benefits:

This would make our lifetime issues much less likely and generated code would follow general COM guidance. This also would allow us to remove ownership tracking marshalling generators like UnmanagedToManagedOwnershipTrackingStrategy

Todo:

• [ ] Generate a local param_native_out variable to hold the marshalled value.
• [ ] #89483
• [ ] Generate a free of the old parameter in the assign out stage
• [ ] Properly clean up each element of [Out] contents marshalling in unmanaged to managed stubs
• [ ] #89463
• [ ] #89462 https://github.com/dotnet/runtime/issues/89465

[ghost]

Tagging subscribers to this area: @dotnet/interop-contrib See info in area-owners.md if you want to be subscribed.

Issue Details

---

See https://github.com/dotnet/runtime/issues/88111 for how the current generated code doesn't follow the rules. ## Motivation: To follow the ownership and lifetime expectations for COM, the generator pipeline should only assign to parameters and return values at the end of the method after unmarshalling / marshalling all parameters has succeeded. The generally recommended way to achieve this is to avoid modifying any parameters until the last basic block that returns a successful HRESULT. For our generator pipeline, this could look like an additional stage where local variables holding the final values of parameters are assigned to the parameters. ## Proposal: The new list of stages would be: 1. Setup 2. Marshal 3. Pin 4. PinnedMarshal 5. Invoke 6. NotifyForSuccessfulInvoke 7. UnmarshalCapture 8. Unmarshal 9. GuaranteedUnmarshal 10. Cleanup 11. AssignOut ```C# << Variable Declarations >> << Setup >> try { << Marshal >> << Pin >> (fixed) { << Pinned Marshal >> << Invoke >> } << Notify For Successful Invoke >> << Unmarshal Capture >> << Unmarshal >> } finally { << GuaranteedUnmarshal >> << Cleanup >> } << AssignOut >> return ; ``` ## Issues: This causes slightly unexpected behavior for arrays of blittable elements that are pinned in the ManagedToUnmanaged stubs. These values could be modified if the pInvoke succeeds, but unmarshalling throws an exception. However, this may be more of a minor issue since this doesn't lead to a memory leak or double free, and can be fixed by allocating and copying all the values to a new temporary array to pass to the Unmanaged COM method. ## Examples: Assign out would look like the following for the following scenarios for ManagedToUnmanaged. - struct, blittable type / primitive: Not applicable. These values cannot pass ownership back to the caller. - reference to struct, blittable type / primitive (`ref StructType parameter`): \ = \ - Arrays: \.CopyTo(parameter) - References to reference type: ref = ref \ And like the following for UnmanagedToManaged: - struct, blittable type / primitive: Not applicable. These values cannot pass ownership back to the caller. - reference to struct, blittable type / primitive (`StructType* parameter`): (*\) = \ - `StructType* value` ```C# _value_native = StructTypeMarshaller.ConvertToManaged(...); ... (*value) = _value_native; ``` - Arrays: \.CopyTo(parameterUnmanagedValuesDestination) - `int* array, int length`: ```C# Span _array_native_parameterSpan = ArrayMarshaller.GetUnmanagedValuesDestination(array, length); ... _array_native_nativeSpan.CopyTo(_array_native_parameterSpan); ``` - References to references: (*) = - `int** array` ```C# int* array_native_nativeSpan = ...; ... (*array) = _array_native_nativeSpan;` ``` ## Drawbacks: This could be a significant amount of work and may only be necessary for a few edge cases that are left. ## Benefits: This would make our lifetime issues much less likely and generated code would follow general COM guidance. This also would allow us to remove ownership tracking marshalling generators like `UnmanagedToManagedOwnershipTrackingStrategy`

Author:	jtschuster
Assignees:	jtschuster
Labels:	`area-System.Runtime.InteropServices`
Milestone:	-