Add the ability to remove COM support using ILLinker #43501

marek-safar · 2020-10-16T07:35:11Z

MichalStrehovsky · 2020-10-16T08:00:41Z

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.Shared.xml

+    <type fullname="System.Runtime.InteropServices.ClassInterfaceAttribute">
+      <attribute internal="RemoveAttributeInstances" />
+    </type>
+    <type fullname="System.Runtime.InteropServices.CoClassAttribute">


Is stripping these attributes buying us anything? If user chooses to turn off COM, but they still have a bunch of COM classes that are kept, wouldn't it be better if they get useful feedback about it?

Stripping these attributes results in obnoxious failure modes like "System.Security.SecurityException: 'ECall methods must be packaged into a system module.'".

(That's the exception you get if you try to new up a class like this: class Foo { [MethodImpl(MethodImplOptions.InternalCall)] public extern Foo(); }, which is what's left of COM after these are stripped.)

Is stripping these attributes buying us anything?

It saves some size but it also correctness fix for us that we don't have a dependency on these attribute which is not guarded under COM feature.

wouldn't it be better if they get useful feedback about it?

They have no meaning for example for MonoVM. Can CoreCLR throw a better error when the runtime starts in COM disabled mode?

It saves some size but it also correctness fix for us that we don't have a dependency on these attribute which is not guarded under COM feature.

Wouldn't it be better to keep these so that output can be inspected for these? I assume if we have a class marked with CoClassAttribute, we would want to remove the entire class because any code calling into it is going to crash with the useless exception I quoted above. That's a much better size saving and also a correctness fix because instead of producing reachable code that might crash at runtime, we can prove the crashing code is not there. Kind of feels like COM stripping should be something where linker is more involved and can issue warnings ("hey, I disabled COM, but you still have COM CoClasses left, things may crash at runtime").

Can CoreCLR throw a better error when the runtime starts in COM disabled mode?

The runtime doesn't know this was a COM class if linker strips all signs of it. It's just a class with an InternalCall constructors that the runtime doesn't know what to do with.

I assume if we have a class marked with CoClassAttribute, we would want to remove the entire class because any code calling into it is going to crash with the useless exception I quoted above.

I don't follow. You apply CoClassAttribute to an interface which type argument of the implementation class. By removing the attribute from the interface you remove the class if there are no further references.

The runtime doesn't know this was a COM class if linker strips all signs of it

It could, this is standard opt-in feature switch so when someone disables it on Windows you will have the information written in *.runtimeconfig.json

/cc @vitek-karas

I don't follow. You apply CoClassAttribute to an interface which type argument of the implementation class. By removing the attribute from the interface you remove the class if there are no further references.

Okay, let's say I referred to/commented on the ComImportAttribute a couple lines below. I didn't have my second coffee yet.

I think we'll have to leave the MTA/STA alone - those have effect outside of COM support - and even if the app itself doesn't use COM, it still may need to set STA for other Windows stuff to work correctly.

MichalStrehovsky · 2020-10-16T08:06:12Z

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.Shared.xml

+    <type fullname="System.Runtime.InteropServices.ComVisibleAttribute">
+      <attribute internal="RemoveAttributeInstances" />
+    </type>
+    <type fullname="System.Runtime.InteropServices.GuidAttribute">


If we want to strip this one, we should make sure System.Type::GUID is somehow unusable. I've seen people use things like this creatively (e.g. I've seen people using the assembly's public key (the one used for assembly binding) as a public key for encrypting communication, etc.). It wouldn't surprise me if people use this as a "type-associated unique ID". Stripping this attribute just silently changes the GUID.

Yeah, that should be doable

I was looking at the implementation and it returns empty GUID in this case. I guess that's better than to throw.

Is that what Mono returns when there's no GuidAttribute? The runtime is supposed to compute a GUID using some weird algorithm from name, I think (it does so on CoreCLR).

Btw the docs for Type.GUID don't mention COM anywhere so this might come as a surprise to some users.

Yeah, Mono return empty GUID. Checking the CoreCLR implementation

runtime/src/coreclr/src/vm/reflectioninvocation.cpp

Lines 2148 to 2171 in 0a69f02

#ifdef FEATURE_COMINTEROP

if (IsComObjectClass(type))

