Skip to content

Commit

Permalink
Share return value handling in trim analysis (#101398)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@sbomer @pull
sbomer authored and pull[bot] committed Jun 20, 2024
1 parent f0f26f1 commit 2727214
Show file tree
Hide file tree
Showing 12 changed files with 912 additions and 1,019 deletions.
556 changes: 556 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs
View file

Large diffs are not rendered by default.

42 changes: 7 additions & 35 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1156,11 +1156,8 @@ private ValueNodeList PopCallArguments(
Stack<StackSlot> currentStack,
MethodDesc methodCalled,
MethodIL containingMethodBody,
bool isNewObj, int ilOffset,
out SingleValue? newObjValue)
bool isNewObj, int ilOffset)
{
newObjValue = null;

int countToPop = 0;
if (!isNewObj && !methodCalled.Signature.IsStatic)
countToPop++;
Expand All @@ -1175,8 +1172,7 @@ private ValueNodeList PopCallArguments(

if (isNewObj)
{
newObjValue = UnknownValue.Instance;
methodParams.Add(newObjValue);
methodParams.Add(UnknownValue.Instance);
}
methodParams.Reverse();
return methodParams;
Expand Down Expand Up @@ -1275,9 +1271,7 @@ private void HandleCall(
{
bool isNewObj = opcode == ILOpcode.newobj;

SingleValue? newObjValue;
ValueNodeList methodArguments = PopCallArguments(currentStack, calledMethod, callingMethodBody, isNewObj,
offset, out newObjValue);
ValueNodeList methodArguments = PopCallArguments(currentStack, calledMethod, callingMethodBody, isNewObj, offset);

// Multi-dimensional array access is represented as a call to a special Get method on the array (runtime provided method)
// We don't track multi-dimensional arrays in any way, so return unknown value.
Expand All @@ -1290,33 +1284,12 @@ private void HandleCall(
var dereferencedMethodParams = new List<MultiValue>();
foreach (var argument in methodArguments)
dereferencedMethodParams.Add(DereferenceValue(callingMethodBody, offset, argument, locals, ref interproceduralState));
MultiValue methodReturnValue;
bool handledFunction = HandleCall(
MultiValue methodReturnValue = HandleCall(
callingMethodBody,
calledMethod,
opcode,
offset,
new ValueNodeList(dereferencedMethodParams),
out methodReturnValue);

// Handle the return value or newobj result
if (!handledFunction)
{
if (isNewObj)
{
if (newObjValue == null)
methodReturnValue = UnknownValue.Instance;
else
methodReturnValue = newObjValue;
}
else
{
if (!calledMethod.Signature.ReturnType.IsVoid)
{
methodReturnValue = UnknownValue.Instance;
}
}
}
new ValueNodeList(dereferencedMethodParams));

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

public abstract bool HandleCall(
public abstract MultiValue HandleCall(
MethodIL callingMethodBody,
MethodDesc calledMethod,
ILOpcode operation,
int offset,
ValueNodeList methodParams,
out MultiValue methodReturnValue);
ValueNodeList methodParams);

// 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.
private const int MaxTrackedArrayValues = 32;
Expand Down
631 changes: 9 additions & 622 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
View file

Large diffs are not rendered by default.

3 changes: 1 addition & 2 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisMethodCallPattern.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ public void MarkAndProduceDiagnostics(ReflectionMarker reflectionMarker, Logger
logger);
ReflectionMethodBodyScanner.HandleCall(MethodBody, CalledMethod, Operation, Instance, Arguments,
diagnosticContext,
reflectionMarker,
out MultiValue _);
reflectionMarker);
}
}
}
114 changes: 113 additions & 1 deletion src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@
using ILLink.RoslynAnalyzer;
using ILLink.RoslynAnalyzer.DataFlow;
using ILLink.RoslynAnalyzer.TrimAnalysis;
using ILLink.Shared.DataFlow;
using ILLink.Shared.TypeSystemProxy;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Operations;
using MultiValue = ILLink.Shared.DataFlow.ValueSet<ILLink.Shared.DataFlow.SingleValue>;

namespace ILLink.Shared.TrimAnalysis
{
Expand All @@ -20,15 +23,124 @@ internal partial struct HandleCallAction
readonly ISymbol _owningSymbol;
readonly IOperation _operation;
readonly ReflectionAccessAnalyzer _reflectionAccessAnalyzer;
ValueSetLattice<SingleValue> _multiValueLattice;

public HandleCallAction (in DiagnosticContext diagnosticContext, ISymbol owningSymbol, IOperation operation)
public HandleCallAction (
in DiagnosticContext diagnosticContext,
ISymbol owningSymbol,
IOperation operation,
ValueSetLattice<SingleValue> multiValueLattice)
{
_owningSymbol = owningSymbol;
_operation = operation;
_diagnosticContext = diagnosticContext;
_annotations = FlowAnnotations.Instance;
_reflectionAccessAnalyzer = default;
_requireDynamicallyAccessedMembersAction = new (diagnosticContext, _reflectionAccessAnalyzer);
_multiValueLattice = multiValueLattice;
}

private partial bool TryHandleIntrinsic (
MethodProxy calledMethod,
MultiValue instanceValue,
IReadOnlyList<MultiValue> argumentValues,
IntrinsicId intrinsicId,
out MultiValue? methodReturnValue)
{
MultiValue? maybeMethodReturnValue = methodReturnValue = null;
ValueSetLattice<SingleValue> multiValueLattice = _multiValueLattice;

switch (intrinsicId) {
case IntrinsicId.Array_Empty:
AddReturnValue (ArrayValue.Create (0));
break;

case IntrinsicId.TypeDelegator_Ctor:
if (_operation is IObjectCreationOperation)
AddReturnValue (argumentValues[0]);

break;

case IntrinsicId.Object_GetType: {
foreach (var valueNode in instanceValue.AsEnumerable ()) {
// Note that valueNode can be statically typed as some generic argument type.
// For example:
// void Method<T>(T instance) { instance.GetType().... }
// But it could be that T is annotated with for example PublicMethods:
// void Method<[DAM(PublicMethods)] T>(T instance) { instance.GetType().GetMethod("Test"); }
// In this case it's in theory possible to handle it, by treating the T basically as a base class
// for the actual type of "instance". But the analysis for this would be pretty complicated (as the marking
// has to happen on the callsite, which doesn't know that GetType() will be used...).
// For now we're intentionally ignoring this case - it will produce a warning.
// The counter example is:
// Method<Base>(new Derived);
// In this case to get correct results, trimmer would have to mark all public methods on Derived. Which
// currently it won't do.

// To emulate IL tools behavior (trimmer, NativeAOT compiler), we're going to intentionally "forget" the static type
// if it is a generic argument type.

ITypeSymbol? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
if (staticType?.TypeKind == TypeKind.TypeParameter)
staticType = null;

if (staticType is null) {
// 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"
AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod));
} else if (staticType.IsSealed || staticType.IsTypeOf ("System", "Delegate") || staticType.TypeKind == TypeKind.Array) {
// We can treat this one the same as if it was a typeof() expression

// We can allow Object.GetType to be modeled as System.Delegate because we keep all methods
// on delegates anyway so reflection on something this approximation would miss is actually safe.

// 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)

// We ignore the fact that the type can be annotated (see below for handling of annotated types)
// This means the annotations (if any) won't be applied - instead we rely on the exact knowledge
// of the type. So for example even if the type is annotated with PublicMethods
// but the code calls GetProperties on it - it will work - mark properties, don't mark methods
// since we ignored the fact that it's annotated.
// This can be seen a little bit as a violation of the annotation, but we already have similar cases
// where a parameter is annotated and if something in the method sets a specific known type to it
// we will also make it just work, even if the annotation doesn't match the usage.
AddReturnValue (new SystemTypeValue (new (staticType)));
} else {
var annotation = FlowAnnotations.GetTypeAnnotation (staticType);
AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod, annotation));
}
}
break;
}

