Skip to content

Commit 8f74726

Browse files
sbomersbomer
authored
Fix trim analysis for assignment to multiple refs in single method call (#105663)
For a call to a method with multiple out parameters, ILLink and ILC model the assignment to the out parameter via a TrimAnalysisAssignmentPattern. This isn't able to differentiate between the two out parameters, so there are spurious warnings because we think one out parameter's annotation may have been applied to the other. This fixes the issue by disambiguating assignments due to a call to a method with out parameters, based on the parameter index. This way each out parameter is analyzed separately.
1 parent 16f752c commit 8f74726

File tree

10 files changed

+68
-55
lines changed

10 files changed

+68
-55
lines changed

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

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -901,7 +901,7 @@ Stack<StackSlot> currentStack
901901
ParameterProxy param = new(methodBody.OwningMethod, paramNum);
902902
var targetValue = GetMethodParameterValue(param);
903903
if (targetValue is MethodParameterValue targetParameterValue)
904-
HandleStoreParameter(methodBody, offset, targetParameterValue, valueToStore.Value);
904+
HandleStoreParameter(methodBody, offset, targetParameterValue, valueToStore.Value, null);
905905

906906
// If the targetValue is MethodThisValue do nothing - it should never happen really, and if it does, there's nothing we can track there
907907
}
@@ -1007,7 +1007,7 @@ private void ScanIndirectStore(
10071007
StackSlot valueToStore = PopUnknown(currentStack, 1, methodBody, offset);
10081008
StackSlot destination = PopUnknown(currentStack, 1, methodBody, offset);
10091009

1010-
StoreInReference(destination.Value, valueToStore.Value, methodBody, offset, locals, curBasicBlock, ref ipState);
1010+
StoreInReference(destination.Value, valueToStore.Value, methodBody, offset, locals, curBasicBlock, ref ipState, null);
10111011
}
10121012

