ILLink: Stop giving special treatment to icalls

[mrvoorhe]

As discussed #113437 (comment)

[mrvoorhe]

fyi @jkotas @MichalStrehovsky @sbomer

[jkotas]

I have taken a look at the coreclr failures. jkotas@6caad79 should fix them. Feel free to cherry-pick this commit to this PR.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/illink
See info in area-owners.md if you want to be subscribed.

[jkotas]

The rest looks like issues with the linker tests and Mono. Let me know if you need help with fixing those.

[sbomer]

LGTM once green, thank you!

For the mono failures, I think we just need to remove the UnconditionalSuppressMessage of IL2110 from RuntimeEnumBuilder and RuntimeTypeBuilder.

[jkotas]

How does the linker know that the Mono unmanaged runtime depends on the RuntimeTypeBuilder fields? Do we need to declare the dependency somewhere?

[sbomer]

The only place I'm aware of for mono is via a descriptor (https://github.com/dotnet/runtime/blob/main/src/mono/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.xml).

I see RuntimeTypeBuilder loaded here:

runtime/src/mono/mono/metadata/reflection.c

Line 3207 in de72cb8

method = mono_class_get_method_from_name_checked (mono_class_get_type_builder_class (), "IsAssignableToInternal", 1, 0, error);

and it looks like the ctor annotations have been set up to preserve fields too:

runtime/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.Mono.cs

Lines 93 to 113 in de72cb8

	[DynamicDependency(nameof(state))] // Automatically keeps all previous fields too due to StructLayout
	[DynamicDependency(nameof(IsAssignableToInternal))] // Used from reflection.c: mono_reflection_call_is_assignable_to
	internal RuntimeTypeBuilder(RuntimeModuleBuilder mb, TypeAttributes attr, int table_idx, bool is_hidden_global_type = false)
	{
	this.is_hidden_global_type = is_hidden_global_type;
	this.parent = null;
	this.attrs = attr;
	this.class_size = UnspecifiedTypeSize;
	this.table_idx = table_idx;
	this.tname = table_idx == 1 ? "<Module>" : "type_" + table_idx.ToString();
	this.nspace = string.Empty;
	this.fullname = TypeIdentifiers.WithoutEscape(this.tname);
	pmodule = mb;
	}
	[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2074:UnrecognizedReflectionPattern",
	Justification = "Linker doesn't analyze ResolveUserType but it's an identity function")]
	[DynamicDependency(nameof(state))] // Automatically keeps all previous fields too due to StructLayout
	[DynamicDependency(nameof(IsAssignableToInternal))] // Used from reflection.c: mono_reflection_call_is_assignable_to
	internal RuntimeTypeBuilder(RuntimeModuleBuilder mb, string name, TypeAttributes attr, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type? parent, Type[]? interfaces, PackingSize packing_size, int type_size, Type? nesting_type)

So I think it would make sense to add an entry for these ctors to the descriptors.

[mrvoorhe]

@sbomer Are all of these failures related to this PR? It's hard to tell.

[jkotas]

Yes, they look related. Mono Wasm depends on FCalls to be in PInvokesListFile that is no longer happening with this change.

[mrvoorhe]

Yes, they look related. Mono Wasm depends on FCalls to be in PInvokesListFile that is no longer happening with this change.

Gotcha. What do you want to do about that?

[jkotas]

Actually, the PInvokesListFile looks fine. But some other dependencies that the MonoVM depends on may be missing - potentially related to #113704 (comment) .

[sbomer]

/azp run runtime dotnet-linker-tests

[azure-pipelines]

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

[sbomer]

@mrvoorhe I resolved the merge conflicts in https://github.com/sbomer/runtime/tree/pr/mrvoorhe/113704, feel free to pull in that change.
I think the test failures might need us to add descriptors for RuntimeTypeBuilder and RuntimeEnumBuilder per #113704 (comment).

[mrvoorhe]

Closing this PR. I pushed a bad set of changes and it pulled in a bunch of reviewers automatically. I'll open a new one.