Skip to content

Commit c199b9a

Browse files
sbomermatouskozak
authored andcommitted
Share return value handling in trim analysis (dotnet#101398)
This shares more of the HandleCall logic across ILLink/ILC/ILLink.RoslynAnalyzer. The return value handling has been moved from each tool's implementation (ReflectionMethodBodyScanner/TrimAnalysisVisitor) into the shared HandleCallAction, and the extra handling in MethodBodyScanner has been removed. * Fix Array_CreateInstance case The PInvoke logic and the CheckAndReportRequires logic should not be needed for this intrinsic, and the old shared HandleCallAction would go to the default case that sets the return value to Top and returns false. Now this instead returns true and lets the shared return value logic kick in.
1 parent 7d1cf35 commit c199b9a

File tree

12 files changed

+912
-1019
lines changed

12 files changed

+912
-1019
lines changed

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs

+556
Large diffs are not rendered by default.

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

+7-35
Original file line numberDiff line numberDiff line change
@@ -1156,11 +1156,8 @@ private ValueNodeList PopCallArguments(
11561156
Stack<StackSlot> currentStack,
11571157
MethodDesc methodCalled,
11581158
MethodIL containingMethodBody,
1159-
bool isNewObj, int ilOffset,
1160-
out SingleValue? newObjValue)
1159+
bool isNewObj, int ilOffset)
11611160
{
1162-
newObjValue = null;
1163-
11641161
int countToPop = 0;
11651162
if (!isNewObj && !methodCalled.Signature.IsStatic)
11661163
countToPop++;
@@ -1175,8 +1172,7 @@ private ValueNodeList PopCallArguments(
11751172

11761173
if (isNewObj)
11771174
{
1178-
newObjValue = UnknownValue.Instance;
1179-
methodParams.Add(newObjValue);
1175+
methodParams.Add(UnknownValue.Instance);
11801176
}
11811177
methodParams.Reverse();
11821178
return methodParams;
@@ -1275,9 +1271,7 @@ private void HandleCall(
12751271
{
12761272
bool isNewObj = opcode == ILOpcode.newobj;
12771273

1278-
SingleValue? newObjValue;
1279-
ValueNodeList methodArguments = PopCallArguments(currentStack, calledMethod, callingMethodBody, isNewObj,
1280-
offset, out newObjValue);
1274+
ValueNodeList methodArguments = PopCallArguments(currentStack, calledMethod, callingMethodBody, isNewObj, offset);
12811275

12821276
// Multi-dimensional array access is represented as a call to a special Get method on the array (runtime provided method)
12831277
// We don't track multi-dimensional arrays in any way, so return unknown value.
@@ -1290,33 +1284,12 @@ private void HandleCall(
12901284
var dereferencedMethodParams = new List<MultiValue>();
12911285
foreach (var argument in methodArguments)
12921286
dereferencedMethodParams.Add(DereferenceValue(callingMethodBody, offset, argument, locals, ref interproceduralState));
1293-
MultiValue methodReturnValue;
1294-
bool handledFunction = HandleCall(
1287+
MultiValue methodReturnValue = HandleCall(
12951288
callingMethodBody,
12961289
calledMethod,
12971290
opcode,
12981291
offset,
1299-
new ValueNodeList(dereferencedMethodParams),
1300-
out methodReturnValue);
1301-
1302-
// Handle the return value or newobj result
1303-
if (!handledFunction)
1304-
{
1305-
if (isNewObj)
1306-
{
1307-
if (newObjValue == null)
1308-
methodReturnValue = UnknownValue.Instance;
1309-
else
1310-
methodReturnValue = newObjValue;
1311-
}
1312-
else
1313-
{
1314-
if (!calledMethod.Signature.ReturnType.IsVoid)
1315-
{
1316-
methodReturnValue = UnknownValue.Instance;
1317-
}
1318-
}
1319-
}
1292+
new ValueNodeList(dereferencedMethodParams));
13201293

13211294
if (isNewObj || !calledMethod.Signature.ReturnType.IsVoid)
13221295
currentStack.Push(new StackSlot(methodReturnValue));
@@ -1335,13 +1308,12 @@ private void HandleCall(
13351308
}
13361309
}
13371310

1338-
public abstract bool HandleCall(
1311+
public abstract MultiValue HandleCall(
13391312
MethodIL callingMethodBody,
13401313
MethodDesc calledMethod,
13411314
ILOpcode operation,
13421315
int offset,
1343-
ValueNodeList methodParams,
1344-
out MultiValue methodReturnValue);
1316+
ValueNodeList methodParams);
13451317

13461318
// Limit tracking array values to 32 values for performance reasons. There are many arrays much longer than 32 elements in .NET, but the interesting ones for trimming are nearly always less than 32 elements.
13471319
private const int MaxTrackedArrayValues = 32;

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

+9-622
Large diffs are not rendered by default.

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisMethodCallPattern.cs

+1-2
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,7 @@ public void MarkAndProduceDiagnostics(ReflectionMarker reflectionMarker, Logger
8686
logger);
8787
ReflectionMethodBodyScanner.HandleCall(MethodBody, CalledMethod, Operation, Instance, Arguments,
8888
diagnosticContext,
89-
reflectionMarker,
90-
out MultiValue _);
89+
reflectionMarker);
9190
}
9291
}
9392
}

