Move the BSTR trailbyte tracking out of the sync block

src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs

@@ -229,6 +229,34 @@
 
     internal static class BSTRMarshaler
     {
+        private class TrailByte(byte trailByte)
+        {
+            public readonly byte Value = trailByte;
+        }
+
+        // In some early version of VB when there were no arrays developers used to use BSTR as arrays
+        // The way this was done was by adding a trail byte at the end of the BSTR
+        // To support this scenario, we need to use a ConditionalWeakTable for this special case and
+        // save the trail character in here.
+        // This stores the trail character when a BSTR is used as an array
+        private static readonly ConditionalWeakTable<string, TrailByte> s_trailByteTable = new();
+
+        internal static bool TryGetTrailByte(string strManaged, out byte trailByte)
+        {
+            if (s_trailByteTable.TryGetValue(strManaged, out TrailByte? trailByteObj))
+            {
+                trailByte = trailByteObj.Value;
+                return true;
+            }
+
+            trailByte = 0;
+            return false;
+        }
+        internal static void SetTrailByte(string strManaged, byte trailByte)
+        {
+            s_trailByteTable.AddOrUpdate(strManaged, new TrailByte(trailByte));
+        }
+
         internal static unsafe IntPtr ConvertToNative(string strManaged, IntPtr pNativeBuffer)
         {
             if (null == strManaged)
@@ -237,7 +265,7 @@
             }
             else
             {
-                bool hasTrailByte = StubHelpers.TryGetStringTrailByte(strManaged, out byte trailByte);
+                bool hasTrailByte = s_trailByteTable.TryGetValue(strManaged, out TrailByte? trailByte);
 
                 uint lengthInBytes = (uint)strManaged.Length * 2;
 
@@ -276,7 +304,7 @@
                 // copy the trail byte if present
                 if (hasTrailByte)
                 {
-                    ptrToFirstChar[lengthInBytes - 1] = trailByte;
+                    ptrToFirstChar[lengthInBytes - 1] = trailByte.Value;
                 }
 
                 // return ptr to first character
@@ -320,8 +348,7 @@
 
                 if ((length & 1) == 1)
                 {
-                    // odd-sized strings need to have the trailing byte saved in their sync block
-                    StubHelpers.SetStringTrailByte(ret, ((byte*)bstr)[length - 1]);
+                    s_trailByteTable.Add(ret, new TrailByte(((byte*)bstr)[length - 1]));
                 }
 
                 return ret;
@@ -1489,19 +1516,12 @@
             }
         }
 
-        // Try to retrieve the extra byte - returns false if not present.
-        [MethodImpl(MethodImplOptions.InternalCall)]
-        internal static extern bool TryGetStringTrailByte(string str, out byte data);
-
         // Set extra byte for odd-sized strings that came from interop as BSTR.
         internal static void SetStringTrailByte(string str, byte data)
         {
-            SetStringTrailByte(new StringHandleOnStack(ref str!), data);
+            BSTRMarshaler.SetTrailByte(str, data);
         }
 
-        [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "StubHelpers_SetStringTrailByte")]
-        private static partial void SetStringTrailByte(StringHandleOnStack str, byte data);
-
         internal static unsafe void FmtClassUpdateNativeInternal(object obj, byte* pNative, ref CleanupWorkListElement? pCleanupWorkList)
         {
             MethodTable* pMT = RuntimeHelpers.GetMethodTable(obj);

src/coreclr/vm/callhelpers.h

@@ -516,6 +516,7 @@ enum DispatchCallSimpleFlags
 #define STRINGREF_TO_ARGHOLDER(x) (LPVOID)STRINGREFToObject(x)
 #define PTR_TO_ARGHOLDER(x) (LPVOID)x
 #define DWORD_TO_ARGHOLDER(x)   (LPVOID)(SIZE_T)x
+#define INT8_TO_ARGHOLDER(x)   (LPVOID)(SIZE_T)x
 #define BOOL_TO_ARGHOLDER(x) DWORD_TO_ARGHOLDER(!!(x))
 
 #define INIT_VARIABLES(count)                               \

src/coreclr/vm/corelib.h

@@ -1130,6 +1130,8 @@ DEFINE_CLASS(BSTRMARSHALER,         StubHelpers,            BSTRMarshaler)
 DEFINE_METHOD(BSTRMARSHALER,        CONVERT_TO_NATIVE,      ConvertToNative,            SM_Str_IntPtr_RetIntPtr)
 DEFINE_METHOD(BSTRMARSHALER,        CONVERT_TO_MANAGED,     ConvertToManaged,           SM_IntPtr_RetStr)
 DEFINE_METHOD(BSTRMARSHALER,        CLEAR_NATIVE,           ClearNative,                SM_IntPtr_RetVoid)
+DEFINE_METHOD(BSTRMARSHALER,        SET_TRAIL_BYTE,         SetTrailByte,               SM_Str_Byte_RetVoid)
+DEFINE_METHOD(BSTRMARSHALER,        TRY_GET_TRAIL_BYTE,    TryGetTrailByte,             SM_Str_RefByte_RetBool)
 
 DEFINE_CLASS(ANSIBSTRMARSHALER,     StubHelpers,            AnsiBSTRMarshaler)
 DEFINE_METHOD(ANSIBSTRMARSHALER,    CONVERT_TO_NATIVE,      ConvertToNative,            SM_Int_Str_RetIntPtr)

src/coreclr/vm/ecalllist.h

@@ -352,7 +352,6 @@ FCFuncEnd()
 
 FCFuncStart(gStubHelperFuncs)
     FCFuncElement("GetDelegateTarget", StubHelpers::GetDelegateTarget)
-    FCFuncElement("TryGetStringTrailByte", StubHelpers::TryGetStringTrailByte)
     FCFuncElement("SetLastError", StubHelpers::SetLastError)
     FCFuncElement("ClearLastError", StubHelpers::ClearLastError)
 #ifdef FEATURE_COMINTEROP

src/coreclr/vm/metasig.h

@@ -219,6 +219,8 @@ DEFINE_METASIG(SM(RetBool, _, F))
 DEFINE_METASIG(SM(IntPtr_RetStr, I, s))
 DEFINE_METASIG(SM(IntPtr_RetBool, I, F))
 DEFINE_METASIG(SM(UInt_IntPtr_RetStr, K I, s))
+DEFINE_METASIG(SM(Str_Byte_RetVoid, s b, v))
+DEFINE_METASIG(SM(Str_RefByte_RetBool, s r(b), F))
 DEFINE_METASIG_T(SM(RuntimeType_RuntimeMethodHandleInternal_RetMethodBase, C(CLASS) g(METHOD_HANDLE_INTERNAL), C(METHOD_BASE) ))
 DEFINE_METASIG_T(SM(RuntimeType_IRuntimeFieldInfo_RetFieldInfo, C(CLASS) C(I_RT_FIELD_INFO), C(FIELD_INFO) ))
 DEFINE_METASIG(SM(Char_Bool_Bool_RetByte, u F F, b))

src/coreclr/vm/object.cpp

@@ -658,47 +658,6 @@ STRINGREF StringObject::NewString(INT32 length) {
     }
 }
 
-
-/*==================================NewString===================================
-**Action: Many years ago, VB didn't have the concept of a byte array, so enterprising
-**        users created one by allocating a BSTR with an odd length and using it to
-**        store bytes.  A generation later, we're still stuck supporting this behavior.
-**        The way that we do this is to take advantage of the difference between the
-**        array length and the string length.  The string length will always be the
-**        number of characters between the start of the string and the terminating 0.
-**        If we need an odd number of bytes, we'll take one wchar after the terminating 0.
-**        (e.g. at position StringLength+1).  The high-order byte of this wchar is
-**        reserved for flags and the low-order byte is our odd byte. This function is
-**        used to allocate a string of that shape, but we don't actually mark the
-**        trailing byte as being in use yet.
-**Returns: A newly allocated string.  Null if length is less than 0.
-**Arguments: length -- the length of the string to allocate
-**           bHasTrailByte -- whether the string also has a trailing byte.
-**Exceptions: OutOfMemoryException if AllocateString fails.
-==============================================================================*/
-STRINGREF StringObject::NewString(INT32 length, BOOL bHasTrailByte) {
-    CONTRACTL {
-        GC_TRIGGERS;
-        MODE_COOPERATIVE;
-        PRECONDITION(length>=0 && length != INT32_MAX);
-    } CONTRACTL_END;
-
-    STRINGREF pString;
-    if (length<0 || length == INT32_MAX) {
-        return NULL;
-    } else if (length == 0) {
-        return GetEmptyString();
-    } else {
-        pString = AllocateString(length);
-        _ASSERTE(pString->GetBuffer()[length]==0);
-        if (bHasTrailByte) {
-            _ASSERTE(pString->GetBuffer()[length+1]==0);
-        }
-    }
-
-    return pString;
-}
-
 //========================================================================
 // Creates a System.String object and initializes from
 // the supplied null-terminated C string.
@@ -887,74 +846,6 @@ STRINGREF* StringObject::InitEmptyStringRefPtr() {
     return EmptyStringRefPtr;
 }
 
-/*============================InternalTrailByteCheck============================
-**Action: Many years ago, VB didn't have the concept of a byte array, so enterprising
-**        users created one by allocating a BSTR with an odd length and using it to
-**        store bytes.  A generation later, we're still stuck supporting this behavior.
-**        The way that we do this is stick the trail byte in the sync block
-**        whenever we encounter such a situation. Since we expect this to be a very corner case
-**        accessing the sync block seems like a good enough solution
-**
-**Returns: True if <CODE>str</CODE> contains a VB trail byte, false otherwise.
-**Arguments: str -- The string to be examined.
-**Exceptions: None
-==============================================================================*/
-BOOL StringObject::HasTrailByte() {
-    WRAPPER_NO_CONTRACT;
-
-    SyncBlock * pSyncBlock = PassiveGetSyncBlock();
-    if(pSyncBlock != NULL)
-    {
-        return pSyncBlock->HasCOMBstrTrailByte();
-    }
-
-    return FALSE;
-}
-
-/*=================================GetTrailByte=================================
-**Action:  If <CODE>str</CODE> contains a vb trail byte, returns a copy of it.
-**Returns: True if <CODE>str</CODE> contains a trail byte.  *bTrailByte is set to
-**         the byte in question if <CODE>str</CODE> does have a trail byte, otherwise
-**         it's set to 0.
-**Arguments: str -- The string being examined.
-**           bTrailByte -- An out param to hold the value of the trail byte.
-**Exceptions: None.
-==============================================================================*/
-BOOL StringObject::GetTrailByte(BYTE *bTrailByte) {
-    CONTRACTL
-    {
-        NOTHROW;
-        GC_NOTRIGGER;
-        MODE_ANY;
-    }
-    CONTRACTL_END;
-    _ASSERTE(bTrailByte);
-    *bTrailByte=0;
-
-    BOOL retValue = HasTrailByte();
-
-    if(retValue)
-    {
-        *bTrailByte = GET_VB_TRAIL_BYTE(GetHeader()->PassiveGetSyncBlock()->GetCOMBstrTrailByte());
-    }
-
-    return retValue;
-}
-
-/*=================================SetTrailByte=================================
-**Action: Sets the trail byte in the sync block
-**Returns: True.
-**Arguments: str -- The string into which to set the trail byte.
-**           bTrailByte -- The trail byte to be added to the string.
-**Exceptions: None.
-==============================================================================*/
-BOOL StringObject::SetTrailByte(BYTE bTrailByte) {
-    WRAPPER_NO_CONTRACT;
-
-    GetHeader()->GetSyncBlock()->SetCOMBstrTrailByte(MAKE_VB_TRAIL_BYTE(bTrailByte));
-    return TRUE;
-}
-
 #ifdef USE_CHECKED_OBJECTREFS
 
 //-------------------------------------------------------------

src/coreclr/vm/object.h

@@ -832,7 +832,6 @@ class StringObject : public Object
     // characters and the null terminator you should pass in 5 and NOT 6.
     //========================================================================
     static STRINGREF NewString(int length);
-    static STRINGREF NewString(int length, BOOL bHasTrailByte);
     static STRINGREF NewString(const WCHAR *pwsz);
     static STRINGREF NewString(const WCHAR *pwsz, int length);
     static STRINGREF NewString(LPCUTF8 psz);
