Skip to content

Commit bcd3527

Browse files
jkotasjkotas
authored
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.
1 parent e1f2bf1 commit bcd3527

File tree

9 files changed

+64
-90
lines changed

9 files changed

+64
-90
lines changed

‎src/coreclr/jit/importer.cpp

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14883,18 +14883,8 @@ void Compiler::impImportBlockCode(BasicBlock* block)
1488314883
// At present this can only be String
1488414884
else if (clsFlags & CORINFO_FLG_VAROBJSIZE)
1488514885
{
14886-
if (IsTargetAbi(CORINFO_CORERT_ABI))
14887-
{
14888-
// The dummy argument does not exist in CoreRT
14889-
newObjThisPtr = nullptr;
14890-
}
14891-
else
14892-
{
14893-
// This is the case for variable-sized objects that are not
14894-
// arrays. In this case, call the constructor with a null 'this'
14895-
// pointer
14896-
newObjThisPtr = gtNewIconNode(0, TYP_REF);
14897-
}
14886+
// Skip this thisPtr argument
14887+
newObjThisPtr = nullptr;
1489814888

1489914889
/* Remember that this basic block contains 'new' of an object */
1490014890
block->bbFlags |= BBF_HAS_NEWOBJ;

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,13 +69,21 @@ public void GetCallRefMap(MethodDesc method, bool isUnboxingStub)
6969
{
7070
TransitionBlock transitionBlock = TransitionBlock.FromTarget(method.Context.Target);
7171

72-
bool hasThis = (method.Signature.Flags & MethodSignatureFlags.Static) == 0;
72+
MethodSignature signature = method.Signature;
73+
74+
bool hasThis = (signature.Flags & MethodSignatureFlags.Static) == 0;
75+
76+
// This pointer is omitted for string constructors
77+
bool fCtorOfVariableSizedObject = hasThis && method.OwningType.IsString && method.IsConstructor;
78+
if (fCtorOfVariableSizedObject)
79+
hasThis = false;
80+
7381
bool isVarArg = false;
74-
TypeHandle returnType = new TypeHandle(method.Signature.ReturnType);
75-
TypeHandle[] parameterTypes = new TypeHandle[method.Signature.Length];
82+
TypeHandle returnType = new TypeHandle(signature.ReturnType);
83+
TypeHandle[] parameterTypes = new TypeHandle[signature.Length];
7684
for (int parameterIndex = 0; parameterIndex < parameterTypes.Length; parameterIndex++)
7785
{
78-
parameterTypes[parameterIndex] = new TypeHandle(method.Signature[parameterIndex]);
86+
parameterTypes[parameterIndex] = new TypeHandle(signature[parameterIndex]);
7987
}
8088
CallingConventions callingConventions = (hasThis ? CallingConventions.ManagedInstance : CallingConventions.ManagedStatic);
8189
bool hasParamType = method.RequiresInstArg() && !isUnboxingStub;

‎src/coreclr/vm/corelib.h

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -864,15 +864,15 @@ DEFINE_FIELD(BITCONVERTER, ISLITTLEENDIAN, IsLittleEndian)
864864
DEFINE_FIELD(STRING, M_FIRST_CHAR, _firstChar)
865865
DEFINE_FIELD(STRING, EMPTY, Empty)
866866
DEFINE_METHOD(STRING, CTOR_CHARPTR, .ctor, IM_PtrChar_RetVoid)
867-
DEFINE_METHOD(STRING, CTORF_CHARARRAY, Ctor, IM_ArrChar_RetStr)
868-
DEFINE_METHOD(STRING, CTORF_CHARARRAY_START_LEN,Ctor, IM_ArrChar_Int_Int_RetStr)
869-
DEFINE_METHOD(STRING, CTORF_CHAR_COUNT, Ctor, IM_Char_Int_RetStr)
870-
DEFINE_METHOD(STRING, CTORF_CHARPTR, Ctor, IM_PtrChar_RetStr)
871-
DEFINE_METHOD(STRING, CTORF_CHARPTR_START_LEN,Ctor, IM_PtrChar_Int_Int_RetStr)
872-
DEFINE_METHOD(STRING, CTORF_READONLYSPANOFCHAR,Ctor, IM_ReadOnlySpanOfChar_RetStr)
873-
DEFINE_METHOD(STRING, CTORF_SBYTEPTR, Ctor, IM_PtrSByt_RetStr)
874-
DEFINE_METHOD(STRING, CTORF_SBYTEPTR_START_LEN, Ctor, IM_PtrSByt_Int_Int_RetStr)
875-
DEFINE_METHOD(STRING, CTORF_SBYTEPTR_START_LEN_ENCODING, Ctor, IM_PtrSByt_Int_Int_Encoding_RetStr)
867+
DEFINE_METHOD(STRING, CTORF_CHARARRAY, Ctor, SM_ArrChar_RetStr)
868+
DEFINE_METHOD(STRING, CTORF_CHARARRAY_START_LEN,Ctor, SM_ArrChar_Int_Int_RetStr)
869+
DEFINE_METHOD(STRING, CTORF_CHAR_COUNT, Ctor, SM_Char_Int_RetStr)
870+
DEFINE_METHOD(STRING, CTORF_CHARPTR, Ctor, SM_PtrChar_RetStr)
871+
DEFINE_METHOD(STRING, CTORF_CHARPTR_START_LEN,Ctor, SM_PtrChar_Int_Int_RetStr)
872+
DEFINE_METHOD(STRING, CTORF_READONLYSPANOFCHAR,Ctor, SM_ReadOnlySpanOfChar_RetStr)
873+
DEFINE_METHOD(STRING, CTORF_SBYTEPTR, Ctor, SM_PtrSByt_RetStr)
874+
DEFINE_METHOD(STRING, CTORF_SBYTEPTR_START_LEN, Ctor, SM_PtrSByt_Int_Int_RetStr)
875+
DEFINE_METHOD(STRING, CTORF_SBYTEPTR_START_LEN_ENCODING, Ctor, SM_PtrSByt_Int_Int_Encoding_RetStr)
876876
DEFINE_METHOD(STRING, INTERNAL_COPY, InternalCopy, SM_Str_IntPtr_Int_RetVoid)
877877
DEFINE_METHOD(STRING, WCSLEN, wcslen, SM_PtrChar_RetInt)
878878
DEFINE_METHOD(STRING, STRLEN, strlen, SM_PtrByte_RetInt)

‎src/coreclr/vm/frames.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1269,6 +1269,10 @@ void TransitionFrame::PromoteCallerStack(promote_func* fn, ScanContext* sc)
12691269

12701270
MetaSig msig(pSig, cbSigSize, pFunction->GetModule(), &typeContext);
12711271

1272+
bool fCtorOfVariableSizedObject = msig.HasThis() && (pFunction->GetMethodTable() == g_pStringClass) && pFunction->IsCtor();
1273+
if (fCtorOfVariableSizedObject)
1274+
msig.ClearHasThis();
1275+
12721276
if (pFunction->RequiresInstArg() && !SuppressParamTypeArg())
12731277
msig.SetHasParamTypeArg();
12741278

@@ -2101,6 +2105,12 @@ void ComputeCallRefMap(MethodDesc* pMD,
21012105
pMD->GetSig(&pSig, &cbSigSize);
21022106
MetaSig msig(pSig, cbSigSize, pMD->GetModule(), &typeContext);
21032107

2108+
bool fCtorOfVariableSizedObject = msig.HasThis() && (pMD->GetMethodTable() == g_pStringClass) && pMD->IsCtor();
2109+
if (fCtorOfVariableSizedObject)
2110+
{
2111+
msig.ClearHasThis();
2112+
}
2113+
21042114
//
21052115
// Shared default interface methods (i.e. virtual interface methods with an implementation) require
21062116
// an instantiation argument. But if we're in a situation where we haven't resolved the method yet

‎src/coreclr/vm/ilmarshalers.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1635,14 +1635,13 @@ void ILVBByValStrWMarshaler::EmitConvertContentsNativeToCLR(ILCodeStream* pslILE
16351635
EmitLoadNativeValue(pslILEmit);
16361636
pslILEmit->EmitBRFALSE(pNullRefLabel);
16371637

1638-
pslILEmit->EmitLDNULL(); // this
16391638
EmitLoadNativeValue(pslILEmit); // ptr
16401639
pslILEmit->EmitLDC(0); // startIndex
16411640
pslILEmit->EmitLDLOC(m_dwCCHLocal); // length
16421641

16431642
// String CtorCharPtrStartLength(char *ptr, int startIndex, int length)
16441643
// TODO Phase5: Why do we call this weirdo?
1645-
pslILEmit->EmitCALL(METHOD__STRING__CTORF_CHARPTR_START_LEN, 4, 1);
1644+
pslILEmit->EmitCALL(METHOD__STRING__CTORF_CHARPTR_START_LEN, 3, 1);
16461645

16471646
EmitStoreManagedValue(pslILEmit);
16481647
pslILEmit->EmitLabel(pNullRefLabel);

‎src/coreclr/vm/metasig.h

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,6 @@ DEFINE_METASIG_T(SM(Type_RetInt, C(TYPE), i))
366366
DEFINE_METASIG(SM(ArrByte_RetObj, a(b), j))
367367
DEFINE_METASIG(SM(ArrByte_Bool_RetObj, a(b) F, j))
368368
DEFINE_METASIG(SM(ArrByte_ArrByte_RefObj_RetObj, a(b) a(b) r(j), j))
369-
DEFINE_METASIG_T(SM(PtrSByt_Int_Int_Encoding_RetStr, P(B) i i C(ENCODING), s))
370369

371370
DEFINE_METASIG_T(SM(UInt_UInt_PtrNativeOverlapped_RetVoid, K K P(g(NATIVEOVERLAPPED)), v))
372371

@@ -410,15 +409,15 @@ DEFINE_METASIG_T(IM(PtrSByt_Int_Int_Encoding_RetVoid, P(B) i i C(ENCODING), v))
410409
DEFINE_METASIG(IM(PtrChar_Int_RetVoid, P(u) i, v))
411410
DEFINE_METASIG(IM(PtrSByt_Int_RetVoid, P(B) i, v))
412411

413-
DEFINE_METASIG(IM(ArrChar_RetStr, a(u), s))
414-
DEFINE_METASIG(IM(ArrChar_Int_Int_RetStr, a(u) i i, s))
415-
DEFINE_METASIG(IM(Char_Int_RetStr, u i, s))
416-
DEFINE_METASIG(IM(PtrChar_RetStr, P(u), s))
417-
DEFINE_METASIG(IM(PtrChar_Int_Int_RetStr, P(u) i i, s))
418-
DEFINE_METASIG_T(IM(ReadOnlySpanOfChar_RetStr, GI(g(READONLY_SPAN), 1, u), s))
419-
DEFINE_METASIG(IM(PtrSByt_RetStr, P(B), s))
420-
DEFINE_METASIG(IM(PtrSByt_Int_Int_RetStr, P(B) i i, s))
421-
DEFINE_METASIG_T(IM(PtrSByt_Int_Int_Encoding_RetStr, P(B) i i C(ENCODING), s))
412+
DEFINE_METASIG(SM(ArrChar_RetStr, a(u), s))
413+
DEFINE_METASIG(SM(ArrChar_Int_Int_RetStr, a(u) i i, s))
414+
DEFINE_METASIG(SM(Char_Int_RetStr, u i, s))
415+
DEFINE_METASIG(SM(PtrChar_RetStr, P(u), s))
416+
DEFINE_METASIG(SM(PtrChar_Int_Int_RetStr, P(u) i i, s))
417+
DEFINE_METASIG_T(SM(ReadOnlySpanOfChar_RetStr, GI(g(READONLY_SPAN), 1, u), s))
418+
DEFINE_METASIG(SM(PtrSByt_RetStr, P(B), s))
419+
DEFINE_METASIG(SM(PtrSByt_Int_Int_RetStr, P(B) i i, s))
420+
DEFINE_METASIG_T(SM(PtrSByt_Int_Int_Encoding_RetStr, P(B) i i C(ENCODING), s))
422421
DEFINE_METASIG(IM(Obj_Int_RetIntPtr, j i, I))
423422

424423
DEFINE_METASIG(IM(ArrByte_Int_Int_RetVoid, a(b) i i, v))

‎src/coreclr/vm/reflectioninvocation.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -540,6 +540,7 @@ class ArgIteratorBaseForMethodInvoke
540540
{
541541
protected:
542542
SIGNATURENATIVEREF * m_ppNativeSig;
543+
bool m_fHasThis;
543544

544545
FORCEINLINE CorElementType GetReturnType(TypeHandle * pthValueType)
545546
{
@@ -567,7 +568,7 @@ class ArgIteratorBaseForMethodInvoke
567568
BOOL HasThis()
568569
{
569570
LIMITED_METHOD_CONTRACT;
570-
return (*m_ppNativeSig)->HasThis();
571+
return m_fHasThis;
571572
}
572573

573574
BOOL HasParamType()
@@ -602,10 +603,12 @@ class ArgIteratorBaseForMethodInvoke
602603
class ArgIteratorForMethodInvoke : public ArgIteratorTemplate<ArgIteratorBaseForMethodInvoke>
603604
{
604605
public:
605-
ArgIteratorForMethodInvoke(SIGNATURENATIVEREF * ppNativeSig)
606+
ArgIteratorForMethodInvoke(SIGNATURENATIVEREF * ppNativeSig, BOOL fCtorOfVariableSizedObject)
606607
{
607608
m_ppNativeSig = ppNativeSig;
608609

610+
m_fHasThis = (*m_ppNativeSig)->HasThis() && !fCtorOfVariableSizedObject;
611+
609612
DWORD dwFlags = (*m_ppNativeSig)->GetArgIteratorFlags();
610613

611614
// Use the cached values if they are available
@@ -803,7 +806,7 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod,
803806
}
804807

805808
{
806-
ArgIteratorForMethodInvoke argit(&gc.pSig);
809+
ArgIteratorForMethodInvoke argit(&gc.pSig, fCtorOfVariableSizedObject);
807810

808811
if (argit.IsActivationNeeded())
809812
pMeth->EnsureActive();
@@ -888,7 +891,7 @@ FCIMPL5(Object*, RuntimeMethodHandle::InvokeMethod,
888891
}
889892

890893
// Copy "this" pointer
891-
if (!pMeth->IsStatic()) {
894+
if (!pMeth->IsStatic() && !fCtorOfVariableSizedObject) {
892895
PVOID pThisPtr;
893896

894897
if (fConstructor)

‎src/coreclr/vm/siginfo.hpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,6 +1109,11 @@ class MetaSig
11091109
m_flags |= TREAT_AS_VARARG;
11101110
}
11111111

1112+
void ClearHasThis()
1113+
{
1114+
LIMITED_METHOD_CONTRACT;
1115+
m_CallConv &= ~IMAGE_CEE_CS_CALLCONV_HASTHIS;
1116+
}
11121117

11131118
// These are protected because Reflection subclasses Metasig
11141119
protected:

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

Lines changed: 9 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -62,13 +62,7 @@ public sealed partial class String : IComparable, IEnumerable, IConvertible, IEn
6262
[DynamicDependency("Ctor(System.Char[])")]
6363
public extern String(char[]? value);
6464

65-
#pragma warning disable CA1822 // Mark members as static
66-
67-
private
68-
#if !CORECLR
69-
static
70-
#endif
71-
string Ctor(char[]? value)
65+
private static string Ctor(char[]? value)
7266
{
7367
if (value == null || value.Length == 0)
7468
return Empty;
@@ -87,11 +81,7 @@ string Ctor(char[]? value)
8781
[DynamicDependency("Ctor(System.Char[],System.Int32,System.Int32)")]
8882
public extern String(char[] value, int startIndex, int length);
8983

90-
private
91-
#if !CORECLR
92-
static
93-
#endif
94-
string Ctor(char[] value, int startIndex, int length)
84+
private static string Ctor(char[] value, int startIndex, int length)
9585
{
9686
if (value == null)
9787
throw new ArgumentNullException(nameof(value));
@@ -123,11 +113,7 @@ string Ctor(char[] value, int startIndex, int length)
123113
[DynamicDependency("Ctor(System.Char*)")]
124114
public extern unsafe String(char* value);
125115

126-
private
127-
#if !CORECLR
128-
static
129-
#endif
130-
unsafe string Ctor(char* ptr)
116+
private static unsafe string Ctor(char* ptr)
131117
{
132118
if (ptr == null)
133119
return Empty;
@@ -151,11 +137,7 @@ unsafe string Ctor(char* ptr)
151137
[DynamicDependency("Ctor(System.Char*,System.Int32,System.Int32)")]
152138
public extern unsafe String(char* value, int startIndex, int length);
153139

154-
private
155-
#if !CORECLR
156-
static
157-
#endif
158-
unsafe string Ctor(char* ptr, int startIndex, int length)
140+
private static unsafe string Ctor(char* ptr, int startIndex, int length)
159141
{
160142
if (length < 0)
161143
throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_NegativeLength);
@@ -190,11 +172,7 @@ unsafe string Ctor(char* ptr, int startIndex, int length)
190172
[DynamicDependency("Ctor(System.SByte*)")]
191173
public extern unsafe String(sbyte* value);
192174

193-
private
194-
#if !CORECLR
195-
static
196-
#endif
197-
unsafe string Ctor(sbyte* value)
175+
private static unsafe string Ctor(sbyte* value)
198176
{
199177
byte* pb = (byte*)value;
200178
if (pb == null)
@@ -210,11 +188,7 @@ unsafe string Ctor(sbyte* value)
210188
[DynamicDependency("Ctor(System.SByte*,System.Int32,System.Int32)")]
211189
public extern unsafe String(sbyte* value, int startIndex, int length);
212190

213-
private
214-
#if !CORECLR
215-
static
216-
#endif
217-
unsafe string Ctor(sbyte* value, int startIndex, int length)
191+
private static unsafe string Ctor(sbyte* value, int startIndex, int length)
218192
{
219193
if (startIndex < 0)
220194
throw new ArgumentOutOfRangeException(nameof(startIndex), SR.ArgumentOutOfRange_StartIndex);
@@ -271,11 +245,7 @@ private static unsafe string CreateStringForSByteConstructor(byte* pb, int numBy
271245
[DynamicDependency("Ctor(System.SByte*,System.Int32,System.Int32,System.Text.Encoding)")]
272246
public extern unsafe String(sbyte* value, int startIndex, int length, Encoding enc);
273247

274-
private
275-
#if !CORECLR
276-
static
277-
#endif
278-
unsafe string Ctor(sbyte* value, int startIndex, int length, Encoding? enc)
248+
private static unsafe string Ctor(sbyte* value, int startIndex, int length, Encoding? enc)
279249
{
280250
if (enc == null)
281251
return new string(value, startIndex, length);
@@ -307,11 +277,7 @@ unsafe string Ctor(sbyte* value, int startIndex, int length, Encoding? enc)
307277
[DynamicDependency("Ctor(System.Char,System.Int32)")]
308278
public extern String(char c, int count);
309279

310-
private
311-
#if !CORECLR
312-
static
313-
#endif
314-
string Ctor(char c, int count)
280+
private static string Ctor(char c, int count)
315281
{
316282
if (count <= 0)
317283
{
@@ -332,11 +298,7 @@ string Ctor(char c, int count)
332298
[DynamicDependency("Ctor(System.ReadOnlySpan{System.Char})")]
333299
public extern String(ReadOnlySpan<char> value);
334300

335-
private
336-
#if !CORECLR
337-
static
338-
#endif
339-
unsafe string Ctor(ReadOnlySpan<char> value)
301+
private static unsafe string Ctor(ReadOnlySpan<char> value)
340302
{
341303
if (value.Length == 0)
342304
return Empty;
@@ -346,8 +308,6 @@ unsafe string Ctor(ReadOnlySpan<char> value)
346308
return result;
347309
}
348310

349-
#pragma warning restore CA1822
350-
351311
public static string Create<TState>(int length, TState state, SpanAction<char, TState> action)
352312
{
353313
if (action == null)

0 commit comments

Comments
 (0)