// Some intrinsics are unimplemented by the analyzer.
// These will fall back to the usual return-value handling.
case IntrinsicId.Array_CreateInstance:
case IntrinsicId.Assembly_GetFile:
case IntrinsicId.Assembly_GetFiles:
case IntrinsicId.AssemblyName_get_EscapedCodeBase:
case IntrinsicId.Assembly_get_Location:
case IntrinsicId.AssemblyName_get_CodeBase:
case IntrinsicId.Delegate_get_Method:
case IntrinsicId.Enum_GetValues:
case IntrinsicId.Marshal_DestroyStructure:
case IntrinsicId.Marshal_GetDelegateForFunctionPointer:
case IntrinsicId.Marshal_OffsetOf:
case IntrinsicId.Marshal_PtrToStructure:
case IntrinsicId.Marshal_SizeOf:
case IntrinsicId.RuntimeReflectionExtensions_GetMethodInfo:
break;

default:
return false;
}

methodReturnValue = maybeMethodReturnValue;
return true;

void AddReturnValue (MultiValue value)
{
maybeMethodReturnValue = (maybeMethodReturnValue is null) ? value : multiValueLattice.Meet ((MultiValue) maybeMethodReturnValue, value);
}
}

private partial IEnumerable<SystemReflectionMethodBaseValue> GetMethodsOnTypeHierarchy (TypeProxy type, string name, BindingFlags? bindingFlags)
Expand Down
90 changes: 7 additions & 83 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,92 +321,16 @@ internal static void HandleCall(
ValueSetLattice<SingleValue> multiValueLattice,
out MultiValue methodReturnValue)
{
var handleCallAction = new HandleCallAction (diagnosticContext, owningSymbol, operation);
var handleCallAction = new HandleCallAction (diagnosticContext, owningSymbol, operation, multiValueLattice);
MethodProxy method = new (calledMethod);
var intrinsicId = Intrinsics.GetIntrinsicIdForMethod (method);
if (!handleCallAction.Invoke (method, instance, arguments, intrinsicId, out methodReturnValue))
UnhandledIntrinsicHelper (intrinsicId);

if (handleCallAction.Invoke (method, instance, arguments, intrinsicId, out methodReturnValue)) {
return;
}

MultiValue? maybeMethodReturnValue = default;

switch (intrinsicId) {
case IntrinsicId.Array_Empty:
AddReturnValue (ArrayValue.Create (0));
break;

case IntrinsicId.TypeDelegator_Ctor:
if (operation is IObjectCreationOperation)
AddReturnValue (arguments[0]);

break;

case IntrinsicId.Object_GetType: {
foreach (var valueNode in instance.AsEnumerable ()) {
// Note that valueNode can be statically typed as some generic argument type.
// For example:
// void Method<T>(T instance) { instance.GetType().... }
// But it could be that T is annotated with for example PublicMethods:
// void Method<[DAM(PublicMethods)] T>(T instance) { instance.GetType().GetMethod("Test"); }
// In this case it's in theory possible to handle it, by treating the T basically as a base class
// for the actual type of "instance". But the analysis for this would be pretty complicated (as the marking
// has to happen on the callsite, which doesn't know that GetType() will be used...).
// For now we're intentionally ignoring this case - it will produce a warning.
// The counter example is:
// Method<Base>(new Derived);
// In this case to get correct results, trimmer would have to mark all public methods on Derived. Which
// currently it won't do.

// To emulate IL tools behavior (trimmer, NativeAOT compiler), we're going to intentionally "forget" the static type
// if it is a generic argument type.

ITypeSymbol? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
if (staticType?.TypeKind == TypeKind.TypeParameter)
staticType = null;

if (staticType is null) {
// 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"
AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (new (calledMethod)));
} else if (staticType.IsSealed || staticType.IsTypeOf ("System", "Delegate") || staticType.TypeKind == TypeKind.Array) {
// We can treat this one the same as if it was a typeof() expression

// We can allow Object.GetType to be modeled as System.Delegate because we keep all methods
// on delegates anyway so reflection on something this approximation would miss is actually safe.

// 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)

// We ignore the fact that the type can be annotated (see below for handling of annotated types)
// This means the annotations (if any) won't be applied - instead we rely on the exact knowledge
// of the type. So for example even if the type is annotated with PublicMethods
// but the code calls GetProperties on it - it will work - mark properties, don't mark methods
// since we ignored the fact that it's annotated.
// This can be seen a little bit as a violation of the annotation, but we already have similar cases
// where a parameter is annotated and if something in the method sets a specific known type to it
// we will also make it just work, even if the annotation doesn't match the usage.
AddReturnValue (new SystemTypeValue (new (staticType)));
} else {
var annotation = FlowAnnotations.GetTypeAnnotation (staticType);
AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (new (calledMethod), annotation));
}
}
}

break;

default:
Debug.Fail ($"Unexpected method {calledMethod.GetDisplayName ()} unhandled by HandleCallAction.");

// 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.
break;
}

methodReturnValue = maybeMethodReturnValue ?? multiValueLattice.Top;

void AddReturnValue (MultiValue value)
{
maybeMethodReturnValue = (maybeMethodReturnValue is null) ? value : multiValueLattice.Meet ((MultiValue) maybeMethodReturnValue, value);
}
// Avoid crashing the analyzer in release builds
[Conditional ("DEBUG")]
static void UnhandledIntrinsicHelper (IntrinsicId intrinsicId)
=> throw new NotImplementedException ($"Unhandled intrinsic: {intrinsicId}");
}

public override void HandleReturnValue (MultiValue returnValue, IOperation operation, in FeatureContext featureContext)
Expand Down
Loading

0 comments on commit 2727214

Please sign in to comment.