Skip to content
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.

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

Add the ability to remove COM support using ILLinker #36659

Closed
eerhardt opened this issue May 18, 2020 · 31 comments
Closed

Add the ability to remove COM support using ILLinker #36659

eerhardt opened this issue May 18, 2020 · 31 comments
Assignees
Labels
area-System.Runtime.InteropServices size-reduction Issues impacting final app size primary for size sensitive workloads
Milestone

Comments

@eerhardt
Copy link
Member

As discussed in:

We are currently "rooting" types in the libraries specifically for COM:

<!-- Required for COM event dispatch -->
<type fullname="System.Runtime.InteropServices.ComEventsSink"/>

<!-- IMarshal is internal and never directly called, but needed by COM -->
<type fullname="System.Runtime.InteropServices.IMarshal" />

A lot of scenarios in .NET don't have COM support, as such we should be able to trim out any COM specific code when we are linking for a scenario that won't ever need COM (Blazor, Xamarin, Linux, etc).

One way of doing this would be to add a new feature switch to the product that would enable/disable COM. We can then add the switch in code to remove any COM specific code. In order to remove the COM types from being rooted by the ILLinkTrim.xml "descriptor XML files" above, we would need the ILLinker to allow "feature switches" to be present in the descriptor XML files, and to include/exclude that root based on the feature switch.

Note that Blazor today is already using something similar when it uses monolinker.exe - --exclude-feature com. See the Blazor build code. However, --exclude-feature is not supported by the ILLinker, only by monolinker.exe.

cc @vitek-karas @MichalStrehovsky @sbomer @marek-safar

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Host untriaged New issue has not been triaged by the area owner labels May 18, 2020
@ghost
Copy link

ghost commented May 18, 2020

Tagging subscribers to this area: @vitek-karas, @swaroop-sridhar
Notify danmosemsft if you want to be subscribed.

@jkotas jkotas added the linkable-framework Issues associated with delivering a linker friendly framework label May 18, 2020
@marek-safar
Copy link
Contributor

In this case, I'd prefer to see a new public property for COM. Something like RuntimeFeatures.IsCOMSupported and use that for all COM related code inside libraries including ComEventsSink or IMarshal dependencies.

@AaronRobinsonMSFT
Copy link
Member

@vitek-karas and @MichalStrehovsky I would be grateful if one of you could provide some suggestions on the best way to solve this issue.

@jkotas
Copy link
Member

jkotas commented Jun 17, 2020

Both IMarshal and ComEventsSink roots above are instances of dotnet/linker#936 . What are the plans for ComImport support in the linker?

@marek-safar
Copy link
Contributor

Linker has all the required features needed to make COM support feature-based. The ball is on the libraries side to decide what level of granularity is needed and add the API (static bool property/properties).

@jkotas
Copy link
Member

jkotas commented Jun 17, 2020

Ok, my proposal would be bool Marshal.IsComSupported, returns true by default on Windows and false by default on Unix. Does it need to be public API, or can it just be internal (for now at least)?

@eerhardt
Copy link
Member Author

Does it need to be public API, or can it just be internal (for now at least)?

I think it can be internal for now, we can make it public later if we see a need.


We would also need a feature switch to allow for trimming System.Runtime.InteropServices.ComEventsSink from Microsoft.CSharp, if we want to make this complete. The route I've been going is to name the feature switch the same as the API, so this would be named System.Runtime.InteropServices.Marshal.IsComSupported

@jkotas
Copy link
Member

jkotas commented Jun 18, 2020

Ideally, System.Runtime.InteropServices.Marshal.IsComSupported would control treatment of types marked with [ComImport]. When COM support is on, the linker should keep all methods on interfaces marked with [ComImport]. Both ComEventsSink and IMarshal would be covered by this logic.

Or do you believe that we should rather ask everybody out there (both in libraries and apps) to mark their [ComImport] interfaces with an extra linker attribute to tie it with IsComSupported?

@eerhardt
Copy link
Member Author

Or do you believe that we should rather ask everybody out there (both in libraries and apps) to mark their [ComImport] interfaces with an extra linker attribute to tie it with IsComSupported?

I guess it depends on how much we want the ILLinker to know about COM. My understanding was that @marek-safar and @vitek-karas were moving away from ILLinker having hard-coded knowledge of features. But since COM is so low in the stack, it may be fine for the ILLinker to know about it.

@vitek-karas
Copy link
Member

I think we're discussing two related but separate issues here:

  1. How to remove anything COM related from apps which don't need/want it - like all of Blazor apps
  2. How to make sure that trimmed apps which do use COM actually work

