[Swift Bindings] Project value types with heap-allocated properties as C# classes

[kotlarmilos]

Description

This PR updates marshalling to project Swift value types that require explicit memory handling as C# classes. Heap-allocated Swift types and value types with heap allocated properties are projected as C# classes. The projected C# classes implement ValueWitnessTable->Destroy in their finalizer to ensure proper memory deallocation.

Changes

• Extended TypeRecord with Kind and Flag enums for marshalling
• Marshalling is made explicit by replacing StructIsMarshalledAsCSStruct and ArgumentIsMarshalledAsCSStruct with IsFrozen and IsHeapAllocated
• Removed redundant MarshalToSwift copy for frozen bound generics in P/Invoke calls
• Replaced local allocations for generics with stackalloc for implicit memory management and perf improvements
• Updated manual bindings to implement correct Dispose handling

Validation

Integration tests and manual bindings tests are expanded to verify memory deallocation and parameter ref counting.

Out of Scope

• inout parameters are not yet supported, so memory handling is not implemented.
• UnmanagedCallersOnly is not yet supported, so memory handling is not implemented
• Debug checks for memory handling are not implemented: [Swift ABI compatibility] Implement debug checks for correct memory handling #3037
• Projections could be simplified by removing the IDisposable pattern and implementing SafeHandlers: [Swift ABI compatibility] Implement Swift payload as SafeHandler to decouple IDisposable from the bindings #3018

Contributes to #2851

Fixes #3035 #3035 #3036 #2985

[rolfbjarne]

Does Dispose really have to be thread-safe? It seems that if two threads are accessing the same instance at the same time, then they're already in trouble, since it seems unlikely both threads will only be calling Dispose (and if any of those two thread are also calling instance members, then that's not thread-safe even if Dispose by itself is).

[kotlarmilos]

Calling instance methods doesn't change ref counts, but calling Dispose does -- that's the main difference. Also, Swift doesn't have mechanism to prevent from calling Destroy more than once.

It is true that we haven't run into a situation where that becomes a problem.

[rolfbjarne]

Calling instance methods doesn't change ref counts, but calling Dispose does -- that's the main difference.

Right, but code like this isn't safe anyways:

// thread 1
Console.WriteLine (obj.ToString ());
obj.Dispose ();

// thread 2
obj.Dispose ();

Because if thread 2's Dispose is executed when thread 1 is executing ToString, a crash will occur. The fact that the Dispose methods are thread-safe doesn't change this.

[kotlarmilos]

Yes, you are right.

[kotlarmilos]

What are the implications? I see these two:

• Perf implications
• Not a valid scenario

[rolfbjarne]

I agree that calling Dispose twice should be handled correctly, but my point is that the extra work of making it thread-safe seems like it's not actually fixing any real-world scenarios.

[kotlarmilos]

I've reverted the thread-safety changes. Both points are valid -- the bindings should be thread-safe, and making only Dispose thread-safe does not guarantee overall thread-safety of the bindings.

I recommend that we revisit this issue when we encounter real-world scenarios.

[jkotas]

Related to #2975 .

we revisit this issue when we encounter real-world scenarios.

This is about how to use the bindings securely. You may want to think it through earlier rather than later. Once you encounter real-world scenarios related to security, it is typically too late.

[kotlarmilos]

Thanks, lets add them here or in a follow-up.

[kotlarmilos]

Follow-up #3052

[jkurdek]

Do we need to place tuples in the type database? Arent they always constructed from other types?

[kotlarmilos]

Void is the only case and it could be special-handled.

[jkurdek]

We might need to have a note somewhere that "Frozen" on the tooling side is not equal to "Frozen" on swift side (e.g. frozen struct containing a reference to a reference type).

Edit: I meant that the fact the a frozen type contains a reference type or a non-frozen struct does not make it "non-frozen" on Swift side. It is still "frozen" but has layout unknown at compile time. In the tooling I made a mistake of starting using "frozen" and "non-frozen" to mean projection as struct / class, which is loosly connected to the above.

AFAIK the rule is:
if the layout is known at compile time && no memory management is necessary: struct
Else: class

Since you are making the names more descriptive (which I really like!) I think it would be good to at least encode that information somewhere

[kotlarmilos]

Thanks, added as a comment.