Convert more COM interop MethodDescCallSite to UCO#125508
Convert more COM interop MethodDescCallSite to UCO#125508am11 wants to merge 19 commits intodotnet:mainfrom
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Failures are unrelated (iOS dead-lettered). |
AaronRobinsonMSFT
left a comment
AaronRobinsonMSFT
left a comment
There was a problem hiding this comment.
One minor request for an assert. Other than that, 
| : ByReference.Create(ref arg); | ||
| } | ||
|
|
||
| return RuntimeMethodHandle.InvokeMethod(target, (void**)pByRefFixedStorage, signature, isConstructor: false); |
There was a problem hiding this comment.
RuntimeMethodHandle.InvokeMethod is a wrapper around CallDescrWorker that we will need to get rid of to get rid of CallDescrWorker. This change is not helping with that - this code will need to be redone.
It would be better to wait with converting these after we have eliminated CallDescrWorker use in RuntimeMethodHandle.InvokeMethod
There was a problem hiding this comment.
One approach is to avoid InvokeMethod like this am11@e5fc85e. Is this enough or do we want to keep using InvokeMethod and replace CallDescrWorkerWithHandler machinery from it?
There was a problem hiding this comment.
InvokeDelegateConstructor and InvokeVoidMethodWithOneArg looks good.
For InvokeArgSlotMethodWithOneArg, do we really need the full set of types for this COM interop path? Do we have test coverage for all the cases?
There was a problem hiding this comment.
I kept InvokeDelegateConstructor and InvokeVoidMethodWithOneArg as-is.
For InvokeArgSlotMethodWithOneArg, I reduced the supported return types to a narrower COM-event-oriented set (void, bool, int/uint, long/ulong, IntPtr/UIntPtr) and removed the broader primitive/reference conversion matrix. Not sure about the test coverage status. Perhaps, @AaronRobinsonMSFT might have an idea? I ran a few interop tests under src/tests/Interop and they seem to be passing.
There was a problem hiding this comment.
I ran a few interop tests under src/tests/Interop and they seem to be passing.
Do they hit this method?
InvokeArgSlotMethodWithOneArg
Maybe leave this out from this PR so the rest can be merged?
There was a problem hiding this comment.
At least void-return seems to be exercised by COM event tests in src/tests/Interop/COM/NETClients/Events/Program.cs (both OnEvent +=/-= and the ComAwareEventInfo add/remove flow). Those calls go through the COM event interface wiring in src/tests/Interop/COM/ServerContracts/Server.Events.cs, then into CLRToCOMEventCallWorker in src/coreclr/vm/clrtocomcall.cpp, which invokes StubHelpers.InvokeClrToComEventProviderMethod, and that calls InvokeArgSlotMethodWithOneArg.
The current test shape here is still void-return (FireEvent and OnEvent are void in src/tests/Interop/COM/ServerContracts/Server.Contracts.cs), so this confirms the helper is hit and the void branch is covered, but this test alone does not go to all non-void branches of the type-dispatch logic. There are other tests as well.
There was a problem hiding this comment.
this test alone does not go to all non-void branches of the type-dispatch logic
Do we have tests that go to any of the non-void branches?
There was a problem hiding this comment.
The COM events tests are a bit sparse. We only added in what was obviously needed for events leveaged by Office PIAs.
| ((delegate*<object, object?, nint, void>)delegateCtorMethodEntryPoint)(*pDelegate, *pSubscriber, (nint)pEventMethodCodePtr); | ||
| } | ||
|
|
||
| nint providerMethodEntryPoint = (nint)pProviderMethodPtr; |
There was a problem hiding this comment.
nint and IntPtr are the same type. Are these casts really needed?
| &gc.ThisObj, | ||
| &gc.EventProviderTypeObj, | ||
| (INT_PTR)pEvProvMD, | ||
| (INT_PTR)pEvProvMD->GetMultiCallableAddrOfCode(), |
There was a problem hiding this comment.
These can use GetSingleCallableAddrOfCode() since the function pointer is used exactly once.
| return 0; | ||
| } | ||
|
|
||
| if (returnType == typeof(bool)) |
There was a problem hiding this comment.
How did you come up with this list of supported return types?
There was a problem hiding this comment.
The return-type list was not based on a separate runtime contract; it was a conservative subset of ARG_SLOT-sized primitives chosen while removing InvokeMethod from this path.
There was a problem hiding this comment.
I am not following. Why are byte, double or enums missing in the list? is it not possible for COM events to return these types? It looks suspect to me that there is nint in the list, but no byte.
There was a problem hiding this comment.
I reduced the full set of types after your comment: #125508 (comment). I think we need some test coverage for non-void types under src/test/Inerop/COM before modifying it further.
3 participants
Contributes to #123864 (group 5: except dispatchinfo.cpp)