src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs

+113-1
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,11 @@
77
using ILLink.RoslynAnalyzer;
88
using ILLink.RoslynAnalyzer.DataFlow;
99
using ILLink.RoslynAnalyzer.TrimAnalysis;
10+
using ILLink.Shared.DataFlow;
1011
using ILLink.Shared.TypeSystemProxy;
1112
using Microsoft.CodeAnalysis;
13+
using Microsoft.CodeAnalysis.Operations;
14+
using MultiValue = ILLink.Shared.DataFlow.ValueSet<ILLink.Shared.DataFlow.SingleValue>;
1215

1316
namespace ILLink.Shared.TrimAnalysis
1417
{
@@ -20,15 +23,124 @@ internal partial struct HandleCallAction
2023
readonly ISymbol _owningSymbol;
2124
readonly IOperation _operation;
2225
readonly ReflectionAccessAnalyzer _reflectionAccessAnalyzer;
26+
ValueSetLattice<SingleValue> _multiValueLattice;
2327

24-
public HandleCallAction (in DiagnosticContext diagnosticContext, ISymbol owningSymbol, IOperation operation)
28+
public HandleCallAction (
29+
in DiagnosticContext diagnosticContext,
30+
ISymbol owningSymbol,
31+
IOperation operation,
32+
ValueSetLattice<SingleValue> multiValueLattice)
2533
{
2634
_owningSymbol = owningSymbol;
2735
_operation = operation;
2836
_diagnosticContext = diagnosticContext;
2937
_annotations = FlowAnnotations.Instance;
3038
_reflectionAccessAnalyzer = default;
3139
_requireDynamicallyAccessedMembersAction = new (diagnosticContext, _reflectionAccessAnalyzer);
40+
_multiValueLattice = multiValueLattice;
41+
}
42+
43+
private partial bool TryHandleIntrinsic (
44+
MethodProxy calledMethod,
45+
MultiValue instanceValue,
46+
IReadOnlyList<MultiValue> argumentValues,
47+
IntrinsicId intrinsicId,
48+
out MultiValue? methodReturnValue)
49+
{
50+
MultiValue? maybeMethodReturnValue = methodReturnValue = null;
51+
ValueSetLattice<SingleValue> multiValueLattice = _multiValueLattice;
52+
53+
switch (intrinsicId) {
54+
case IntrinsicId.Array_Empty:
55+
AddReturnValue (ArrayValue.Create (0));
56+
break;
57+
58+
case IntrinsicId.TypeDelegator_Ctor:
59+
if (_operation is IObjectCreationOperation)
60+
AddReturnValue (argumentValues[0]);
61+
62+
break;
63+
64+
case IntrinsicId.Object_GetType: {
65+
foreach (var valueNode in instanceValue.AsEnumerable ()) {
66+
// Note that valueNode can be statically typed as some generic argument type.
67+
// For example:
68+
// void Method<T>(T instance) { instance.GetType().... }
69+
// But it could be that T is annotated with for example PublicMethods:
70+
// void Method<[DAM(PublicMethods)] T>(T instance) { instance.GetType().GetMethod("Test"); }
71+
// In this case it's in theory possible to handle it, by treating the T basically as a base class
72+
// for the actual type of "instance". But the analysis for this would be pretty complicated (as the marking
73+
// has to happen on the callsite, which doesn't know that GetType() will be used...).
74+
// For now we're intentionally ignoring this case - it will produce a warning.
75+
// The counter example is:
76+
// Method<Base>(new Derived);
77+
// In this case to get correct results, trimmer would have to mark all public methods on Derived. Which
78+
// currently it won't do.
79+
80+
// To emulate IL tools behavior (trimmer, NativeAOT compiler), we're going to intentionally "forget" the static type
81+
// if it is a generic argument type.
82+
83+
ITypeSymbol? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
84+
if (staticType?.TypeKind == TypeKind.TypeParameter)
85+
staticType = null;
86+
87+
if (staticType is null) {
88+
// We don't know anything about the type GetType was called on. Track this as a usual "result of a method call without any annotations"
89+
AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod));
90+
} else if (staticType.IsSealed || staticType.IsTypeOf ("System", "Delegate") || staticType.TypeKind == TypeKind.Array) {
91+
// We can treat this one the same as if it was a typeof() expression
92+
93+
// We can allow Object.GetType to be modeled as System.Delegate because we keep all methods
94+
// on delegates anyway so reflection on something this approximation would miss is actually safe.
95+
96+
// We can also treat all arrays as "sealed" since it's not legal to derive from Array type (even though it is not sealed itself)
97+
98+
// We ignore the fact that the type can be annotated (see below for handling of annotated types)
99+
// This means the annotations (if any) won't be applied - instead we rely on the exact knowledge
100+
// of the type. So for example even if the type is annotated with PublicMethods
101+
// but the code calls GetProperties on it - it will work - mark properties, don't mark methods
102+
// since we ignored the fact that it's annotated.
103+
// This can be seen a little bit as a violation of the annotation, but we already have similar cases
104+
// where a parameter is annotated and if something in the method sets a specific known type to it
105+
// we will also make it just work, even if the annotation doesn't match the usage.
106+
AddReturnValue (new SystemTypeValue (new (staticType)));
107+
} else {
108+
var annotation = FlowAnnotations.GetTypeAnnotation (staticType);
109+
AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod, annotation));
110+
}
111+
}
112+
break;
113+
}
114+
115+
// Some intrinsics are unimplemented by the analyzer.
116+
// These will fall back to the usual return-value handling.
117+
case IntrinsicId.Array_CreateInstance:
118+
case IntrinsicId.Assembly_GetFile:
119+
case IntrinsicId.Assembly_GetFiles:
120+
case IntrinsicId.AssemblyName_get_EscapedCodeBase:
121+
case IntrinsicId.Assembly_get_Location:
122+
case IntrinsicId.AssemblyName_get_CodeBase:
123+
case IntrinsicId.Delegate_get_Method:
124+
case IntrinsicId.Enum_GetValues:
125+
case IntrinsicId.Marshal_DestroyStructure:
126+
case IntrinsicId.Marshal_GetDelegateForFunctionPointer:
127+
case IntrinsicId.Marshal_OffsetOf:
128+
case IntrinsicId.Marshal_PtrToStructure:
129+
case IntrinsicId.Marshal_SizeOf:
130+
case IntrinsicId.RuntimeReflectionExtensions_GetMethodInfo:
131+
break;
132+
133+
default:
134+
return false;
135+
}
136+
137+
methodReturnValue = maybeMethodReturnValue;
138+
return true;
139+
140+
void AddReturnValue (MultiValue value)
141+
{
142+
maybeMethodReturnValue = (maybeMethodReturnValue is null) ? value : multiValueLattice.Meet ((MultiValue) maybeMethodReturnValue, value);
143+
}
32144
}
33145