The former is something we're trying to solve for .NET 5. The latter is something we should probably punt to later releases. I also don't think the solutions will be that related, so we should feel free to do what's necessary to make the .NET 5 goals work.

@eerhardt
Copy link
Member Author

If we aren't trying to fully solve this issue in .NET 5, I believe the only thing necessary to strip it from Blazor would be to remove these lines from the System.Private.CoreLib's Shared ILLink.Descriptors file:

<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="Internal.Runtime.InteropServices.IClassFactory2" />

And move them into the CoreCLR specific System.Private.CoreLib's ILLink.Descriptors file. Then the Mono S.P.C. won't ever have these types - since my assumption is Mono doesn't support COM at all (but I could be wrong there).

For Microsoft.CSharp and System.Runtime.InteropServices, I don't believe these assemblies will typically be used in Blazor, at least not for the majority of Blazor apps. So we can just leave them as-is. They will get trimmed away since they aren't used.

@vitek-karas
Copy link
Member

That might work... although it would be safer to introduce the feature switch and make sure that Blazor runs it off - just so that we don't run into surprises in the future.

@marek-safar
Copy link
Contributor

Does it need to be public API, or can it just be internal (for now at least)?

I suspect we'll need it public at least in scope for our libraries. I guess there will be some code outside of SPC which could be conditionally excluded based on this property.

It'd also be helpful for libraries authors so they don' have to rely on
exceptions handling.

Ideally, System.Runtime.InteropServices.Marshal.IsComSupported would control treatment of types marked with [ComImport]. When COM support is on, the linker should keep all methods on interfaces marked with [ComImport]. Both ComEventsSink and IMarshal would be covered by this logic.

The COM types are already recognized by the linker and handled differently. I don't think we have should bound that logic directly with System.Runtime.InteropServices.Marshal.IsComSupported but it'd be helpful to control this linker behaviour via the same feature-switch which we'll use to set System.Runtime.InteropServices.Marshal.IsComSupported

@jkotas
Copy link
Member

jkotas commented Jun 18, 2020

The COM types are already recognized by the linker and handled differently.

What does linker do for them? If it did the right thing (tm) for them, we would not need the manual entries for IMarshal and ComEventsSink?

@marek-safar
Copy link
Contributor

What does linker do for them?

I think it marks the default constructor and all fields on the type

@jkotas
Copy link
Member

jkotas commented Jun 18, 2020

Fields do not matter for COM interop. Did you meant to say methods?

@marek-safar
Copy link
Contributor

I couldn't find the code which is handling this so it looks like it was removed when we stopped caring about COM. We could add it back to linker though.

@jkotas
Copy link
Member

jkotas commented Jun 19, 2020

remove these lines from the System.Private.CoreLib's Shared ILLink.Descriptors file
and move them into the CoreCLR specific System.Private.CoreLib

That's sounds like the first thing to do here. These types do not even exist in the shared CoreLib...

eerhardt added a commit to eerhardt/runtime that referenced this issue Jun 19, 2020
These types only exist in CoreCLR, they don't exist in Mono.

Contributes to dotnet#36659
@AaronRobinsonMSFT
Copy link
Member

I'm not convinced this is a "must have" for 5.0.

Okay. I will move this to .NET 6.0. Feel free to remark if this becomes a need for .NET 5.0.

@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 6.0.0 milestone Jul 20, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Jul 20, 2020
@jkotas
Copy link
Member

jkotas commented Jul 20, 2020

So checking for feature support is spread across various locations?

Yes, each feature exposed as API is responsible to expose its own local bool IsSupported properties as necessary. IIRC, it was discussed during API reviews of this problem space and the local bool IsSupported properties were agreed upon as the right pattern to follow. This pattern scales well to 3rd party libraries among other things.

@vitek-karas
Copy link
Member

vitek-karas commented Oct 19, 2020

I created #43604 to track the work around breaking hard coded dependencies in ComActivator.

@eerhardt
Copy link
Member Author

@LakshanF - is this work completed? Or is there still more to do?

@LakshanF
Copy link
Member

There are still some cleanup work around test coverage and rooting native types. I'm planning to spend some time this week to review and will re-visit this after that

@SamMonoRT
Copy link
Member

SamMonoRT commented Jun 17, 2021

@LakshanF @eerhardt - are we on track to get the related cleanup work in by Preview 7 ?

@LakshanF
Copy link
Member

yes

@LakshanF
Copy link
Member

Closing this now

@ghost ghost locked as resolved and limited conversation to collaborators Jul 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants