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

Special casing System.Guid for COM VARIANT marshalling #100377

Merged
merged 11 commits into from
Apr 5, 2024

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Mar 27, 2024

The majority of the PR contains tests changes.

This is a basic proposal for how marshalling of System.Guid could be supported for those porting from .NET Framework to .NET 8+. This PR also adds testing for the most basic VARIANT scenarios that would be supported.

At present, marshalling of any .NET provided value type is difficult since .NET doesn't provide a TLB. This means that common "primitive" types like System.Guid can't be marshalled in an VARIANT (that is, object).

/cc @dotnet/interop-contrib @jkotas

VARIANT marshalling in .NET 5+ requires a TLB
for COM records (i.e., ValueType instances). This
means that without a runtime provided TLB, users
must define their own TLB for runtime types or
define their own transfer types.

We address this here by deferring to the NetFX
mscorlib's TLB.
@AaronRobinsonMSFT AaronRobinsonMSFT added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-Interop-coreclr labels Mar 27, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 9.0.0 milestone Mar 27, 2024
@huoyaoyuan
Copy link
Member

huoyaoyuan commented Mar 28, 2024

Well I've just locally done the refactor to convert VARIANT marshalling to mostly managed yesterday, for the follow up of #100176
This should be representable in managed code. If the TLB and type ID matches GUID, return a GUID. Doing in native code looks more consistent though.

@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review March 28, 2024 21:21
@AaronRobinsonMSFT AaronRobinsonMSFT removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 28, 2024
@AaronRobinsonMSFT
Copy link
Member Author

@dotnet/interop-contrib @jkotas I've investigated various COM-RPC scenarios involving boxing and resulting IUnknown scenarios that could cause an indirect .NET Framework activation. I've not been able to identify anything. Debugging the COM-RPC scenarios, it seems since the IRecordInfo case doesn't expose interfaces of the marshalled type there is no look up other than the struct details itself.

I can't see any risks with this support other than enabling a tricky scenario that has impacted many large enterprise COM based products.

src/coreclr/vm/stdinterfaces.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stdinterfaces.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/stdinterfaces.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/olevariant.cpp Show resolved Hide resolved
AaronRobinsonMSFT and others added 2 commits April 4, 2024 08:59
Co-authored-by: Elinor Fung <elfung@microsoft.com>
@jkoritzinsky
Copy link
Member

Do we want to add support in source-generated COM as well (as a separate PR, not in this one)?

I'd rather not (as implementing it in a portable manner would be very difficult), but I wanted to raise the issue.

@AaronRobinsonMSFT
Copy link
Member Author

Do we want to add support in source-generated COM as well (as a separate PR, not in this one)?

I'd rather not (as implementing it in a portable manner would be very difficult), but I wanted to raise the issue.

I don't think so. Our model and expectations for the source generated approach seem different. It sounds like you would agree. I think we should look at the entire source generated solution as green field and we should create solutions that are tailored to its design. This is merely an affordance for built-in COM transitions from .NET Framework.

Co-authored-by: Elinor Fung <elfung@microsoft.com>
@AaronRobinsonMSFT
Copy link
Member Author

/backport to release/8.0-staging

Copy link
Contributor

github-actions bot commented Apr 5, 2024

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8576746637

@AaronRobinsonMSFT AaronRobinsonMSFT deleted the sysguid_commarshal branch April 5, 2024 23:14
Copy link
Contributor

github-actions bot commented Apr 5, 2024

@AaronRobinsonMSFT backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Support System.Guid marshalling via VARIANT
Applying: Add marshalling of System.Guid in the other direction.
Applying: Adding testing for COM VARIANT marshalling
Applying: Fix contract
Applying: Fix libraries tests
Applying: Apply suggestions from code review
Applying: Review feedback
Applying: Add Guid test for GetObjectForNativeVariant()
Applying: Handle the Nano server case.
Applying: Add test for dynamic scenarios
Using index info to reconstruct a base tree...
A	src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/ComInterop/DynamicVariantExtensions.cs
A	src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/ComVariant.cs
M	src/tests/Interop/COM/Dynamic/BasicTest.cs
Falling back to patching base and 3-way merge...
Auto-merging src/tests/Interop/COM/Dynamic/BasicTest.cs
CONFLICT (modify/delete): src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/ComVariant.cs deleted in HEAD and modified in Add test for dynamic scenarios. Version Add test for dynamic scenarios of src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/ComVariant.cs left in tree.
CONFLICT (modify/delete): src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/ComInterop/DynamicVariantExtensions.cs deleted in HEAD and modified in Add test for dynamic scenarios. Version Add test for dynamic scenarios of src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/ComInterop/DynamicVariantExtensions.cs left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0010 Add test for dynamic scenarios
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

github-actions bot commented Apr 5, 2024

@AaronRobinsonMSFT an error occurred while backporting to release/8.0-staging, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

AaronRobinsonMSFT added a commit to AaronRobinsonMSFT/runtime that referenced this pull request Apr 7, 2024
* Support System.Guid marshalling via VARIANT

VARIANT marshalling in .NET 5+ requires a TLB
for COM records (i.e., ValueType instances). This
means that without a runtime provided TLB, users
must define their own TLB for runtime types or
define their own transfer types.

We address this here by deferring to the NetFX
mscorlib's TLB.

Co-authored-by: Elinor Fung <elfung@microsoft.com>
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* Support System.Guid marshalling via VARIANT

VARIANT marshalling in .NET 5+ requires a TLB
for COM records (i.e., ValueType instances). This
means that without a runtime provided TLB, users
must define their own TLB for runtime types or
define their own transfer types.

We address this here by deferring to the NetFX
mscorlib's TLB.

Co-authored-by: Elinor Fung <elfung@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants