The root cause is the fact that `ArrayValue` is mutable, so it needs to be `DeepCopy` everywhere. So the first set of fixes is to make sure that even hoisted locals are correctly copied.
The second problem is that we use `HashSet` to store multiple values in `ValueSet`/`MultiValue`. HashSet assumes that items don't change identity (equality group) while they're stored inside. But `ArrayValue` does exactly that. So we can't rely on pure `HashSet` functionality to implement `Equals`.
Changes:
* Make Maybe<T> an IDeepCopyValue and make sure it deep copies its value - because it can store values which require a deep copy
* Make ValueSet<T> an IDeepCopyValue since it can store values which require a deep copy (this causes an effective rename of MultiValue.Clone to MultiValue.DeepCopy - lot of files touched)
* Modify ValueSet<T> to be careful when comparing for equality - we can't rely on immutability of values and since we store them in a hashset, the hashset itself effectively breaks. Maybe we should consider switching to a simple List? But then we want to to unify same values.
* Modify array equality comparison - previously it caused an allocation of a `HashSet` - we compare values quite a bit (in analyzer during multi-pass analysis). Currently we rely on values in arrays to be immutable.
Future looking:
The data flow analysis simply doesn't have a solution for mutable values (mostly mutable refence types, but I think similar set of problems might occur with structs as well). ILLink implements by-ref values, which are probably the closest approximation yet, but the full CFG data flow doesn't support those yet. And ultimately it's probably more complex then just by-ref values.
Proper fix is complex, and requires some design work first.
Note that we will want to backport this to .NET 7 very likely - I haven't looked yet how well it will port.
if (!equals)
return false;
- // If both sets T and O are the same size and "T intersect O" is empty, then T == O.
- HashSet<KeyValuePair<int, ValueBasicBlockPair>> thisValueSet = new(IndexValues);
- HashSet<KeyValuePair<int, ValueBasicBlockPair>> otherValueSet = new(otherArr.IndexValues);
- thisValueSet.ExceptWith(otherValueSet);
- return thisValueSet.Count == 0;
+ // Here we rely on the assumption that we can't store mutable values in arrays. The only mutable value
+ // which we currently support are array values, but those are not allowed in an array (to avoid complexity).
+ // As such we can rely on the values to be immutable, and thus if the counts are equal
+ // then the arrays are equal if items from one can be directly found in the other.
+ foreach (var kvp in IndexValues)
+ if (!otherArr.IndexValues.TryGetValue(kvp.Key, out ValueBasicBlockPair value) || !kvp.Value.Equals(value))
+ return false;
+
+ return true;
}
public override SingleValue DeepCopy()
System.Diagnostics.Debug.Assert(v is not ArrayValue);
}
#endif
- newValue.IndexValues.Add(kvp.Key, new ValueBasicBlockPair(kvp.Value.Value.Clone(), kvp.Value.BasicBlockIndex));
+ newValue.IndexValues.Add(kvp.Key, new ValueBasicBlockPair(kvp.Value.Value.DeepCopy(), kvp.Value.BasicBlockIndex));
}
return newValue;
public override int GetHashCode() => base.GetHashCode();
public InterproceduralState Clone()
- => new(_ilProvider, MethodBodies.Clone(), HoistedLocals.Clone(), lattice);
+ => new(_ilProvider, MethodBodies.DeepCopy(), HoistedLocals.Clone(), lattice);
public void TrackMethod(MethodDesc method)
{
internal TrimAnalysisAssignmentPattern(MultiValue source, MultiValue target, MessageOrigin origin, string reason)
{
- Source = source.Clone();
- Target = target.Clone();
+ Source = source.DeepCopy();
+ Target = target.DeepCopy();
Origin = origin;
Reason = reason;
}
Operation = operation;
Offset = offset;
CalledMethod = calledMethod;
- Instance = instance.Clone();
+ Instance = instance.DeepCopy();
if (arguments.IsEmpty)
{
Arguments = ImmutableArray<MultiValue>.Empty;
{
var builder = ImmutableArray.CreateBuilder<MultiValue>();
foreach (var argument in arguments)
- builder.Add(argument.Clone());
+ builder.Add(argument.DeepCopy());
Arguments = builder.ToImmutableArray();
}
Origin = origin;
=> throw new NotImplementedException ();
public InterproceduralState<TValue, TValueLattice> Clone ()
- => new (Methods.Clone (),
+ => new (Methods.DeepCopy (),
HoistedLocals.Clone (), lattice);
public void TrackMethod (MethodBodyValue method)
if (!equals)
return false;
- // If both sets T and O are the same size and "T intersect O" is empty, then T == O.
- HashSet<KeyValuePair<int, MultiValue>> thisValueSet = new (IndexValues);
- thisValueSet.ExceptWith (otherArr.IndexValues);
- return thisValueSet.Count == 0;
+ // Here we rely on the assumption that we can't store mutable values in arrays. The only mutable value
+ // which we currently support are array values, but those are not allowed in an array (to avoid complexity).
+ // As such we can rely on the values to be immutable, and thus if the counts are equal
+ // then the arrays are equal if items from one can be directly found in the other.
+ foreach (var kvp in IndexValues)
+ if (!otherArr.IndexValues.TryGetValue (kvp.Key, out MultiValue value) || !kvp.Value.Equals (value))
+ return false;
+
+ return true;
}
// Lattice Meet() is supposed to copy values, so we need to make a deep copy since ArrayValue is mutable through IndexValues
}
#endif
- newArray.IndexValues.Add (kvp.Key, kvp.Value.Clone ());
+ newArray.IndexValues.Add (kvp.Key, kvp.Value.DeepCopy ());
}
return newArray;
public TrimAnalysisAssignmentPattern (MultiValue source, MultiValue target, IOperation operation)
{
- Source = source.Clone ();
- Target = target.Clone ();
+ Source = source.DeepCopy ();
+ Target = target.DeepCopy ();
Operation = operation;
}
ISymbol owningSymbol)
{
CalledMethod = calledMethod;
- Instance = instance.Clone ();
+ Instance = instance.DeepCopy ();
if (arguments.IsEmpty) {
Arguments = ImmutableArray<MultiValue>.Empty;
} else {
var builder = ImmutableArray.CreateBuilder<MultiValue> ();
foreach (var argument in arguments) {
- builder.Add (argument.Clone ());
+ builder.Add (argument.DeepCopy ());
}
Arguments = builder.ToImmutableArray ();
}
// Wrapper for Nullable<T> which implements IEquatable so that this may
// be used as a lattice value. Nullable types can't satisfy interface constraints;
// see for example https://docs.microsoft.com/dotnet/csharp/misc/cs0313.
- public struct Maybe<T> : IEquatable<Maybe<T>>
+ public struct Maybe<T> : IEquatable<Maybe<T>>, IDeepCopyValue<Maybe<T>>
where T : struct, IEquatable<T>
{
public T? MaybeValue;
public bool Equals (Maybe<T> other) => MaybeValue?.Equals (other.MaybeValue) ?? other.MaybeValue == null;
public override bool Equals (object? obj) => obj is Maybe<T> other && Equals (other);
public override int GetHashCode () => MaybeValue?.GetHashCode () ?? 0;
- public Maybe<T> Clone ()
+ public Maybe<T> DeepCopy ()
{
if (MaybeValue is not T value)
return default;
public Maybe<T> Meet (Maybe<T> left, Maybe<T> right)
{
if (left.MaybeValue is not T leftValue)
- return right.Clone ();
+ return right.DeepCopy ();
if (right.MaybeValue is not T rightValue)
- return left.Clone ();
+ return left.DeepCopy ();
return new Maybe<T> (ValueLattice.Meet (leftValue, rightValue));
}
}
namespace ILLink.Shared.DataFlow
{
- public readonly struct ValueSet<TValue> : IEquatable<ValueSet<TValue>>, IEnumerable<TValue>
+ public readonly struct ValueSet<TValue> : IEquatable<ValueSet<TValue>>, IEnumerable<TValue>, IDeepCopyValue<ValueSet<TValue>>
where TValue : notnull
{
// Since we're going to do lot of type checks for this class a lot, it is much more efficient
hashCode = HashUtils.Combine (hashCode, item);
return hashCode;
}
+
+ public bool Equals (EnumerableValues other)
+ {
+ // Unfortunately if some of the values are ArrayValues then they can mutate
+ // after being added to the set, in which case their "hashing" is broken
+ // The set will self-heal on every Meet since we recreate the HashSet
+ // but equality is not guaranteed in the interim state.
+ // So to workaround this for now, iterate over both sets and check
+ // that the item can be found in the other set.
+ foreach (TValue item in this)
+ if (!other.Contains (item))
+ return false;
+
+ foreach (TValue item in other)
+ if (!Contains (item))
+ return false;
+
+ return true;
+ }
+
+ public bool Equals (TValue other)
+ {
+ // As described above, it's possible to end up with a hashset which has multiple
+ // values which are equal (due to mutability). So we can't rely on item count.
+ bool found = false;
+ foreach (TValue item in this) {
+ if (!item.Equals (other))
+ return false;
+ found = true;
+ }
+
+ return found;
+ }
}
public struct Enumerator : IEnumerator<TValue>, IDisposable, IEnumerator
{
private readonly object? _value;
- private int _state; // 0 before begining, 1 at item, 2 after end
+ private int _state; // 0 before beginning, 1 at item, 2 after end
private readonly IEnumerator<TValue>? _enumerator;
internal Enumerator (object? values)
if (_values is EnumerableValues enumerableValues) {
if (other._values is EnumerableValues otherValuesSet) {
- return enumerableValues.SetEquals (otherValuesSet);
+ return enumerableValues.Equals (otherValuesSet);
} else
- return false;
+ return enumerableValues.Equals ((TValue) other._values);
} else {
- if (other._values is EnumerableValues) {
- return false;
+ if (other._values is EnumerableValues otherEnumerableValues) {
+ return otherEnumerableValues.Equals ((TValue) _values);
}
return EqualityComparer<TValue>.Default.Equals ((TValue) _values, (TValue) other._values);
internal static ValueSet<TValue> Meet (ValueSet<TValue> left, ValueSet<TValue> right)
{
if (left._values == null)
- return right.Clone ();
+ return right.DeepCopy ();
if (right._values == null)
- return left.Clone ();
+ return left.DeepCopy ();
if (left._values is not EnumerableValues && right.Contains ((TValue) left._values))
- return right.Clone ();
+ return right.DeepCopy ();
if (right._values is not EnumerableValues && left.Contains ((TValue) right._values))
- return left.Clone ();
+ return left.DeepCopy ();
- var values = new EnumerableValues (left.Clone ());
- values.UnionWith (right.Clone ());
+ var values = new EnumerableValues (left.DeepCopy ());
+ values.UnionWith (right.DeepCopy ());
return new ValueSet<TValue> (values);
}
// Meet should copy the values, but most SingleValues are immutable.
// Clone returns `this` if there are no mutable SingleValues (SingleValues that implement IDeepCopyValue), otherwise creates a new ValueSet with copies of the copiable Values
- public ValueSet<TValue> Clone ()
+ public ValueSet<TValue> DeepCopy ()
{
if (_values is null)
return this;
if (!equals)
return false;
- // If both sets T and O are the same size and "T intersect O" is empty, then T == O.
- HashSet<KeyValuePair<int, ValueBasicBlockPair>> thisValueSet = new (IndexValues);
- HashSet<KeyValuePair<int, ValueBasicBlockPair>> otherValueSet = new (otherArr.IndexValues);
- thisValueSet.ExceptWith (otherValueSet);
- return thisValueSet.Count == 0;
+ // Here we rely on the assumption that we can't store mutable values in arrays. The only mutable value
+ // which we currently support are array values, but those are not allowed in an array (to avoid complexity).
+ // As such we can rely on the values to be immutable, and thus if the counts are equal
+ // then the arrays are equal if items from one can be directly found in the other.
+ foreach (var kvp in IndexValues)
+ if (!otherArr.IndexValues.TryGetValue (kvp.Key, out ValueBasicBlockPair value) || !kvp.Value.Equals (value))
+ return false;
+
+ return true;
}
public override SingleValue DeepCopy ()
}
#endif
- newValue.IndexValues.Add (kvp.Key, new ValueBasicBlockPair (kvp.Value.Value.Clone (), kvp.Value.BasicBlockIndex));
+ newValue.IndexValues.Add (kvp.Key, new ValueBasicBlockPair (kvp.Value.Value.DeepCopy (), kvp.Value.BasicBlockIndex));
}
return newValue;
public override int GetHashCode () => HashUtils.Combine (MethodBodies.GetHashCode (), HoistedLocals.GetHashCode ());
public InterproceduralState Clone ()
- => new (MethodBodies.Clone (), HoistedLocals.Clone (), lattice);
+ => new (MethodBodies.DeepCopy (), HoistedLocals.Clone (), lattice);
public void TrackMethod (MethodDefinition method)
{
public TrimAnalysisAssignmentPattern (MultiValue source, MultiValue target, MessageOrigin origin)
{
- Source = source.Clone ();
- Target = target.Clone ();
+ Source = source.DeepCopy ();
+ Target = target.DeepCopy ();
Origin = origin;
}
Debug.Assert (origin.Provider is MethodDefinition);
Operation = operation;
CalledMethod = calledMethod;
- Instance = instance.Clone ();
+ Instance = instance.DeepCopy ();
if (arguments.IsEmpty) {
Arguments = ImmutableArray<MultiValue>.Empty;
} else {
var builder = ImmutableArray.CreateBuilder<MultiValue> ();
foreach (var argument in arguments)
- builder.Add (argument.Clone ());
+ builder.Add (argument.DeepCopy ());
Arguments = builder.ToImmutableArray ();
}
Origin = origin;
WriteCapturedArrayElement.Test ();
ConstantFieldValuesAsIndex.Test ();
+
+ HoistedArrayMutation.Test ();
}
[ExpectedWarning ("IL2062", nameof (DataFlowTypeExtensions.RequiresPublicMethods))]
}
}
+ class HoistedArrayMutation
+ {
+ static void LoopAssignmentWithInitAfter ()
+ {
+ // This is a repro for https://github.com/dotnet/runtime/issues/86379
+ // The array value is a hoisted local
+ // It's first used in the main method (in the for loop)
+ // this doesn't get a deep clone of the value, it takes the value from
+ // the hoisted locals dictionary - and modifies it.
+ // The local function Initialize then creates a deep copy and uses that.
+ // Because of a bug, the changes done in the main body method are also
+ // visible in the "old" interprocedural state. So the state never settles
+ // and this causes an endless loop in the analyzer.
+ int[] arr;
+
+ Initialize ();
+ for (int i = 0; i < arr.Length; i++) {
+ arr[i] = 0;
+ }
+
+ void Initialize ()
+ {
+ arr = new int[10];
+ }
+ }
+
+ public static void Test ()
+ {
+ LoopAssignmentWithInitAfter ();
+ }
+ }
+
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors)]
private static Type GetTypeWithPublicConstructors ()
{
projects / platform / upstream / dotnet / runtime.git / commitdiff