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

Enable ComWrappers API on all platforms #36582

Closed
sdmaclea opened this issue May 15, 2020 · 19 comments · Fixed by #54838
Closed

Enable ComWrappers API on all platforms #36582

sdmaclea opened this issue May 15, 2020 · 19 comments · Fixed by #54838

Comments

@sdmaclea
Copy link
Contributor

sdmaclea commented May 15, 2020

Our debug interfaces uses COM interfaces.

To properly support these on all platforms it would be ideal to have the underlying wrappers supported.

/cc @AaronRobinsonMSFT @jkoritzinsky

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Interop-coreclr untriaged New issue has not been triaged by the area owner labels May 15, 2020
@sdmaclea sdmaclea added this to the 5.0 milestone May 15, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Enable CCW RCW wrappers on all platforms Enable ComWrappers API on all platforms May 15, 2020
@leculver
Copy link
Contributor

I don't object to enabling COM on non-Windows platforms. As a slight counterpoint though the diagnostics interfaces are just plain old C++ interfaces with Add/Release mechanics. IE not requiring other COM goo like activation.

For example, ClrMD solved this issue without needing COM interop enabled by using well-defined vtable layout and calling addref/release. The COMInterop part of the library defines our own CallableComWrapper which handles addref/release. To use this, we define the vtable layout for an interface, then simply use delegates to call them. It's a lot of typing to build the VTable layout, but it's fairly painless to use once they are defined.

I would ask this question in the opposite direction (not specifically to you Steve but to the diagnostics team/@tommcdon): It's 2020, why is the CLR diagnostics layer still built on COM? Why don't we invest into converting the DBI/ICorDebug into a C# based NuGet package? The benefits are:

  1. We can move faster. COM interfaces have a ton of inertia, and it takes forever to code a new feature in the C++ DBI codebase. A C# codebase could move much faster.
  2. No need for COM interop to use them.
  3. More cross-plat friendly.

Just a few thoughts here. As I said, I don't object if folks really want/need COM turned on for other platforms.

@sdmaclea
Copy link
Contributor Author

plain old C++ interfaces with Add/Release mechanics.

As far as I was concerned the new COM wrapper API was basically that + IUnknown identity semantics.

We tried to leverage the ClrMD wrappers, but I am told the lack of compliant identity & ICastable semantics was causing issues for us.

I would ask this question in the opposite direction (not specifically to you Steve but to the diagnostics team/@tommcdon): It's 2020, why is the CLR diagnostics layer still built on COM? Why don't we invest into converting the DBI/ICorDebug into a C# based NuGet package?

/cc @dotnet/dotnet-diag

@AaronRobinsonMSFT
Copy link
Member

@leculver The ComWrappers API might imply all of COM, but that isn't accurate. The ComWrappers is designed to support the IUnknown ABI with no additional requirements to support anything else related to COM (i.e. apartments, aggregation, etc).

just plain old C++ interfaces with Add/Release mechanics

That's not quite accurate. There is nothing "plain old" about C++ type layout. The implementation is really the IUnknown ABI. C++ has no standard ABI definition here which is why the API is called ComWrappers, but as mentioned could have more accurately been called "IUnknownWrappers".

I don't think anyone wants to support COM as it is on Windows, but the IUnknown contract is very appealing to many in the community for various interop related scenarios.

@leculver
Copy link
Contributor

We tried to leverage the ClrMD wrappers, but I am told the lack of compliant identity & ICastable semantics was causing issues for us.

Ahh yeah, that would cause issues. Good to know!

@AaronRobinsonMSFT
Copy link
Member

Speaking of ICastable - #36654. Unsure if ClrMD wrappers could add support.

@noahfalk
Copy link
Member

I would ask this question in the opposite direction (not specifically to you Steve but to the diagnostics team/@tommcdon): It's 2020, why is the CLR diagnostics layer still built on COM? Why don't we invest into converting the DBI/ICorDebug into a C# based NuGet package?

