From 1d629ace4c14f76ed3bd0396ebac16bc2be33cd0 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Mon, 29 Jul 2024 14:52:05 -0700 Subject: [PATCH 1/8] Add test --- .../DataFlowTests.g.cs | 8 +++- .../DataFlow/MultipleReturnsDataFlow.cs | 42 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/MultipleReturnsDataFlow.cs diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs index 8f435261c79396..7fda453570e3e1 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Threading.Tasks; using Xunit; @@ -25,6 +25,12 @@ public Task MethodByRefParameterDataFlow () return RunTest (allowMissingWarnings: true); } + [Fact] + public Task MultipleReturnsDataFlow () + { + return RunTest (allowMissingWarnings: true); + } + [Fact] public Task StaticInterfaceMethodDataflow () { diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/MultipleReturnsDataFlow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/MultipleReturnsDataFlow.cs new file mode 100644 index 00000000000000..9026f13ec06568 --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/MultipleReturnsDataFlow.cs @@ -0,0 +1,42 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System; +using System.Diagnostics.CodeAnalysis; +using Mono.Linker.Tests.Cases.Expectations.Assertions; +using Mono.Linker.Tests.Cases.Expectations.Helpers; +using Mono.Linker.Tests.Cases.Expectations.Metadata; + +namespace Mono.Linker.Tests.Cases.DataFlow +{ + [SkipKeptItemsValidation] + [ExpectedNoWarnings] + // Optimized code makes it easier to produce a method body with multiple return statements in IL. + [SetupCompileArgument ("/optimize+")] + public class MultipleReturnsDataFlow + { + public static void Main () + { + MultipleReturnsOfSameValueMismatch (); + } + + static Type GetUnannotatedType () => null; + + [ExpectedWarning ("IL2073", + nameof (MultipleReturnsDataFlow) + "." + nameof (MultipleReturnsOfSameValueMismatch))] + [ExpectedWarning ("IL2073", + nameof (MultipleReturnsDataFlow) + "." + nameof (MultipleReturnsOfSameValueMismatch))] + [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors)] + static Type MultipleReturnsOfSameValueMismatch (int condition = 0) + { + var value = GetUnannotatedType (); + if (condition is 0) + return value; + + if (condition is 1) + return null; + + return GetUnannotatedType (); + } + } +} From 3105c3728d6a5360366e8bdc8019a266e2634d93 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Mon, 29 Jul 2024 14:53:26 -0700 Subject: [PATCH 2/8] Fix linker behavior --- .../Linker.Dataflow/MethodBodyScanner.cs | 15 ++++++------- .../ReflectionMethodBodyScanner.cs | 21 ++++--------------- .../TrimAnalysisPatternStore.cs | 16 +++++--------- 3 files changed, 17 insertions(+), 35 deletions(-) diff --git a/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs b/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs index 34e5797ffd03b9..536588c6cc61f0 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs @@ -54,8 +54,6 @@ protected MethodBodyScanner (LinkContext context) this.InterproceduralStateLattice = new InterproceduralStateLattice (default, default, context); } - internal MultiValue ReturnValue { get; private set; } - protected virtual void WarnAboutInvalidILInMethod (MethodBody method, int ilOffset) { } @@ -289,7 +287,6 @@ protected virtual void Scan (MethodIL methodIL, ref InterproceduralState interpr BasicBlockIterator blockIterator = new BasicBlockIterator (methodIL); - ReturnValue = default; foreach (Instruction operation in methodIL.Instructions) { int curBasicBlock = blockIterator.MoveNext (operation); @@ -655,10 +652,12 @@ protected virtual void Scan (MethodIL methodIL, ref InterproceduralState interpr WarnAboutInvalidILInMethod (methodBody, operation.Offset); } if (hasReturnValue) { - StackSlot retValue = PopUnknown (currentStack, 1, methodBody, operation.Offset); + StackSlot retValueSlot = PopUnknown (currentStack, 1, methodBody, operation.Offset); // If the return value is a reference, treat it as the value itself for now // We can handle ref return values better later - ReturnValue = MultiValueLattice.Meet (ReturnValue, DereferenceValue (retValue.Value, locals, ref interproceduralState)); + MultiValue retValue = DereferenceValue (retValueSlot.Value, locals, ref interproceduralState); + var methodReturnValue = GetReturnValue (methodBody.Method); + HandleReturnValue (thisMethod, methodReturnValue, operation, retValue); ValidateNoReferenceToReference (locals, methodBody.Method, operation.Offset); } ClearStack (ref currentStack); @@ -873,7 +872,7 @@ when GetMethodParameterValue (parameterReference.Parameter) is MethodParameterVa break; case MethodReturnValue methodReturnValue: // 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 - HandleStoreMethodReturnValue (method, methodReturnValue, operation, source); + HandleReturnValue (method, methodReturnValue, operation, source); break; case FieldValue fieldValue: HandleStoreField (method, fieldValue, operation, DereferenceValue (source, locals, ref ipState)); @@ -897,6 +896,8 @@ when GetMethodParameterValue (parameterReference.Parameter) is MethodParameterVa protected abstract MultiValue GetFieldValue (FieldDefinition field); + protected abstract MethodReturnValue GetReturnValue (MethodDefinition method); + private void ScanLdfld ( Instruction operation, Stack currentStack, @@ -934,7 +935,7 @@ protected virtual void HandleStoreParameter (MethodDefinition method, MethodPara { } - protected virtual void HandleStoreMethodReturnValue (MethodDefinition method, MethodReturnValue thisParameter, Instruction operation, MultiValue sourceValue) + protected virtual void HandleReturnValue (MethodDefinition method, MethodReturnValue thisParameter, Instruction operation, MultiValue valueToReturn) { } diff --git a/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs b/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs index 3f5109bfeff9bd..832358f2181467 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs @@ -72,13 +72,6 @@ protected override void Scan (MethodIL methodIL, ref InterproceduralState interp { _origin = new MessageOrigin (methodIL.Method); base.Scan (methodIL, ref interproceduralState); - - if (!methodIL.Method.ReturnsVoid ()) { - var method = methodIL.Method; - var methodReturnValue = _annotations.GetMethodReturnValue (method, isNewObj: false); - if (methodReturnValue.DynamicallyAccessedMemberTypes != 0) - HandleAssignmentPattern (_origin, ReturnValue, methodReturnValue); - } } protected override void WarnAboutInvalidILInMethod (MethodBody method, int ilOffset) @@ -100,11 +93,13 @@ MethodParameterValue GetMethodParameterValue (ParameterProxy parameter, Dynamica protected override MultiValue GetFieldValue (FieldDefinition field) => _annotations.GetFieldValue (field); + protected override MethodReturnValue GetReturnValue (MethodDefinition method) => _annotations.GetMethodReturnValue (method, isNewObj: false); + private void HandleStoreValueWithDynamicallyAccessedMembers (ValueWithDynamicallyAccessedMembers targetValue, Instruction operation, MultiValue sourceValue) { if (targetValue.DynamicallyAccessedMemberTypes != 0) { _origin = _origin.WithInstructionOffset (operation.Offset); - HandleAssignmentPattern (_origin, sourceValue, targetValue); + TrimAnalysisPatterns.Add (new TrimAnalysisAssignmentPattern (sourceValue, targetValue, _origin)); } } @@ -114,7 +109,7 @@ protected override void HandleStoreField (MethodDefinition method, FieldValue fi protected override void HandleStoreParameter (MethodDefinition method, MethodParameterValue parameter, Instruction operation, MultiValue valueToStore) => HandleStoreValueWithDynamicallyAccessedMembers (parameter, operation, valueToStore); - protected override void HandleStoreMethodReturnValue (MethodDefinition method, MethodReturnValue returnValue, Instruction operation, MultiValue valueToStore) + protected override void HandleReturnValue (MethodDefinition method, MethodReturnValue returnValue, Instruction operation, MultiValue valueToStore) => HandleStoreValueWithDynamicallyAccessedMembers (returnValue, operation, valueToStore); public override MultiValue HandleCall (MethodBody callingMethodBody, MethodReference calledMethod, Instruction operation, ValueNodeList methodParams) @@ -248,14 +243,6 @@ static bool IsComInterop (IMarshalInfoProvider marshalInfoProvider, TypeReferenc return false; } - void HandleAssignmentPattern ( - in MessageOrigin origin, - in MultiValue value, - ValueWithDynamicallyAccessedMembers targetValue) - { - TrimAnalysisPatterns.Add (new TrimAnalysisAssignmentPattern (value, targetValue, origin)); - } - internal static bool IsPInvokeDangerous (MethodDefinition methodDefinition, LinkContext context, out bool comDangerousMethod) { // The method in ILLink only detects one condition - COM Dangerous, but it's structured like this diff --git a/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisPatternStore.cs b/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisPatternStore.cs index 169656ca74ddfc..48a09f466410f0 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisPatternStore.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisPatternStore.cs @@ -10,14 +10,14 @@ namespace Mono.Linker.Dataflow { public readonly struct TrimAnalysisPatternStore { - readonly Dictionary<(MessageOrigin, bool), TrimAnalysisAssignmentPattern> AssignmentPatterns; + readonly Dictionary AssignmentPatterns; readonly Dictionary MethodCallPatterns; readonly ValueSetLattice Lattice; readonly LinkContext _context; public TrimAnalysisPatternStore (ValueSetLattice lattice, LinkContext context) { - AssignmentPatterns = new Dictionary<(MessageOrigin, bool), TrimAnalysisAssignmentPattern> (); + AssignmentPatterns = new Dictionary (); MethodCallPatterns = new Dictionary (); Lattice = lattice; _context = context; @@ -25,18 +25,12 @@ public TrimAnalysisPatternStore (ValueSetLattice lattice, LinkConte public void Add (TrimAnalysisAssignmentPattern pattern) { - // While trimming, each pattern should have a unique origin (which has ILOffset) - // but we don't track the correct ILOffset for return instructions. - // https://github.com/dotnet/linker/issues/2778 - // For now, work around it with a separate bit. - bool isReturnValue = pattern.Target.AsSingleValue () is MethodReturnValue; - - if (!AssignmentPatterns.TryGetValue ((pattern.Origin, isReturnValue), out var existingPattern)) { - AssignmentPatterns.Add ((pattern.Origin, isReturnValue), pattern); + if (!AssignmentPatterns.TryGetValue (pattern.Origin, out var existingPattern)) { + AssignmentPatterns.Add (pattern.Origin, pattern); return; } - AssignmentPatterns[(pattern.Origin, isReturnValue)] = pattern.Merge (Lattice, existingPattern); + AssignmentPatterns[pattern.Origin] = pattern.Merge (Lattice, existingPattern); } public void Add (TrimAnalysisMethodCallPattern pattern) From f542aa7a68d90d52f6e674e40663daf7201237eb Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Mon, 29 Jul 2024 15:07:45 -0700 Subject: [PATCH 3/8] Fix ILC behavior --- .../Compiler/Dataflow/MethodBodyScanner.cs | 15 ++++++------ .../Dataflow/ReflectionMethodBodyScanner.cs | 23 ++++--------------- .../Dataflow/TrimAnalysisPatternStore.cs | 16 ++++--------- .../Linker.Dataflow/MethodBodyScanner.cs | 8 +++---- 4 files changed, 21 insertions(+), 41 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs index 76da7a29ab8c98..13129864e6ef6e 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs @@ -52,8 +52,6 @@ internal abstract partial class MethodBodyScanner protected readonly FlowAnnotations _annotations; - internal MultiValue ReturnValue { get; private set; } - protected MethodBodyScanner(FlowAnnotations annotations) { _annotations = annotations; @@ -356,7 +354,6 @@ protected virtual void Scan(MethodIL methodBody, ref InterproceduralState interp BasicBlockIterator blockIterator = new BasicBlockIterator(methodBody); - ReturnValue = default(MultiValue); ILReader reader = new ILReader(methodBody.GetILBytes()); while (reader.HasNext) { @@ -797,10 +794,12 @@ protected virtual void Scan(MethodIL methodBody, ref InterproceduralState interp } if (hasReturnValue) { - StackSlot retValue = PopUnknown(currentStack, 1, methodBody, offset); + StackSlot retStackSlot = PopUnknown(currentStack, 1, methodBody, offset); // If the return value is a reference, treat it as the value itself for now // We can handle ref return values better later - ReturnValue = MultiValueLattice.Meet(ReturnValue, DereferenceValue(methodBody, offset, retValue.Value, locals, ref interproceduralState)); + MultiValue retValue = DereferenceValue(methodBody, offset, retStackSlot.Value, locals, ref interproceduralState); + var methodReturnValue = GetReturnValue(methodBody); + HandleReturnValue(methodBody, offset, methodReturnValue, retValue); ValidateNoReferenceToReference(locals, methodBody, offset); } ClearStack(ref currentStack); @@ -871,6 +870,8 @@ private static void ScanExceptionInformation(Dictionary> k protected abstract SingleValue GetMethodParameterValue(ParameterProxy parameter); + protected abstract MethodReturnValue GetReturnValue (MethodIL method); + private void ScanLdarg(ILOpcode opcode, int parameterIndex, Stack currentStack, MethodDesc thisMethod) { ParameterIndex paramNum = (ParameterIndex)parameterIndex; @@ -1036,7 +1037,7 @@ when GetMethodParameterValue(parameterReference.Parameter) is MethodParameterVal break; case MethodReturnValue methodReturnValue: // 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 - HandleStoreMethodReturnValue(method, offset, methodReturnValue, source); + HandleReturnValue(method, offset, methodReturnValue, source); break; case FieldValue fieldValue: HandleStoreField(method, offset, fieldValue, DereferenceValue(method, offset, source, locals, ref ipState)); @@ -1115,7 +1116,7 @@ protected virtual void HandleStoreParameter(MethodIL method, int offset, MethodP { } - protected virtual void HandleStoreMethodReturnValue(MethodIL method, int offset, MethodReturnValue thisParameter, MultiValue sourceValue) + protected virtual void HandleReturnValue(MethodIL method, int offset, MethodReturnValue thisParameter, MultiValue sourceValue) { } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs index 6a00b2df965762..8a057741814cad 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs @@ -114,14 +114,6 @@ protected override void Scan(MethodIL methodBody, ref InterproceduralState inter { _origin = new MessageOrigin(methodBody.OwningMethod); base.Scan(methodBody, ref interproceduralState); - - if (!methodBody.OwningMethod.Signature.ReturnType.IsVoid) - { - var method = methodBody.OwningMethod; - var methodReturnValue = _annotations.GetMethodReturnValue(method, isNewObj: false); - if (methodReturnValue.DynamicallyAccessedMemberTypes != 0) - HandleAssignmentPattern(_origin, ReturnValue, methodReturnValue, method.GetDisplayName()); - } } public static DependencyList ScanAndProcessReturnValue(NodeFactory factory, FlowAnnotations annotations, Logger logger, MethodIL methodBody, out List runtimeDependencies) @@ -195,6 +187,8 @@ protected override ValueWithDynamicallyAccessedMembers GetMethodParameterValue(P private MethodParameterValue GetMethodParameterValue(ParameterProxy parameter, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes) => _annotations.GetMethodParameterValue(parameter, dynamicallyAccessedMemberTypes); + protected override MethodReturnValue GetReturnValue (MethodIL method) => _annotations.GetMethodReturnValue (method.OwningMethod, isNewObj: false); + /// /// HandleGetField is called every time the scanner needs to represent a value of the field /// either as a source or target. It is not called when just a reference to field is created, @@ -219,7 +213,7 @@ private void HandleStoreValueWithDynamicallyAccessedMembers(MethodIL methodBody, if (targetValue.DynamicallyAccessedMemberTypes != 0) { _origin = _origin.WithInstructionOffset(methodBody, offset); - HandleAssignmentPattern(_origin, sourceValue, targetValue, reason); + TrimAnalysisPatterns.Add(new TrimAnalysisAssignmentPattern(sourceValue, targetValue, _origin, reason)); } } @@ -229,7 +223,7 @@ protected override void HandleStoreField(MethodIL methodBody, int offset, FieldV protected override void HandleStoreParameter(MethodIL methodBody, int offset, MethodParameterValue parameter, MultiValue valueToStore) => HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, parameter, valueToStore, parameter.Parameter.Method.GetDisplayName()); - protected override void HandleStoreMethodReturnValue(MethodIL methodBody, int offset, MethodReturnValue returnValue, MultiValue valueToStore) + protected override void HandleReturnValue(MethodIL methodBody, int offset, MethodReturnValue returnValue, MultiValue valueToStore) => HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, returnValue, valueToStore, returnValue.Method.GetDisplayName()); protected override void HandleTypeTokenAccess(MethodIL methodBody, int offset, TypeDesc accessedType) @@ -414,15 +408,6 @@ private static bool IsComInterop(MarshalAsDescriptor? marshalInfoProvider, TypeD return false; } - private void HandleAssignmentPattern( - in MessageOrigin origin, - in MultiValue value, - ValueWithDynamicallyAccessedMembers targetValue, - string reason) - { - TrimAnalysisPatterns.Add(new TrimAnalysisAssignmentPattern(value, targetValue, origin, reason)); - } - private void ProcessGenericArgumentDataFlow(MethodDesc method) { // We only need to validate static methods and then all generic methods diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisPatternStore.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisPatternStore.cs index 258f2bcfef26cc..76bc92cf80e4c7 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisPatternStore.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisPatternStore.cs @@ -13,7 +13,7 @@ namespace ILCompiler.Dataflow { public readonly struct TrimAnalysisPatternStore { - private readonly Dictionary<(MessageOrigin, bool), TrimAnalysisAssignmentPattern> AssignmentPatterns; + private readonly Dictionary AssignmentPatterns; private readonly Dictionary MethodCallPatterns; private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisTokenAccessPattern> TokenAccessPatterns; private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern> GenericInstantiations; @@ -23,7 +23,7 @@ public readonly struct TrimAnalysisPatternStore public TrimAnalysisPatternStore(ValueSetLattice lattice, Logger logger) { - AssignmentPatterns = new Dictionary<(MessageOrigin, bool), TrimAnalysisAssignmentPattern>(); + AssignmentPatterns = new Dictionary(); MethodCallPatterns = new Dictionary(); TokenAccessPatterns = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisTokenAccessPattern>(); GenericInstantiations = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern>(); @@ -34,19 +34,13 @@ public TrimAnalysisPatternStore(ValueSetLattice lattice, Logger log public void Add(TrimAnalysisAssignmentPattern pattern) { - // While trimming, each pattern should have a unique origin (which has ILOffset) - // but we don't track the correct ILOffset for return instructions. - // https://github.com/dotnet/linker/issues/2778 - // For now, work around it with a separate bit. - bool isReturnValue = pattern.Target.AsSingleValue() is MethodReturnValue; - - if (!AssignmentPatterns.TryGetValue((pattern.Origin, isReturnValue), out var existingPattern)) + if (!AssignmentPatterns.TryGetValue(pattern.Origin, out var existingPattern)) { - AssignmentPatterns.Add((pattern.Origin, isReturnValue), pattern); + AssignmentPatterns.Add(pattern.Origin, pattern); return; } - AssignmentPatterns[(pattern.Origin, isReturnValue)] = pattern.Merge(Lattice, existingPattern); + AssignmentPatterns[pattern.Origin] = pattern.Merge(Lattice, existingPattern); } public void Add(TrimAnalysisMethodCallPattern pattern) diff --git a/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs b/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs index 536588c6cc61f0..cb8869f40dd993 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs @@ -652,10 +652,10 @@ protected virtual void Scan (MethodIL methodIL, ref InterproceduralState interpr WarnAboutInvalidILInMethod (methodBody, operation.Offset); } if (hasReturnValue) { - StackSlot retValueSlot = PopUnknown (currentStack, 1, methodBody, operation.Offset); + StackSlot retStackSlot = PopUnknown (currentStack, 1, methodBody, operation.Offset); // If the return value is a reference, treat it as the value itself for now // We can handle ref return values better later - MultiValue retValue = DereferenceValue (retValueSlot.Value, locals, ref interproceduralState); + MultiValue retValue = DereferenceValue (retStackSlot.Value, locals, ref interproceduralState); var methodReturnValue = GetReturnValue (methodBody.Method); HandleReturnValue (thisMethod, methodReturnValue, operation, retValue); ValidateNoReferenceToReference (locals, methodBody.Method, operation.Offset); @@ -718,6 +718,8 @@ private static void ScanExceptionInformation (Dictionary> protected abstract SingleValue GetMethodParameterValue (ParameterProxy parameter); + protected abstract MethodReturnValue GetReturnValue (MethodDefinition method); + private void ScanLdarg (Instruction operation, Stack currentStack, MethodDefinition thisMethod) { Code code = operation.OpCode.Code; @@ -896,8 +898,6 @@ when GetMethodParameterValue (parameterReference.Parameter) is MethodParameterVa protected abstract MultiValue GetFieldValue (FieldDefinition field); - protected abstract MethodReturnValue GetReturnValue (MethodDefinition method); - private void ScanLdfld ( Instruction operation, Stack currentStack, From 0447405fcc5e68c7e3f3a487d4574908335a5ed0 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Mon, 29 Jul 2024 15:11:55 -0700 Subject: [PATCH 4/8] Clean up analyzer implementation --- .../TrimAnalysis/TrimAnalysisPatternStore.cs | 12 ++++++------ .../TrimAnalysis/TrimAnalysisVisitor.cs | 8 ++------ .../ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs | 6 ++++++ .../DataFlowTests.g.cs | 6 +++--- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisPatternStore.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisPatternStore.cs index dd66d802934be6..aeeca93888ed84 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisPatternStore.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisPatternStore.cs @@ -12,7 +12,7 @@ namespace ILLink.RoslynAnalyzer.TrimAnalysis { public readonly struct TrimAnalysisPatternStore { - readonly Dictionary<(IOperation, bool), TrimAnalysisAssignmentPattern> AssignmentPatterns; + readonly Dictionary AssignmentPatterns; readonly Dictionary FieldAccessPatterns; readonly Dictionary GenericInstantiationPatterns; readonly Dictionary MethodCallPatterns; @@ -25,7 +25,7 @@ public TrimAnalysisPatternStore ( ValueSetLattice lattice, FeatureContextLattice featureContextLattice) { - AssignmentPatterns = new Dictionary<(IOperation, bool), TrimAnalysisAssignmentPattern> (); + AssignmentPatterns = new Dictionary (); FieldAccessPatterns = new Dictionary (); GenericInstantiationPatterns = new Dictionary (); MethodCallPatterns = new Dictionary (); @@ -35,7 +35,7 @@ public TrimAnalysisPatternStore ( FeatureContextLattice = featureContextLattice; } - public void Add (TrimAnalysisAssignmentPattern trimAnalysisPattern, bool isReturnValue) + public void Add (TrimAnalysisAssignmentPattern trimAnalysisPattern) { // Finally blocks will be analyzed multiple times, once for normal control flow and once // for exceptional control flow, and these separate analyses could produce different @@ -45,12 +45,12 @@ public void Add (TrimAnalysisAssignmentPattern trimAnalysisPattern, bool isRetur // of the normal control-flow state. // We still add patterns to the operation, rather than replacing, to make this resilient to // changes in the analysis algorithm. - if (!AssignmentPatterns.TryGetValue ((trimAnalysisPattern.Operation, isReturnValue), out var existingPattern)) { - AssignmentPatterns.Add ((trimAnalysisPattern.Operation, isReturnValue), trimAnalysisPattern); + if (!AssignmentPatterns.TryGetValue (trimAnalysisPattern.Operation, out var existingPattern)) { + AssignmentPatterns.Add (trimAnalysisPattern.Operation, trimAnalysisPattern); return; } - AssignmentPatterns[(trimAnalysisPattern.Operation, isReturnValue)] = trimAnalysisPattern.Merge (Lattice, FeatureContextLattice, existingPattern); + AssignmentPatterns[trimAnalysisPattern.Operation] = trimAnalysisPattern.Merge (Lattice, FeatureContextLattice, existingPattern); } public void Add (TrimAnalysisFieldAccessPattern pattern) diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs index 2d9cb6cd84160d..5352ce1cfa8ece 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs @@ -238,9 +238,7 @@ public override void HandleAssignment (MultiValue source, MultiValue target, IOp // annotated with DAMT. TrimAnalysisPatterns.Add ( // This will copy the values if necessary - new TrimAnalysisAssignmentPattern (source, target, operation, OwningSymbol, featureContext), - isReturnValue: false - ); + new TrimAnalysisAssignmentPattern (source, target, operation, OwningSymbol, featureContext)); } public override MultiValue HandleArrayElementRead (MultiValue arrayValue, MultiValue indexValue, IOperation operation) @@ -350,9 +348,7 @@ public override void HandleReturnValue (MultiValue returnValue, IOperation opera var returnParameter = new MethodReturnValue (method, isNewObj: false); TrimAnalysisPatterns.Add ( - new TrimAnalysisAssignmentPattern (returnValue, returnParameter, operation, OwningSymbol, featureContext), - isReturnValue: true - ); + new TrimAnalysisAssignmentPattern (returnValue, returnParameter, operation, OwningSymbol, featureContext)); } } diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs index 167794b4fee4b8..b790a3a708678a 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs @@ -215,6 +215,12 @@ public Task MethodByRefReturnDataFlow () return RunTest (); } + [Fact] + public Task MultipleReturnsDataFlow () + { + return RunTest (); + } + [Fact] public Task GetInterfaceDataFlow () { diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs index 7fda453570e3e1..95dfd9c0f72a0b 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/DataFlowTests.g.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Threading.Tasks; using Xunit; @@ -20,13 +20,13 @@ public Task GenericParameterDataFlowMarking () } [Fact] - public Task MethodByRefParameterDataFlow () + public Task InterfaceImplementedThroughBaseValidation () { return RunTest (allowMissingWarnings: true); } [Fact] - public Task MultipleReturnsDataFlow () + public Task MethodByRefParameterDataFlow () { return RunTest (allowMissingWarnings: true); } From eafaa6de531d7b809677e172cd8322ef3597ba46 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Mon, 29 Jul 2024 16:18:47 -0700 Subject: [PATCH 5/8] Fix linker behavior --- .../Linker.Dataflow/MethodBodyScanner.cs | 27 ++++++++++--------- .../ReflectionMethodBodyScanner.cs | 14 +++++----- .../TrimAnalysisAssignmentPattern.cs | 11 ++++++-- .../TrimAnalysisPatternStore.cs | 11 ++++---- .../DataFlow/ByRefDataflow.cs | 2 -- 5 files changed, 37 insertions(+), 28 deletions(-) diff --git a/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs b/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs index cb8869f40dd993..9f939bef8f40ec 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs @@ -750,7 +750,7 @@ private void ScanStarg ( ParameterProxy param = new (thisMethod, paramNum); var targetValue = GetMethodParameterValue (param); if (targetValue is MethodParameterValue targetParameterValue) - HandleStoreParameter (thisMethod, targetParameterValue, operation, valueToStore.Value); + HandleStoreParameter (thisMethod, targetParameterValue, operation, valueToStore.Value, null); // If the targetValue is MethodThisValue do nothing - it should never happen really, and if it does, there's nothing we can track there } @@ -846,7 +846,7 @@ private void ScanIndirectStore ( StackSlot valueToStore = PopUnknown (currentStack, 1, methodBody, operation.Offset); StackSlot destination = PopUnknown (currentStack, 1, methodBody, operation.Offset); - StoreInReference (destination.Value, valueToStore.Value, methodBody.Method, operation, locals, curBasicBlock, ref ipState); + StoreInReference (destination.Value, valueToStore.Value, methodBody.Method, operation, locals, curBasicBlock, ref ipState, null); } /// @@ -856,8 +856,9 @@ private void ScanIndirectStore ( /// The value to store /// The method body that contains the operation causing the store /// The instruction causing the store + /// For assignment due to a call to a method with out params, the index of the out parameter. /// Throws if is not a valid target for an indirect store. - protected void StoreInReference (MultiValue target, MultiValue source, MethodDefinition method, Instruction operation, LocalVariableStore locals, int curBasicBlock, ref InterproceduralState ipState) + protected void StoreInReference (MultiValue target, MultiValue source, MethodDefinition method, Instruction operation, LocalVariableStore locals, int curBasicBlock, ref InterproceduralState ipState, int? parameterIndex) { foreach (var value in target.AsEnumerable ()) { switch (value) { @@ -866,18 +867,18 @@ protected void StoreInReference (MultiValue target, MultiValue source, MethodDef break; case FieldReferenceValue fieldReference when GetFieldValue (fieldReference.FieldDefinition).AsSingleValue () is FieldValue fieldValue: - HandleStoreField (method, fieldValue, operation, source); + HandleStoreField (method, fieldValue, operation, source, parameterIndex); break; case ParameterReferenceValue parameterReference when GetMethodParameterValue (parameterReference.Parameter) is MethodParameterValue parameterValue: - HandleStoreParameter (method, parameterValue, operation, source); + HandleStoreParameter (method, parameterValue, operation, source, parameterIndex); break; case MethodReturnValue methodReturnValue: // 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 HandleReturnValue (method, methodReturnValue, operation, source); break; case FieldValue fieldValue: - HandleStoreField (method, fieldValue, operation, DereferenceValue (source, locals, ref ipState)); + HandleStoreField (method, fieldValue, operation, DereferenceValue (source, locals, ref ipState), parameterIndex); break; case IValueWithStaticType valueWithStaticType: if (valueWithStaticType.StaticType is not null && _context.Annotations.FlowAnnotations.IsTypeInterestingForDataflow (valueWithStaticType.StaticType.Value.Type)) @@ -927,11 +928,11 @@ private void ScanLdfld ( currentStack.Push (new StackSlot (value)); } - protected virtual void HandleStoreField (MethodDefinition method, FieldValue field, Instruction operation, MultiValue valueToStore) + protected virtual void HandleStoreField (MethodDefinition method, FieldValue field, Instruction operation, MultiValue valueToStore, int? parameterIndex) { } - protected virtual void HandleStoreParameter (MethodDefinition method, MethodParameterValue parameter, Instruction operation, MultiValue valueToStore) + protected virtual void HandleStoreParameter (MethodDefinition method, MethodParameterValue parameter, Instruction operation, MultiValue valueToStore, int? parameterIndex) { } @@ -967,7 +968,7 @@ private void ScanStfld ( // Incomplete handling of ref fields -- if we're storing a reference to a value, pretend it's just the value MultiValue valueToStore = DereferenceValue (valueToStoreSlot.Value, locals, ref interproceduralState); - HandleStoreField (thisMethod, fieldValue, operation, valueToStore); + HandleStoreField (thisMethod, fieldValue, operation, valueToStore, null); } } } @@ -1062,15 +1063,17 @@ protected void AssignRefAndOutParameters ( if (parameter.GetReferenceKind () is not (ReferenceKind.Ref or ReferenceKind.Out)) continue; var newByRefValue = _context.Annotations.FlowAnnotations.GetMethodParameterValue (parameter); - StoreInReference (methodArguments[(int) parameter.Index], newByRefValue, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState); + StoreInReference (methodArguments[(int) parameter.Index], newByRefValue, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState, parameter.Index.Index); } } else { // We couldn't resolve the method, so we put unknown values into the ref and out arguments // Should be a very cold path, so using Linq.Zip should be okay - foreach (var (argument, refKind) in methodArguments.Zip (calledMethod.GetParameterReferenceKinds ())) { + var argumentRefKinds = methodArguments.Zip (calledMethod.GetParameterReferenceKinds ()).ToList (); + for (int index = 0; index < argumentRefKinds.Count; index++) { + var (argument, refKind) = argumentRefKinds[index]; if (refKind is not (ReferenceKind.Ref or ReferenceKind.Out)) continue; - StoreInReference (argument, UnknownValue.Instance, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState); + StoreInReference (argument, UnknownValue.Instance, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState, index); } } } diff --git a/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs b/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs index 832358f2181467..e22d97c91b52d6 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs @@ -95,22 +95,22 @@ MethodParameterValue GetMethodParameterValue (ParameterProxy parameter, Dynamica protected override MethodReturnValue GetReturnValue (MethodDefinition method) => _annotations.GetMethodReturnValue (method, isNewObj: false); - private void HandleStoreValueWithDynamicallyAccessedMembers (ValueWithDynamicallyAccessedMembers targetValue, Instruction operation, MultiValue sourceValue) + private void HandleStoreValueWithDynamicallyAccessedMembers (ValueWithDynamicallyAccessedMembers targetValue, Instruction operation, MultiValue sourceValue, int? parameterIndex) { if (targetValue.DynamicallyAccessedMemberTypes != 0) { _origin = _origin.WithInstructionOffset (operation.Offset); - TrimAnalysisPatterns.Add (new TrimAnalysisAssignmentPattern (sourceValue, targetValue, _origin)); + TrimAnalysisPatterns.Add (new TrimAnalysisAssignmentPattern (sourceValue, targetValue, _origin, parameterIndex)); } } - protected override void HandleStoreField (MethodDefinition method, FieldValue field, Instruction operation, MultiValue valueToStore) - => HandleStoreValueWithDynamicallyAccessedMembers (field, operation, valueToStore); + protected override void HandleStoreField (MethodDefinition method, FieldValue field, Instruction operation, MultiValue valueToStore, int? parameterIndex) + => HandleStoreValueWithDynamicallyAccessedMembers (field, operation, valueToStore, parameterIndex); - protected override void HandleStoreParameter (MethodDefinition method, MethodParameterValue parameter, Instruction operation, MultiValue valueToStore) - => HandleStoreValueWithDynamicallyAccessedMembers (parameter, operation, valueToStore); + protected override void HandleStoreParameter (MethodDefinition method, MethodParameterValue parameter, Instruction operation, MultiValue valueToStore, int? parameterIndex) + => HandleStoreValueWithDynamicallyAccessedMembers (parameter, operation, valueToStore, parameterIndex); protected override void HandleReturnValue (MethodDefinition method, MethodReturnValue returnValue, Instruction operation, MultiValue valueToStore) - => HandleStoreValueWithDynamicallyAccessedMembers (returnValue, operation, valueToStore); + => HandleStoreValueWithDynamicallyAccessedMembers (returnValue, operation, valueToStore, null); public override MultiValue HandleCall (MethodBody callingMethodBody, MethodReference calledMethod, Instruction operation, ValueNodeList methodParams) { diff --git a/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisAssignmentPattern.cs b/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisAssignmentPattern.cs index 90e742381e5572..dedf5769c3b4e5 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisAssignmentPattern.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisAssignmentPattern.cs @@ -15,21 +15,28 @@ public readonly record struct TrimAnalysisAssignmentPattern public MultiValue Target { get; init; } public MessageOrigin Origin { get; init; } - public TrimAnalysisAssignmentPattern (MultiValue source, MultiValue target, MessageOrigin origin) + // For assignment of a method parameter, we store the parameter index to disambiguate + // assignments from different out parameters ouf a single method call. + public int? ParameterIndex { get; init; } + + public TrimAnalysisAssignmentPattern (MultiValue source, MultiValue target, MessageOrigin origin, int? parameterIndex) { Source = source.DeepCopy (); Target = target.DeepCopy (); Origin = origin; + ParameterIndex = parameterIndex; } public TrimAnalysisAssignmentPattern Merge (ValueSetLattice lattice, TrimAnalysisAssignmentPattern other) { Debug.Assert (Origin == other.Origin); + Debug.Assert (ParameterIndex == other.ParameterIndex); return new TrimAnalysisAssignmentPattern ( lattice.Meet (Source, other.Source), lattice.Meet (Target, other.Target), - Origin); + Origin, + ParameterIndex); } public void MarkAndProduceDiagnostics (ReflectionMarker reflectionMarker, LinkContext context) diff --git a/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisPatternStore.cs b/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisPatternStore.cs index 48a09f466410f0..6c53694e1d9927 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisPatternStore.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisPatternStore.cs @@ -10,14 +10,14 @@ namespace Mono.Linker.Dataflow { public readonly struct TrimAnalysisPatternStore { - readonly Dictionary AssignmentPatterns; + readonly Dictionary<(MessageOrigin, int?), TrimAnalysisAssignmentPattern> AssignmentPatterns; readonly Dictionary MethodCallPatterns; readonly ValueSetLattice Lattice; readonly LinkContext _context; public TrimAnalysisPatternStore (ValueSetLattice lattice, LinkContext context) { - AssignmentPatterns = new Dictionary (); + AssignmentPatterns = new Dictionary<(MessageOrigin, int?), TrimAnalysisAssignmentPattern> (); MethodCallPatterns = new Dictionary (); Lattice = lattice; _context = context; @@ -25,12 +25,13 @@ public TrimAnalysisPatternStore (ValueSetLattice lattice, LinkConte public void Add (TrimAnalysisAssignmentPattern pattern) { - if (!AssignmentPatterns.TryGetValue (pattern.Origin, out var existingPattern)) { - AssignmentPatterns.Add (pattern.Origin, pattern); + var key = (pattern.Origin, pattern.ParameterIndex); + if (!AssignmentPatterns.TryGetValue (key, out var existingPattern)) { + AssignmentPatterns.Add (key, pattern); return; } - AssignmentPatterns[pattern.Origin] = pattern.Merge (Lattice, existingPattern); + AssignmentPatterns[key] = pattern.Merge (Lattice, existingPattern); } public void Add (TrimAnalysisMethodCallPattern pattern) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ByRefDataflow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ByRefDataflow.cs index a268d1f70b2e4a..a0a1463ee753e7 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ByRefDataflow.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ByRefDataflow.cs @@ -181,8 +181,6 @@ static void TwoOutRefs ( } [Kept] - [ExpectedWarning ("IL2069", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/85464")] - [ExpectedWarning ("IL2069", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/85464")] public static void Test () { TwoOutRefs (out _publicMethodsField, out _publicPropertiesField); From a4c0fb8335c006f4276086804385249dd1e355fd Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Mon, 29 Jul 2024 16:30:49 -0700 Subject: [PATCH 6/8] Fix ILC behavior --- .../Compiler/Dataflow/MethodBodyScanner.cs | 20 +++++++++---------- .../Dataflow/ReflectionMethodBodyScanner.cs | 14 ++++++------- .../Dataflow/TrimAnalysisAssignmentPattern.cs | 6 +++++- .../Dataflow/TrimAnalysisPatternStore.cs | 11 +++++----- 4 files changed, 28 insertions(+), 23 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs index 13129864e6ef6e..169eacf85af432 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs @@ -901,7 +901,7 @@ Stack currentStack ParameterProxy param = new(methodBody.OwningMethod, paramNum); var targetValue = GetMethodParameterValue(param); if (targetValue is MethodParameterValue targetParameterValue) - HandleStoreParameter(methodBody, offset, targetParameterValue, valueToStore.Value); + HandleStoreParameter(methodBody, offset, targetParameterValue, valueToStore.Value, null); // If the targetValue is MethodThisValue do nothing - it should never happen really, and if it does, there's nothing we can track there } @@ -1007,7 +1007,7 @@ private void ScanIndirectStore( StackSlot valueToStore = PopUnknown(currentStack, 1, methodBody, offset); StackSlot destination = PopUnknown(currentStack, 1, methodBody, offset); - StoreInReference(destination.Value, valueToStore.Value, methodBody, offset, locals, curBasicBlock, ref ipState); + StoreInReference(destination.Value, valueToStore.Value, methodBody, offset, locals, curBasicBlock, ref ipState, null); } /// @@ -1018,7 +1018,7 @@ private void ScanIndirectStore( /// The method body that contains the operation causing the store /// The instruction offset causing the store /// Throws if is not a valid target for an indirect store. - protected void StoreInReference(MultiValue target, MultiValue source, MethodIL method, int offset, ValueBasicBlockPair?[] locals, int curBasicBlock, ref InterproceduralState ipState) + protected void StoreInReference(MultiValue target, MultiValue source, MethodIL method, int offset, ValueBasicBlockPair?[] locals, int curBasicBlock, ref InterproceduralState ipState, int? parameterIndex) { foreach (var value in target.AsEnumerable ()) { @@ -1029,18 +1029,18 @@ protected void StoreInReference(MultiValue target, MultiValue source, MethodIL m break; case FieldReferenceValue fieldReference when HandleGetField(method, offset, fieldReference.FieldDefinition).AsSingleValue() is FieldValue fieldValue: - HandleStoreField(method, offset, fieldValue, source); + HandleStoreField(method, offset, fieldValue, source, parameterIndex); break; case ParameterReferenceValue parameterReference when GetMethodParameterValue(parameterReference.Parameter) is MethodParameterValue parameterValue: - HandleStoreParameter(method, offset, parameterValue, source); + HandleStoreParameter(method, offset, parameterValue, source, parameterIndex); break; case MethodReturnValue methodReturnValue: // 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 HandleReturnValue(method, offset, methodReturnValue, source); break; case FieldValue fieldValue: - HandleStoreField(method, offset, fieldValue, DereferenceValue(method, offset, source, locals, ref ipState)); + HandleStoreField(method, offset, fieldValue, DereferenceValue(method, offset, source, locals, ref ipState), parameterIndex); break; case IValueWithStaticType valueWithStaticType: if (valueWithStaticType.StaticType is not null && FlowAnnotations.IsTypeInterestingForDataflow(valueWithStaticType.StaticType.Value.Type)) @@ -1108,11 +1108,11 @@ private void ScanLdfld( currentStack.Push(new StackSlot(value)); } - protected virtual void HandleStoreField(MethodIL method, int offset, FieldValue field, MultiValue valueToStore) + protected virtual void HandleStoreField(MethodIL method, int offset, FieldValue field, MultiValue valueToStore, int? parameterIndex) { } - protected virtual void HandleStoreParameter(MethodIL method, int offset, MethodParameterValue parameter, MultiValue valueToStore) + protected virtual void HandleStoreParameter(MethodIL method, int offset, MethodParameterValue parameter, MultiValue valueToStore, int? parameterIndex) { } @@ -1149,7 +1149,7 @@ private void ScanStfld( // Incomplete handling of ref fields -- if we're storing a reference to a value, pretend it's just the value MultiValue valueToStore = DereferenceValue(methodBody, offset, valueToStoreSlot.Value, locals, ref interproceduralState); - HandleStoreField(methodBody, offset, fieldValue, valueToStore); + HandleStoreField(methodBody, offset, fieldValue, valueToStore, null); } } @@ -1241,7 +1241,7 @@ protected void AssignRefAndOutParameters( if (parameter.GetReferenceKind() is not (ReferenceKind.Ref or ReferenceKind.Out)) continue; var newByRefValue = _annotations.GetMethodParameterValue(parameter); - StoreInReference(methodArguments[(int)parameter.Index], newByRefValue, callingMethodBody, offset, locals, curBasicBlock, ref ipState); + StoreInReference(methodArguments[(int)parameter.Index], newByRefValue, callingMethodBody, offset, locals, curBasicBlock, ref ipState, parameter.Index.Index); } } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs index 8a057741814cad..d79389acd3f413 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs @@ -208,23 +208,23 @@ protected override MultiValue HandleGetField(MethodIL methodBody, int offset, Fi return _annotations.GetFieldValue(field); } - private void HandleStoreValueWithDynamicallyAccessedMembers(MethodIL methodBody, int offset, ValueWithDynamicallyAccessedMembers targetValue, MultiValue sourceValue, string reason) + private void HandleStoreValueWithDynamicallyAccessedMembers(MethodIL methodBody, int offset, ValueWithDynamicallyAccessedMembers targetValue, MultiValue sourceValue, int? parameterIndex, string reason) { if (targetValue.DynamicallyAccessedMemberTypes != 0) { _origin = _origin.WithInstructionOffset(methodBody, offset); - TrimAnalysisPatterns.Add(new TrimAnalysisAssignmentPattern(sourceValue, targetValue, _origin, reason)); + TrimAnalysisPatterns.Add(new TrimAnalysisAssignmentPattern(sourceValue, targetValue, _origin, parameterIndex, reason)); } } - protected override void HandleStoreField(MethodIL methodBody, int offset, FieldValue field, MultiValue valueToStore) - => HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, field, valueToStore, field.Field.GetDisplayName()); + protected override void HandleStoreField(MethodIL methodBody, int offset, FieldValue field, MultiValue valueToStore, int? parameterIndex) + => HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, field, valueToStore, parameterIndex, field.Field.GetDisplayName()); - protected override void HandleStoreParameter(MethodIL methodBody, int offset, MethodParameterValue parameter, MultiValue valueToStore) - => HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, parameter, valueToStore, parameter.Parameter.Method.GetDisplayName()); + protected override void HandleStoreParameter(MethodIL methodBody, int offset, MethodParameterValue parameter, MultiValue valueToStore, int? parameterIndex) + => HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, parameter, valueToStore, parameterIndex, parameter.Parameter.Method.GetDisplayName()); protected override void HandleReturnValue(MethodIL methodBody, int offset, MethodReturnValue returnValue, MultiValue valueToStore) - => HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, returnValue, valueToStore, returnValue.Method.GetDisplayName()); + => HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, returnValue, valueToStore, null, returnValue.Method.GetDisplayName()); protected override void HandleTypeTokenAccess(MethodIL methodBody, int offset, TypeDesc accessedType) { diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs index afb09e131a31de..40466c7edef60a 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs @@ -18,24 +18,28 @@ public readonly record struct TrimAnalysisAssignmentPattern public MultiValue Source { get; init; } public MultiValue Target { get; init; } public MessageOrigin Origin { get; init; } + public int? ParameterIndex { get; init; } internal string Reason { get; init; } - internal TrimAnalysisAssignmentPattern(MultiValue source, MultiValue target, MessageOrigin origin, string reason) + internal TrimAnalysisAssignmentPattern(MultiValue source, MultiValue target, MessageOrigin origin, int? parameterIndex, string reason) { Source = source.DeepCopy(); Target = target.DeepCopy(); Origin = origin; + ParameterIndex = parameterIndex; Reason = reason; } public TrimAnalysisAssignmentPattern Merge(ValueSetLattice lattice, TrimAnalysisAssignmentPattern other) { Debug.Assert(Origin == other.Origin); + Debug.Assert(ParameterIndex == other.ParameterIndex); return new TrimAnalysisAssignmentPattern( lattice.Meet(Source, other.Source), lattice.Meet(Target, other.Target), Origin, + ParameterIndex, Reason); } diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisPatternStore.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisPatternStore.cs index 76bc92cf80e4c7..9293fdd7e87b61 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisPatternStore.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisPatternStore.cs @@ -13,7 +13,7 @@ namespace ILCompiler.Dataflow { public readonly struct TrimAnalysisPatternStore { - private readonly Dictionary AssignmentPatterns; + private readonly Dictionary<(MessageOrigin, int?), TrimAnalysisAssignmentPattern> AssignmentPatterns; private readonly Dictionary MethodCallPatterns; private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisTokenAccessPattern> TokenAccessPatterns; private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern> GenericInstantiations; @@ -23,7 +23,7 @@ public readonly struct TrimAnalysisPatternStore public TrimAnalysisPatternStore(ValueSetLattice lattice, Logger logger) { - AssignmentPatterns = new Dictionary(); + AssignmentPatterns = new Dictionary<(MessageOrigin, int?), TrimAnalysisAssignmentPattern>(); MethodCallPatterns = new Dictionary(); TokenAccessPatterns = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisTokenAccessPattern>(); GenericInstantiations = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern>(); @@ -34,13 +34,14 @@ public TrimAnalysisPatternStore(ValueSetLattice lattice, Logger log public void Add(TrimAnalysisAssignmentPattern pattern) { - if (!AssignmentPatterns.TryGetValue(pattern.Origin, out var existingPattern)) + var key = (pattern.Origin, pattern.ParameterIndex); + if (!AssignmentPatterns.TryGetValue(key, out var existingPattern)) { - AssignmentPatterns.Add(pattern.Origin, pattern); + AssignmentPatterns.Add(key, pattern); return; } - AssignmentPatterns[pattern.Origin] = pattern.Merge(Lattice, existingPattern); + AssignmentPatterns[key] = pattern.Merge(Lattice, existingPattern); } public void Add(TrimAnalysisMethodCallPattern pattern) From 2ee0ffe6c9fd7b35ad053f13cda8d91aff5c5130 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Mon, 29 Jul 2024 16:30:56 -0700 Subject: [PATCH 7/8] Update test --- .../test/Mono.Linker.Tests.Cases/DataFlow/RefFieldDataFlow.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/RefFieldDataFlow.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/RefFieldDataFlow.cs index 94db35301c6b7c..65b37ce09aeb16 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/RefFieldDataFlow.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/RefFieldDataFlow.cs @@ -13,10 +13,6 @@ namespace Mono.Linker.Tests.Cases.DataFlow class RefFieldDataFlow { [Kept] - [UnexpectedWarning ("IL2069", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/85464")] - [UnexpectedWarning ("IL2069", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/85464")] - [UnexpectedWarning ("IL2069", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/85464")] - [UnexpectedWarning ("IL2069", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/85464")] public static void Main () { RefFieldWithMethods withMethods = new (ref fieldWithMethods); From f7536b35a81267d122287a3f73b43531531bdcfe Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Wed, 7 Aug 2024 21:44:09 +0000 Subject: [PATCH 8/8] Cleanup --- .../Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs | 3 +++ .../linker/Linker.Dataflow/TrimAnalysisAssignmentPattern.cs | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs index 40466c7edef60a..2f44acb76c90b6 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs @@ -18,6 +18,9 @@ public readonly record struct TrimAnalysisAssignmentPattern public MultiValue Source { get; init; } public MultiValue Target { get; init; } public MessageOrigin Origin { get; init; } + + // For assignment of a method parameter, we store the parameter index to disambiguate + // assignments from different out parameters of a single method call. public int? ParameterIndex { get; init; } internal string Reason { get; init; } diff --git a/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisAssignmentPattern.cs b/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisAssignmentPattern.cs index dedf5769c3b4e5..2d76af158a09c2 100644 --- a/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisAssignmentPattern.cs +++ b/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisAssignmentPattern.cs @@ -16,7 +16,7 @@ public readonly record struct TrimAnalysisAssignmentPattern public MessageOrigin Origin { get; init; } // For assignment of a method parameter, we store the parameter index to disambiguate - // assignments from different out parameters ouf a single method call. + // assignments from different out parameters of a single method call. public int? ParameterIndex { get; init; } public TrimAnalysisAssignmentPattern (MultiValue source, MultiValue target, MessageOrigin origin, int? parameterIndex)