ILLink : Fix instantiation tracking issue with icall/pinvoke parameters

[mrvoorhe]

Along the lines of #113434. The existing ProcessInteropMethod has a bug where the return type and/or out parameter types will not be marked as instantiated. In this case, the bug happens when the return type or parameter types do not have a .ctor(). This is because ProcessInteropMethod will call MarkDefaultConstructor on the type which will cause the type to be marked instantiated if the .ctor() exists.

[dotnet-policy-service]

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

[MichalStrehovsky]

Is it possible to hit this with anything but COM marshalling? IMHO the added test cases all result in COM marshalling that we generate warnings on because it's not trim safe and not marking type as instantiated is the least of the problems for COM scenarios. Or is this for some internal Unity interop scenarios?

The default constructor case covers marshalling classes with StructLayout != Auto (and that one wouldn't be COM marshalling).

[sbomer]

The default constructor case covers marshalling classes with StructLayout != Auto (and that one wouldn't be COM marshalling).

For my own understanding - what does the default constructor have to do with non-auto layout marshalling?

@jkotas @MichalStrehovsky For consistency, would it make sense to handle parameters of interop/intrinsic methods the same as parameters of any other method with respect to instantiation? That is: don't mark constructors or consider types instantiated just because they are parameters of an interop/intrinsic method (and instead rely on the interop scenario or runtime descriptors to root the required types/ctors)?

[mrvoorhe]

I didn't open this PR because I encountered a problem in any particular. I opened it because during the scenario outlined in #113434 the reason CoreCLR held together was because

[MethodImpl(MethodImplOptions.InternalCall)]
[Intrinsic]
public extern Type GetType();

Is an icall which led to ProcessInteropMethod being called and because System.Type had a .ctor() everything holds together. If System.Type did not have a .ctor(), then this icall scenario would have ran into the same thing as the other PR.

Which got me thinking, the foundation of the not instantiated optimizations is that the linker can accurately track when a given type is created. For methods that don't have a body, or method's whose managed body may never be the real code that is executed, the linker no longer has awareness as to what is going to happen. That can of course create all sorts of trimming problems, but why would we not attempt to keep things glue together and at least not apply the not instantiated optimizations? I appreciate it's not perfect, for example, maybe the type that is created is a derived type, but even then marking the base type as instantiated is at least closer to correct.

I understand this doesn't fully fix all holes. It reduces risk while causing no more marking than a correctly annotated set of code would mark. That said, I can think of 1 exception. The exception I can think of would be when you have an interop method that you know will always return null. For example

#if PLATFORMA
[DynamicDependency("FooClass", ".ctor()")]
public static extern FooClass LetsSayThisIsAPlatformSpecificMethod();
#else
public static extern FooClass LetsSayThisIsAPlatformSpecificMethod();

And if the platform isn't PLATFORMA then LetsSayThisIsAPlatformSpecificMethod will always return null (or always throw/assert). In this case, this PR would prevent the not instantiated optimizations when they could possibly be applied. This example applies more to [Intrinsic] methods than icalls because ProcessInteropMethod will currently call MarkDefaultConstructor which undermines this case.

I guess what I'm getting at is, the current discrepancy between how the linker handles icalls/pinvokes vs intrinsics feels odd. That's not to say that they have to be handled 100% the same, but it feels undesirable that the test that originally caught this DataFlow.NullableAnnoations would pass when System.Object.GetType() is defined as

[MethodImpl(MethodImplOptions.InternalCall)]
[Intrinsic]
public extern Type GetType();

and fail when defined as

    [Intrinsic]
    public Type GetType()
    {
        return GetType();
    }

Wouldn't it be better if both held together or both resulted in problems without a DynamicDependencyAttribute?

[jkotas]

For my own understanding - what does the default constructor have to do with non-auto layout marshalling?

The marshalling rules for built-in interop will instantiate a non-auto layout type when it is returned from the method. For example:

[StructLayout(LayoutKind.Sequential)]
class MyType
{
    public int Value;
}

// This will call RuntimeHelpers.GetUninitializedObject(typeof(MyType)) under the covers
[DllImport("dll1.dll")]
[return: MarshalAs(UnmanagedType.LPStruct)]
extern static MyType M();

I do not think that the type even needs to have a default constructor in this case.

What's our current take on handling built-in interop in the trimmer? Are we replicating built-in interop rules in the trimmer or did we give up on that?

[jkotas]

In any case, the special handling of built-in interop marshaling rules should be unnecessary for assemblies with DisableRuntimeMarshallingAttribute. There is no magic going on underneath when the assembly is marked with DisableRuntimeMarshallingAttribute.

[MichalStrehovsky]

For my own understanding - what does the default constructor have to do with non-auto layout marshalling?

The marshalling logic lives here. Basically whether the class has layout or not decides how it will be marshalled. If it's Auto layout, it's COM. If it's not Auto, it's marshalled field-by-field/as blittable:

runtime/src/coreclr/vm/mlinfo.cpp

Lines 1558 to 1578 in cc01298

	else if (m_pMT->IsBlittable())
	{
	if (!(nativeType == NATIVE_TYPE_DEFAULT || nativeType == (IsFieldScenario() ? NATIVE_TYPE_STRUCT : NATIVE_TYPE_LPSTRUCT)))
	{
	m_resID = IsFieldScenario() ? IDS_EE_BADMARSHALFIELD_LAYOUTCLASS : IDS_EE_BADMARSHAL_CLASS;
	IfFailGoto(E_FAIL, lFail);
	}
	m_type = IsFieldScenario() ? MARSHAL_TYPE_BLITTABLE_LAYOUTCLASS : MARSHAL_TYPE_BLITTABLEPTR;
	m_args.m_pMT = m_pMT;
	}
	else if (m_pMT->HasLayout())
	{
	if (!(nativeType == NATIVE_TYPE_DEFAULT || nativeType == (IsFieldScenario() ? NATIVE_TYPE_STRUCT : NATIVE_TYPE_LPSTRUCT)))
	{
	m_resID = IsFieldScenario() ? IDS_EE_BADMARSHALFIELD_LAYOUTCLASS : IDS_EE_BADMARSHAL_CLASS;
	IfFailGoto(E_FAIL, lFail);
	}
	m_type = IsFieldScenario() ? MARSHAL_TYPE_LAYOUTCLASS : MARSHAL_TYPE_LAYOUTCLASSPTR;
	m_args.m_pMT = m_pMT;
	}
	else if (m_pMT->IsObjectClass())

What's our current take on handling built-in interop in the trimmer? Are we replicating built-in interop rules in the trimmer or did we give up on that?

We should be replicating them or generating trim warnings. We generate trim warnings for COM, there might be other problematic scenarios where we still don't generate a warning (AsAny?). There's an issue tracking this somewhere.

// This will call RuntimeHelpers.GetUninitializedObject(typeof(MyType)) under the covers

Oh, interesting, I was under the impression we call parameterless constructor. In that case this change looks good to me. It actually fixes a p/invoke bug (and we might be keeping the parameterless constructor unnecessarily for this scenario, but who knows what other interop scenario the parameterless ctor might be covering - illink doesn't really bother trying to distinguish these). We might want to update the test to use a class with layout just in case we want to tighten the classification in the future and then we'll be scratching our head why we are testing COM.

Is an icall which led to ProcessInteropMethod being called and because System.Type had a .ctor() everything holds together. If System.Type did not have a .ctor(), then this icall scenario would have ran into the same thing as the other PR.

I think the problem with icalls is that they can make up different types (in this case it will actually be System.RuntimeType in any of our runtimes) and ILLink cannot predict all that - the runtime needs to declare what the icall will do using DynamicDependency/ILLink.xml. Pinvokes have better defined rules and there's no possible way a more derived type could come through (at least I hope, not an interop expert).

[jkotas]

Oh, interesting, I was under the impression we call parameterless constructor.

FWIW, it is done here:

runtime/src/coreclr/vm/ilmarshalers.cpp

Line 2230 in cc01298

pslILEmit->EmitCALL(METHOD__RUNTIME_HELPERS__GET_UNINITIALIZED_OBJECT, 1, 1);

[jkotas]

In that case this change looks good to me. It actually fixes a p/invoke bug

+1

Other changes to consider based on the discussion:

• Stop calling ProcessInteropMethod for icalls. This special handling does not make sense for them.
• Stop calling ProcessInteropMethod for assemblies marked with DisableRuntimeMarshallingAttribute. The special handling should be unnecessary for them.

[mrvoorhe]

I'm going to leave this PR open for a bit. My plan is to

1. Open a PR to address

Stop calling ProcessInteropMethod for icalls. This special handling does not make sense for them.

2. Open a PR to address

Stop calling ProcessInteropMethod for assemblies marked with DisableRuntimeMarshallingAttribute. The special handling should be unnecessary for them.

3. Rebase this PR and remove the icall test changes.

Question for @MichalStrehovsky when I get back to this PR. Do you want me to keep the MarkDefaultConstructor calls for pinvokes or remove them?

[MichalStrehovsky]

Question for @MichalStrehovsky when I get back to this PR. Do you want me to keep the MarkDefaultConstructor calls for pinvokes or remove them?

If we wanted to go that route, we'd need to audit the marshalling rules and replicate some of them in illink. When I wrote "who knows what other interop scenario the parameterless ctor might be covering - illink doesn't really bother trying to distinguish these", I actually had a scenario in mind already - anything that derives from SafeHandle would get activated by calling the parameterless constructor and we need to keep it. We had issues in the past when SafeHandle constructor was not getting kept because we never saw it in a p/invoke during repo build (#45633 (comment)).

I'd be weary to touch this. We now set DisableRuntimeMarshallingAttribute pretty much everywhere in the repo so breaking runtime marshalling is unlikely to be found in our testing. Only third party code would run into it.