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

Wrong StructLayoutAttribute.Size for struct unions with no data fields #18125

Open
ghost opened this issue Dec 9, 2024 · 3 comments
Open

Wrong StructLayoutAttribute.Size for struct unions with no data fields #18125

ghost opened this issue Dec 9, 2024 · 3 comments
Labels
Area-Compiler-CodeGen IlxGen, ilwrite and things at the backend Bug good first issue help wanted Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code.
Milestone

Comments

@ghost
Copy link

ghost commented Dec 9, 2024

Consider this type:

[<Struct>]
type ABC = A | B | C

F# 9 compiles it to a struct with this attribute (among others):

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Auto, Size = 1)]

However, because of the compiler-generated _tag field, the size is actually 4:

> sizeof<ABC>;;                                                        
val it: int = 4

This seems not to cause any problems, but the documentation states "This field must be equal or greater than the total size, in bytes, of the members of the class or structure," which suggests this should be corrected.

@vzarytovskii
Copy link
Member

vzarytovskii commented Dec 9, 2024

It's probably some flaw in logic around:

if
tycon.AllFieldsArray |> Array.exists (fun f -> not f.IsStatic)
||
// Reflection emit doesn't let us emit 'pack' and 'size' for generic structs.
// In that case we generate a dummy field instead
(cenv.options.workAroundReflectionEmitBugs && not tycon.TyparsNoRange.IsEmpty)
then
ILTypeDefLayout.Sequential { Size = None; Pack = None }, ILDefaultPInvokeEncoding.Ansi
else
ILTypeDefLayout.Sequential { Size = Some 1; Pack = Some 0us }, ILDefaultPInvokeEncoding.Ansi

Should be relatively easy to investigate and test. Should be a good first issue, gonna mark it like this.

@vzarytovskii vzarytovskii added good first issue Area-Compiler-CodeGen IlxGen, ilwrite and things at the backend help wanted labels Dec 9, 2024
@0101 0101 added Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code. and removed Needs-Triage labels Dec 16, 2024
@timofey256
Copy link

timofey256 commented Mar 6, 2025

Hello, I would like to try fix this.

  1. What would be the expected Size for the struct would be? I see a few lines below that GenTypeDef specifies it as None. Is this the expected behavior for value type DUs too?
  2. If so, would something like this work?
if
    tycon.AllFieldsArray |> Array.exists (fun f -> not f.IsStatic)
    ||
    // Reflection emit doesn't let us emit 'pack' and 'size' for generic structs.
    // In that case we generate a dummy field instead
    (cenv.options.workAroundReflectionEmitBugs && not tycon.TyparsNoRange.IsEmpty)
    ||
    **// Discriminated unions have a compiler-generated _tag field even when they don't have any data fields,
    // so we should not explicitly set Size = 1 for them
    (match tycon.TypeReprInfo with
     | TFSharpTyconRepr { fsobjmodel_kind = TFSharpUnion } -> true
     | _ -> false)**
then
    ILTypeDefLayout.Sequential { Size = None; Pack = None }, ILDefaultPInvokeEncoding.Ansi
else
    ILTypeDefLayout.Sequential { Size = Some 1; Pack = Some 0us }, ILDefaultPInvokeEncoding.Ansi
  1. Where are tests for this part of code? Unfortunately, couldn't locate.

@vzarytovskii
Copy link
Member

This would work, if the goal is to not set the size.

You shouldn't match tycon directly, it should have IsUnionTycon tester.

Tests can be added independently to the component tests suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compiler-CodeGen IlxGen, ilwrite and things at the backend Bug good first issue help wanted Impact-Low (Internal MS Team use only) Describes an issue with limited impact on existing code.
Projects
Status: New
Development

No branches or pull requests

3 participants