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

Don't ignore the case when dataflow doesn't know what's going on #101031

Merged
merged 1 commit into from
Apr 16, 2024
Merged

Don't ignore the case when dataflow doesn't know what's going on #101031

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,12 @@ public static bool HandleCall(
if (Intrinsics.GetIntrinsicIdForMethod(callingMethodDefinition) == IntrinsicId.RuntimeReflectionExtensions_GetMethodInfo)
break;

if (param.IsEmpty())
{
// The static value is unknown and the below `foreach` won't execute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the value unknown in my repro?

I would expect that the dataflow should be able to trace through new Action(Test<string>).Method without problems.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Known limitation of the dataflow analysis (#93720 is the tracking issue).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought #93720 was about not tracking variable types for GetType - how does that explain this issue? If we don't trace through new Action(Test<string>), I would have expected it to show up as an unknown value, not empty.

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought #93720 was about not tracking variable types for GetType - how does that explain this issue? If we don't trace through new Action(Test<string>), I would have expected it to show up as an unknown value, not empty.

I thought unknown/empty are interchangeable but now I understand the difference. We eventually want to know the type allocated through the new Action so that GetType (or in this case Delegate.get_Method) know the type it's operating on.

In this case, we end up with empty instead of unknown because constructors take this path (they "return void"):

runtime/src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs

Lines 1168 to 1170 in 3d0da2c

// For now, if the intrinsic doesn't set a return value, fall back on the annotations.
// Note that this will be DynamicallyAccessedMembers.None for the intrinsics which don't return types.
returnValue ??= calledMethod.ReturnsVoid () ? MultiValueLattice.Top : annotatedMethodReturnValue;

And we can no longer bash it to unknown here because the HandleCallAction returned true (i.e. handledFunction):

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

Lines 1302 to 1319 in 3d0da2c

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, that looks like something we should fix (not that it'll help with the issue this PR is addressing). Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We eventually want to know the type allocated through the new Action so that GetType (or in this case Delegate.get_Method) know the type it's operating on.

To fix tracking of new Action(Test<string>), I think we'd need to introduce a new dataflow value for Action that tracks the ldftn result, and handle that case in the get_Method intrinsic. It seems like just knowing that the input has type Action doesn't solve the problem, so I'm not sure #93720 is the right tracking issue. I think that one is more specific to GetType handling.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to know what method the Action points to, just the concrete type the Delegate.get_Method method is called on (System.Action in this case).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing something - why don't we need to know the method it points to? Is it because we keep metadata for all delegate targets if there's any call to get_Method? I was assuming we'd ideally only keep metadata for those delegate targets that had a get_Method call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler keeps track of delegate type + method. If we know delegate type, we know all the methods it can be used with.

We don't track this at method granularity because doing new Action(Foo).Method is very rare. People only do this if they want to work around lack of methodof in C#. What happens 95% of time is that we construct a delegate somewhere and a different part of the program will call Delegate.get_Method on it. That's why we only care about delegate type.

Dataflow losing track of the type in the trivial new Action(Foo).Method pattern is throwing a wrench in this because if someone uses this to work around methodof, we only see Delegate.get_Method was called on some unknown thing and we need to disable the optimization globally because this could be any delegate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thank you!

reflectionMarker.Dependencies.Add(reflectionMarker.Factory.ReflectedDelegate(null), "Delegate.Method access on unknown delegate type");
}

foreach (var valueNode in param.AsEnumerable())
{
TypeDesc? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
Expand Down
Loading