ILLink should keep parameter names for methods which are used to create a delagate

[vitek-karas]

This is a fix for dotnet/linker#3061.

If the app creates a delegate over a method and later on access the Delegate.Method property it can get a MethodInfo which it can use to inspect some aspects of the method, specifically parameter names.

NativeAOT already handles this correctly, but ILLink doesn't and will trim the parameter names away. The correct behavior is to treat such method as reflection accessible (we really don't want to mark the Delegate.Method property as trim incompatible).

The ideal way to implement this is to perform full data flow on method addresses, which would allow us to detect which method is the delegate created for. (this is what NativeAOT ends up doing through JIT)

But that is rather complex, an easier solution is to treat any method which address is accessed as reflection enabled. This covers all delegates. It might mark more methods as reflection enabled, but it's very uncommon.

So this change does that, it will mark all methods accessed via ldftn (or similar) as reflection enabled.
Additionally this cleans up marking of reflection enabled members in the MarkStep. Specifically there was a distinction between reflection accessed and indirect accessed methods, but the latter only kicked in for the obsolete preserve attributes. It's better to reflection mark those as well. This might cause a small size regression, but it's very unlikely to be even observable.

This requires lot more test changes to enable sane testing:

• In the ILC test infra, track if method was kept with or without reflection metadata (as that is the difference in NativeAOT which will provide parameter names or not)
• Teach both the ILLink and ILC test infras how to handle compiler generated code with the Kept attribute validation
  • This is in no way complete, but it's an improvement
  • Mostly it means that we don't fail the test if a compiler generated code is kept without it being marked with the Kept attribute (as that's not always possible)
  • Note that we have some support for this for things like backing fields and private implementation details, but nothing which can handle state machines, and closure types for lambdas. These should now be workable, without the ability to verify that they were indeed kept (and how)
  • Finally adds tests verifying the new functionality.

[ghost]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This is a fix for dotnet/linker#3061.

If the app creates a delegate over a method and later on access the Delegate.Method property it can get a MethodInfo which it can use to inspect some aspects of the method, specifically parameter names.

NativeAOT already handles this correctly, but ILLink doesn't and will trim the parameter names away. The correct behavior is to treat such method as reflection accessible (we really don't want to mark the Delegate.Method property as trim incompatible).

The ideal way to implement this is to perform full data flow on method addresses, which would allow us to detect which method is the delegate created for. (this is what NativeAOT ends up doing through JIT)

But that is rather complex, an easier solution is to treat any method which address is accessed as reflection enabled. This covers all delegates. It might mark more methods as reflection enabled, but it's very uncommon.

So this change does that, it will mark all methods accessed via ldftn (or similar) as reflection enabled.
Additionally this cleans up marking of reflection enabled members in the MarkStep. Specifically there was a distinction between reflection accessed and indirect accessed methods, but the latter only kicked in for the obsolete preserve attributes. It's better to reflection mark those as well. This might cause a small size regression, but it's very unlikely to be even observable.

This requires lot more test changes to enable sane testing:

• In the ILC test infra, track if method was kept with or without reflection metadata (as that is the difference in NativeAOT which will provide parameter names or not)
• Teach both the ILLink and ILC test infras how to handle compiler generated code with the Kept attribute validation
  • This is in no way complete, but it's an improvement
  • Mostly it means that we don't fail the test if a compiler generated code is kept without it being marked with the Kept attribute (as that's not always possible)
  • Note that we have some support for this for things like backing fields and private implementation details, but nothing which can handle state machines, and closure types for lambdas. These should now be workable, without the ability to verify that they were indeed kept (and how)
  • Finally adds tests verifying the new functionality.

Author:	vitek-karas
Assignees:	vitek-karas
Labels:	area-Tools-ILLink
Milestone:	8.0.0

[sbomer]

What's the reason for disabling the custom attribute check? Might be worth a comment here.

[vitek-karas]

VerifyCustomAttributes has not been ported to NativeAOT yet, so it doesn't work. There's lot of code in this file which is not even ifdefed out, but never called, so you can see some possible callsites for this method, but none are actually active.

There's LOT of things to do to get even close to the ILLink functionality in this area. And lot of things are either not possible, or unreasonably hard because NativeAOT doesn't store the information, or the way it's stored is very different and complex.

[sbomer]

LGTM, thanks!