10131013
/// <summary>
@@ -1018,7 +1018,7 @@ private void ScanIndirectStore(
10181018
/// <param name="method">The method body that contains the operation causing the store</param>
10191019
/// <param name="offset">The instruction offset causing the store</param>
10201020
/// <exception cref="LinkerFatalErrorException">Throws if <paramref name="target"/> is not a valid target for an indirect store.</exception>
1021-
protected void StoreInReference(MultiValue target, MultiValue source, MethodIL method, int offset, ValueBasicBlockPair?[] locals, int curBasicBlock, ref InterproceduralState ipState)
1021+
protected void StoreInReference(MultiValue target, MultiValue source, MethodIL method, int offset, ValueBasicBlockPair?[] locals, int curBasicBlock, ref InterproceduralState ipState, int? parameterIndex)
10221022
{
10231023
foreach (var value in target.AsEnumerable ())
10241024
{
@@ -1029,18 +1029,18 @@ protected void StoreInReference(MultiValue target, MultiValue source, MethodIL m
10291029
break;
10301030
case FieldReferenceValue fieldReference
10311031
when HandleGetField(method, offset, fieldReference.FieldDefinition).AsSingleValue() is FieldValue fieldValue:
1032-
HandleStoreField(method, offset, fieldValue, source);
1032+
HandleStoreField(method, offset, fieldValue, source, parameterIndex);
10331033
break;
10341034
case ParameterReferenceValue parameterReference
10351035
when GetMethodParameterValue(parameterReference.Parameter) is MethodParameterValue parameterValue:
1036-
HandleStoreParameter(method, offset, parameterValue, source);
1036+
HandleStoreParameter(method, offset, parameterValue, source, parameterIndex);
10371037
break;
10381038
case MethodReturnValue methodReturnValue:
10391039
// Ref returns don't have special ReferenceValue values, so assume if the target here is a MethodReturnValue then it must be a ref return value
10401040
HandleReturnValue(method, offset, methodReturnValue, source);
10411041
break;
10421042
case FieldValue fieldValue:
1043-
HandleStoreField(method, offset, fieldValue, DereferenceValue(method, offset, source, locals, ref ipState));
1043+
HandleStoreField(method, offset, fieldValue, DereferenceValue(method, offset, source, locals, ref ipState), parameterIndex);
10441044
break;
10451045
case IValueWithStaticType valueWithStaticType:
10461046
if (valueWithStaticType.StaticType is not null && FlowAnnotations.IsTypeInterestingForDataflow(valueWithStaticType.StaticType.Value.Type))
@@ -1108,11 +1108,11 @@ private void ScanLdfld(
11081108
currentStack.Push(new StackSlot(value));
11091109
}
11101110

1111-
protected virtual void HandleStoreField(MethodIL method, int offset, FieldValue field, MultiValue valueToStore)
1111+
protected virtual void HandleStoreField(MethodIL method, int offset, FieldValue field, MultiValue valueToStore, int? parameterIndex)
11121112
{
11131113
}
11141114

1115-
protected virtual void HandleStoreParameter(MethodIL method, int offset, MethodParameterValue parameter, MultiValue valueToStore)
1115+
protected virtual void HandleStoreParameter(MethodIL method, int offset, MethodParameterValue parameter, MultiValue valueToStore, int? parameterIndex)
11161116
{
11171117
}
11181118

@@ -1149,7 +1149,7 @@ private void ScanStfld(
11491149
// Incomplete handling of ref fields -- if we're storing a reference to a value, pretend it's just the value
11501150
MultiValue valueToStore = DereferenceValue(methodBody, offset, valueToStoreSlot.Value, locals, ref interproceduralState);
11511151

1152-
HandleStoreField(methodBody, offset, fieldValue, valueToStore);
1152+
HandleStoreField(methodBody, offset, fieldValue, valueToStore, null);
11531153
}
11541154
}
11551155

@@ -1241,7 +1241,7 @@ protected void AssignRefAndOutParameters(
12411241
if (parameter.GetReferenceKind() is not (ReferenceKind.Ref or ReferenceKind.Out))
12421242
continue;
12431243
var newByRefValue = _annotations.GetMethodParameterValue(parameter);
1244-
StoreInReference(methodArguments[(int)parameter.Index], newByRefValue, callingMethodBody, offset, locals, curBasicBlock, ref ipState);
1244+
StoreInReference(methodArguments[(int)parameter.Index], newByRefValue, callingMethodBody, offset, locals, curBasicBlock, ref ipState, parameter.Index.Index);
12451245
}
12461246
}
12471247

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -208,23 +208,23 @@ protected override MultiValue HandleGetField(MethodIL methodBody, int offset, Fi
208208
return _annotations.GetFieldValue(field);
209209
}
210210

211-
private void HandleStoreValueWithDynamicallyAccessedMembers(MethodIL methodBody, int offset, ValueWithDynamicallyAccessedMembers targetValue, MultiValue sourceValue, string reason)
211+
private void HandleStoreValueWithDynamicallyAccessedMembers(MethodIL methodBody, int offset, ValueWithDynamicallyAccessedMembers targetValue, MultiValue sourceValue, int? parameterIndex, string reason)
212212
{
213213
if (targetValue.DynamicallyAccessedMemberTypes != 0)
214214
{
215215
_origin = _origin.WithInstructionOffset(methodBody, offset);
216-
TrimAnalysisPatterns.Add(new TrimAnalysisAssignmentPattern(sourceValue, targetValue, _origin, reason));
216+
TrimAnalysisPatterns.Add(new TrimAnalysisAssignmentPattern(sourceValue, targetValue, _origin, parameterIndex, reason));
217217
}
218218
}
219219

220-
protected override void HandleStoreField(MethodIL methodBody, int offset, FieldValue field, MultiValue valueToStore)
221-
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, field, valueToStore, field.Field.GetDisplayName());
220+
protected override void HandleStoreField(MethodIL methodBody, int offset, FieldValue field, MultiValue valueToStore, int? parameterIndex)
221+
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, field, valueToStore, parameterIndex, field.Field.GetDisplayName());
222222

223-
protected override void HandleStoreParameter(MethodIL methodBody, int offset, MethodParameterValue parameter, MultiValue valueToStore)
224-
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, parameter, valueToStore, parameter.Parameter.Method.GetDisplayName());
223+
protected override void HandleStoreParameter(MethodIL methodBody, int offset, MethodParameterValue parameter, MultiValue valueToStore, int? parameterIndex)
224+
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, parameter, valueToStore, parameterIndex, parameter.Parameter.Method.GetDisplayName());
225225

226226
protected override void HandleReturnValue(MethodIL methodBody, int offset, MethodReturnValue returnValue, MultiValue valueToStore)
227-
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, returnValue, valueToStore, returnValue.Method.GetDisplayName());
227+
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, returnValue, valueToStore, null, returnValue.Method.GetDisplayName());
228228

