Simple ComWrappers APIs #120114

rcj1 · 2025-09-25T20:00:32Z

Adding GetComWrappersCCWData, IsComWrappersCCW, and IsComWrappersRCW cDAC APIs
Fixing DAC bug on ARM. Previously DACGetComWrappersCCWVTableQIAddress had cleared the thumb code on the qiAddress, only for it to be compared with an entrypoint with the thumb code. I just removed the clearing of the thumb code.
Renaming GetPrimitiveType to GetBinderType to be used more broadly to get method tables stored in the CoreLibBinder

Copilot

Pull Request Overview

This pull request implements cDAC (Contract-based Data Access Component) APIs for simple ComWrappers functionality. The main purpose is to add three new cDAC APIs: GetComWrappersCCWData, IsComWrappersCCW, and IsComWrappersRCW, while also fixing a DAC bug on ARM architecture and renaming GetPrimitiveType to GetBinderType for broader use.

Key changes include:

Implementation of three new ComWrappers cDAC APIs with full functionality
ARM DAC bug fix by removing incorrect thumb code clearing
Renaming and broadening the scope of type lookup functionality

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
SOSDacImpl.cs	Implements the three new ComWrappers cDAC APIs and updates method name usage
ManagedObjectWrapperLayout.cs	New data contract for managed object wrapper reference count
ManagedObjectWrapperHolderObject.cs	New data contract for managed object wrapper holder
SignatureTypeProvider.cs	Updates method call to use new GetBinderType name
RuntimeTypeSystem_1.cs	Renames GetPrimitiveType to GetBinderType for broader usage
ComWrappers_1.cs	Implements core ComWrappers contract functionality
Constants.cs	Adds global constants for ComWrappers functionality
DataType.cs	Adds new data types for managed object wrapper contracts
IRuntimeTypeSystem.cs	Updates interface to use GetBinderType instead of GetPrimitiveType
IComWrappers.cs	Adds new method signatures to ComWrappers interface
interoplibinterface_comwrappers.h	Adds cdac_data template specialization for ManagedObjectWrapperHolderObject
datadescriptor.inc	Adds new data type definitions and global variables
datadescriptor.h	Includes ComWrappers header for feature support
appdomain.cpp	Caches NativeObjectWrapper method table for cDAC usage
interoplibabi.h	Adds cdac_data template specialization for ManagedObjectWrapperLayout
comwrappers.hpp	Adds forward declarations for QueryInterface methods
request.cpp	Fixes ARM DAC bug by removing thumb code clearing
SignatureDecoder.md	Updates documentation to reflect GetBinderType rename
ComWrappers.md	Updates documentation with new API definitions and implementation details

dotnet-policy-service · 2025-09-25T20:01:53Z

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

jkotas · 2025-09-25T20:22:08Z

src/coreclr/vm/appdomain.cpp


+        // cache the method table here for cDAC IsComWrappersRCW
+    #ifdef FEATURE_COMWRAPPERS
+        CoreLibBinder::GetClass(CLASS__NATIVE_OBJECT_WRAPPER);


What would it take to avoid this? Can we check for the type by name instead?

ComWrappers are only used by a fraction of applications. It would be unfortunate to bring them ComWrappers in during startup of every apps.

It depends on how much input validation we want to do for DumpRCW in SOS.

If we want to do the minimum validation (only validate which type of RCW to try to dump: built-in COM vs ComWrappers), then we can drop this type check.

I tried to make the checks more in-depth to prevent giving out bad info if an invalid pointer is passed to DumpRCW, but we can make this more lax if desired.

@jkotas in the DAC we do look for the type by name, which boils down into FindClassModuleThrowing.
Basically, it iterates through all of the assembly's modules until a name match is found in a module's AvailableClassHashTable.

Figured this wouldn't be worth the inefficiency and code complexity when we can just store it in the array that's already in CoreLibBinder.

@jkoritzinsky IsComWrappersRCW is directly exposed to the user via DumpRCW, who could then input any value, so I believe we should have the validation.

@jkotas in the DAC we do look for the type by name, which boils down into FindClassModuleThrowing.
Basically, it iterates through all of the assembly's modules until a name match is found in a module's AvailableClassHashTable.

Checking whether the type is of a specific name and getting a type of a specific name are very common operations needed by higher-level debugger functionality. It is going to important for them to be fast. We do not want to make them fast by hardcoding every type that may be interesting for higher-level debugger functionality.

