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.

Sign up for GitHub

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

Fix explicit offset of ByRefLike fields. #111584

Merged
merged 16 commits into from
Jan 31, 2025
Merged

Fix explicit offset of ByRefLike fields. #111584

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
948d73b
Fix explicit offset of ByRefLike fields.
AaronRobinsonMSFT Jan 19, 2025
b0cda63
Improve tests coverage
AaronRobinsonMSFT Jan 19, 2025
f87bb60
Fix mono error checking
AaronRobinsonMSFT Jan 27, 2025
405b24e
Update native AOT.
AaronRobinsonMSFT Jan 27, 2025
d0fbad1
Feedback
AaronRobinsonMSFT Jan 28, 2025
f762b1d
Update native AOT tests
AaronRobinsonMSFT Jan 28, 2025
3c86eb9
Feedback
AaronRobinsonMSFT Jan 28, 2025
c8285e9
Fix test build warning
AaronRobinsonMSFT Jan 29, 2025
c458d74
AOT should needs to pass along the base offset.
AaronRobinsonMSFT Jan 29, 2025
3b789f2
Add property for type containing any byrefs.
AaronRobinsonMSFT Jan 29, 2025
247e5e5
Feedback
AaronRobinsonMSFT Jan 29, 2025
8cfee36
Feedback
AaronRobinsonMSFT Jan 30, 2025
59f7ec4
Update docs/coding-guidelines/breaking-change-rules.md
AaronRobinsonMSFT Jan 30, 2025
ac4e3d2
Fix native AOT predicate
AaronRobinsonMSFT Jan 30, 2025
47a7a13
Fix style
AaronRobinsonMSFT Jan 30, 2025
7f86488
Revert changes to breaking-change-rules.md
AaronRobinsonMSFT Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/coding-guidelines/breaking-change-rules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,8 @@ Breaking Change Rules

* Changing a `struct` type to a `ref struct` type and vice versa

* Adding a new `ref` field to a type that didn't have any `ref` field before (recursively)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not following what's the rationale for this. How is this different from adding/removing any fields? We have no guarantees for internal type layout compatibility, unless the guarantee is explicitly documented.

"Changing a struct type to a ref struct type and vice versa" is very different - it is very obvious compile time breaking change.

Copy link
Member Author

@AaronRobinsonMSFT AaronRobinsonMSFT Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point of it is that now adding a ref field could break someone if they were using the type in an Explicit offset scenario. For example, the BCL has some ref struct that just has an int field. A user consumes that type in a user defined type as an Explicit field and the containing library is published to NuGet. In the next major version a ref int field is added. When that NuGet package is consumed on a new BCL, the library fails to load because the ref int field forces a new alignment restriction.

By definition this would now break. We can argue if it matters, but it would cause a failure at type load time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point of it is that now adding a ref field could break someone if they were using the type in an Explicit offset scenario.

It is the case for any field changes in public BCL value types. You can get the same break by adding/removing int or object fields too. There is nothing special about ref fields here.

Field layout of BCL value types is not a public contract that people can depend on, unless it is explicitly documented for specific types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Changing a struct type to a ref struct type and vice versa" is very different - it is very obvious compile time breaking change.

It is the case for any field changes in public BCL value types. You can get the same break by adding/removing int or object fields too. There is nothing special about ref fields here.

Between the two statements above, it looks like the distinction in this case is compile time vs run time failure. Is that the conversation we are having? In the sense that compile time failure is not acceptable, but run time failure is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There happens to be a compile time vs. runtime time difference between these two case, but that's just accidental. Runtime breaking changes are not acceptable either in general.

I think that the discussion that we are having here is about distinction between what we define as a breaking change vs. change that can break apps that depend on undocumented behaviors.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the case for any field changes in public BCL value types. You can get the same break by adding/removing int or object fields too. There is nothing special about ref fields here.

I thought we already consider such things breaking as well - for example adding an object typed field to a struct makes the struct no longer meet unmanaged constraint. I recall Roslyn team even did work to put a dummy object field into such struct for reference assemblies because these non-contractual private implementation details actually matter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the libraries folks agree, I think it would be fine to explicitly match the current Roslyn behavior and say that adding first field of a new kind to an existing value type is disallowed change. Kind can be any of objref, byref or generic type argument, and the rule applies recursively.

Introducing new significant state in existing value types is close to impossible to do in compatible way for other reasons (e.g. it is why we have both DateTime and DateTimeOffset), so these situations do not come up in practice. I guess it is the reason why nobody bothered to mention it in the breaking changes doc up until now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will create a discussion issue for this topic and loop in libraries folks. Updating the markdown is a cheaper PR than continually running the build/test legs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This falls into the same category as dangers of private reflection that are not particularly well documented either.

I have created #112114

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention it in https://learn.microsoft.com/en-us/dotnet/standard/native-interop/best-practices

Opened dotnet/docs#44672


* Changing the underlying type of an enum

This is a compile-time and behavioral breaking change as well as a binary breaking change which can make attribute arguments unparsable.
Expand Down
6 changes: 6 additions & 0 deletions src/coreclr/tools/Common/Compiler/Int128FieldLayoutAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ public override bool ComputeContainsGCPointers(DefType type)
return false;
}

public override bool ComputeContainsByRefs(DefType type)
{
Debug.Assert(!_fallbackAlgorithm.ComputeContainsByRefs(type));
return false;
}

public override bool ComputeIsUnsafeValueType(DefType type)
{
Debug.Assert(!_fallbackAlgorithm.ComputeIsUnsafeValueType(type));
Expand Down
6 changes: 6 additions & 0 deletions src/coreclr/tools/Common/Compiler/VectorFieldLayoutAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,12 @@ public override bool ComputeContainsGCPointers(DefType type)
return false;
}

public override bool ComputeContainsByRefs(DefType type)
{
Debug.Assert(!_fallbackAlgorithm.ComputeContainsByRefs(type));
return false;
}

public override bool ComputeIsUnsafeValueType(DefType type)
{
Debug.Assert(!_fallbackAlgorithm.ComputeIsUnsafeValueType(type));
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/Common/JitInterface/SwiftPhysicalLowering.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ protected override LoweredType GetIntervalDataForType(int offset, TypeDesc field
return LoweredType.Opaque;
}

protected override bool NeedsRecursiveLayout(int offset, TypeDesc fieldType) => fieldType.IsValueType && !fieldType.IsPrimitiveNumeric;
protected override bool NeedsRecursiveLayout(TypeDesc fieldType) => fieldType.IsValueType && !fieldType.IsPrimitiveNumeric;

private List<FieldLayoutInterval> CreateConsolidatedIntervals()
{
Expand Down
39 changes: 39 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/Common/DefType.FieldLayout.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@ private static class FieldLayoutFlags
/// True if the type transitively has a Vector<T> in it or is Vector<T>
/// </summary>
public const int IsVectorTOrHasVectorTFields = 0x1000;

/// <summary>
/// True if ContainsByRefs has been computed
/// </summary>
public const int ComputedContainsByRefs = 0x2000;

/// <summary>
/// True if the type contains byrefs
/// </summary>
public const int ContainsByRefs = 0x4000;
}

private sealed class StaticBlockInfo
Expand Down Expand Up @@ -113,6 +123,21 @@ public bool ContainsGCPointers
}
}

/// <summary>
/// Does a type transitively have any fields which are byrefs
/// </summary>
public bool ContainsByRefs
{
get
{
if (!_fieldLayoutFlags.HasFlags(FieldLayoutFlags.ComputedContainsByRefs))
{
ComputeTypeContainsByRefs();
}
return _fieldLayoutFlags.HasFlags(FieldLayoutFlags.ContainsByRefs);
}
}

/// <summary>
/// Does a type transitively have any fields which are marked with System.Runtime.CompilerServices.UnsafeValueTypeAttribute
/// </summary>
Expand Down Expand Up @@ -545,6 +570,20 @@ public void ComputeTypeContainsGCPointers()
_fieldLayoutFlags.AddFlags(flagsToAdd);
}

