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

[jtschuster]

See #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.
• ComInterfaceGenerator should generate a free of the local variable in the catch clause of unmanagedToManaged stubs, and free the old parameter value in the AssignOut stage #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
• Different marshaller structs should be used for unmarshalling and marshalling elements in UnmanagedToManaged stubs #89463
• Track stateful marshallers for each element in unmanaged to managed stubs #89462 ComInterfaceGenerator should warn when a collection's element marshaller is stateful #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 #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>
• Arrays: <SpanOfUnmarshalledValues>.CopyTo(parameter)
• References to reference type: ref = ref <UnmarshalledValue>

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

    _value_native = StructTypeMarshaller.ConvertToManaged(...);
    ...
    (*value) = _value_native;

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

    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_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:	-