Skip to content

Commit d9ed370

Browse files
sbomersbomer
authored
Fix ILLink/ILC hang when tracking too many hoisted local values (#95041)
#87634 introduced a limit on the number of values tracked in dataflow analysis, but this approach was incorrect because resetting the set of tracked values was effectively moving up the dataflow lattice, breaking the invariant and causing potential hangs. The hang was observed in #94831, where (as found by @vitek-karas) the hoisted local `state` field of an async method with many await points was getting a large number of tracked values, hitting the limit, being reset to "empty", but then growing again, causing the analysis not to converge. The fix is to instead introduce a special value `ValueSet<TValue>.Unknown` that is used for this case. It acts like "bottom" in the lattice, so that it destroys any other inputs on meet ("bottom" meet anything else is "bottom"). In this testcase the `state` field isn't involved in dataflow warnings, so this actually allows the method to be analyzed correctly, producing the expected warning for the `Type` local that flows across all of the await points. Fixes #94831
1 parent a26802a commit d9ed370

24 files changed

+264
-139
lines changed

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ArrayValue.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ internal partial record ArrayValue
1919
public static MultiValue Create(MultiValue size, TypeDesc elementType)
2020
{
2121
MultiValue result = MultiValueLattice.Top;
22-
foreach (var sizeValue in size)
22+
foreach (var sizeValue in size.AsEnumerable ())
2323
{
2424
result = MultiValueLattice.Meet(result, new MultiValue(new ArrayValue(sizeValue, elementType)));
2525
}
@@ -92,7 +92,7 @@ public override SingleValue DeepCopy()
9292
// Since it's possible to store a reference to array as one of its own elements
9393
// simple deep copy could lead to endless recursion.
9494
// So instead we simply disallow arrays as element values completely - and treat that case as "too complex to analyze".
95-
foreach (SingleValue v in kvp.Value.Value)
95+
foreach (SingleValue v in kvp.Value.Value.AsEnumerable ())
9696
{
9797
System.Diagnostics.Debug.Assert(v is not ArrayValue);
9898
}
@@ -123,7 +123,7 @@ public override string ToString()
123123
result.Append(element.Key);
124124
result.Append(",(");
125125
bool firstValue = true;
126-
foreach (var v in element.Value.Value)
126+
foreach (var v in element.Value.Value.AsEnumerable ())
127127
{
128128
if (firstValue)
129129
{

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/InterproceduralState.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,8 @@ public void TrackMethod(MethodIL methodBody)
6767
methodBody = GetInstantiatedMethodIL(methodBody);
6868

6969
// Work around the fact that ValueSet is readonly
70-
var methodsList = new List<MethodBodyValue>(MethodBodies);
70+
Debug.Assert (!MethodBodies.IsUnknown ());
71+
var methodsList = new List<MethodBodyValue>(MethodBodies.GetKnownValues ());
7172
methodsList.Add(new MethodBodyValue(methodBody));
7273

7374
// For state machine methods, also scan the state machine members.

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ private static void ValidateNoReferenceToReference(ValueBasicBlockPair?[] locals
210210
continue;
211211

212212
MultiValue localValue = localVariable.Value.Value;
213-
foreach (var val in localValue)
213+
foreach (var val in localValue.AsEnumerable ())
214214
{
215215
if (val is LocalVariableReferenceValue localReference && localReference.ReferencedType.IsByRefOrPointer())
216216
{
@@ -309,7 +309,8 @@ public virtual void InterproceduralScan(MethodIL startingMethodBody)
309309

310310
// Flow state through all methods encountered so far, as long as there
311311
// are changes discovered in the hoisted local state on entry to any method.
312-
foreach (var methodBodyValue in oldInterproceduralState.MethodBodies)
312+
Debug.Assert (!oldInterproceduralState.MethodBodies.IsUnknown ());
313+
foreach (var methodBodyValue in oldInterproceduralState.MethodBodies.GetKnownValues ())
313314
Scan(methodBodyValue.MethodBody, ref interproceduralState);
314315
}
315316

@@ -327,7 +328,8 @@ public virtual void InterproceduralScan(MethodIL startingMethodBody)
327328
}
328329
else
329330
{
330-
Debug.Assert(interproceduralState.MethodBodies.Count() == 1);
331+
Debug.Assert (!interproceduralState.MethodBodies.IsUnknown ());
332+
Debug.Assert(interproceduralState.MethodBodies.GetKnownValues ().Count() == 1);
331333
}
332334
#endif
333335
}
@@ -1018,7 +1020,7 @@ private void ScanIndirectStore(
10181020
/// <exception cref="LinkerFatalErrorException">Throws if <paramref name="target"/> is not a valid target for an indirect store.</exception>
10191021
protected void StoreInReference(MultiValue target, MultiValue source, MethodIL method, int offset, ValueBasicBlockPair?[] locals, int curBasicBlock, ref InterproceduralState ipState)
10201022
{
1021-
foreach (var value in target)
1023+
foreach (var value in target.AsEnumerable ())
10221024
{
10231025
switch (value)
10241026
{
@@ -1137,7 +1139,7 @@ private void ScanStfld(
11371139
return;
11381140
}
11391141

1140-
foreach (var value in HandleGetField(methodBody, offset, field))
1142+
foreach (var value in HandleGetField(methodBody, offset, field).AsEnumerable ())
11411143
{
11421144
// GetFieldValue may return different node types, in which case they can't be stored to.
11431145
// At least not yet.
@@ -1189,7 +1191,7 @@ internal MultiValue DereferenceValue(
11891191
ref InterproceduralState interproceduralState)
11901192
{
11911193
MultiValue dereferencedValue = MultiValueLattice.Top;
1192-
foreach (var value in maybeReferenceValue)
1194+
foreach (var value in maybeReferenceValue.AsEnumerable ())
11931195
{
11941196
switch (value)
11951197
{
@@ -1324,7 +1326,7 @@ private void HandleCall(
13241326

13251327
foreach (var param in methodArguments)
13261328
{
1327-
foreach (var v in param)
1329+
foreach (var v in param.AsEnumerable ())
13281330
{
13291331
if (v is ArrayValue arr)
13301332
{
@@ -1366,7 +1368,7 @@ private void ScanStelem(
13661368
StackSlot indexToStoreAt = PopUnknown(currentStack, 1, methodBody, offset);
13671369
StackSlot arrayToStoreIn = PopUnknown(currentStack, 1, methodBody, offset);
13681370
int? indexToStoreAtInt = indexToStoreAt.Value.AsConstInt();
1369-
foreach (var array in arrayToStoreIn.Value)
1371+
foreach (var array in arrayToStoreIn.Value.AsEnumerable ())
13701372
{
13711373
if (array is ArrayValue arrValue)
13721374
{

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -431,7 +431,7 @@ public static bool HandleCall(
431431
// type instead).
432432
//
433433
// At least until we have shared enum code, this needs extra handling to get it right.
434-
foreach (var value in argumentValues[0])
434+
foreach (var value in argumentValues[0].AsEnumerable ())
435435
{
436436
if (value is SystemTypeValue systemTypeValue
437437
&& !systemTypeValue.RepresentedType.Type.IsGenericDefinition
@@ -466,7 +466,7 @@ public static bool HandleCall(
466466
? 0 : 1;
467467

468468
// We need the data to do struct marshalling.
469-
foreach (var value in argumentValues[paramIndex])
469+
foreach (var value in argumentValues[paramIndex].AsEnumerable ())
470470
{
471471
if (value is SystemTypeValue systemTypeValue
472472
&& !systemTypeValue.RepresentedType.Type.IsGenericDefinition
@@ -497,7 +497,7 @@ public static bool HandleCall(
497497
case IntrinsicId.Marshal_GetDelegateForFunctionPointer:
498498
{
499499
// We need the data to do delegate marshalling.
500-
foreach (var value in argumentValues[1])
500+
foreach (var value in argumentValues[1].AsEnumerable ())
501501
{
502502
if (value is SystemTypeValue systemTypeValue
503503
&& !systemTypeValue.RepresentedType.Type.IsGenericDefinition
@@ -521,7 +521,7 @@ public static bool HandleCall(
521521
//
522522
case IntrinsicId.Object_GetType:
523523
{
524-
foreach (var valueNode in instanceValue)
524+
foreach (var valueNode in instanceValue.AsEnumerable ())
525525
{
526526
// Note that valueNode can be statically typed in IL as some generic argument type.
527527
// For example:
@@ -619,7 +619,7 @@ public static bool HandleCall(
619619
// Validate that the return value has the correct annotations as per the method return value annotations
620620
if (annotatedMethodReturnValue.DynamicallyAccessedMemberTypes != 0)
621621
{
622-
foreach (var uniqueValue in methodReturnValue)
622+
foreach (var uniqueValue in methodReturnValue.AsEnumerable ())
623623
{
624624
if (uniqueValue is ValueWithDynamicallyAccessedMembers methodReturnValueWithMemberTypes)
625625
{

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,9 @@ public void MarkAndProduceDiagnostics(ReflectionMarker reflectionMarker, Logger
4848
logger.ShouldSuppressAnalysisWarningsForRequires(Origin.MemberDefinition, DiagnosticUtilities.RequiresAssemblyFilesAttribute),
4949
logger);
5050

51-
foreach (var sourceValue in Source)
51+
foreach (var sourceValue in Source.AsEnumerable ())
5252
{
53-
foreach (var targetValue in Target)
53+
foreach (var targetValue in Target.AsEnumerable ())
5454
{
5555
if (targetValue is not ValueWithDynamicallyAccessedMembers targetWithDynamicallyAccessedMembers)
5656
throw new NotImplementedException();

‎src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/FeatureContextLattice.cs‎

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -14,51 +14,43 @@ namespace ILLink.RoslynAnalyzer.DataFlow
1414
public struct FeatureContext : IEquatable<FeatureContext>, IDeepCopyValue<FeatureContext>
1515
{
1616
// The set of features known to be enabled in this context.
17-
// Null represents "all possible features".
18-
public ValueSet<string>? EnabledFeatures;
17+
// Unknown represents "all possible features".
18+
public ValueSet<string> EnabledFeatures;
1919

20-
public static readonly FeatureContext All = new FeatureContext (null);
20+
public static readonly FeatureContext All = new FeatureContext (ValueSet<string>.Unknown);
2121

2222
public static readonly FeatureContext None = new FeatureContext (ValueSet<string>.Empty);
2323

24-
public FeatureContext (ValueSet<string>? enabled)
24+
public FeatureContext (ValueSet<string> enabled)
2525
{
2626
EnabledFeatures = enabled;
2727
}
2828

2929
public bool IsEnabled (string feature)
3030
{
31-
return EnabledFeatures == null || EnabledFeatures.Value.Contains (feature);
31+
return EnabledFeatures.IsUnknown () || EnabledFeatures.Contains (feature);
3232
}
3333

3434
public bool Equals (FeatureContext other) => EnabledFeatures == other.EnabledFeatures;
3535
public override bool Equals (object? obj) => obj is FeatureContext other && Equals (other);
36-
public override int GetHashCode () => EnabledFeatures?.GetHashCode () ?? typeof (FeatureContext).GetHashCode ();
36+
public override int GetHashCode () => EnabledFeatures.GetHashCode ();
3737

3838
public static bool operator == (FeatureContext left, FeatureContext right) => left.Equals (right);
3939
public static bool operator != (FeatureContext left, FeatureContext right) => !left.Equals (right);
4040

4141
public FeatureContext DeepCopy ()
4242
{
43-
return new FeatureContext (EnabledFeatures?.DeepCopy ());
43+
return new FeatureContext (EnabledFeatures.DeepCopy ());
4444
}
4545

4646
public FeatureContext Intersection (FeatureContext other)
4747
{
48-
if (EnabledFeatures == null)
49-
return other.DeepCopy ();
50-
if (other.EnabledFeatures == null)
51-
return this.DeepCopy ();
52-
return new FeatureContext (ValueSet<string>.Intersection (EnabledFeatures.Value, other.EnabledFeatures.Value));
48+
return new FeatureContext (ValueSet<string>.Intersection (EnabledFeatures, other.EnabledFeatures));
5349
}
5450

5551
public FeatureContext Union (FeatureContext other)
5652
{
57-
if (EnabledFeatures == null)
58-
return this.DeepCopy ();
59-
if (other.EnabledFeatures == null)
60-
return other.DeepCopy ();
61-
return new FeatureContext (ValueSet<string>.Union (EnabledFeatures.Value, other.EnabledFeatures.Value));
53+
return new FeatureContext (ValueSet<string>.Union (EnabledFeatures, other.EnabledFeatures));
6254
}
6355
}
6456

‎src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/InterproceduralState.cs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using System;
55
using System.Collections.Generic;
6+
using System.Diagnostics;
67
using System.Diagnostics.CodeAnalysis;
78
using ILLink.Shared.DataFlow;
89

@@ -50,7 +51,8 @@ public InterproceduralState<TValue, TValueLattice> Clone ()
5051

5152
public void TrackMethod (MethodBodyValue method)
5253
{
53-
var methodsList = new List<MethodBodyValue> (Methods);
54+
Debug.Assert (!Methods.IsUnknown ());
55+
var methodsList = new List<MethodBodyValue> (Methods.GetKnownValues ());
5456
methodsList.Add (method);
5557
Methods = new ValueSet<MethodBodyValue> (methodsList);
5658
}

‎src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowAnalysis.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,8 @@ public void InterproceduralAnalyze ()
7979
while (!interproceduralState.Equals (oldInterproceduralState)) {
8080
oldInterproceduralState = interproceduralState.Clone ();
8181

82-
foreach (var method in oldInterproceduralState.Methods) {
82+
Debug.Assert (!oldInterproceduralState.Methods.IsUnknown ());
83+
foreach (var method in oldInterproceduralState.Methods.GetKnownValues ()) {
8384
AnalyzeMethod (method, ref interproceduralState);
8485
}
8586
}

‎src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -390,9 +390,10 @@ TValue ProcessAssignment (IAssignmentOperation operation, LocalDataFlowState<TVa
390390
Debug.Assert (IsLValueFlowCapture (flowCaptureReference.Id));
391391
Debug.Assert (flowCaptureReference.GetValueUsageInfo (OwningSymbol).HasFlag (ValueUsageInfo.Write));
392392
var capturedReferences = state.Current.LocalState.CapturedReferences.Get (flowCaptureReference.Id);
393+
Debug.Assert (!capturedReferences.IsUnknown ());
393394
if (!capturedReferences.HasMultipleValues) {
394395
// Single captured reference. Treat this as an overwriting assignment.
395-
var enumerator = capturedReferences.GetEnumerator ();
396+
var enumerator = capturedReferences.GetKnownValues ().GetEnumerator ();
396397
enumerator.MoveNext ();
397398
targetOperation = enumerator.Current.Reference;
398399
return ProcessSingleTargetAssignment (targetOperation, operation, state, merge: false);
@@ -409,7 +410,7 @@ TValue ProcessAssignment (IAssignmentOperation operation, LocalDataFlowState<TVa
409410
// if the RHS has dataflow warnings.
410411

411412
TValue value = TopValue;
412-
foreach (var capturedReference in capturedReferences) {
413+
foreach (var capturedReference in capturedReferences.GetKnownValues ()) {
413414
targetOperation = capturedReference.Reference;
414415
var singleValue = ProcessSingleTargetAssignment (targetOperation, operation, state, merge: true);
415416
value = LocalStateAndContextLattice.LocalStateLattice.Lattice.ValueLattice.Meet (value, singleValue);
@@ -517,7 +518,9 @@ public override TValue VisitFlowCapture (IFlowCaptureOperation operation, LocalD
517518
// If an r-value captures an l-value, we must dereference the l-value
518519
// and copy out the value to capture.
519520
capturedValue = TopValue;
520-
foreach (var capturedReference in state.Current.LocalState.CapturedReferences.Get (captureRef.Id)) {
521+
var capturedReferences = state.Current.LocalState.CapturedReferences.Get (captureRef.Id);
522+
Debug.Assert (!capturedReferences.IsUnknown ());
523+
foreach (var capturedReference in capturedReferences.GetKnownValues ()) {
521524
var value = Visit (capturedReference.Reference, state);
522525
capturedValue = LocalStateAndContextLattice.LocalStateLattice.Lattice.ValueLattice.Meet (capturedValue, value);
523526
}

‎src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalStateAndContextLattice.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ public LocalStateAndContextLattice (LocalStateLattice<TValue, TValueLattice> loc
4141
{
4242
LocalStateLattice = localStateLattice;
4343
ContextLattice = contextLattice;
44+
Top = new (LocalStateLattice.Top, ContextLattice.Top);
4445
}
4546

4647
public LocalStateAndContext<TValue, TContext> Top { get; }

0 commit comments

Comments
 (0)