From bcd35278ca879554ed98e522c007dc0025a19303 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Fri, 17 Dec 2021 14:25:23 -0800 Subject: [PATCH] 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 | 14 +---- .../ReadyToRun/GCRefMapBuilder.cs | 16 +++-- src/coreclr/vm/corelib.h | 18 +++--- src/coreclr/vm/frames.cpp | 10 ++++ src/coreclr/vm/ilmarshalers.cpp | 3 +- src/coreclr/vm/metasig.h | 19 +++--- src/coreclr/vm/reflectioninvocation.cpp | 11 ++-- src/coreclr/vm/siginfo.hpp | 5 ++ .../src/System/String.cs | 58 +++---------------- 9 files changed, 64 insertions(+), 90 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index bde0cce79ab60..57c3604899b7c 100644 --- a/src/coreclr/jit/importer.cpp +++ b/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; diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GCRefMapBuilder.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GCRefMapBuilder.cs index 6fcb4e2c1d287..ad294d7227d46 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GCRefMapBuilder.cs +++ b/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; diff --git a/src/coreclr/vm/corelib.h b/src/coreclr/vm/corelib.h index 6fb9d0e3e3f21..919a5352c1bb0 100644 --- a/src/coreclr/vm/corelib.h +++ b/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) diff --git a/src/coreclr/vm/frames.cpp b/src/coreclr/vm/frames.cpp index d5de56de06a67..77bcb178bb49c 100644 --- a/src/coreclr/vm/frames.cpp +++ b/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 diff --git a/src/coreclr/vm/ilmarshalers.cpp b/src/coreclr/vm/ilmarshalers.cpp index ede736543d52b..4782df6281b12 100644 --- a/src/coreclr/vm/ilmarshalers.cpp +++ b/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); diff --git a/src/coreclr/vm/metasig.h b/src/coreclr/vm/metasig.h index a684163030d56..54d3d4780b6f2 100644 --- a/src/coreclr/vm/metasig.h +++ b/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)) diff --git a/src/coreclr/vm/reflectioninvocation.cpp b/src/coreclr/vm/reflectioninvocation.cpp index 68268942e562f..8b61726f44726 100644 --- a/src/coreclr/vm/reflectioninvocation.cpp +++ b/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 @@ class ArgIteratorBaseForMethodInvoke BOOL HasThis() { LIMITED_METHOD_CONTRACT; - return (*m_ppNativeSig)->HasThis(); + return m_fHasThis; } BOOL HasParamType() @@ -602,10 +603,12 @@ class ArgIteratorBaseForMethodInvoke class ArgIteratorForMethodInvoke : public ArgIteratorTemplate { 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) diff --git a/src/coreclr/vm/siginfo.hpp b/src/coreclr/vm/siginfo.hpp index 7489cd349f056..25ea2d4d0a1a5 100644 --- a/src/coreclr/vm/siginfo.hpp +++ b/src/coreclr/vm/siginfo.hpp @@ -1109,6 +1109,11 @@ class MetaSig 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: diff --git a/src/libraries/System.Private.CoreLib/src/System/String.cs b/src/libraries/System.Private.CoreLib/src/System/String.cs index 76048da7b75c2..260c87b5fd911 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.cs +++ b/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 value); - private -#if !CORECLR - static -#endif - unsafe string Ctor(ReadOnlySpan value) + private static unsafe string Ctor(ReadOnlySpan value) { if (value.Length == 0) return Empty; @@ -346,8 +308,6 @@ unsafe string Ctor(ReadOnlySpan value) return result; } -#pragma warning restore CA1822 - public static string Create(int length, TState state, SpanAction action) { if (action == null)