Convert exception help context parsing to managed (#70251)

* Move help link parsing to managed

* Cleanup wtoi and nativeIsDigit

* Fix metasig declaration

* Guard GetHelpContext under FEATURE_COMINTEROP

* Remove definition of GetHelpLink and guard more methods under cominterop

* DWORD should be uint

* Add help context marshaling test

* Adjust marshaling test

* Fix #ifdef with ILLink

* Fix method signature mismatch

* Specify string marshaling

* Throwing test should not test successful HRESULT

* Implement ISupportErrorInfo

* Test interface in InterfaceSupportsErrorInfo

src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs

@@ -274,5 +274,32 @@ private bool CanSetRemoteStackTrace()
 
             return true;
         }
+
+        // used by vm
+        internal string? GetHelpContext(out uint helpContext)
+        {
+            helpContext = 0;
+            string? helpFile = HelpLink;
+
+            int poundPos, digitEnd;
+
+            if (helpFile is null || (poundPos = helpFile.LastIndexOf('#')) == -1)
+            {
+                return helpFile;
+            }
+
+            for (digitEnd = poundPos + 1; digitEnd < helpFile.Length; digitEnd++)
+            {
+                if (char.IsWhiteSpace(helpFile[digitEnd]))
+                    break;
+            }
+
+            if (uint.TryParse(helpFile.AsSpan(poundPos + 1, digitEnd - poundPos - 1), out helpContext))
+            {
+                helpFile = helpFile.Substring(0, poundPos);
+            }
+
+            return helpFile;
+        }
     }
 }

src/coreclr/vm/comutilnative.cpp

@@ -41,69 +41,6 @@
 
 #include "arraynative.inl"
 
-/*===================================IsDigit====================================
-**Returns a bool indicating whether the character passed in represents a   **
-**digit.
-==============================================================================*/
-bool IsDigit(WCHAR c, int radix, int *result)
-{
-    CONTRACTL
-    {
-        NOTHROW;
-        GC_NOTRIGGER;
-        MODE_ANY;
-        PRECONDITION(CheckPointer(result));
-    }
-    CONTRACTL_END;
-
-    if (IS_DIGIT(c)) {
-        *result = DIGIT_TO_INT(c);
-    }
-    else if (c>='A' && c<='Z') {
-        //+10 is necessary because A is actually 10, etc.
-        *result = c-'A'+10;
-    }
-    else if (c>='a' && c<='z') {
-        //+10 is necessary because a is actually 10, etc.
-        *result = c-'a'+10;
-    }
-    else {
-        *result = -1;
-    }
-
-    if ((*result >=0) && (*result < radix))
-        return true;
-
-    return false;
-}
-
-INT32 wtoi(_In_reads_(length) WCHAR* wstr, DWORD length)
-{
-    CONTRACTL
-    {
-        NOTHROW;
-        GC_NOTRIGGER;
-        MODE_ANY;
-        PRECONDITION(CheckPointer(wstr));
-        PRECONDITION(length >= 0);
-    }
-    CONTRACTL_END;
-
-    DWORD i = 0;
-    int value;
-    INT32 result = 0;
-
-    while ( (i < length) && (IsDigit(wstr[i], 10 ,&value)) ) {
-        //Read all of the digits and convert to a number
-        result = result*10 + value;
-        i++;
-    }
-
-    return result;
-}
-
-
-
 //
 //
 // EXCEPTION NATIVE