Here is an example from diagnostic repo where higher level debugger functionality checks type name: https://github.com/dotnet/diagnostics/blob/46eddddffcb272342504d143b46cba130783e75b/src/Microsoft.Diagnostics.ExtensionCommands/DumpAsyncCommand.cs#L884-L894 . There is a lot of code like that in the closed source Visual Studio debugger.

Historically, COM interop has been intertwined with the core runtime, and it is why a lot of it is in the legacy DAC APIs. We have been pushing COM interop out of core runtime where possible. We should keep doing the same here.

I think we should:

Do not force initialization of NativeObjectWrapper during runtime startup just to make cDAC work. Startup is our number one perf problem. We should not be making it worse. (It is death by thousand paper cuts problem. As you can see, there are many types initialized already for various reasons to do anything. I would rather see us working on deleting from the list of types that must be initialized.)

Keep CoreLibBinder in cDac as is, just for core primitive types

Use name-based checks and lookups for NativeObjectWrapper

Open an issue to look into perf of make name-based checks and lookups if we do not have one already

…mwrappers

max-charlamb · 2025-09-26T20:30:41Z

src/coreclr/debug/daccess/request.cpp

-
-#ifdef TARGET_ARM
-    // clear the THUMB bit on qiAddress before comparing with known vtable entry
-    *qiAddress &= ~THUMB_CODE;
-#endif
-


Could we handle this outside the function?
In theory this would cause problems with the other usage of DACGetComWrappersCCWVTableQIAddress in ClrDataAccess::DumpStowedExceptionObject. While it shouldn't matter because we don't build Windows Arm32 the other code path won't be exercised, I still think it is better practice as that would be an annoying bug to track down if we ever do run into that path.