{

SyncBlock* pSyncBlock = refThis->GetSyncBlock();

#ifdef FEATURE_COMINTEROP_UNMANAGED_ACTIVATION

ComClassFactory* pComClsFac = pSyncBlock->GetInteropInfo()->GetComClassFactory();

if (pComClsFac)

{

memcpyNoGCRefs(result, &pComClsFac->m_rclsid, sizeof(GUID));

}

else

#endif // FEATURE_COMINTEROP_UNMANAGED_ACTIVATION

{

memset(result, 0, sizeof(GUID));

}

goto lExit;

}

#endif // FEATURE_COMINTEROP

GUID guid;

type.AsMethodTable()->GetGuid(&guid, TRUE);

memcpyNoGCRefs(result, &guid, sizeof(GUID));

it looks like this should work fine. Basically making no-com on Windows to behave the same as CoreCLR on Unix and generating some random GUID.

In that case we need to make it so that the behavior is the same without linker in the picture. So this change would need to incorporate change to CoreCLR which looks at the runtime property and ignores the attribute in that case.

I would leave the GuidAttribute. This is not a COM support thing. I have observe many projects use this for making their type's Guid X instead of it being generated. I think removing this is a bit aggressive.

Thinking about this more, I would be fine removing the GuidAttribute but then I would argue for making Type.Guid match the behavior of mono.

My reasoning here is that mono is the canonical runtime implementation with no COM support. This means it should be considered the source of truth semantically speaking. Making this change would also help to validate the assumption that the GuidAttribute is only for COM scenarios in the sense that if we start returning Guid.Empty when one disables COM support and no one balks then the assumption is true if there are no other uses of COM in the system. However, if users complain that the behavior of Type.Guid has now changed in CoreCLR and we confirm they are not using COM in any manner than it invalidates that assumption and indicates that Type.Guid and therefore GuidAttribute are relied upon in non-COM scenarios.

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.NoCom.cs

vitek-karas · 2020-10-16T09:19:33Z

This change is by no means a complete fix for "remove COM feature switch" - there are more things we need to "disable" - COM object creation APIs, resolve the ComActivator and related classes, possibly more...

vitek-karas · 2020-10-16T09:41:05Z

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml

+  <assembly fullname="System.Private.CoreLib" feature="System.Runtime.InteropServices.Marshal.IsComSupported" featurevalue="false">
+    <type fullname="System.Type" >
+      <method signature="System.Boolean get_IsCOMObject()" body="stub" value="false" />
+    </type>
+    <type fullname="System.Runtime.InteropServices.Marshal">
+      <method signature="System.Boolean IsComObject(System.Object)" body="stub" value="false" />
+      <method signature="System.Boolean IsTypeVisibleFromCom(System.Type)" body="stub" value="false" />
+    </type>
+  </assembly>


The feature switch should only substitute the IsComSupported property.
Everything else should be done in code using the IsComSupported property - so that we get consistent behavior with/without linker.

vitek-karas · 2020-10-16T09:41:41Z

src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs

@@ -445,6 +445,8 @@ public static object GetUniqueObjectForIUnknown(IntPtr unknown)
        [MethodImpl(MethodImplOptions.InternalCall)]
        public static extern bool IsComObject(object o);

+        internal static bool IsComSupported => true;


This needs to read the runtime property to correctly react to the feature switch.

AaronRobinsonMSFT

Do we have numbers on what this PR will save us? I keep hearing "substantial" but it would be great to see the real numbers.

AaronRobinsonMSFT · 2020-10-16T15:08:09Z

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.Shared.xml

+    <type fullname="System.Runtime.InteropServices.ComVisibleAttribute">
+      <attribute internal="RemoveAttributeInstances" />
+    </type>
+    <type fullname="System.Runtime.InteropServices.GuidAttribute">


I would leave the GuidAttribute. This is not a COM support thing. I have observe many projects use this for making their type's Guid X instead of it being generated. I think removing this is a bit aggressive.

eerhardt · 2020-10-16T16:48:44Z

Can we update https://github.com/dotnet/runtime/blob/master/docs/workflow/trimming/feature-switches.md with the new feature switch?

eerhardt · 2020-10-16T16:55:12Z

Can we also make these descriptors conditional on the new feature switch?

runtime/src/coreclr/src/System.Private.CoreLib/src/ILLink/ILLinkTrim.xml

