Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Share return value handling in trim analysis #101398

Merged
merged 7 commits into from
Apr 25, 2024
Merged

Share return value handling in trim analysis #101398

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2b64adc
Share logic for handling return values
sbomer Apr 22, 2024
56d1e24
Remove MethodBodyScanner return value handling
sbomer Apr 22, 2024
a3e3c88
Merge remote-tracking branch 'origin/main' into intrinsicsCleanup
sbomer Apr 24, 2024
3b8cc81
Avoid throwing from analyzer in Release builds
sbomer Apr 24, 2024
69017ea
Fix ILC callsites
sbomer Apr 24, 2024
b898b27
Fix Array_CreateInstance case
sbomer Apr 24, 2024
c450da6
PR feedback
sbomer Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
559 changes: 559 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs
View file
Open in desktop

Large diffs are not rendered by default.

35 changes: 5 additions & 30 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs
View file
Open in desktop
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 @@ -1291,33 +1285,14 @@ private void HandleCall(
foreach (var argument in methodArguments)
dereferencedMethodParams.Add(DereferenceValue(callingMethodBody, offset, argument, locals, ref interproceduralState));
MultiValue methodReturnValue;
bool handledFunction = HandleCall(
HandleCall(
sbomer marked this conversation as resolved.
Show resolved Hide resolved
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;
}
}
}

if (isNewObj || !calledMethod.Signature.ReturnType.IsVoid)
currentStack.Push(new StackSlot(methodReturnValue));

Expand All @@ -1335,7 +1310,7 @@ private void HandleCall(
}
}

public abstract bool HandleCall(
public abstract void HandleCall(
MethodIL callingMethodBody,
MethodDesc calledMethod,
ILOpcode operation,
Expand Down
625 changes: 6 additions & 619 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
View file
Open in desktop

Large diffs are not rendered by default.

106 changes: 105 additions & 1 deletion src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs
View file
Open in desktop
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,116 @@ 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. Analyzer should avoid crashing for these even though they are unimplemented.
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:
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
86 changes: 2 additions & 84 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,92 +321,10 @@ 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)) {
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);
}
handleCallAction.Invoke (method, instance, arguments, intrinsicId, out methodReturnValue);
}

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