@@ -371,79 +308,17 @@ static void GetExceptionHelp(OBJECTREF objException, BSTR *pbstrHelpFile, DWORD
 
     GCPROTECT_BEGIN(objException);
 
-    // read Exception.HelpLink property
-    MethodDescCallSite getHelpLink(METHOD__EXCEPTION__GET_HELP_LINK, &objException);
+    // call managed code to parse help context
+    MethodDescCallSite getHelpContext(METHOD__EXCEPTION__GET_HELP_CONTEXT, &objException);
 
-    ARG_SLOT GetHelpLinkArgs[] = { ObjToArgSlot(objException)};
-    *pbstrHelpFile = BStrFromString(getHelpLink.Call_RetSTRINGREF(GetHelpLinkArgs));
+    ARG_SLOT GetHelpContextArgs[] =
+    {
+        ObjToArgSlot(objException),
+        PtrToArgSlot(pdwHelpContext)
+    };
+    *pbstrHelpFile = BStrFromString(getHelpContext.Call_RetSTRINGREF(GetHelpContextArgs));
 
     GCPROTECT_END();
-
-    // parse the help file to check for the presence of helpcontext
-    int len = SysStringLen(*pbstrHelpFile);
-    int pos = len;
-    WCHAR *pwstr = *pbstrHelpFile;
-    if (pwstr) {
-        BOOL fFoundPound = FALSE;
-
-        for (pos = len - 1; pos >= 0; pos--) {
-            if (pwstr[pos] == W('#')) {
-                fFoundPound = TRUE;
-                break;
-            }
-        }
-
-        if (fFoundPound) {
-            int PoundPos = pos;
-            int NumberStartPos = -1;
-            BOOL bNumberStarted = FALSE;
-            BOOL bNumberFinished = FALSE;
-            BOOL bInvalidDigitsFound = FALSE;
-
-            _ASSERTE(pwstr[pos] == W('#'));
-
-            // Check to see if the string to the right of the pound a valid number.
-            for (pos++; pos < len; pos++) {
-                if (bNumberFinished) {
-                    if (!COMCharacter::nativeIsWhiteSpace(pwstr[pos])) {
-                        bInvalidDigitsFound = TRUE;
-                        break;
-                    }
-                }
-                else if (bNumberStarted) {
-                    if (COMCharacter::nativeIsWhiteSpace(pwstr[pos])) {
-                        bNumberFinished = TRUE;
-                    }
-                    else if (!COMCharacter::nativeIsDigit(pwstr[pos])) {
-                        bInvalidDigitsFound = TRUE;
-                        break;
-                    }
-                }
-                else {
-                    if (COMCharacter::nativeIsDigit(pwstr[pos])) {
-                        NumberStartPos = pos;
-                        bNumberStarted = TRUE;
-                    }
-                    else if (!COMCharacter::nativeIsWhiteSpace(pwstr[pos])) {
-                        bInvalidDigitsFound = TRUE;
-                        break;
-                    }
-                }
-            }
-
-            if (bNumberStarted && !bInvalidDigitsFound) {
-                // Grab the help context and remove it from the help file.
-                *pdwHelpContext = (DWORD)wtoi(&pwstr[NumberStartPos], len - NumberStartPos);
-
-                // Allocate a new help file string of the right length.
-                BSTR strOld = *pbstrHelpFile;
-                *pbstrHelpFile = SysAllocStringLen(strOld, PoundPos);
-                SysFreeString(strOld);
-                if (!*pbstrHelpFile)
-                    COMPlusThrowOM();
-            }
-        }
-    }
 }
 
 // NOTE: caller cleans up any partially initialized BSTRs in pED

src/coreclr/vm/corelib.h

@@ -330,11 +330,12 @@ DEFINE_FIELD_U(_xptrs,             ExceptionObject,    _xptrs)
 DEFINE_FIELD_U(_xcode,             ExceptionObject,    _xcode)
 DEFINE_FIELD_U(_HResult,           ExceptionObject,    _HResult)
 DEFINE_CLASS(EXCEPTION,             System,                 Exception)
+DEFINE_METHOD(EXCEPTION,            INTERNAL_PRESERVE_STACK_TRACE, InternalPreserveStackTrace, IM_RetVoid)
+// Following Exception members are only used when FEATURE_COMINTEROP
 DEFINE_METHOD(EXCEPTION,            GET_CLASS_NAME,         GetClassName,               IM_RetStr)
 DEFINE_PROPERTY(EXCEPTION,          MESSAGE,                Message,                    Str)
 DEFINE_PROPERTY(EXCEPTION,          SOURCE,                 Source,                     Str)
-DEFINE_PROPERTY(EXCEPTION,          HELP_LINK,              HelpLink,                   Str)
-DEFINE_METHOD(EXCEPTION,            INTERNAL_PRESERVE_STACK_TRACE, InternalPreserveStackTrace, IM_RetVoid)
+DEFINE_METHOD(EXCEPTION,            GET_HELP_CONTEXT,       GetHelpContext,             IM_RefUInt_RetStr)
 
 
 DEFINE_CLASS(SYSTEM_EXCEPTION,      System,                 SystemException)

src/coreclr/vm/metasig.h

@@ -543,6 +543,9 @@ DEFINE_METASIG(SM(Str_RetArrStr, s, a(s)))
 // Execution Context
 DEFINE_METASIG_T(SM(SyncCtx_ArrIntPtr_Bool_Int_RetInt, C(SYNCHRONIZATION_CONTEXT) a(I) F i, i))
 
+// Exception
+DEFINE_METASIG(IM(RefUInt_RetStr, r(K), s))
+
 #ifdef FEATURE_COMINTEROP
 // The signature of the method System.Runtime.InteropServices.ICustomQueryInterface.GetInterface
 DEFINE_METASIG_T(IM(RefGuid_OutIntPtr_RetCustomQueryInterfaceResult, r(g(GUID)) r(I), g(CUSTOMQUERYINTERFACERESULT)))

src/coreclr/vm/util.cpp

@@ -1982,23 +1982,6 @@ BOOL COMCharacter::nativeIsWhiteSpace(WCHAR c)
 #endif // !TARGET_UNIX
 }
 
