-
Notifications
You must be signed in to change notification settings - Fork 152
Conversation
Hmm, CI failure doesn't give me much to go on. Am going to rerun. @dotnet-bot retest this please |
@dotnet-bot retest this please |
|
||
// Add the current field to the overlap set. | ||
uint32_t FieldSize = DataLayout->getTypeSizeInBits(FieldTy) / 8; | ||
OverlappingFields.push_back(std::make_pair(FieldOffset, FieldTy)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be here? If the field is a scalar, it looks like it's redundant with the subsequent call to addFieldsRecursively
. If the field is a struct, it seems odd that we'd want top-level struct fields on the list but not struct-typed sub-fields (which addFieldsRecursively
will not include in the list)..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this should not be there, it's left over from an earlier iteration. Thanks for spotting it.
LGTM aside from the two points above. |
Closes dotnet#275. Types with explicit layout may have fields that overlap one another (aka union types). Before this change we'd fail to compile any method that referenced one of these union types. We now model them as best we can using LLVM types. LLVM doesn't provide a strong way to describe unions. Instead we generally provide a byte array that covers the extent of the union as a representative placeholder. That means for some fields there is no exact LLMV type counterpart. Downstream consumers cope with this as follows: for any field within the extent of a set of overlapping fields, we omit that field handle from the `FieldIndexMap`, so that when a ldfld or similar goes to find out what LLVM type index to use, it comes up empty-handed. We already had a fall-back path here to simply use the EE provided field offset when this happens, and so now that path kicks in for accesses to overlapping fields, and subsequent pointer casts then fix up the types properly. However things are not quite that simple. We also want the LLVM type to fully describe the location of any GC references within the type, and it is valid for GC references to appear in one of these overlap sets. GC references must fully overlap one another and can't be safely overlapped with non-GC data (see Ecma-355, II.10.7). This means the extents of the GC references in an union partition the union into ranges of either non-GC data or GC references. We report the former using the byte arrays, and the latter as object references. Thus for instance a type `C` like ``` [StructLayout(LayoutKind.Explicit)] public struct A { [FieldOffset(0)]public string Name; [FieldOffset(8)]public int Size; } [StructLayout(LayoutKind.Explicit)] public struct C { [FieldOffset(0)]public A X; [FieldOffset(0)]public string Name; [FieldOffset(8)]public int Size; } ``` would on x64 be described as ``` type <{ %System.Object addrspace(1)*, [8 x i8] }> ``` This special handling for GC references is strictly only necessary for value types (since GC reference locations for value types on the stack must be reported at GC safe points) but for uniformity we do it for all types. We also continue to double-check value type GC locations against the GC pointer info provided by the EE.
Implement support for union types.
Closes #275.
Types with explicit layout may have fields that overlap one another (aka union types). Before this change we'd fail to compile any method that referenced one of these union types. We now model them as best we can using LLVM types.
LLVM doesn't provide a strong way to describe unions. Instead we generally provide a byte array that covers the extent of the union as a representative placeholder. That means for some fields there is no exact LLMV type counterpart. Downstream consumers cope with this as follows: for any field within the extent of a set of overlapping fields, we omit that field handle from the
FieldIndexMap
, so that when a ldfld or similar goes to find out what LLVM type index to use, it comes up empty-handed. We already had a fall-back path here to simply use the EE provided field offset when this happens, and so now that path kicks in for accesses to overlapping fields, and subsequent pointer casts then fix up the types properly.However things are not quite that simple. We also want the LLVM type to fully describe the location of any GC references within the type, and it is valid for GC references to appear in one of these overlap sets. GC references must fully overlap one another and can't be safely overlapped with non-GC data (see Ecma-355, II.10.7). This means the extents of the GC references in an union partition the union into ranges of either non-GC data or GC references. We report the former using the byte arrays, and the latter as object references. Thus for instance a type
C
likewould on x64 be described as
This special handling for GC references is strictly only necessary for value types (since GC reference locations for value types on the stack must be reported at GC safe points) but for uniformity we do it for all types. We also continue to double-check value type GC locations against the GC pointer info provided by the EE.