There are also native C++ consumers of the COM API (VS is a big one, SOS is another, and then we've got 3rd party tools). If we rewrote all of mscordbi in C# either we have to implement COM-interop (CCW this time) to preserve our public native COM interfaces for existing tools OR we force all tools to rewrite against a new interface. FWIW we have made slow movements towards C# over the years and I expect we'll keep doing it, but the engineering costs to shift 20 years of investment in C++ are quite sizable.

@leculver
Copy link
Contributor

(@sdmaclea I apologize for derailing this issue, I'd be happy to take further replies offline or move them into a new issue.)

Speaking of ICastable - #36654. Unsure if ClrMD wrappers could add support.

That proposal is interesting! Most of the COM wrappers in ClrMD are meant for internal use, they are only exposed as public for a "last resort" when you need to do something directly with ISOSDac or similar interfaces. (Usually it's just me trying to build direct repros for dac issues and not having to special compile ClrMD...)

If something like that ICastableObject design were implemented though I'd likely pick it up in ClrMD, assuming I could continue to support desktop CLR at the same time without needing two separate wrappers.

There are also native C++ consumers of the COM API (VS is a big one, SOS is another, and then we've got 3rd party tools). If we rewrote all of mscordbi in C# either we have to implement COM-interop (CCW this time) to preserve our public native COM interfaces for existing tools OR we force all tools to rewrite against a new interface.

Yep, I understand. I've worked in the DBI codebase and I've dug through VS's debugger implementation several times, so I know how big of a project it would be. =) I won't wade too deeply into design, here but you are right that we'd have to come up with a migration path for existing tools, but we've have built bigger projects before.

I think my more concrete feedback here is that I find it interesting that we haven't moved forward ICorDebug very much in .Net Core when I look at the file history of something like cordebug.idl. If you asked my 5-6 years ago what the future of ClrMD was I would have said "non-existent, because the functionality would mostly be in ICorDebug", but the debugger API seems stagnant.

If I could summarize my thoughts a bit differently: Steve filing this issue made me start thinking about how much value we get out of ICorDebug versus how much effort it is to maintain it and the inertia the com-based codebase generates. My feedback here is "let's dream a bit bigger here instead of just turning the crank and limping along". Moving ICorDebug or some subset of it to C# would be one way to approach that...it's not the end goal.

@noahfalk
Copy link
Member

My feedback here is "let's dream a bit bigger here instead of just turning the crank and limping along"

I don't view the lack of progress around ICorDebug as a lack of dreaming big (admitedly I am surely biased - alternate opinions are welcome). I think it is the opposite. The diagnostics team has been continually broadening our ownership and investments so that it includes a wide set of diagnostic APIs, instrumentation, infrastructure and tools of which ICorDebug is just one small part. ICorDebug hasn't changed much because we looked at all the different options where we could deploy our limited time/efforts and decided that we had many opportunities more valuable and more urgent than ICorDebug investments.

@Serentty
Copy link
Contributor

How much of COM would this allow other platforms to use through .NET? Consuming COM APIs? Creating them?

@AaronRobinsonMSFT
Copy link
Member

@Serentty The name of the API and issue implies a bit more than what the support would actually enable. The ComWrappers API is about enabling support in the runtime for efficient IUnknown identity mapping and support for the Reference Tracker API. Supporting this API in a cross-platform manner wouldn't magically enable COM everywhere rather it would enable internal/current/future tooling to target an API surface that works on all platforms. That tooling is what enables COM or WinRT in the case of C#/WinRT.

@AaronRobinsonMSFT
Copy link
Member

Unfortunately time has run out for this work. The scope here should ideally add support in Mono as well and although CoreCLR may be a minor change the work in Mono would be substantial. Moving this to .NET 6 so we can add support in unison.

@b-iotti
Copy link

b-iotti commented May 17, 2021

Good morning everyone, I'd like to announce that my team and I have been engaged to work on Linux support for the ComWrappers feature. We're working towards enabling this on Linux for x86_64 as part of the dotnet 6.0.0 release or shortly thereafter. Of course, ComWrappers doesn't live in a vacuum, so we might have to work on some related features, like comInterop, but we'd like to keep it to the bare minimum if possible. Please expect us to open new tickets and issues as we run into obstacles along this path. Of course, we'd greatly appreciate any help or guidance we can receive. Thank you!

@raffaeler
Copy link

Good to hear that @b-iotti. Could you please introduce yourself and your team please?
Thank you

@b-iotti
Copy link

b-iotti commented May 17, 2021

Sure, just a quick introduction with the relevant bits: I'm a solution architect for a large IT consulting firm. One of our customers has a pressing need for this feature, and has commissioned us to contribute it back to the project. The team is composed of myself and several engineers with proven skills in C++, C# and other applicable technologies, and past experience with COM programming.

@raffaeler
Copy link

Thank you @b-iotti
I implemented some time ago a code generator to use and expose the CoreCLR Debugger COM Abi on Linux. I may possibly help in this feature as well, time permitting.

@AaronRobinsonMSFT
Copy link
Member

@kant2002 has also expressed interest in a cross-platform solution for the ComWrappers API.

@raffaeler
Copy link

@AaronRobinsonMSFT good to hear that.
What about preparing some specs so that each one can work without overlapping with the others?
For sure you have at least a list of dos and donts.

@AaronRobinsonMSFT
Copy link
Member

What about preparing some specs so that each one can work without overlapping with the others?

The spec is really around making this build and separating the FEATURE_COMINTEROP, FEATURE_COMINTEROP_APARTMENT_SUPPORT, and FEATURE_COMWRAPPERS. There are some places where the 3 intersect that are simply bugs. I would be surprised if there are functional issues once we get it to build correctly.

For sure you have at least a list of dos and donts.

The biggest do here is FEATURE_COMWRAPPERS should be independent of any other COM related feature. The only "don't" I can think of immediately would be to copy Windows headers into the repo. Instead, we need to update the PAL to contain basic definitions/implementations of the COM related API surface area that is needed – this should be minimal (e.g. IUnknown, create some __uuidof macro or update throughout).

@b-iotti
Copy link

b-iotti commented Jun 1, 2021

PR #53503 is the first part of our contribution to this. We're getting familiar with the PR process and will then contribute more code.

Interop-CoreCLR 6.0 automation moved this from To do to Done Jul 29, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

8 participants