Unify implementation of string constructors accross runtimes (#62936)

String constructors implementation methods had a dummy this argument on CoreCLR, but not on other runtimes.  It required ifdefs in the implementation. This change removes the ifdefs and makes the string constructors implementation methods uniform accross all runtimes. It is possible to do this cleanup now since we have just bumped R2R version band.

src/coreclr/jit/importer.cpp

@@ -14883,18 +14883,8 @@ void Compiler::impImportBlockCode(BasicBlock* block)
                 // At present this can only be String
                 else if (clsFlags & CORINFO_FLG_VAROBJSIZE)
                 {
-                    if (IsTargetAbi(CORINFO_CORERT_ABI))
-                    {
-                        // The dummy argument does not exist in CoreRT
-                        newObjThisPtr = nullptr;
-                    }
-                    else
-                    {
-                        // This is the case for variable-sized objects that are not
-                        // arrays.  In this case, call the constructor with a null 'this'
-                        // pointer
-                        newObjThisPtr = gtNewIconNode(0, TYP_REF);
-                    }
+                    // Skip this thisPtr argument
+                    newObjThisPtr = nullptr;
 
                     /* Remember that this basic block contains 'new' of an object */
                     block->bbFlags |= BBF_HAS_NEWOBJ;

src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GCRefMapBuilder.cs

@@ -69,13 +69,21 @@ public void GetCallRefMap(MethodDesc method, bool isUnboxingStub)
         {
             TransitionBlock transitionBlock = TransitionBlock.FromTarget(method.Context.Target);
 
-            bool hasThis = (method.Signature.Flags & MethodSignatureFlags.Static) == 0;
+            MethodSignature signature = method.Signature;
+
+            bool hasThis = (signature.Flags & MethodSignatureFlags.Static) == 0;
+
+            // This pointer is omitted for string constructors
+            bool fCtorOfVariableSizedObject = hasThis && method.OwningType.IsString && method.IsConstructor;
+            if (fCtorOfVariableSizedObject)
+                hasThis = false;
+
             bool isVarArg = false;
-            TypeHandle returnType = new TypeHandle(method.Signature.ReturnType);
-            TypeHandle[] parameterTypes = new TypeHandle[method.Signature.Length];
+            TypeHandle returnType = new TypeHandle(signature.ReturnType);
+            TypeHandle[] parameterTypes = new TypeHandle[signature.Length];
             for (int parameterIndex = 0; parameterIndex < parameterTypes.Length; parameterIndex++)
             {
-                parameterTypes[parameterIndex] = new TypeHandle(method.Signature[parameterIndex]);
+                parameterTypes[parameterIndex] = new TypeHandle(signature[parameterIndex]);
             }
             CallingConventions callingConventions = (hasThis ? CallingConventions.ManagedInstance : CallingConventions.ManagedStatic);
             bool hasParamType = method.RequiresInstArg() && !isUnboxingStub;

src/coreclr/vm/corelib.h

@@ -864,15 +864,15 @@ DEFINE_FIELD(BITCONVERTER,          ISLITTLEENDIAN,         IsLittleEndian)
 DEFINE_FIELD(STRING,                M_FIRST_CHAR,           _firstChar)
 DEFINE_FIELD(STRING,                EMPTY,                  Empty)
 DEFINE_METHOD(STRING,               CTOR_CHARPTR,           .ctor,                      IM_PtrChar_RetVoid)
-DEFINE_METHOD(STRING,               CTORF_CHARARRAY,        Ctor,                       IM_ArrChar_RetStr)
-DEFINE_METHOD(STRING,               CTORF_CHARARRAY_START_LEN,Ctor,                     IM_ArrChar_Int_Int_RetStr)
-DEFINE_METHOD(STRING,               CTORF_CHAR_COUNT,       Ctor,                       IM_Char_Int_RetStr)
-DEFINE_METHOD(STRING,               CTORF_CHARPTR,          Ctor,                       IM_PtrChar_RetStr)
-DEFINE_METHOD(STRING,               CTORF_CHARPTR_START_LEN,Ctor,                       IM_PtrChar_Int_Int_RetStr)
-DEFINE_METHOD(STRING,               CTORF_READONLYSPANOFCHAR,Ctor,                      IM_ReadOnlySpanOfChar_RetStr)
-DEFINE_METHOD(STRING,               CTORF_SBYTEPTR,         Ctor,                       IM_PtrSByt_RetStr)
-DEFINE_METHOD(STRING,               CTORF_SBYTEPTR_START_LEN, Ctor,                     IM_PtrSByt_Int_Int_RetStr)
-DEFINE_METHOD(STRING,               CTORF_SBYTEPTR_START_LEN_ENCODING, Ctor,            IM_PtrSByt_Int_Int_Encoding_RetStr)
+DEFINE_METHOD(STRING,               CTORF_CHARARRAY,        Ctor,                       SM_ArrChar_RetStr)
+DEFINE_METHOD(STRING,               CTORF_CHARARRAY_START_LEN,Ctor,                     SM_ArrChar_Int_Int_RetStr)
+DEFINE_METHOD(STRING,               CTORF_CHAR_COUNT,       Ctor,                       SM_Char_Int_RetStr)
+DEFINE_METHOD(STRING,               CTORF_CHARPTR,          Ctor,                       SM_PtrChar_RetStr)
+DEFINE_METHOD(STRING,               CTORF_CHARPTR_START_LEN,Ctor,                       SM_PtrChar_Int_Int_RetStr)
+DEFINE_METHOD(STRING,               CTORF_READONLYSPANOFCHAR,Ctor,                      SM_ReadOnlySpanOfChar_RetStr)
+DEFINE_METHOD(STRING,               CTORF_SBYTEPTR,         Ctor,                       SM_PtrSByt_RetStr)
+DEFINE_METHOD(STRING,               CTORF_SBYTEPTR_START_LEN, Ctor,                     SM_PtrSByt_Int_Int_RetStr)
+DEFINE_METHOD(STRING,               CTORF_SBYTEPTR_START_LEN_ENCODING, Ctor,            SM_PtrSByt_Int_Int_Encoding_RetStr)
 DEFINE_METHOD(STRING,               INTERNAL_COPY,          InternalCopy,               SM_Str_IntPtr_Int_RetVoid)
 DEFINE_METHOD(STRING,               WCSLEN,                 wcslen,                     SM_PtrChar_RetInt)
 DEFINE_METHOD(STRING,               STRLEN,                 strlen,                     SM_PtrByte_RetInt)

src/coreclr/vm/frames.cpp

@@ -1269,6 +1269,10 @@ void TransitionFrame::PromoteCallerStack(promote_func* fn, ScanContext* sc)
 
         MetaSig msig(pSig, cbSigSize, pFunction->GetModule(), &typeContext);
 
+        bool fCtorOfVariableSizedObject = msig.HasThis() && (pFunction->GetMethodTable() == g_pStringClass) && pFunction->IsCtor();
+        if (fCtorOfVariableSizedObject)
+            msig.ClearHasThis();
+
         if (pFunction->RequiresInstArg() && !SuppressParamTypeArg())
             msig.SetHasParamTypeArg();
 
@@ -2101,6 +2105,12 @@ void ComputeCallRefMap(MethodDesc* pMD,
     pMD->GetSig(&pSig, &cbSigSize);
     MetaSig msig(pSig, cbSigSize, pMD->GetModule(), &typeContext);
 
+    bool fCtorOfVariableSizedObject = msig.HasThis() && (pMD->GetMethodTable() == g_pStringClass) && pMD->IsCtor();
+    if (fCtorOfVariableSizedObject)
+    {
+        msig.ClearHasThis();
+    }
+
     //
     // Shared default interface methods (i.e. virtual interface methods with an implementation) require
     // an instantiation argument. But if we're in a situation where we haven't resolved the method yet

src/coreclr/vm/ilmarshalers.cpp

@@ -1635,14 +1635,13 @@ void ILVBByValStrWMarshaler::EmitConvertContentsNativeToCLR(ILCodeStream* pslILE
     EmitLoadNativeValue(pslILEmit);
     pslILEmit->EmitBRFALSE(pNullRefLabel);
 
-    pslILEmit->EmitLDNULL();            // this
     EmitLoadNativeValue(pslILEmit);     // ptr
     pslILEmit->EmitLDC(0);              // startIndex
     pslILEmit->EmitLDLOC(m_dwCCHLocal); // length
 
     // String CtorCharPtrStartLength(char *ptr, int startIndex, int length)
     // TODO Phase5: Why do we call this weirdo?
-    pslILEmit->EmitCALL(METHOD__STRING__CTORF_CHARPTR_START_LEN, 4, 1);
+    pslILEmit->EmitCALL(METHOD__STRING__CTORF_CHARPTR_START_LEN, 3, 1);
 
     EmitStoreManagedValue(pslILEmit);
     pslILEmit->EmitLabel(pNullRefLabel);

src/coreclr/vm/metasig.h

@@ -366,7 +366,6 @@ DEFINE_METASIG_T(SM(Type_RetInt, C(TYPE), i))
 DEFINE_METASIG(SM(ArrByte_RetObj, a(b), j))
 DEFINE_METASIG(SM(ArrByte_Bool_RetObj, a(b) F, j))
 DEFINE_METASIG(SM(ArrByte_ArrByte_RefObj_RetObj, a(b) a(b) r(j), j))
-DEFINE_METASIG_T(SM(PtrSByt_Int_Int_Encoding_RetStr, P(B) i i C(ENCODING), s))
 
 DEFINE_METASIG_T(SM(UInt_UInt_PtrNativeOverlapped_RetVoid, K K P(g(NATIVEOVERLAPPED)), v))
 
@@ -410,15 +409,15 @@ DEFINE_METASIG_T(IM(PtrSByt_Int_Int_Encoding_RetVoid, P(B) i i C(ENCODING), v))
 DEFINE_METASIG(IM(PtrChar_Int_RetVoid, P(u) i, v))
 DEFINE_METASIG(IM(PtrSByt_Int_RetVoid, P(B) i, v))
 
-DEFINE_METASIG(IM(ArrChar_RetStr, a(u), s))
-DEFINE_METASIG(IM(ArrChar_Int_Int_RetStr, a(u) i i, s))
-DEFINE_METASIG(IM(Char_Int_RetStr, u i, s))
-DEFINE_METASIG(IM(PtrChar_RetStr, P(u), s))
-DEFINE_METASIG(IM(PtrChar_Int_Int_RetStr, P(u) i i, s))
-DEFINE_METASIG_T(IM(ReadOnlySpanOfChar_RetStr, GI(g(READONLY_SPAN), 1, u), s))
-DEFINE_METASIG(IM(PtrSByt_RetStr, P(B), s))
-DEFINE_METASIG(IM(PtrSByt_Int_Int_RetStr, P(B) i i, s))
-DEFINE_METASIG_T(IM(PtrSByt_Int_Int_Encoding_RetStr, P(B) i i C(ENCODING), s))
+DEFINE_METASIG(SM(ArrChar_RetStr, a(u), s))
+DEFINE_METASIG(SM(ArrChar_Int_Int_RetStr, a(u) i i, s))
+DEFINE_METASIG(SM(Char_Int_RetStr, u i, s))
+DEFINE_METASIG(SM(PtrChar_RetStr, P(u), s))
+DEFINE_METASIG(SM(PtrChar_Int_Int_RetStr, P(u) i i, s))
+DEFINE_METASIG_T(SM(ReadOnlySpanOfChar_RetStr, GI(g(READONLY_SPAN), 1, u), s))
+DEFINE_METASIG(SM(PtrSByt_RetStr, P(B), s))
+DEFINE_METASIG(SM(PtrSByt_Int_Int_RetStr, P(B) i i, s))
+DEFINE_METASIG_T(SM(PtrSByt_Int_Int_Encoding_RetStr, P(B) i i C(ENCODING), s))
 DEFINE_METASIG(IM(Obj_Int_RetIntPtr, j i, I))
 
 DEFINE_METASIG(IM(ArrByte_Int_Int_RetVoid, a(b) i i, v))

src/coreclr/vm/reflectioninvocation.cpp

@@ -540,6 +540,7 @@ class ArgIteratorBaseForMethodInvoke
 {
 protected:
     SIGNATURENATIVEREF * m_ppNativeSig;
+    bool m_fHasThis;
 
     FORCEINLINE CorElementType GetReturnType(TypeHandle * pthValueType)
     {
@@ -567,7 +568,7 @@ public:
     BOOL HasThis()
     {
         LIMITED_METHOD_CONTRACT;
-        return (*m_ppNativeSig)->HasThis();
+        return m_fHasThis;
     }
 
     BOOL HasParamType()
@@ -602,10 +603,12 @@ public:
 class ArgIteratorForMethodInvoke : public ArgIteratorTemplate<ArgIteratorBaseForMethodInvoke>
 {
 public:
-    ArgIteratorForMethodInvoke(SIGNATURENATIVEREF * ppNativeSig)
+    ArgIteratorForMethodInvoke(SIGNATURENATIVEREF * ppNativeSig, BOOL fCtorOfVariableSizedObject)
     {
         m_ppNativeSig = ppNativeSig;
 
+        m_fHasThis = (*m_ppNativeSig)->HasThis() && !fCtorOfVariableSizedObject;
+
         DWORD dwFlags = (*m_ppNativeSig)->GetArgIteratorFlags();
 
         // Use the cached values if they are available
@@ -803,7 +806,7 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod,
     }
 
     {
-    ArgIteratorForMethodInvoke argit(&gc.pSig);
+    ArgIteratorForMethodInvoke argit(&gc.pSig, fCtorOfVariableSizedObject);
 
     if (argit.IsActivationNeeded())
         pMeth->EnsureActive();
@@ -888,7 +891,7 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod,
     }
 
     // Copy "this" pointer
-    if (!pMeth->IsStatic()) {
+    if (!pMeth->IsStatic() && !fCtorOfVariableSizedObject) {
         PVOID pThisPtr;
 
         if (fConstructor)

src/coreclr/vm/siginfo.hpp

@@ -1109,6 +1109,11 @@ public:
             m_flags |= TREAT_AS_VARARG;
         }
 
+        void ClearHasThis()
+        {
+            LIMITED_METHOD_CONTRACT;
+            m_CallConv &= ~IMAGE_CEE_CS_CALLCONV_HASTHIS;
+        }
 
     // These are protected because Reflection subclasses Metasig
     protected:

src/libraries/System.Private.CoreLib/src/System/String.cs

@@ -62,13 +62,7 @@ public sealed partial class String : IComparable, IEnumerable, IConvertible, IEn
         [DynamicDependency("Ctor(System.Char[])")]
         public extern String(char[]? value);
 
-#pragma warning disable CA1822 // Mark members as static
-
-        private
-#if !CORECLR
-        static
-#endif
-        string Ctor(char[]? value)
+        private static string Ctor(char[]? value)
         {
             if (value == null || value.Length == 0)
                 return Empty;
@@ -87,11 +81,7 @@ string Ctor(char[]? value)
         [DynamicDependency("Ctor(System.Char[],System.Int32,System.Int32)")]
         public extern String(char[] value, int startIndex, int length);
 
-        private
-#if !CORECLR
-        static
-#endif
-        string Ctor(char[] value, int startIndex, int length)
+        private static string Ctor(char[] value, int startIndex, int length)
         {
             if (value == null)
                 throw new ArgumentNullException(nameof(value));
@@ -123,11 +113,7 @@ string Ctor(char[] value, int startIndex, int length)
         [DynamicDependency("Ctor(System.Char*)")]
         public extern unsafe String(char* value);
 
-        private
-#if !CORECLR
-        static
-#endif
-        unsafe string Ctor(char* ptr)
+        private static unsafe string Ctor(char* ptr)
         {
             if (ptr == null)
                 return Empty;
@@ -151,11 +137,7 @@ unsafe string Ctor(char* ptr)
         [DynamicDependency("Ctor(System.Char*,System.Int32,System.Int32)")]
         public extern unsafe String(char* value, int startIndex, int length);
 
-        private
-#if !CORECLR
-        static
-#endif
-        unsafe string Ctor(char* ptr, int startIndex, int length)
+        private static unsafe string Ctor(char* ptr, int startIndex, int length)
         {
             if (length < 0)
                 throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_NegativeLength);
@@ -190,11 +172,7 @@ unsafe string Ctor(char* ptr, int startIndex, int length)
         [DynamicDependency("Ctor(System.SByte*)")]
         public extern unsafe String(sbyte* value);
 
-        private
-#if !CORECLR
-        static
-#endif
-        unsafe string Ctor(sbyte* value)
+        private static unsafe string Ctor(sbyte* value)
         {
             byte* pb = (byte*)value;
             if (pb == null)
@@ -210,11 +188,7 @@ unsafe string Ctor(sbyte* value)
         [DynamicDependency("Ctor(System.SByte*,System.Int32,System.Int32)")]
         public extern unsafe String(sbyte* value, int startIndex, int length);
 
-        private
-#if !CORECLR
-        static
-#endif
-        unsafe string Ctor(sbyte* value, int startIndex, int length)
+        private static unsafe string Ctor(sbyte* value, int startIndex, int length)
         {
             if (startIndex < 0)
                 throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndex);
@@ -271,11 +245,7 @@ private static unsafe string CreateStringForSByteConstructor(byte* pb, int numBy
         [DynamicDependency("Ctor(System.SByte*,System.Int32,System.Int32,System.Text.Encoding)")]
         public extern unsafe String(sbyte* value, int startIndex, int length, Encoding enc);
 
-        private
-#if !CORECLR
-        static
-#endif
-        unsafe string Ctor(sbyte* value, int startIndex, int length, Encoding? enc)
+        private static unsafe string Ctor(sbyte* value, int startIndex, int length, Encoding? enc)
         {
             if (enc == null)
                 return new string(value, startIndex, length);
@@ -307,11 +277,7 @@ unsafe string Ctor(sbyte* value, int startIndex, int length, Encoding? enc)
         [DynamicDependency("Ctor(System.Char,System.Int32)")]
         public extern String(char c, int count);
 
-        private
-#if !CORECLR
-        static
-#endif
-        string Ctor(char c, int count)
+        private static string Ctor(char c, int count)
         {
             if (count <= 0)
             {
@@ -332,11 +298,7 @@ string Ctor(char c, int count)
         [DynamicDependency("Ctor(System.ReadOnlySpan{System.Char})")]
         public extern String(ReadOnlySpan<char> value);
 
-        private
-#if !CORECLR
-        static
-#endif
-        unsafe string Ctor(ReadOnlySpan<char> value)
+        private static unsafe string Ctor(ReadOnlySpan<char> value)
         {
             if (value.Length == 0)
                 return Empty;
@@ -346,8 +308,6 @@ unsafe string Ctor(ReadOnlySpan<char> value)
             return result;
         }
 
-#pragma warning restore CA1822
-
         public static string Create<TState>(int length, TState state, SpanAction<char, TState> action)
         {
             if (action == null)