229229
protected override void HandleTypeTokenAccess(MethodIL methodBody, int offset, TypeDesc accessedType)
230230
{

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,24 +18,31 @@ public readonly record struct TrimAnalysisAssignmentPattern
1818
public MultiValue Source { get; init; }
1919
public MultiValue Target { get; init; }
2020
public MessageOrigin Origin { get; init; }
21+
22+
// For assignment of a method parameter, we store the parameter index to disambiguate
23+
// assignments from different out parameters of a single method call.
24+
public int? ParameterIndex { get; init; }
2125
internal string Reason { get; init; }
2226

23-
internal TrimAnalysisAssignmentPattern(MultiValue source, MultiValue target, MessageOrigin origin, string reason)
27+
internal TrimAnalysisAssignmentPattern(MultiValue source, MultiValue target, MessageOrigin origin, int? parameterIndex, string reason)
2428
{
2529
Source = source.DeepCopy();
2630
Target = target.DeepCopy();
2731
Origin = origin;
32+
ParameterIndex = parameterIndex;
2833
Reason = reason;
2934
}
3035

3136
public TrimAnalysisAssignmentPattern Merge(ValueSetLattice<SingleValue> lattice, TrimAnalysisAssignmentPattern other)
3237
{
3338
Debug.Assert(Origin == other.Origin);
39+
Debug.Assert(ParameterIndex == other.ParameterIndex);
3440

3541
return new TrimAnalysisAssignmentPattern(
3642
lattice.Meet(Source, other.Source),
3743
lattice.Meet(Target, other.Target),
3844
Origin,
45+
ParameterIndex,
3946
Reason);
4047
}
4148

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ namespace ILCompiler.Dataflow
1313
{
1414
public readonly struct TrimAnalysisPatternStore
1515
{
16-
private readonly Dictionary<MessageOrigin, TrimAnalysisAssignmentPattern> AssignmentPatterns;
16+
private readonly Dictionary<(MessageOrigin, int?), TrimAnalysisAssignmentPattern> AssignmentPatterns;
1717
private readonly Dictionary<MessageOrigin, TrimAnalysisMethodCallPattern> MethodCallPatterns;
1818
private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisTokenAccessPattern> TokenAccessPatterns;
1919
private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern> GenericInstantiations;
@@ -23,7 +23,7 @@ public readonly struct TrimAnalysisPatternStore
2323

2424
public TrimAnalysisPatternStore(ValueSetLattice<SingleValue> lattice, Logger logger)
2525
{
26-
AssignmentPatterns = new Dictionary<MessageOrigin, TrimAnalysisAssignmentPattern>();
26+
AssignmentPatterns = new Dictionary<(MessageOrigin, int?), TrimAnalysisAssignmentPattern>();
2727
MethodCallPatterns = new Dictionary<MessageOrigin, TrimAnalysisMethodCallPattern>();
2828
TokenAccessPatterns = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisTokenAccessPattern>();
2929
GenericInstantiations = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern>();
@@ -34,13 +34,14 @@ public TrimAnalysisPatternStore(ValueSetLattice<SingleValue> lattice, Logger log
3434

3535
public void Add(TrimAnalysisAssignmentPattern pattern)
3636
{
37-
if (!AssignmentPatterns.TryGetValue(pattern.Origin, out var existingPattern))
37+
var key = (pattern.Origin, pattern.ParameterIndex);
38+
if (!AssignmentPatterns.TryGetValue(key, out var existingPattern))
3839
{
39-
AssignmentPatterns.Add(pattern.Origin, pattern);
40+
AssignmentPatterns.Add(key, pattern);
4041
return;
4142
}
4243

43-
AssignmentPatterns[pattern.Origin] = pattern.Merge(Lattice, existingPattern);
44+
AssignmentPatterns[key] = pattern.Merge(Lattice, existingPattern);
4445
}
4546

4647
public void Add(TrimAnalysisMethodCallPattern pattern)

‎src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -750,7 +750,7 @@ private void ScanStarg (
750750
ParameterProxy param = new (thisMethod, paramNum);
751751
var targetValue = GetMethodParameterValue (param);
752752
if (targetValue is MethodParameterValue targetParameterValue)
753-
HandleStoreParameter (thisMethod, targetParameterValue, operation, valueToStore.Value);
753+
HandleStoreParameter (thisMethod, targetParameterValue, operation, valueToStore.Value, null);
754754

755755
// If the targetValue is MethodThisValue do nothing - it should never happen really, and if it does, there's nothing we can track there
756756
}
@@ -846,7 +846,7 @@ private void ScanIndirectStore (
846846
StackSlot valueToStore = PopUnknown (currentStack, 1, methodBody, operation.Offset);
847847
StackSlot destination = PopUnknown (currentStack, 1, methodBody, operation.Offset);
848848

849-
StoreInReference (destination.Value, valueToStore.Value, methodBody.Method, operation, locals, curBasicBlock, ref ipState);
849+
StoreInReference (destination.Value, valueToStore.Value, methodBody.Method, operation, locals, curBasicBlock, ref ipState, null);
850850
}
851851

852852
/// <summary>
@@ -856,8 +856,9 @@ private void ScanIndirectStore (
856856
/// <param name="source">The value to store</param>
857857
/// <param name="method">The method body that contains the operation causing the store</param>
858858
/// <param name="operation">The instruction causing the store</param>
859+
/// <param name="parameterIndex">For assignment due to a call to a method with out params, the index of the out parameter.</param>
859860
/// <exception cref="LinkerFatalErrorException">Throws if <paramref name="target"/> is not a valid target for an indirect store.</exception>
860-
protected void StoreInReference (MultiValue target, MultiValue source, MethodDefinition method, Instruction operation, LocalVariableStore locals, int curBasicBlock, ref InterproceduralState ipState)
861+
protected void StoreInReference (MultiValue target, MultiValue source, MethodDefinition method, Instruction operation, LocalVariableStore locals, int curBasicBlock, ref InterproceduralState ipState, int? parameterIndex)
861862
{
862863
foreach (var value in target.AsEnumerable ()) {
863864
switch (value) {
@@ -866,18 +867,18 @@ protected void StoreInReference (MultiValue target, MultiValue source, MethodDef
866867
break;
867868
case FieldReferenceValue fieldReference
868869
when GetFieldValue (fieldReference.FieldDefinition).AsSingleValue () is FieldValue fieldValue:
869-
HandleStoreField (method, fieldValue, operation, source);
870+
HandleStoreField (method, fieldValue, operation, source, parameterIndex);
870871
break;
871872
case ParameterReferenceValue parameterReference
872873
when GetMethodParameterValue (parameterReference.Parameter) is MethodParameterValue parameterValue:
873-
HandleStoreParameter (method, parameterValue, operation, source);
874+
HandleStoreParameter (method, parameterValue, operation, source, parameterIndex);
874875
break;
875876
case MethodReturnValue methodReturnValue:
876877
// Ref returns don't have special ReferenceValue values, so assume if the target here is a MethodReturnValue then it must be a ref return value
877878
HandleReturnValue (method, methodReturnValue, operation, source);
878879
break;
879880
case FieldValue fieldValue:
880-
HandleStoreField (method, fieldValue, operation, DereferenceValue (source, locals, ref ipState));
881+
HandleStoreField (method, fieldValue, operation, DereferenceValue (source, locals, ref ipState), parameterIndex);
881882
break;
882883
case IValueWithStaticType valueWithStaticType:
883884
if (valueWithStaticType.StaticType is not null && _context.Annotations.FlowAnnotations.IsTypeInterestingForDataflow (valueWithStaticType.StaticType.Value.Type))
@@ -927,11 +928,11 @@ private void ScanLdfld (
927928
currentStack.Push (new StackSlot (value));
928929
}
929930

930-
protected virtual void HandleStoreField (MethodDefinition method, FieldValue field, Instruction operation, MultiValue valueToStore)
931+
protected virtual void HandleStoreField (MethodDefinition method, FieldValue field, Instruction operation, MultiValue valueToStore, int? parameterIndex)
931932
{
932933
}
933934

934-
protected virtual void HandleStoreParameter (MethodDefinition method, MethodParameterValue parameter, Instruction operation, MultiValue valueToStore)
935+
protected virtual void HandleStoreParameter (MethodDefinition method, MethodParameterValue parameter, Instruction operation, MultiValue valueToStore, int? parameterIndex)
935936
{
936937
}
937938

@@ -967,7 +968,7 @@ private void ScanStfld (
967968
// Incomplete handling of ref fields -- if we're storing a reference to a value, pretend it's just the value
968969
MultiValue valueToStore = DereferenceValue (valueToStoreSlot.Value, locals, ref interproceduralState);
969970

970-
HandleStoreField (thisMethod, fieldValue, operation, valueToStore);
971+
HandleStoreField (thisMethod, fieldValue, operation, valueToStore, null);
971972
}
972973
}
973974
}
@@ -1062,15 +1063,17 @@ protected void AssignRefAndOutParameters (
10621063
if (parameter.GetReferenceKind () is not (ReferenceKind.Ref or ReferenceKind.Out))
10631064
continue;
10641065
var newByRefValue = _context.Annotations.FlowAnnotations.GetMethodParameterValue (parameter);
1065-
StoreInReference (methodArguments[(int) parameter.Index], newByRefValue, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState);
1066+
StoreInReference (methodArguments[(int) parameter.Index], newByRefValue, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState, parameter.Index.Index);
10661067
}
10671068
} else {
10681069
// We couldn't resolve the method, so we put unknown values into the ref and out arguments
10691070
// Should be a very cold path, so using Linq.Zip should be okay
1070-
foreach (var (argument, refKind) in methodArguments.Zip (calledMethod.GetParameterReferenceKinds ())) {
1071+
var argumentRefKinds = methodArguments.Zip (calledMethod.GetParameterReferenceKinds ()).ToList ();
1072+
for (int index = 0; index < argumentRefKinds.Count; index++) {
1073+
var (argument, refKind) = argumentRefKinds[index];
10711074
if (refKind is not (ReferenceKind.Ref or ReferenceKind.Out))
10721075
continue;
1073-
StoreInReference (argument, UnknownValue.Instance, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState);
1076+
StoreInReference (argument, UnknownValue.Instance, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState, index);
10741077
}
10751078
}
10761079
}

0 commit comments

Comments
 (0)