Skip to content

Add the ability to remove COM support using ILLinker #43501

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

Closed
wants to merge 4 commits into from
Closed

Add the ability to remove COM support using ILLinker #43501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a796341
Add the ability to remove COM support using ILLinker
marek-safar Oct 15, 2020
4ca605a
PR feedback
marek-safar Oct 16, 2020
7f81937
Remove STA/MTA attributes exclusion
marek-safar Oct 16, 2020
5d65822
Undo mono change
marek-safar Oct 16, 2020
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
2 changes: 2 additions & 0 deletions src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,8 @@ public static IntPtr CreateAggregatedObject<T>(IntPtr pOuter, T o) where T : not
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern bool IsComObject(object o);

internal static bool IsComSupported => true;
Copy link
Member

Choose a reason for hiding this comment

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

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


/// <summary>
/// Release the COM component and if the reference hits 0 zombie this object.
/// Further usage of this Object might throw an exception
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/Microsoft.CSharp/src/ILLink/ILLinkTrim.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<linker>
<assembly fullname="Microsoft.CSharp">
<assembly fullname="Microsoft.CSharp" feature="System.Runtime.InteropServices.Marshal.IsComSupported" featurevalue="true" featuredefault="true">
<!-- Required for COM event dispatch -->
<type fullname="System.Runtime.InteropServices.ComEventsSink"/>
</assembly>
Expand Down
25 changes: 25 additions & 0 deletions src/libraries/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.Shared.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,29 @@
<attribute internal="RemoveAttributeInstances" />
</type>
</assembly>

<!-- COM specific attributes -->
<assembly fullname="System.Private.CoreLib" feature="System.Runtime.InteropServices.Marshal.IsComSupported" featurevalue="false">
<type fullname="System.Runtime.InteropServices.ClassInterfaceAttribute">
<attribute internal="RemoveAttributeInstances" />
</type>
<type fullname="System.Runtime.InteropServices.CoClassAttribute">
Copy link
Member

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

<attribute internal="RemoveAttributeInstances" />
</type>
<type fullname="System.Runtime.InteropServices.ComDefaultInterfaceAttribute">
<attribute internal="RemoveAttributeInstances" />
</type>
<type fullname="System.Runtime.InteropServices.ComImportAttribute">
<attribute internal="RemoveAttributeInstances" />
</type>
<type fullname="System.Runtime.InteropServices.ComVisibleAttribute">
<attribute internal="RemoveAttributeInstances" />
</type>
<type fullname="System.Runtime.InteropServices.GuidAttribute">
Copy link
Member

@MichalStrehovsky MichalStrehovsky Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that should be doable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

<attribute internal="RemoveAttributeInstances" />
</type>
<type fullname="System.Runtime.InteropServices.ProgIdAttribute">
<attribute internal="RemoveAttributeInstances" />
</type>
</assembly>
</linker>
9 changes: 9 additions & 0 deletions src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,13 @@
<field name="s_asyncDebuggingEnabled" value="false" initialize="false" />
</type>
</assembly>
<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>
Comment on lines +22 to +30
Copy link
Member

Choose a reason for hiding this comment

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

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.

</linker>
2 changes: 2 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.NoCom.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,8 @@ public static bool IsTypeVisibleFromCom(Type t)
return false;
}

internal static bool IsComSupported => false;

[SupportedOSPlatform("windows")]
public static int QueryInterface(IntPtr pUnk, ref Guid iid, out IntPtr ppv)
{
Expand Down