Additionally, my understanding is that we should (but don't always) use PCODE to represent a value with masking bits rather than a TADDR.

runtime/src/coreclr/inc/daccess.h

Lines 2469 to 2471 in 5925e65

// A PCODE is a valid PC/IP value -- a pointer to an instruction, possibly including some processor mode bits.

// (On ARM, for example, a PCODE value should have the low-order THUMB_CODE bit set if the code should

// be executed in that mode.)

I see, we can keep this code then and replace GetEEFuncEntryPoint with GFN_TADDR as this does not set the thumb code.

src/coreclr/interop/inc/interoplibabi.h

...e/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ComWrappers_1.cs

jkoritzinsky · 2025-09-27T01:03:53Z

src/coreclr/interop/comwrappers.hpp

Move the changes in this file into interoplibabi.h

jkoritzinsky · 2025-09-27T01:05:40Z

src/coreclr/vm/datadescriptor/datadescriptor.h

+#ifdef FEATURE_COMWRAPPERS
+#include "../interop/comwrappers.hpp"
+#endif // FEATURE_COMWRAPPERS


Can be removed once the definitions in comwrappers.cpp are moved to interoplibabi.h.

Suggested change

#ifdef FEATURE_COMWRAPPERS

#include "../interop/comwrappers.hpp"

#endif // FEATURE_COMWRAPPERS

jkoritzinsky · 2025-09-27T01:10:27Z

src/coreclr/interop/comwrappers.hpp

+struct ComWrappersVtablePtrs
+{
+    decltype(&ManagedObjectWrapper_QueryInterface) MowQueryInterface;
+    decltype(&TrackerTarget_QueryInterface) TtQueryInterface;
+};
+
+const ComWrappersVtablePtrs g_ComWrappersVtablePtrs = {
+    &ManagedObjectWrapper_QueryInterface,
+    &TrackerTarget_QueryInterface
+};


Can we instead do this as an array of interesting function pointers (and initialize the value in comwrappers.cpp)?

That way we don't need to hard-code these names across the interoplib/VM/DAC boundary.

Something like the following:

// in interoplibabi.h namespace InteropLib::ABI { using QueryInterfaceMethod = HRESULT(STDMETHODCALLTYPE *)(_In_ ComInterfaceDispatch* disp, REFIID riid, _COM_Outptr_ void __RPC_FAR* __RPC_FAR* ppvObject); constexpr size_t KnownQueryInterfaceImplementationsCount = 2; extern QueryInterfaceMethod KnownQueryInterfaceImplementations[KnownQueryInterfaceImplementationsCount]; } // in comwrappers.cpp QueryInterfaceMethod InteropLib::ABI:KnownQueryInterfaceImplementations[InteropLib::ABI::KnownQueryInterfaceImplementationsCount] = { &ManagedObjectWrapper_QueryInterface, &TrackerTarget_QueryInterface }

Then (if the DAC is moved to use this table like the CDAC), we could even move these methods back into the anonymous namespace and not expose the symbols (which we'd prefer if possible).

We can do this, the downside that I see is it would be slightly less readable.. Instead of GFUNC(ManagedObjectWrapper_QueryInterface) we would be reading g_interestingPtrs[0]. If you are ok with this then I'd be happy to work on this and just add some comments about which pointer is where.

I'm fine with that (as long as the same is something like the name I gave above).

jkoritzinsky · 2025-09-27T01:14:28Z

src/coreclr/vm/datadescriptor/datadescriptor.inc

+CDAC_TYPE_BEGIN(ComWrappersVtablePtrs)
+CDAC_TYPE_FIELD(ComWrappersVtablePtrs, /*pointer*/, MowQueryInterface, offsetof(ComWrappersVtablePtrs, MowQueryInterface))
+CDAC_TYPE_FIELD(ComWrappersVtablePtrs, /*pointer*/, TtQueryInterface, offsetof(ComWrappersVtablePtrs, TtQueryInterface))
+CDAC_TYPE_END(ComWrappersVtablePtrs)


We could instead structure this as a well-known size struct instead of with individual members (using the idea in the comment in comwrappers.hpp):

Suggested change

CDAC_TYPE_BEGIN(ComWrappersVtablePtrs)

CDAC_TYPE_FIELD(ComWrappersVtablePtrs, /*pointer*/, MowQueryInterface, offsetof(ComWrappersVtablePtrs, MowQueryInterface))

CDAC_TYPE_FIELD(ComWrappersVtablePtrs, /*pointer*/, TtQueryInterface, offsetof(ComWrappersVtablePtrs, TtQueryInterface))

CDAC_TYPE_END(ComWrappersVtablePtrs)

CDAC_TYPE_BEGIN(ComWrappersVtablePtrs)

CDAC_TYPE_SIZE(sizeof(InteropLib::ABI:KnownQueryInterfaceImplementations))

CDAC_TYPE_END(ComWrappersVtablePtrs)

Again, slight decrease in readability

rcj1 added 3 commits September 21, 2025 16:16

e

2888d44

merge

d52af16

adding simple comwrappers cDAC APIs

8a54a7a

rcj1 requested review from jkoritzinsky, noahfalk, jkotas, davidwrighton, hoyosjs, max-charlamb and Copilot September 25, 2025 20:00

Copilot AI reviewed Sep 25, 2025

View reviewed changes

github-actions bot added the area-Diagnostics-coreclr label Sep 25, 2025

dotnet-policy-service bot assigned rcj1 Sep 25, 2025

rcj1 mentioned this pull request Sep 24, 2025

[cDAC] Support APIs used by SOS #117782

Open

jkotas reviewed Sep 25, 2025

View reviewed changes

rcj1 added 5 commits September 25, 2025 15:00

uint

1a145d9

fixing function pointer cast

82361c3

Merge branch 'main' into comwrappers

e42c7fa

e

8439a7d

Merge branch 'comwrappers' of https://github.com/rcj1/runtime into co…

f68b75c

…mwrappers

max-charlamb reviewed Sep 26, 2025

View reviewed changes

code review

759f27f

jkoritzinsky reviewed Sep 27, 2025

View reviewed changes

AaronRobinsonMSFT added this to the 11.0.0 milestone Oct 2, 2025

	// A PCODE is a valid PC/IP value -- a pointer to an instruction, possibly including some processor mode bits.
	// (On ARM, for example, a PCODE value should have the low-order THUMB_CODE bit set if the code should
	// be executed in that mode.)

	#ifdef FEATURE_COMWRAPPERS
	#include "../interop/comwrappers.hpp"
	#endif // FEATURE_COMWRAPPERS

Simple ComWrappers APIs #120114

Are you sure you want to change the base?

Simple ComWrappers APIs #120114

Conversation

rcj1 commented Sep 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

dotnet-policy-service bot commented Sep 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcj1 Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkotas Sep 28, 2025 •

edited

Loading

rcj1 Sep 26, 2025 •

edited

Loading