Lines 13 to 19 in 149964a

    
           <type fullname="System.Runtime.InteropServices.ComTypes.IEnumerable" /> 
        
           <type fullname="System.Runtime.InteropServices.ComTypes.IEnumerator" /> 
        
           <type fullname="System.Runtime.InteropServices.CustomMarshalers.*" /> 
        
           <!-- Workaround for https://github.com/mono/linker/issues/378 --> 
        
           <type fullname="System.Runtime.InteropServices.IDispatch" /> 
        
           <type fullname="System.Runtime.InteropServices.IMarshal" /> 
        
           <type fullname="Internal.Runtime.InteropServices.IClassFactory2" />

ViktorHofer · 2020-12-08T14:51:39Z

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

Add the ability to remove COM support using ILLinker

a796341

Fixes dotnet#36659

Dotnet-GitSync-Bot added the area-System.Runtime.InteropServices label Oct 16, 2020

marek-safar requested review from jkotas and eerhardt October 16, 2020 07:35

MichalStrehovsky reviewed Oct 16, 2020

View reviewed changes

vitek-karas reviewed Oct 16, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.NoCom.cs Outdated Show resolved Hide resolved

vitek-karas requested a review from AaronRobinsonMSFT October 16, 2020 09:17

marek-safar requested review from akoeplinger, EgorBo, steveisok and vargaz as code owners October 16, 2020 09:33

marek-safar force-pushed the com branch from 189fb63 to 98082ee Compare October 16, 2020 09:37

PR feedback

4ca605a

marek-safar force-pushed the com branch from 98082ee to 4ca605a Compare October 16, 2020 09:39

vitek-karas reviewed Oct 16, 2020

View reviewed changes

marek-safar added 2 commits October 16, 2020 11:44

Remove STA/MTA attributes exclusion

7f81937

Undo mono change

5d65822

runfoapp bot mentioned this pull request Oct 16, 2020

System.Net.Security.Tests timing out #43403

Closed

AaronRobinsonMSFT reviewed Oct 16, 2020

View reviewed changes

marek-safar closed this Jan 13, 2021

ghost locked as resolved and limited conversation to collaborators Feb 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to remove COM support using ILLinker #43501

Add the ability to remove COM support using ILLinker #43501

marek-safar commented Oct 16, 2020

MichalStrehovsky Oct 16, 2020

marek-safar Oct 16, 2020

MichalStrehovsky Oct 16, 2020

marek-safar Oct 16, 2020

MichalStrehovsky Oct 16, 2020

vitek-karas Oct 16, 2020

MichalStrehovsky Oct 16, 2020 •

edited

Loading

marek-safar Oct 16, 2020

marek-safar Oct 16, 2020

MichalStrehovsky Oct 16, 2020

MichalStrehovsky Oct 16, 2020

marek-safar Oct 16, 2020

vitek-karas Oct 16, 2020

AaronRobinsonMSFT Oct 16, 2020

AaronRobinsonMSFT Oct 16, 2020

vitek-karas commented Oct 16, 2020

vitek-karas Oct 16, 2020

vitek-karas Oct 16, 2020

AaronRobinsonMSFT left a comment

AaronRobinsonMSFT Oct 16, 2020

eerhardt commented Oct 16, 2020

eerhardt commented Oct 16, 2020

ViktorHofer commented Dec 8, 2020

	#ifdef FEATURE_COMINTEROP
	if (IsComObjectClass(type))
	{
	SyncBlock* pSyncBlock = refThis->GetSyncBlock();

	#ifdef FEATURE_COMINTEROP_UNMANAGED_ACTIVATION
	ComClassFactory* pComClsFac = pSyncBlock->GetInteropInfo()->GetComClassFactory();
	if (pComClsFac)
	{
	memcpyNoGCRefs(result, &pComClsFac->m_rclsid, sizeof(GUID));
	}
	else
	#endif // FEATURE_COMINTEROP_UNMANAGED_ACTIVATION
	{
	memset(result, 0, sizeof(GUID));
	}

	goto lExit;
	}
	#endif // FEATURE_COMINTEROP

	GUID guid;
	type.AsMethodTable()->GetGuid(&guid, TRUE);
	memcpyNoGCRefs(result, &guid, sizeof(GUID));

Add the ability to remove COM support using ILLinker #43501

Add the ability to remove COM support using ILLinker #43501

Conversation

marek-safar commented Oct 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitek-karas commented Oct 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Oct 16, 2020

eerhardt commented Oct 16, 2020

ViktorHofer commented Dec 8, 2020

MichalStrehovsky Oct 16, 2020 •

edited

Loading