public void ComputeTypeContainsByRefs()
{
if (_fieldLayoutFlags.HasFlags(FieldLayoutFlags.ComputedContainsByRefs))
return;

int flagsToAdd = FieldLayoutFlags.ComputedContainsByRefs;
if (this.Context.GetLayoutAlgorithmForType(this).ComputeContainsByRefs(this))
{
flagsToAdd |= FieldLayoutFlags.ContainsByRefs;
}

_fieldLayoutFlags.AddFlags(flagsToAdd);
}

public void ComputeIsUnsafeValueType()
{
if (_fieldLayoutFlags.HasFlags(FieldLayoutFlags.ComputedIsUnsafeValueType))
Expand Down
18 changes: 7 additions & 11 deletions src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ protected override FieldLayoutTag GetIntervalDataForType(int offset, TypeDesc fi
else if (fieldType.IsValueType)
{
MetadataType mdType = (MetadataType)fieldType;
if (!mdType.ContainsGCPointers && !mdType.IsByRefLike)
if (!mdType.ContainsGCPointers && !mdType.ContainsByRefs)
{
// Plain value type, mark the entire range as NonORef
return FieldLayoutTag.NonORef;
}
Debug.Fail("We should recurse on value types with GC pointers or ByRefLike types");
Debug.Fail("We should recurse on value types with GC pointers or byrefs");
return FieldLayoutTag.Empty;
}
else
Expand All @@ -74,20 +74,16 @@ protected override FieldLayoutTag GetIntervalDataForType(int offset, TypeDesc fi
}
}

protected override bool NeedsRecursiveLayout(int offset, TypeDesc fieldType)
protected override bool NeedsRecursiveLayout(TypeDesc fieldType)
{
if (!fieldType.IsValueType || !((MetadataType)fieldType).ContainsGCPointers && !fieldType.IsByRefLike)
if (!fieldType.IsValueType)
{
return false;
}

if (offset % PointerSize != 0)
{
// Misaligned struct with GC pointers or ByRef
ThrowFieldLayoutError(offset);
}

return true;
// Valuetypes with GC references or byrefs need to be checked for overlaps and alignment
MetadataType mdType = (MetadataType)fieldType;
return mdType.ContainsGCPointers || mdType.ContainsByRefs;
}

private void ThrowFieldLayoutError(int offset)
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ public abstract class FieldLayoutAlgorithm
/// </summary>
public abstract bool ComputeContainsGCPointers(DefType type);

/// <summary>
/// Compute whether the fields of the specified type contains a byref.
/// </summary>
public abstract bool ComputeContainsByRefs(DefType type);

/// <summary>
/// Compute whether the specified type is a value type that transitively has UnsafeValueTypeAttribute
/// </summary>
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutIntervalCalculator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public FieldLayoutIntervalCalculator(int pointerSize)
PointerSize = pointerSize;
}

protected abstract bool NeedsRecursiveLayout(int offset, TypeDesc fieldType);
protected abstract bool NeedsRecursiveLayout(TypeDesc fieldType);

protected abstract TIntervalTag GetIntervalDataForType(int offset, TypeDesc fieldType);

Expand Down Expand Up @@ -84,7 +84,7 @@ private int GetFieldSize(TypeDesc fieldType)

public void AddToFieldLayout(int offset, TypeDesc fieldType, bool addTrailingEmptyInterval)
{
if (NeedsRecursiveLayout(offset, fieldType))
if (NeedsRecursiveLayout(fieldType))
{
if (fieldType is MetadataType { IsInlineArray: true } mdType)
{
Expand Down Expand Up @@ -126,7 +126,7 @@ public void AddToFieldLayout(int offset, TypeDesc fieldType, bool addTrailingEmp

private void AddToFieldLayout(List<FieldLayoutInterval> fieldLayout, int offset, TypeDesc fieldType)
{
if (NeedsRecursiveLayout(offset, fieldType))
if (NeedsRecursiveLayout(fieldType))
{
if (fieldType is MetadataType { IsInlineArray: true } mdType)
{
Expand Down
Loading
Loading