@@ -843,10 +842,6 @@ class StringObject : public Object
 
     static STRINGREF* InitEmptyStringRefPtr();
 
-    BOOL HasTrailByte();
-    BOOL GetTrailByte(BYTE *bTrailByte);
-    BOOL SetTrailByte(BYTE bTrailByte);
-
     /*=================RefInterpretGetStringValuesDangerousForGC======================
     **N.B.: This performs no range checking and relies on the caller to have done this.
     **Args: (IN)ref -- the String to be interpretted.

src/coreclr/vm/olevariant.cpp

@@ -4265,10 +4265,8 @@ void OleVariant::AllocateEmptyStringForBSTR(BSTR bstr, STRINGREF *pStringObj)
     if (length > MAX_SIZE_FOR_INTEROP)
         COMPlusThrow(kMarshalDirectiveException, IDS_EE_STRING_TOOLONG);
 
-    // Check to see if the BSTR has trailing odd byte.
-    BOOL bHasTrailByte = ((length%sizeof(WCHAR)) != 0);
     length = length / sizeof(WCHAR);
-    SetObjectReference((OBJECTREF*)pStringObj, (OBJECTREF)StringObject::NewString(length, bHasTrailByte));
+    SetObjectReference((OBJECTREF*)pStringObj, (OBJECTREF)StringObject::NewString(length));
 }
 
 void OleVariant::ConvertContentsBSTRToString(BSTR bstr, STRINGREF *pStringObj)
@@ -4298,8 +4296,15 @@ void OleVariant::ConvertContentsBSTRToString(BSTR bstr, STRINGREF *pStringObj)
     if (hasTrailByte)
     {
         BYTE* buff = (BYTE*)bstr;
-        //set the trail byte
-        (*pStringObj)->SetTrailByte(buff[length-1]);
+
+        ASSERT_PROTECTED(pStringObj);
+
+        PREPARE_NONVIRTUAL_CALLSITE(METHOD__BSTRMARSHALER__SET_TRAIL_BYTE);
+        DECLARE_ARGHOLDER_ARRAY(args, 2);
+        args[ARGNUM_0] = STRINGREF_TO_ARGHOLDER(*pStringObj);
+        args[ARGNUM_1] = INT8_TO_ARGHOLDER(buff[length-1]);
+
+        CALL_MANAGED_METHOD_NORET(METHOD__BSTRMARSHALER__SET_TRAIL_BYTE, args);
     }
 
     // null terminate the StringRef
@@ -4348,7 +4353,16 @@ BSTR OleVariant::AllocateEmptyBSTRForString(STRINGREF *pStringObj)
         COMPlusThrow(kMarshalDirectiveException, IDS_EE_STRING_TOOLONG);
 
     length = length*sizeof(WCHAR);
-    if ((*pStringObj)->HasTrailByte())
+
+    ASSERT_PROTECTED(pStringObj);
+    PREPARE_NONVIRTUAL_CALLSITE(METHOD__BSTRMARSHALER__TRY_GET_TRAIL_BYTE);
+    DECLARE_ARGHOLDER_ARRAY(args, 2);
+    args[ARGNUM_0] = STRINGREF_TO_ARGHOLDER(*pStringObj);
+    BYTE trailByte;
+    args[ARGNUM_1] = PTR_TO_ARGHOLDER(&trailByte);
+    CLR_BOOL hasTrailByte = CALL_MANAGED_METHOD(CLR_BOOL, METHOD__BSTRMARSHALER__TRY_GET_TRAIL_BYTE, args);
+
+    if (hasTrailByte)
     {
         length += 1;
     }
@@ -4381,13 +4395,17 @@ void OleVariant::ConvertContentsStringToBSTR(STRINGREF *pStringObj, BSTR bstr)
 
     memcpyNoGCRefs(bstr, (*pStringObj)->GetBuffer(), byteLen);
 
-    if ((*pStringObj)->HasTrailByte())
+    ASSERT_PROTECTED(pStringObj);
+    PREPARE_NONVIRTUAL_CALLSITE(METHOD__BSTRMARSHALER__TRY_GET_TRAIL_BYTE);
+    DECLARE_ARGHOLDER_ARRAY(args, 2);
+    args[ARGNUM_0] = STRINGREF_TO_ARGHOLDER(*pStringObj);
+    BYTE trailByte;
+    args[ARGNUM_1] = PTR_TO_ARGHOLDER(&trailByte);
+    CLR_BOOL hasTrailByte = CALL_MANAGED_METHOD(CLR_BOOL, METHOD__BSTRMARSHALER__TRY_GET_TRAIL_BYTE, args);
+
+    if (hasTrailByte)
     {
-        BYTE b;
-        BOOL hasTrailB;
-        hasTrailB = (*pStringObj)->GetTrailByte(&b);
-        _ASSERTE(hasTrailB);
-        buff[byteLen] = b;
+        buff[byteLen] = trailByte;
     }
     else
     {

src/coreclr/vm/qcallentrypoints.cpp

@@ -505,7 +505,6 @@ static const Entry s_QCall[] =
     DllImportEntry(X86BaseCpuId)
 #endif
     DllImportEntry(StubHelpers_CreateCustomMarshaler)
-    DllImportEntry(StubHelpers_SetStringTrailByte)
     DllImportEntry(StubHelpers_ThrowInteropParamException)
     DllImportEntry(StubHelpers_MarshalToManagedVaList)
     DllImportEntry(StubHelpers_MarshalToUnmanagedVaList)

src/coreclr/vm/stubhelpers.cpp

@@ -481,29 +481,6 @@ FCIMPL1(void*, StubHelpers::GetDelegateTarget, DelegateObject *pThisUNSAFE)
 }
 FCIMPLEND
 
-#include <optsmallperfcritical.h>
-FCIMPL2(FC_BOOL_RET, StubHelpers::TryGetStringTrailByte, StringObject* thisRefUNSAFE, UINT8 *pbData)
-{
-    FCALL_CONTRACT;
-
-    STRINGREF thisRef = ObjectToSTRINGREF(thisRefUNSAFE);
-    FC_RETURN_BOOL(thisRef->GetTrailByte(pbData));
-}
-FCIMPLEND
-#include <optdefault.h>
-
-extern "C" void QCALLTYPE StubHelpers_SetStringTrailByte(QCall::StringHandleOnStack str, UINT8 bData)
-{
-    QCALL_CONTRACT;
-
-    BEGIN_QCALL;
-
-    GCX_COOP();
-    str.Get()->SetTrailByte(bData);
-
-    END_QCALL;
-}
-
 extern "C" void QCALLTYPE StubHelpers_ThrowInteropParamException(INT resID, INT paramIdx)
 {
     QCALL_CONTRACT;

src/coreclr/vm/stubhelpers.h

@@ -33,8 +33,6 @@ class StubHelpers
     static FCDECL0(void,            ClearLastError          );
     static FCDECL1(void*,           GetDelegateTarget,      DelegateObject *pThisUNSAFE);
 
-    static FCDECL2(FC_BOOL_RET,     TryGetStringTrailByte,  StringObject* thisRefUNSAFE, UINT8 *pbData);
-
     static FCDECL2(void,            LogPinnedArgument, MethodDesc *localDesc, Object *nativeArg);
     static FCDECL1(DWORD,           CalcVaListSize, VARARGS *varargs);
 };
@@ -56,7 +54,6 @@ extern "C" IUnknown* QCALLTYPE InterfaceMarshaler_ConvertToNative(QCall::ObjectH
 extern "C" void QCALLTYPE InterfaceMarshaler_ConvertToManaged(IUnknown** ppUnk, MethodTable* pItfMT, MethodTable* pClsMT, DWORD dwFlags, QCall::ObjectHandleOnStack retObject);
 #endif
 
-extern "C" void QCALLTYPE StubHelpers_SetStringTrailByte(QCall::StringHandleOnStack str, UINT8 bData);
 extern "C" void QCALLTYPE StubHelpers_ThrowInteropParamException(INT resID, INT paramIdx);
 
 extern "C" void QCALLTYPE StubHelpers_MarshalToManagedVaList(va_list va, VARARGS* pArgIterator);