34146
private partial IEnumerable<SystemReflectionMethodBaseValue> GetMethodsOnTypeHierarchy (TypeProxy type, string name, BindingFlags? bindingFlags)

src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs

+7-83
Original file line numberDiff line numberDiff line change
@@ -321,92 +321,16 @@ internal static void HandleCall(
321321
ValueSetLattice<SingleValue> multiValueLattice,
322322
out MultiValue methodReturnValue)
323323
{
324-
var handleCallAction = new HandleCallAction (diagnosticContext, owningSymbol, operation);
324+
var handleCallAction = new HandleCallAction (diagnosticContext, owningSymbol, operation, multiValueLattice);
325325
MethodProxy method = new (calledMethod);
326326
var intrinsicId = Intrinsics.GetIntrinsicIdForMethod (method);
327+
if (!handleCallAction.Invoke (method, instance, arguments, intrinsicId, out methodReturnValue))
328+
UnhandledIntrinsicHelper (intrinsicId);
327329

328-
if (handleCallAction.Invoke (method, instance, arguments, intrinsicId, out methodReturnValue)) {
329-
return;
330-
}
331-
332-
MultiValue? maybeMethodReturnValue = default;
333-
334-
switch (intrinsicId) {
335-
case IntrinsicId.Array_Empty:
336-
AddReturnValue (ArrayValue.Create (0));
337-
break;
338-
339-
case IntrinsicId.TypeDelegator_Ctor:
340-
if (operation is IObjectCreationOperation)
341-
AddReturnValue (arguments[0]);
342-
343-
break;
344-
345-
case IntrinsicId.Object_GetType: {
346-
foreach (var valueNode in instance.AsEnumerable ()) {
347-
// Note that valueNode can be statically typed as some generic argument type.
348-
// For example:
349-
// void Method<T>(T instance) { instance.GetType().... }
350-
// But it could be that T is annotated with for example PublicMethods:
351-
// void Method<[DAM(PublicMethods)] T>(T instance) { instance.GetType().GetMethod("Test"); }
352-
// In this case it's in theory possible to handle it, by treating the T basically as a base class
353-
// for the actual type of "instance". But the analysis for this would be pretty complicated (as the marking
354-
// has to happen on the callsite, which doesn't know that GetType() will be used...).
355-
// For now we're intentionally ignoring this case - it will produce a warning.
356-
// The counter example is:
357-
// Method<Base>(new Derived);
358-
// In this case to get correct results, trimmer would have to mark all public methods on Derived. Which
359-
// currently it won't do.
360-
361-
// To emulate IL tools behavior (trimmer, NativeAOT compiler), we're going to intentionally "forget" the static type
362-
// if it is a generic argument type.
363-
364-
ITypeSymbol? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
365-
if (staticType?.TypeKind == TypeKind.TypeParameter)
366-
staticType = null;
367-
368-
if (staticType is null) {
369-
// We don't know anything about the type GetType was called on. Track this as a usual "result of a method call without any annotations"
370-
AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (new (calledMethod)));
371-
} else if (staticType.IsSealed || staticType.IsTypeOf ("System", "Delegate") || staticType.TypeKind == TypeKind.Array) {
372-
// We can treat this one the same as if it was a typeof() expression
373-
374-
// We can allow Object.GetType to be modeled as System.Delegate because we keep all methods
375-
// on delegates anyway so reflection on something this approximation would miss is actually safe.
376-
377-
// We can also treat all arrays as "sealed" since it's not legal to derive from Array type (even though it is not sealed itself)
378-
379-
// We ignore the fact that the type can be annotated (see below for handling of annotated types)
380-
// This means the annotations (if any) won't be applied - instead we rely on the exact knowledge
381-
// of the type. So for example even if the type is annotated with PublicMethods
382-
// but the code calls GetProperties on it - it will work - mark properties, don't mark methods
383-
// since we ignored the fact that it's annotated.
384-
// This can be seen a little bit as a violation of the annotation, but we already have similar cases
385-
// where a parameter is annotated and if something in the method sets a specific known type to it
386-
// we will also make it just work, even if the annotation doesn't match the usage.
387-
AddReturnValue (new SystemTypeValue (new (staticType)));
388-
} else {
389-
var annotation = FlowAnnotations.GetTypeAnnotation (staticType);
390-
AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (new (calledMethod), annotation));
391-
}
392-
}
393-
}
394-
395-
break;
396-
397-
default:
398-
Debug.Fail ($"Unexpected method {calledMethod.GetDisplayName ()} unhandled by HandleCallAction.");
399-
400-
// Do nothing even if we reach a point which we didn't expect - the analyzer should never crash as it's a too disruptive experience for the user.
401-
break;
402-
}
403-
404-
methodReturnValue = maybeMethodReturnValue ?? multiValueLattice.Top;
405-
406-
void AddReturnValue (MultiValue value)
407-
{
408-
maybeMethodReturnValue = (maybeMethodReturnValue is null) ? value : multiValueLattice.Meet ((MultiValue) maybeMethodReturnValue, value);
409-
}
330+
// Avoid crashing the analyzer in release builds
331+
[Conditional ("DEBUG")]
332+
static void UnhandledIntrinsicHelper (IntrinsicId intrinsicId)
333+
=> throw new NotImplementedException ($"Unhandled intrinsic: {intrinsicId}");
410334
}
411335

412336
public override void HandleReturnValue (MultiValue returnValue, IOperation operation, in FeatureContext featureContext)

0 commit comments

Comments
 (0)