-/*================================nativeIsDigit=================================
-**The locally available version of IsDigit.  Designed to be called by other
-**native methods.  The work is mostly done by GetCharacterInfoHelper
-**Args:  c -- the character to check.
-**Returns: true if c is whitespace, false otherwise.
-**Exceptions:  Only those thrown by GetCharacterInfoHelper.
-==============================================================================*/
-BOOL COMCharacter::nativeIsDigit(WCHAR c)
-{
-    WRAPPER_NO_CONTRACT;
-#ifndef TARGET_UNIX
-    return((GetCharacterInfoHelper(c, CT_CTYPE1) & C1_DIGIT)!=0);
-#else // !TARGET_UNIX
-    return iswdigit(c);
-#endif // !TARGET_UNIX
-}
-
 BOOL RuntimeFileNotFound(HRESULT hr)
 {
     LIMITED_METHOD_CONTRACT;

src/coreclr/vm/util.hpp

@@ -799,7 +799,6 @@ class COMCharacter {
 public:
     //These are here for support from native code.  They are never called from our managed classes.
     static BOOL nativeIsWhiteSpace(WCHAR c);
-    static BOOL nativeIsDigit(WCHAR c);
 };
 
 // ======================================================================================

src/tests/Interop/COM/NETClients/Primitives/ErrorTests.cs

@@ -19,6 +19,7 @@ public void Run()
         {
             this.VerifyExpectedException();
             this.VerifyReturnHResult();
+            this.VerifyHelpLink();
         }
 
         private void VerifyExpectedException()
@@ -40,21 +41,29 @@ private void VerifyReturnHResult()
 
             var hrs = new[]
             {
-                    unchecked((int)0x80004001),
-                    unchecked((int)0x80004003),
-                    unchecked((int)0x80070005),
-                    unchecked((int)0x80070057),
-                    unchecked((int)0x8000ffff),
-                    -1,
-                    1,
-                    2
-                };
+                unchecked((int)0x80004001),
+                unchecked((int)0x80004003),
+                unchecked((int)0x80070005),
+                unchecked((int)0x80070057),
+                unchecked((int)0x8000ffff),
+                -1,
+                1,
+                2
+            };
 
             foreach (var hr in hrs)
             {
                 Assert.Equal(hr, this.server.Return_As_HResult(hr));
                 Assert.Equal(hr, this.server.Return_As_HResult_Struct(hr).hr);
             }
         }
+
+        private void VerifyHelpLink()
+        {
+            string helpLink = "X:\\NotA\\RealPath\\dummy.hlp";
+            uint helpContext = 5678;
+            var ex = Assert.Throws<COMException>(() => { this.server.Throw_HResult_HelpLink(unchecked((int)-1), helpLink, helpContext); });
+            Assert.Equal($"{helpLink}#{helpContext}", ex.HelpLink);
+        }
     }
 }

src/tests/Interop/COM/NETServer/ErrorMarshalTesting.cs

@@ -30,4 +30,13 @@ public Server.Contract.HResult Return_As_HResult_Struct(int hresultToReturn)
     {
         return new Server.Contract.HResult { hr = hresultToReturn };
     }
+
+    public void Throw_HResult_HelpLink(int hresultToReturn, string helpLink, uint helpContext)
+    {
+        Marshal.GetExceptionForHR(hresultToReturn);
+
+        Exception e = Marshal.GetExceptionForHR(hresultToReturn);
+        e.HelpLink = $"{helpLink}#{helpContext}";
+        throw e;
+    }
 }

src/tests/Interop/COM/NativeClients/Primitives/ErrorTests.cpp

@@ -75,6 +75,44 @@ namespace
             THROW_FAIL_IF_FALSE(hr == hrMaybe);
         }
     }
+
+    void VerifyHelpContext(_In_ IErrorMarshalTesting *et)
+    {
+        ::printf("Verify expected helplink and context\n");
+
+        HRESULT hrs[] =
+        {
+            E_NOTIMPL,
+            E_POINTER,
+            E_ACCESSDENIED,
+            E_OUTOFMEMORY,
+            E_INVALIDARG,
+            E_UNEXPECTED,
+            HRESULT{-1}
+        };
+
+        LPCWSTR helpLink = L"X:\\NotA\\RealPath\\dummy.hlp";
+
+        for (int i = 0; i < ARRAY_SIZE(hrs); ++i)
+        {
+            HRESULT hr = hrs[i];
+            DWORD helpContext = (DWORD)(i + 0x1234);
+            HRESULT hrMaybe = et->Throw_HResult_HelpLink(hr, helpLink, helpContext);
+            THROW_FAIL_IF_FALSE(hr == hrMaybe);
+
+            ComSmartPtr<IErrorInfo> pErrInfo;
+            THROW_IF_FAILED(GetErrorInfo(0, &pErrInfo));
+
+            BSTR helpLinkMaybe;
+            THROW_IF_FAILED(pErrInfo->GetHelpFile(&helpLinkMaybe));
+            THROW_FAIL_IF_FALSE(TP_wcmp_s(helpLink, helpLinkMaybe) == 0);
+            SysFreeString(helpLinkMaybe);
+
+            DWORD helpContextMaybe;
+            THROW_IF_FAILED(pErrInfo->GetHelpContext(&helpContextMaybe));
+            THROW_FAIL_IF_FALSE(helpContext == helpContextMaybe);
+        }
+    }
 }
 
 void Run_ErrorTests()
@@ -89,4 +127,5 @@ void Run_ErrorTests()
     VerifyExpectedException(errorMarshal);
     VerifyReturnHResult(errorMarshal);
     VerifyReturnHResultStruct(errorMarshal);
+    VerifyHelpContext(errorMarshal);
 }