From 45db259af1b149fbffe2b22366c6e772cb3158ee Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 21 Sep 2023 16:56:20 -0700 Subject: [PATCH 1/4] IDispatch should accept HRESULT as valuetype This is a regression from .NET Framework. The current behavior has existed since IDispatch was introduced into .NET Core. Added tests for the current behavior. --- src/coreclr/vm/callsiteinspect.cpp | 3 +- src/coreclr/vm/callsiteinspect.h | 1 + src/coreclr/vm/clrtocomcall.cpp | 9 ++++-- .../COM/NETClients/IDispatch/Program.cs | 15 +++++++++ .../Interop/COM/NETServer/DispatchTesting.cs | 1 + .../COM/NativeServer/DispatchTesting.h | 2 ++ .../COM/ServerContracts/Server.Contracts.cs | 31 +++++++++++++++++++ .../COM/ServerContracts/Server.Contracts.h | 1 + 8 files changed, 60 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/callsiteinspect.cpp b/src/coreclr/vm/callsiteinspect.cpp index dabbe89a497720..8209e41e6a7d44 100644 --- a/src/coreclr/vm/callsiteinspect.cpp +++ b/src/coreclr/vm/callsiteinspect.cpp @@ -433,7 +433,8 @@ void CallsiteInspect::PropagateOutParametersBackToCallsite( *(ARG_SLOT *)(frame->GetReturnValuePtr()) = retVal; } #ifdef ENREGISTERED_RETURNTYPE_MAXSIZE - else if (argit.HasNonStandardByvalReturn()) + else if (argit.HasNonStandardByvalReturn() + && !(flags & CallsiteDetails::HResultReturn)) { // In these cases, put the pointer to the return buffer into // the frame's return value slot. diff --git a/src/coreclr/vm/callsiteinspect.h b/src/coreclr/vm/callsiteinspect.h index 373b9347dfd9c9..4ca66eca9feba2 100644 --- a/src/coreclr/vm/callsiteinspect.h +++ b/src/coreclr/vm/callsiteinspect.h @@ -25,6 +25,7 @@ struct CallsiteDetails BeginInvoke = 0x01, EndInvoke = 0x02, Ctor = 0x04, + HResultReturn = 0x08, }; INT32 Flags; }; diff --git a/src/coreclr/vm/clrtocomcall.cpp b/src/coreclr/vm/clrtocomcall.cpp index 06d28f507249b4..f92b6e699a4ae7 100644 --- a/src/coreclr/vm/clrtocomcall.cpp +++ b/src/coreclr/vm/clrtocomcall.cpp @@ -364,7 +364,7 @@ UINT32 CLRToCOMEventCallWorker(ComPlusMethodFrame* pFrame, ComPlusCallMethodDesc return 0; } -CallsiteDetails CreateCallsiteDetails(_In_ FramedMethodFrame *pFrame) +static CallsiteDetails CreateCallsiteDetails(_In_ FramedMethodFrame *pFrame) { CONTRACTL { @@ -442,10 +442,15 @@ CallsiteDetails CreateCallsiteDetails(_In_ FramedMethodFrame *pFrame) SigTypeContext::InitTypeContext(pMD, actualType, &typeContext); } + // If the signature is marked preserve sig, then the return + // is required to be an HRESULT. + if (IsMiPreserveSig(pMD->GetImplAttrs())) + callsiteFlags |= CallsiteDetails::HResultReturn; + _ASSERTE(!signature.IsEmpty() && pModule != nullptr); // Create details - return CallsiteDetails{ { signature, pModule, &typeContext }, pFrame, pMD, fIsDelegate }; + return CallsiteDetails{ { signature, pModule, &typeContext }, pFrame, pMD, fIsDelegate, callsiteFlags }; } UINT32 CLRToCOMLateBoundWorker( diff --git a/src/tests/Interop/COM/NETClients/IDispatch/Program.cs b/src/tests/Interop/COM/NETClients/IDispatch/Program.cs index 71839d9afffb8f..fc9144575f0d91 100644 --- a/src/tests/Interop/COM/NETClients/IDispatch/Program.cs +++ b/src/tests/Interop/COM/NETClients/IDispatch/Program.cs @@ -139,6 +139,21 @@ static void Validate_Exception() Assert.Equal(GetErrorCodeFromHResult(e.HResult), errorCode); // Failing HRESULT exceptions contain CLR generated messages } + + // Calling methods through IDispatch::Invoke() (i.e., late-bound) doesn't + // propagate the HRESULT when marked with PreserveSig. It is always 0. + { + Console.WriteLine($"Calling {nameof(DispatchTesting.TriggerException)} (PreserveSig) with {nameof(IDispatchTesting_Exception.Int)} {errorCode}..."); + var dispatchTesting2 = (IDispatchTestingPreserveSig1)dispatchTesting; + Assert.Equal(0, dispatchTesting2.TriggerException(IDispatchTesting_Exception.Int, errorCode)); + } + + { + // Validate the HRESULT as a value type construct works for IDispatch. + Console.WriteLine($"Calling {nameof(DispatchTesting.TriggerException)} (PreserveSig, ValueType) with {nameof(IDispatchTesting_Exception.Int)} {errorCode}..."); + var dispatchTesting3 = (IDispatchTestingPreserveSig2)dispatchTesting; + Assert.Equal(0, dispatchTesting3.TriggerException(IDispatchTesting_Exception.Int, errorCode).Value); + } } static void Validate_StructNotSupported() diff --git a/src/tests/Interop/COM/NETServer/DispatchTesting.cs b/src/tests/Interop/COM/NETServer/DispatchTesting.cs index 477e5751f69e73..66461b8c7e47f2 100644 --- a/src/tests/Interop/COM/NETServer/DispatchTesting.cs +++ b/src/tests/Interop/COM/NETServer/DispatchTesting.cs @@ -57,6 +57,7 @@ public void TriggerException(IDispatchTesting_Exception excep, int errorCode) case IDispatchTesting_Exception.Disp: throw new Exception(); case IDispatchTesting_Exception.HResult: + case IDispatchTesting_Exception.Int: throw new System.ComponentModel.Win32Exception(errorCode); } } diff --git a/src/tests/Interop/COM/NativeServer/DispatchTesting.h b/src/tests/Interop/COM/NativeServer/DispatchTesting.h index 927439fe03dc48..fbe7db6c1bad7f 100644 --- a/src/tests/Interop/COM/NativeServer/DispatchTesting.h +++ b/src/tests/Interop/COM/NativeServer/DispatchTesting.h @@ -243,6 +243,8 @@ class DispatchTesting : public UnknownImpl, public IDispatchTesting return DISP_E_EXCEPTION; case IDispatchTesting_Exception_HResult: return HRESULT_FROM_WIN32(errorCode); + case IDispatchTesting_Exception_Int: + return errorCode; default: return S_FALSE; // Return a success case to indicate failure to trigger a failure. } diff --git a/src/tests/Interop/COM/ServerContracts/Server.Contracts.cs b/src/tests/Interop/COM/ServerContracts/Server.Contracts.cs index 758c200acaabae..4ded718636c3bf 100644 --- a/src/tests/Interop/COM/ServerContracts/Server.Contracts.cs +++ b/src/tests/Interop/COM/ServerContracts/Server.Contracts.cs @@ -209,6 +209,7 @@ public enum IDispatchTesting_Exception { Disp, HResult, + Int, } [StructLayout(LayoutKind.Sequential)] @@ -220,6 +221,12 @@ public struct HFA_4 public float w; } + [StructLayout(LayoutKind.Sequential)] + public struct HRESULT + { + public int Value; + } + [ComVisible(true)] [Guid("a5e04c1c-474e-46d2-bbc0-769d04e12b54")] [InterfaceType(ComInterfaceType.InterfaceIsIDispatch)] @@ -257,6 +264,30 @@ void DoubleNumeric_ReturnByRef ( System.Collections.IEnumerator GetEnumerator(); } + [ComVisible(true)] + [Guid("a5e04c1c-474e-46d2-bbc0-769d04e12b54")] + [InterfaceType(ComInterfaceType.InterfaceIsIDispatch)] + public interface IDispatchTestingPreserveSig1 + { + void Reserved1(); + void Reserved2(); + + [PreserveSig] + int TriggerException(IDispatchTesting_Exception excep, int errorCode); + } + + [ComVisible(true)] + [Guid("a5e04c1c-474e-46d2-bbc0-769d04e12b54")] + [InterfaceType(ComInterfaceType.InterfaceIsIDispatch)] + public interface IDispatchTestingPreserveSig2 + { + void Reserved1(); + void Reserved2(); + + [PreserveSig] + HRESULT TriggerException(IDispatchTesting_Exception excep, int errorCode); + } + [ComVisible(true)] [Guid("83AFF8E4-C46A-45DB-9D91-2ADB5164545E")] [InterfaceType(ComInterfaceType.InterfaceIsIDispatch)] diff --git a/src/tests/Interop/COM/ServerContracts/Server.Contracts.h b/src/tests/Interop/COM/ServerContracts/Server.Contracts.h index 3c9a1fcb06cbe1..1eb0528aae4b78 100644 --- a/src/tests/Interop/COM/ServerContracts/Server.Contracts.h +++ b/src/tests/Interop/COM/ServerContracts/Server.Contracts.h @@ -385,6 +385,7 @@ enum IDispatchTesting_Exception { IDispatchTesting_Exception_Disp, IDispatchTesting_Exception_HResult, + IDispatchTesting_Exception_Int, }; struct __declspec(uuid("a5e04c1c-474e-46d2-bbc0-769d04e12b54")) From 1bb47b694f03749aaf058e059dafebdda1cd02d1 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 21 Sep 2023 17:00:30 -0700 Subject: [PATCH 2/4] Ensure vtables line up for early bound. --- src/tests/Interop/COM/ServerContracts/Server.Contracts.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/tests/Interop/COM/ServerContracts/Server.Contracts.cs b/src/tests/Interop/COM/ServerContracts/Server.Contracts.cs index 4ded718636c3bf..0bac21e66ee17e 100644 --- a/src/tests/Interop/COM/ServerContracts/Server.Contracts.cs +++ b/src/tests/Interop/COM/ServerContracts/Server.Contracts.cs @@ -271,6 +271,7 @@ public interface IDispatchTestingPreserveSig1 { void Reserved1(); void Reserved2(); + void Reserved3(); [PreserveSig] int TriggerException(IDispatchTesting_Exception excep, int errorCode); @@ -283,6 +284,7 @@ public interface IDispatchTestingPreserveSig2 { void Reserved1(); void Reserved2(); + void Reserved3(); [PreserveSig] HRESULT TriggerException(IDispatchTesting_Exception excep, int errorCode); From d4e5b2179f84ff260a87f9952bb20ca18cd5d103 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 21 Sep 2023 17:20:14 -0700 Subject: [PATCH 3/4] Add larger comment to workaround. --- src/coreclr/vm/clrtocomcall.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/clrtocomcall.cpp b/src/coreclr/vm/clrtocomcall.cpp index f92b6e699a4ae7..f3bcedb28193f6 100644 --- a/src/coreclr/vm/clrtocomcall.cpp +++ b/src/coreclr/vm/clrtocomcall.cpp @@ -443,7 +443,12 @@ static CallsiteDetails CreateCallsiteDetails(_In_ FramedMethodFrame *pFrame) } // If the signature is marked preserve sig, then the return - // is required to be an HRESULT. + // is required to be an HRESULT, per COM rules. We set a flag to + // indicate this state to avoid issues when a C# developers define + // an HRESULT in C# as a ValueClass with a single int field. This + // is convenient but does violate the COM ABI. Setting the flag + // lets us permit this convention and allow either a 4 byte primitive + // or the commonly used C# type "struct HResult { int Value; }". if (IsMiPreserveSig(pMD->GetImplAttrs())) callsiteFlags |= CallsiteDetails::HResultReturn; From 635c52c046c39e3b374a559e7fb8615c423f4367 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 21 Sep 2023 17:21:48 -0700 Subject: [PATCH 4/4] Comment --- src/coreclr/vm/clrtocomcall.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/clrtocomcall.cpp b/src/coreclr/vm/clrtocomcall.cpp index f3bcedb28193f6..c604a6c8a90116 100644 --- a/src/coreclr/vm/clrtocomcall.cpp +++ b/src/coreclr/vm/clrtocomcall.cpp @@ -444,7 +444,7 @@ static CallsiteDetails CreateCallsiteDetails(_In_ FramedMethodFrame *pFrame) // If the signature is marked preserve sig, then the return // is required to be an HRESULT, per COM rules. We set a flag to - // indicate this state to avoid issues when a C# developers define + // indicate this state to avoid issues when a C# developer defines // an HRESULT in C# as a ValueClass with a single int field. This // is convenient but does violate the COM ABI. Setting the flag // lets us permit this convention and allow either a 4 byte primitive