dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.76k forks source link

[Swift Interop] Lowering fixed buffers #107620

Open jkurdek opened 2 months ago

jkurdek commented 2 months ago
[StructLayout(LayoutKind.Sequential, Size = 16)]
public unsafe struct Data
{
    public fixed byte bytes[16];
}

Running the swift lowering algorithm on the following struct results in incorrect lowering. I did debug on mono and the resulting lowering is a single unsigned byte. I would expect this to be lowered into two int64.

@jkoritzinsky should such constructs be supported by the swift struct lowering algorithm?

cc: @kotlarmilos

jkoritzinsky commented 2 months ago

Yes, I would expect fixed buffers to be supported for Swift lowering. I believe NativeAOT and CoreCLR already implement this correctly, but I may be misremembering.

jkurdek commented 2 months ago

I'm pretty sure CoreCLR (preview 7) side doesn't work as well. I tried passing this struct as parameter to swift using coreclr backend and failed, which got me looking at runtime details in the first place. I haven't debuged on coreclr so I can't say whether the behaviour is the same as on mono.

jkoritzinsky commented 2 months ago

Looking at this again, it looks like we added support for InlineArray types, not the legacy fixed buffers.

I think it's fine say that legacy fixed buffers aren't supported due to the difficulties of recognizing legacy fixed buffers at the runtime level and the existing limitations of fixed buffers in interop scenarios. What do you think?

jkurdek commented 2 months ago

I do think that InlineArray should be sufficient. @kotlarmilos we will have to adjust the projection here https://github.com/dotnet/runtimelab/issues/2584

We do lack the support for struct lowering them in mono though. Do we have a tracking issue for adding this to mono, or should I create one?

The only thing which worries me about fixed buffers is that lowering them results in unexpected result. I do not know what should the convention be. Should we report an error when trying to lower unsupported type?

kotlarmilos commented 2 months ago

We do lack the support for struct lowering them in mono though. Do we have a tracking issue for adding this to mono, or should I create one?

Yes, please create a tracking issue for InlineArray in Mono.

The only thing which worries me about fixed buffers is that lowering them results in unexpected result. I do not know what should the convention be. Should we report an error when trying to lower unsupported type?

Yes, that would be ideal.

jkurdek commented 2 months ago

Support for InlineArray in mono was fixed with https://github.com/dotnet/runtime/pull/107744

jkurdek commented 2 months ago

We can leave this issue to track the effort on handling the fixed buffer. I do think that we should eventually either implement support for it or detect it and raise an appropriate error.

dotnet-policy-service[bot] commented 1 month ago

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