From c2abe892b227740c4b8baa51f5433dfc1101a52f Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 12 Nov 2018 10:58:03 -0800 Subject: [PATCH] Allow jit to examine type of initonly static ref typed fields (#20886) The jit incorporates the value of integer and float typed initonly static fields into its codegen, if the class initializer has already run. The jit can't incorporate the values of ref typed initonly static fields, but the types of those values can't change, and the jit can use this knowledge to enable type based optimizations like devirtualization. In particular for static fields initialized by complex class factory logic the jit can now see the end result of that logic instead of having to try and deduce the type of object that will initialize or did initialize the field. Examples of this factory pattern in include `EqualityComparer.Default` and `Comparer.Default`. The former is already optimized in some cases by via special-purpose modelling in the framework, jit, and runtime (see #14125) but the latter is not. With this change calls through `Comparer.Default` may now also devirtualize (though won't yet inline as the devirtualization happens late). Also update the reflection code to throw an exception instead of changing the value of a fully initialized static readonly field. Closes #4108. --- .../superpmi/superpmi-shared/icorjitinfoimpl.h | 3 + src/ToolBox/superpmi/superpmi-shared/lwmlist.h | 1 + .../superpmi/superpmi-shared/methodcontext.cpp | 31 +++++ .../superpmi/superpmi-shared/methodcontext.h | 12 +- .../superpmi-shim-collector/icorjitinfo.cpp | 14 ++ .../superpmi/superpmi-shim-counter/icorjitinfo.cpp | 7 + .../superpmi/superpmi-shim-simple/icorjitinfo.cpp | 6 + src/ToolBox/superpmi/superpmi/icorjitinfo.cpp | 7 + src/dlls/mscorrc/mscorrc.rc | 2 + src/dlls/mscorrc/resource.h | 2 + src/inc/corinfo.h | 26 +++- src/jit/compiler.h | 6 +- src/jit/gentree.cpp | 149 ++++++++++++++------- src/jit/jitconfigvalues.h | 1 + src/vm/jitinterface.cpp | 98 ++++++++++++++ src/vm/jitinterface.h | 3 + src/vm/reflectioninvocation.cpp | 31 ++++- src/zap/zapinfo.cpp | 10 ++ src/zap/zapinfo.h | 3 + tests/CoreFX/CoreFX.issues.json | 16 ++- .../src/JIT/opt/Devirtualization/readonlystatic.cs | 51 +++++++ .../JIT/opt/Devirtualization/readonlystatic.csproj | 34 +++++ .../SetValue/TrySetReadonlyStaticField.cs | 124 +++++++++++++++++ .../SetValue/TrySetReadonlyStaticField.csproj | 21 +++ 24 files changed, 599 insertions(+), 59 deletions(-) create mode 100644 tests/src/JIT/opt/Devirtualization/readonlystatic.cs create mode 100644 tests/src/JIT/opt/Devirtualization/readonlystatic.csproj create mode 100644 tests/src/reflection/SetValue/TrySetReadonlyStaticField.cs create mode 100644 tests/src/reflection/SetValue/TrySetReadonlyStaticField.csproj diff --git a/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h b/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h index e21f72b..295bcb2 100644 --- a/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h +++ b/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h @@ -877,6 +877,9 @@ unsigned getClassDomainID(CORINFO_CLASS_HANDLE cls, void** ppIndirection = NULL) // return the data's address (for static fields only) void* getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppIndirection = NULL); +// return the class handle for the current value of a static field +CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *pIsSpeculative); + // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) CORINFO_VARARGS_HANDLE getVarArgsHandle(CORINFO_SIG_INFO* pSig, void** ppIndirection = NULL); diff --git a/src/ToolBox/superpmi/superpmi-shared/lwmlist.h b/src/ToolBox/superpmi/superpmi-shared/lwmlist.h index 42ffc01..0a7daab 100644 --- a/src/ToolBox/superpmi/superpmi-shared/lwmlist.h +++ b/src/ToolBox/superpmi/superpmi-shared/lwmlist.h @@ -80,6 +80,7 @@ LWM(GetDelegateCtor, Agnostic_GetDelegateCtorIn, Agnostic_GetDelegateCtorOut) LWM(GetEEInfo, DWORD, Agnostic_CORINFO_EE_INFO) LWM(GetEHinfo, DLD, Agnostic_CORINFO_EH_CLAUSE) LWM(GetFieldAddress, DWORDLONG, Agnostic_GetFieldAddress) +LWM(GetStaticFieldCurrentClass, DWORDLONG, Agnostic_GetStaticFieldCurrentClass) LWM(GetFieldClass, DWORDLONG, DWORDLONG) LWM(GetFieldInClass, DLD, DWORDLONG) LWM(GetFieldInfo, Agnostic_GetFieldInfo, Agnostic_CORINFO_FIELD_INFO) diff --git a/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp b/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp index ea6dbd4..6e839b5 100644 --- a/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp @@ -3466,6 +3466,37 @@ void* MethodContext::repGetFieldAddress(CORINFO_FIELD_HANDLE field, void** ppInd return temp; } +void MethodContext::recGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool isSpeculative, CORINFO_CLASS_HANDLE result) +{ + if (GetStaticFieldCurrentClass == nullptr) + GetStaticFieldCurrentClass = new LightWeightMap(); + + Agnostic_GetStaticFieldCurrentClass value; + + value.classHandle = (DWORDLONG)result; + value.isSpeculative = isSpeculative; + + GetStaticFieldCurrentClass->Add((DWORDLONG)field, value); + DEBUG_REC(dmpGetFieldAddress((DWORDLONG)field, value)); +} +void MethodContext::dmpGetStaticFieldCurrentClass(DWORDLONG key, const Agnostic_GetStaticFieldCurrentClass& value) +{ + printf("GetStaticFieldCurrentClass key fld-%016llX, value clsHnd-%016llX isSpeculative-%u", key, value.classHandle, value.isSpeculative); +} +CORINFO_CLASS_HANDLE MethodContext::repGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative) +{ + Agnostic_GetStaticFieldCurrentClass value = GetStaticFieldCurrentClass->Get((DWORDLONG) field); + + if (pIsSpeculative != nullptr) + { + *pIsSpeculative = value.isSpeculative; + } + + CORINFO_CLASS_HANDLE result = (CORINFO_CLASS_HANDLE) value.classHandle; + DEBUG_REP(dmpGetStaticFieldCurrentValue((DWORDLONG)field, value)); + return result; +} + void MethodContext::recGetClassGClayout(CORINFO_CLASS_HANDLE cls, BYTE* gcPtrs, unsigned len, unsigned result) { if (GetClassGClayout == nullptr) diff --git a/src/ToolBox/superpmi/superpmi-shared/methodcontext.h b/src/ToolBox/superpmi/superpmi-shared/methodcontext.h index fa7d699..a10ab65 100644 --- a/src/ToolBox/superpmi/superpmi-shared/methodcontext.h +++ b/src/ToolBox/superpmi/superpmi-shared/methodcontext.h @@ -174,6 +174,11 @@ public: DWORDLONG fieldAddress; DWORD fieldValue; }; + struct Agnostic_GetStaticFieldCurrentClass + { + DWORDLONG classHandle; + bool isSpeculative; + }; struct Agnostic_CORINFO_RESOLVED_TOKEN { Agnostic_CORINFO_RESOLVED_TOKENin inValue; @@ -923,6 +928,10 @@ public: void dmpGetFieldAddress(DWORDLONG key, const Agnostic_GetFieldAddress& value); void* repGetFieldAddress(CORINFO_FIELD_HANDLE field, void** ppIndirection); + void recGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool isSpeculative, CORINFO_CLASS_HANDLE result); + void dmpGetStaticFieldCurrentClass(DWORDLONG key, const Agnostic_GetStaticFieldCurrentClass& value); + CORINFO_CLASS_HANDLE repGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative); + void recGetClassGClayout(CORINFO_CLASS_HANDLE cls, BYTE* gcPtrs, unsigned len, unsigned result); void dmpGetClassGClayout(DWORDLONG key, const Agnostic_GetClassGClayout& value); unsigned repGetClassGClayout(CORINFO_CLASS_HANDLE cls, BYTE* gcPtrs); @@ -1317,7 +1326,7 @@ private: }; // ********************* Please keep this up-to-date to ease adding more *************** -// Highest packet number: 171 +// Highest packet number: 172 // ************************************************************************************* enum mcPackets { @@ -1396,6 +1405,7 @@ enum mcPackets Packet_GetEEInfo = 50, Packet_GetEHinfo = 51, Packet_GetFieldAddress = 52, + Packet_GetStaticFieldCurrentClass = 172, // Added 11/7/2018 Packet_GetFieldClass = 53, Packet_GetFieldInClass = 54, Packet_GetFieldInfo = 55, diff --git a/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp index 200152a..c628f16 100644 --- a/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp @@ -1816,6 +1816,20 @@ void* interceptor_ICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppInd return temp; } +// return the class handle for the current value of a static field +CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *pIsSpeculative) +{ + mc->cr->AddCall("getStaticFieldCurrentClass"); + bool localIsSpeculative = false; + CORINFO_CLASS_HANDLE result = original_ICorJitInfo->getStaticFieldCurrentClass(field, &localIsSpeculative); + mc->recGetStaticFieldCurrentClass(field, localIsSpeculative, result); + if (pIsSpeculative != nullptr) + { + *pIsSpeculative = localIsSpeculative; + } + return result; +} + // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) CORINFO_VARARGS_HANDLE interceptor_ICJI::getVarArgsHandle(CORINFO_SIG_INFO* pSig, void** ppIndirection) { diff --git a/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp index c2100e2..d6dbecd 100644 --- a/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp @@ -1403,6 +1403,13 @@ void* interceptor_ICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppInd return original_ICorJitInfo->getFieldAddress(field, ppIndirection); } +// return the class handle for the current value of a static field +CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *pIsSpeculative) +{ + mcs->AddCall("getStaticFieldCurrentClass"); + return original_ICorJitInfo->getStaticFieldCurrentClass(field, pIsSpeculative); +} + // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) CORINFO_VARARGS_HANDLE interceptor_ICJI::getVarArgsHandle(CORINFO_SIG_INFO* pSig, void** ppIndirection) { diff --git a/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp index 4ebd09a..04c0ba0 100644 --- a/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp @@ -1258,6 +1258,12 @@ void* interceptor_ICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppInd return original_ICorJitInfo->getFieldAddress(field, ppIndirection); } +// return the class handle for the current value of a static field +CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *pIsSpeculative) +{ + return original_ICorJitInfo->getStaticFieldCurrentClass(field, pIsSpeculative); +} + // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) CORINFO_VARARGS_HANDLE interceptor_ICJI::getVarArgsHandle(CORINFO_SIG_INFO* pSig, void** ppIndirection) { diff --git a/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp index ac1abcc..60cf36c 100644 --- a/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp @@ -1516,6 +1516,13 @@ void* MyICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppIndirection) return jitInstance->mc->repGetFieldAddress(field, ppIndirection); } +// return the class handle for the current value of a static field +CORINFO_CLASS_HANDLE MyICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *pIsSpeculative) +{ + jitInstance->mc->cr->AddCall("getStaticFieldCurrentClass"); + return jitInstance->mc->repGetStaticFieldCurrentClass(field, pIsSpeculative); +} + // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) CORINFO_VARARGS_HANDLE MyICJI::getVarArgsHandle(CORINFO_SIG_INFO* pSig, void** ppIndirection) { diff --git a/src/dlls/mscorrc/mscorrc.rc b/src/dlls/mscorrc/mscorrc.rc index 0e35c29..bb39c9a 100644 --- a/src/dlls/mscorrc/mscorrc.rc +++ b/src/dlls/mscorrc/mscorrc.rc @@ -961,6 +961,8 @@ BEGIN IDS_E_TYPEACCESS "Attempt by method '%1' to access type '%2' failed.%3" IDS_INVOKE_NULLREF_RETURNED "The target method returned a null reference." + + IDS_EE_CANNOT_SET_INITONLY_STATIC_FIELD "Cannot set initonly static field '%1%' after type '%2' is initialized." END // These strings are used for Event Log Entry diff --git a/src/dlls/mscorrc/resource.h b/src/dlls/mscorrc/resource.h index 512b22e..8c315e1 100644 --- a/src/dlls/mscorrc/resource.h +++ b/src/dlls/mscorrc/resource.h @@ -716,3 +716,5 @@ #define IDS_EE_ERROR_COM 0x2641 #define IDS_INVOKE_NULLREF_RETURNED 0x2642 + +#define IDS_EE_CANNOT_SET_INITONLY_STATIC_FIELD 0x2643 diff --git a/src/inc/corinfo.h b/src/inc/corinfo.h index f456bed..059dbbe 100644 --- a/src/inc/corinfo.h +++ b/src/inc/corinfo.h @@ -213,11 +213,11 @@ TODO: Talk about initializing strutures before use #define SELECTANY extern __declspec(selectany) #endif -SELECTANY const GUID JITEEVersionIdentifier = { /* 3be99428-36f8-4a6c-acde-b42778b0f8bf */ - 0x3be99428, - 0x36f8, - 0x4a6c, - {0xac, 0xde, 0xb4, 0x27, 0x78, 0xb0, 0xf8, 0xbf} +SELECTANY const GUID JITEEVersionIdentifier = { /* b2da2a6e-72fa-4730-b47c-4c9275e1c5ce */ + 0xb2da2a6e, + 0x72fa, + 0x4730, + {0xb4, 0x7c, 0x4c, 0x92, 0x75, 0xe1, 0xc5, 0xce} }; ////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -3119,6 +3119,22 @@ public: void **ppIndirection = NULL ) = 0; + // If pIsSpeculative is NULL, return the class handle for the value of ref-class typed + // static readonly fields, if there is a unique location for the static and the class + // is already initialized. + // + // If pIsSpeculative is not NULL, fetch the class handle for the value of all ref-class + // typed static fields, if there is a unique location for the static and the field is + // not null. + // + // Set *pIsSpeculative true if this type may change over time (field is not readonly or + // is readonly but class has not yet finished initialization). Set *pIsSpeculative false + // if this type will not change. + virtual CORINFO_CLASS_HANDLE getStaticFieldCurrentClass( + CORINFO_FIELD_HANDLE field, + bool *pIsSpeculative = NULL + ) = 0; + // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) virtual CORINFO_VARARGS_HANDLE getVarArgsHandle( CORINFO_SIG_INFO *pSig, diff --git a/src/jit/compiler.h b/src/jit/compiler.h index d7ba970..ad555b8 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -2603,15 +2603,17 @@ public: // Get the handle, and assert if not found. CORINFO_CLASS_HANDLE gtGetStructHandle(GenTree* tree); // Get the handle for a ref type. - CORINFO_CLASS_HANDLE gtGetClassHandle(GenTree* tree, bool* isExact, bool* isNonNull); + CORINFO_CLASS_HANDLE gtGetClassHandle(GenTree* tree, bool* pIsExact, bool* pIsNonNull); // Get the class handle for an helper call - CORINFO_CLASS_HANDLE gtGetHelperCallClassHandle(GenTreeCall* call, bool* isExact, bool* isNonNull); + CORINFO_CLASS_HANDLE gtGetHelperCallClassHandle(GenTreeCall* call, bool* pIsExact, bool* pIsNonNull); // Get the element handle for an array of ref type. CORINFO_CLASS_HANDLE gtGetArrayElementClassHandle(GenTree* array); // Get a class handle from a helper call argument CORINFO_CLASS_HANDLE gtGetHelperArgClassHandle(GenTree* array, unsigned* runtimeLookupCount = nullptr, GenTree** handleTree = nullptr); + // Get the class handle for a field + CORINFO_CLASS_HANDLE gtGetFieldClassHandle(CORINFO_FIELD_HANDLE fieldHnd, bool* pIsExact, bool* pIsNonNull); // Check if this tree is a gc static base helper call bool gtIsStaticGCBaseHelperCall(GenTree* tree); diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp index 99109db..b685bf2 100644 --- a/src/jit/gentree.cpp +++ b/src/jit/gentree.cpp @@ -16110,22 +16110,22 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandle(GenTree* tree) // // Arguments: // tree -- tree to find handle for -// isExact [out] -- whether handle is exact type -// isNonNull [out] -- whether tree value is known not to be null +// pIsExact [out] -- whether handle is exact type +// pIsNonNull [out] -- whether tree value is known not to be null // // Return Value: // nullptr if class handle is unknown, // otherwise the class handle. -// isExact set true if tree type is known to be exactly the handle type, +// *pIsExact set true if tree type is known to be exactly the handle type, // otherwise actual type may be a subtype. -// isNonNull set true if tree value is known not to be null, +// *pIsNonNull set true if tree value is known not to be null, // otherwise a null value is possible. -CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bool* isNonNull) +CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, bool* pIsNonNull) { // Set default values for our out params. - *isNonNull = false; - *isExact = false; + *pIsNonNull = false; + *pIsExact = false; CORINFO_CLASS_HANDLE objClass = nullptr; // Bail out if we're just importing and not generating code, since @@ -16161,8 +16161,8 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo // For locals, pick up type info from the local table. const unsigned objLcl = obj->AsLclVar()->GetLclNum(); - objClass = lvaTable[objLcl].lvClassHnd; - *isExact = lvaTable[objLcl].lvClassIsExact; + objClass = lvaTable[objLcl].lvClassHnd; + *pIsExact = lvaTable[objLcl].lvClassIsExact; break; } @@ -16173,12 +16173,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo if (fieldHnd != nullptr) { - CORINFO_CLASS_HANDLE fieldClass = nullptr; - CorInfoType fieldCorType = info.compCompHnd->getFieldType(fieldHnd, &fieldClass); - if (fieldCorType == CORINFO_TYPE_CLASS) - { - objClass = fieldClass; - } + objClass = gtGetFieldClassHandle(fieldHnd, pIsExact, pIsNonNull); } break; @@ -16189,7 +16184,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo // If we see a RET_EXPR, recurse through to examine the // return value expression. GenTree* retExpr = tree->gtRetExpr.gtInlineCandidate; - objClass = gtGetClassHandle(retExpr, isExact, isNonNull); + objClass = gtGetClassHandle(retExpr, pIsExact, pIsNonNull); break; } @@ -16260,9 +16255,9 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo // This is a constructor call. const unsigned methodFlags = info.compCompHnd->getMethodAttribs(method); assert((methodFlags & CORINFO_FLG_CONSTRUCTOR) != 0); - objClass = info.compCompHnd->getMethodClass(method); - *isExact = true; - *isNonNull = true; + objClass = info.compCompHnd->getMethodClass(method); + *pIsExact = true; + *pIsNonNull = true; } else { @@ -16272,7 +16267,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo } else if (call->gtCallType == CT_HELPER) { - objClass = gtGetHelperCallClassHandle(call, isExact, isNonNull); + objClass = gtGetHelperCallClassHandle(call, pIsExact, pIsNonNull); } break; @@ -16286,8 +16281,8 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo { CORINFO_CLASS_HANDLE runtimeType = info.compCompHnd->getBuiltinClass(CLASSID_RUNTIME_TYPE); objClass = runtimeType; - *isExact = false; - *isNonNull = true; + *pIsExact = false; + *pIsNonNull = true; } break; @@ -16297,9 +16292,9 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo { // For literal strings, we know the class and that the // value is not null. - objClass = impGetStringClass(); - *isExact = true; - *isNonNull = true; + objClass = impGetStringClass(); + *pIsExact = true; + *pIsNonNull = true; break; } @@ -16320,7 +16315,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo { const unsigned objLcl = lcl->GetLclNum(); objClass = lvaTable[objLcl].lvClassHnd; - *isExact = lvaTable[objLcl].lvClassIsExact; + *pIsExact = lvaTable[objLcl].lvClassIsExact; } else if (base->OperGet() == GT_ARR_ELEM) { @@ -16328,9 +16323,9 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo GenTree* array = base->AsArrElem()->gtArrObj; - objClass = gtGetArrayElementClassHandle(array); - *isExact = false; - *isNonNull = false; + objClass = gtGetArrayElementClassHandle(array); + *pIsExact = false; + *pIsNonNull = false; } else if (base->OperGet() == GT_ADD) { @@ -16354,13 +16349,16 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo fieldSeq = fieldSeq->m_next; } + assert(!fieldSeq->IsPseudoField()); + + // No benefit to calling gtGetFieldClassHandle here, as + // the exact field being accessed can vary. CORINFO_FIELD_HANDLE fieldHnd = fieldSeq->m_fieldHnd; CORINFO_CLASS_HANDLE fieldClass = nullptr; CorInfoType fieldCorType = info.compCompHnd->getFieldType(fieldHnd, &fieldClass); - if (fieldCorType == CORINFO_TYPE_CLASS) - { - objClass = fieldClass; - } + + assert(fieldCorType == CORINFO_TYPE_CLASS); + objClass = fieldClass; } } } @@ -16379,8 +16377,8 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo assert(boxTemp->IsLocal()); const unsigned boxTempLcl = boxTemp->AsLclVar()->GetLclNum(); objClass = lvaTable[boxTempLcl].lvClassHnd; - *isExact = lvaTable[boxTempLcl].lvClassIsExact; - *isNonNull = true; + *pIsExact = lvaTable[boxTempLcl].lvClassIsExact; + *pIsNonNull = true; break; } @@ -16388,9 +16386,9 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo { GenTree* array = obj->AsIndex()->Arr(); - objClass = gtGetArrayElementClassHandle(array); - *isExact = false; - *isNonNull = false; + objClass = gtGetArrayElementClassHandle(array); + *pIsExact = false; + *pIsNonNull = false; break; } @@ -16409,19 +16407,19 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo // // Arguments: // call - helper call to examine -// isExact - [OUT] true if type is known exactly -// isNonNull - [OUT] true if return value is not null +// pIsExact - [OUT] true if type is known exactly +// pIsNonNull - [OUT] true if return value is not null // // Return Value: // nullptr if helper call result is not a ref class, or the class handle // is unknown, otherwise the class handle. -CORINFO_CLASS_HANDLE Compiler::gtGetHelperCallClassHandle(GenTreeCall* call, bool* isExact, bool* isNonNull) +CORINFO_CLASS_HANDLE Compiler::gtGetHelperCallClassHandle(GenTreeCall* call, bool* pIsExact, bool* pIsNonNull) { assert(call->gtCallType == CT_HELPER); - *isNonNull = false; - *isExact = false; + *pIsNonNull = false; + *pIsExact = false; CORINFO_CLASS_HANDLE objClass = nullptr; const CorInfoHelpFunc helper = eeGetHelperNum(call->gtCallMethHnd); @@ -16437,8 +16435,8 @@ CORINFO_CLASS_HANDLE Compiler::gtGetHelperCallClassHandle(GenTreeCall* call, boo const bool helperResultNonNull = (helper == CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE); CORINFO_CLASS_HANDLE runtimeType = info.compCompHnd->getBuiltinClass(CLASSID_RUNTIME_TYPE); - objClass = runtimeType; - *isNonNull = helperResultNonNull; + objClass = runtimeType; + *pIsNonNull = helperResultNonNull; break; } @@ -16481,7 +16479,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetHelperCallClassHandle(GenTreeCall* call, boo if (castHnd == nullptr) { GenTree* valueArg = args->Rest()->Current(); - castHnd = gtGetClassHandle(valueArg, isExact, isNonNull); + castHnd = gtGetClassHandle(valueArg, pIsExact, pIsNonNull); } // We don't know at jit time if the cast will succeed or fail, but if it @@ -16544,6 +16542,65 @@ CORINFO_CLASS_HANDLE Compiler::gtGetArrayElementClassHandle(GenTree* array) } //------------------------------------------------------------------------ +// gtGetFieldClassHandle: find class handle for a field +// +// Arguments: +// fieldHnd - field handle for field in question +// pIsExact - [OUT] true if type is known exactly +// pIsNonNull - [OUT] true if field value is not null +// +// Return Value: +// nullptr if helper call result is not a ref class, or the class handle +// is unknown, otherwise the class handle. +// +// May examine runtime state of static field instances. + +CORINFO_CLASS_HANDLE Compiler::gtGetFieldClassHandle(CORINFO_FIELD_HANDLE fieldHnd, bool* pIsExact, bool* pIsNonNull) +{ + CORINFO_CLASS_HANDLE fieldClass = nullptr; + CorInfoType fieldCorType = info.compCompHnd->getFieldType(fieldHnd, &fieldClass); + + if (fieldCorType == CORINFO_TYPE_CLASS) + { + // Optionally, look at the actual type of the field's value + bool queryForCurrentClass = true; + INDEBUG(queryForCurrentClass = (JitConfig.JitQueryCurrentStaticFieldClass() > 0);); + + if (queryForCurrentClass) + { + +#if DEBUG + const char* fieldClassName = nullptr; + const char* fieldName = eeGetFieldName(fieldHnd, &fieldClassName); + JITDUMP("Querying runtime about current class of field %s.%s (declared as %s)\n", fieldClassName, fieldName, + eeGetClassName(fieldClass)); +#endif // DEBUG + + // Is this a fully initialized init-only static field? + // + // Note we're not asking for speculative results here, yet. + CORINFO_CLASS_HANDLE currentClass = info.compCompHnd->getStaticFieldCurrentClass(fieldHnd); + + if (currentClass != NO_CLASS_HANDLE) + { + // Yes! We know the class exactly and can rely on this to always be true. + fieldClass = currentClass; + *pIsExact = true; + *pIsNonNull = true; + JITDUMP("Runtime reports field is init-only and initialized and has class %s\n", + eeGetClassName(fieldClass)); + } + else + { + JITDUMP("Field's current class not available\n"); + } + } + } + + return fieldClass; +} + +//------------------------------------------------------------------------ // gtIsGCStaticBaseHelperCall: true if tree is fetching the gc static base // for a subsequent static field access // diff --git a/src/jit/jitconfigvalues.h b/src/jit/jitconfigvalues.h index 07f0ab1..f238fee 100644 --- a/src/jit/jitconfigvalues.h +++ b/src/jit/jitconfigvalues.h @@ -95,6 +95,7 @@ CONFIG_INTEGER(JitNoRegLoc, W("JitNoRegLoc"), 0) CONFIG_INTEGER(JitNoStructPromotion, W("JitNoStructPromotion"), 0) // Disables struct promotion in Jit32 CONFIG_INTEGER(JitNoUnroll, W("JitNoUnroll"), 0) CONFIG_INTEGER(JitOrder, W("JitOrder"), 0) +CONFIG_INTEGER(JitQueryCurrentStaticFieldClass, W("JitQueryCurrentStaticFieldClass"), 1) CONFIG_INTEGER(JitReportFastTailCallDecisions, W("JitReportFastTailCallDecisions"), 0) CONFIG_INTEGER(JitPInvokeCheckEnabled, W("JITPInvokeCheckEnabled"), 0) CONFIG_INTEGER(JitPInvokeEnabled, W("JITPInvokeEnabled"), 1) diff --git a/src/vm/jitinterface.cpp b/src/vm/jitinterface.cpp index 658e610..efaa340 100644 --- a/src/vm/jitinterface.cpp +++ b/src/vm/jitinterface.cpp @@ -11756,6 +11756,94 @@ void* CEEJitInfo::getFieldAddress(CORINFO_FIELD_HANDLE fieldHnd, return result; } +/*********************************************************************/ +CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE fieldHnd, + bool* pIsSpeculative) +{ + CONTRACTL { + SO_TOLERANT; + THROWS; + GC_TRIGGERS; + MODE_PREEMPTIVE; + } CONTRACTL_END; + + CORINFO_CLASS_HANDLE result = NULL; + + if (pIsSpeculative != NULL) + { + *pIsSpeculative = true; + } + + // Only examine the field's value if we are producing jitted code. + if (isVerifyOnly() || IsCompilingForNGen() || IsReadyToRunCompilation()) + { + return result; + } + + JIT_TO_EE_TRANSITION(); + + FieldDesc* field = (FieldDesc*) fieldHnd; + bool isClassInitialized = false; + + // We're only interested in ref class typed static fields + // where the field handle specifies a unique location. + if (field->IsStatic() && field->IsObjRef() && !field->IsThreadStatic()) + { + MethodTable* pEnclosingMT = field->GetEnclosingMethodTable(); + + if (!pEnclosingMT->IsSharedByGenericInstantiations()) + { + // Allocate space for the local class if necessary, but don't trigger + // class construction. + DomainLocalModule *pLocalModule = pEnclosingMT->GetDomainLocalModule(); + pLocalModule->PopulateClass(pEnclosingMT); + + GCX_COOP(); + + OBJECTREF fieldObj = field->GetStaticOBJECTREF(); + VALIDATEOBJECTREF(fieldObj); + + // Check for initialization before looking at the value + isClassInitialized = !!pEnclosingMT->IsClassInited(); + + if (fieldObj != NULL) + { + MethodTable *pObjMT = fieldObj->GetMethodTable(); + + // TODO: Check if the jit is allowed to embed this handle in jitted code. + // Note for the initonly cases it probably won't embed. + result = (CORINFO_CLASS_HANDLE) pObjMT; + } + } + } + + // Did we find a class? + if (result != NULL) + { + // Figure out what to report back. + bool isResultImmutable = isClassInitialized && IsFdInitOnly(field->GetAttributes()); + + if (pIsSpeculative != NULL) + { + // Caller is ok with potentially mutable results. + *pIsSpeculative = !isResultImmutable; + } + else + { + // Caller only wants to see immutable results. + if (!isResultImmutable) + { + result = NULL; + } + } + } + + EE_TO_JIT_TRANSITION(); + + return result; +} + +/*********************************************************************/ static void *GetClassSync(MethodTable *pMT) { STANDARD_VM_CONTRACT; @@ -13865,6 +13953,16 @@ void* CEEInfo::getFieldAddress(CORINFO_FIELD_HANDLE fieldHnd, return (void *)0x10; } +CORINFO_CLASS_HANDLE CEEInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE fieldHnd, + bool* pIsSpeculative) +{ + LIMITED_METHOD_CONTRACT; + _ASSERTE(isVerifyOnly()); + if (pIsSpeculative != NULL) + *pIsSpeculative = true; + return NULL; +} + void* CEEInfo::getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, void **ppIndirection) { diff --git a/src/vm/jitinterface.h b/src/vm/jitinterface.h index d99a0ff..4ec732f 100644 --- a/src/vm/jitinterface.h +++ b/src/vm/jitinterface.h @@ -853,6 +853,8 @@ public: void* getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection); + CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative); + // ICorDebugInfo stuff void * allocateArray(ULONG cBytes); void freeArray(void *array); @@ -1492,6 +1494,7 @@ public: InfoAccessType constructStringLiteral(CORINFO_MODULE_HANDLE scopeHnd, mdToken metaTok, void **ppValue); InfoAccessType emptyStringLiteral(void ** ppValue); void* getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection); + CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative); void* getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, void **ppIndirection); void BackoutJitData(EEJitManager * jitMgr); diff --git a/src/vm/reflectioninvocation.cpp b/src/vm/reflectioninvocation.cpp index 6f5011f..a8d2aa7 100644 --- a/src/vm/reflectioninvocation.cpp +++ b/src/vm/reflectioninvocation.cpp @@ -339,6 +339,22 @@ FCIMPL7(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Ob HELPER_METHOD_FRAME_BEGIN_PROTECT(gc); + // Verify we're not trying to set the value of a static initonly field + // once the class has been initialized. + if (pFieldDesc->IsStatic()) + { + MethodTable* pEnclosingMT = pFieldDesc->GetEnclosingMethodTable(); + if (pEnclosingMT->IsClassInited() && IsFdInitOnly(pFieldDesc->GetAttributes())) + { + DefineFullyQualifiedNameForClassW(); + SString ssFieldName(SString::Utf8, pFieldDesc->GetName()); + COMPlusThrow(kFieldAccessException, + IDS_EE_CANNOT_SET_INITONLY_STATIC_FIELD, + ssFieldName.GetUnicode(), + GetFullyQualifiedNameForClassW(pEnclosingMT)); + } + } + //TODO: cleanup this function InvokeUtil::SetValidField(fieldType.GetSignatureCorElementType(), fieldType, pFieldDesc, &gc.target, &gc.value, declaringType, pDomainInitialized); @@ -1709,10 +1725,17 @@ FCIMPL5(void, RuntimeFieldHandle::SetValueDirect, ReflectFieldObject *pFieldUNSA // Verify that the value passed can be widened into the target InvokeUtil::ValidField(fieldType, &gc.oValue); - // Verify that this is not a Final Field - DWORD attr = pField->GetAttributes(); // should we cache? - if (IsFdLiteral(attr)) - COMPlusThrow(kFieldAccessException,W("Acc_ReadOnly")); + // Verify we're not trying to set the value of a static initonly field + // once the class has been initialized. + if (pField->IsStatic() && pEnclosingMT->IsClassInited() && IsFdInitOnly(pField->GetAttributes())) + { + DefineFullyQualifiedNameForClassW(); + SString ssFieldName(SString::Utf8, pField->GetName()); + COMPlusThrow(kFieldAccessException, + IDS_EE_CANNOT_SET_INITONLY_STATIC_FIELD, + ssFieldName.GetUnicode(), + GetFullyQualifiedNameForClassW(pEnclosingMT)); + } // Verify the callee/caller access if (!pField->IsPublic() || (pEnclosingMT != NULL && !pEnclosingMT->IsExternallyVisible())) diff --git a/src/zap/zapinfo.cpp b/src/zap/zapinfo.cpp index 486cc18..8efeedd 100644 --- a/src/zap/zapinfo.cpp +++ b/src/zap/zapinfo.cpp @@ -2339,6 +2339,16 @@ void * ZapInfo::getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection return NULL; } +CORINFO_CLASS_HANDLE ZapInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative) +{ + if (pIsSpeculative != NULL) + { + *pIsSpeculative = true; + } + + return NULL; +} + DWORD ZapInfo::getFieldThreadLocalStoreID(CORINFO_FIELD_HANDLE field, void **ppIndirection) { diff --git a/src/zap/zapinfo.h b/src/zap/zapinfo.h index 70d6332..ded4d53 100644 --- a/src/zap/zapinfo.h +++ b/src/zap/zapinfo.h @@ -417,6 +417,9 @@ public: void * getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection); + CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, + bool* pIsSpeculative); + DWORD getFieldThreadLocalStoreID (CORINFO_FIELD_HANDLE field, void **ppIndirection); CORINFO_VARARGS_HANDLE getVarArgsHandle(CORINFO_SIG_INFO *sig, diff --git a/tests/CoreFX/CoreFX.issues.json b/tests/CoreFX/CoreFX.issues.json index 4520001..bc0c131 100644 --- a/tests/CoreFX/CoreFX.issues.json +++ b/tests/CoreFX/CoreFX.issues.json @@ -792,5 +792,19 @@ }, ] } - }, + }, + { + "name": "System.Linq.Expressions.Tests", + "enabled": true, + "exclusions": { + "namespaces": null, + "classes": null, + "methods": [ + { + "name": "System.Linq.Expressions.Tests.BindTests.StaticReadonlyField", + "reason": "https://github.com/dotnet/coreclr/pull/20886 and https://github.com/dotnet/corefx/pull/33413" + } + ] + } + } ] diff --git a/tests/src/JIT/opt/Devirtualization/readonlystatic.cs b/tests/src/JIT/opt/Devirtualization/readonlystatic.cs new file mode 100644 index 0000000..5cafa23 --- /dev/null +++ b/tests/src/JIT/opt/Devirtualization/readonlystatic.cs @@ -0,0 +1,51 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.CompilerServices; + +class B +{ + public virtual int F() => -1; +} + +class D : B +{ + public override int F() => 100; +} + +class X +{ + static readonly B S; + static int R; + + static X() + { + S = new B(); + R = During(); + S = new D(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int During() + { + // Jit should not be able to devirtualize here + return S.F(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int After() + { + // Jit should be able to devirtualize here + return S.F(); + } + + public static int Main() + { + var p = S; + int a = After(); + int d = During(); + return a + d + R - 99; + } +} diff --git a/tests/src/JIT/opt/Devirtualization/readonlystatic.csproj b/tests/src/JIT/opt/Devirtualization/readonlystatic.csproj new file mode 100644 index 0000000..592b35c --- /dev/null +++ b/tests/src/JIT/opt/Devirtualization/readonlystatic.csproj @@ -0,0 +1,34 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + + + + + + False + + + + None + True + + + + + + + + + + \ No newline at end of file diff --git a/tests/src/reflection/SetValue/TrySetReadonlyStaticField.cs b/tests/src/reflection/SetValue/TrySetReadonlyStaticField.cs new file mode 100644 index 0000000..7a5d726 --- /dev/null +++ b/tests/src/reflection/SetValue/TrySetReadonlyStaticField.cs @@ -0,0 +1,124 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Reflection; + +class X +{ + readonly static string S; + readonly static string S_Expected; + readonly static bool B0; + readonly static bool B1; + + static X() + { + Console.WriteLine("Begin initializing class X"); + S = "0"; + B0 = Set("1", false); + B1 = SetDirect("2", false); + S_Expected = S; + Console.WriteLine("Done initializing class X"); + } + + static bool Set(string s, bool shouldThrow) + { + var type = typeof(X); + var field = type.GetField("S", BindingFlags.NonPublic | BindingFlags.Static); + bool threw = false; + bool unexpected = false; + + Console.WriteLine($"Attempting to update {field.Name} via SetValue, current value is '{S}', desired new value is '{s}'"); + + try + { + field.SetValue(null, s); + } + catch (FieldAccessException f) + { + Console.WriteLine("Caught {0}expected exception", shouldThrow ? "" : "un"); + Console.WriteLine(f); + threw = true; + } + catch (Exception e) + { + Console.WriteLine("Caught unexpected exception: {0}", e.GetType()); + Console.WriteLine(e); + threw = true; + unexpected = true; + } + + if (!threw) + { + Console.WriteLine($"Updated {field.Name} to '{S}'"); + } + + return (shouldThrow == threw) && !unexpected; + } + + static bool SetDirect(string s, bool shouldThrow) + { + int i = 0; + TypedReference t = __makeref(i); + var type = typeof(X); + var field = type.GetField("S", BindingFlags.NonPublic | BindingFlags.Static); + bool threw = false; + bool unexpected = false; + + Console.WriteLine($"Attempting to update {field.Name} via SetValueDirect, current value is '{S}', desired new value is '{s}'"); + + try + { + field.SetValueDirect(t, s); + } + catch (FieldAccessException f) + { + Console.WriteLine("Caught {0}expected exception", shouldThrow ? "" : "un"); + Console.WriteLine(f); + threw = true; + + } + catch (Exception e) + { + Console.WriteLine("Caught unexpected exception: {0}", e.GetType()); + Console.WriteLine(e); + unexpected = true; + threw = true; + } + + if (!threw) + { + Console.WriteLine($"Updated {field.Name} to '{S}'"); + } + + return (shouldThrow == threw) && !unexpected; + } + + public static int Main() + { + var s = S; + bool b0 = Set("3", true); + bool b1 = SetDirect("4", true); + bool v = (S == S_Expected); + if (!B0) Console.WriteLine("SetValue during class init unexpectedly threw"); + if (!B1) Console.WriteLine("SetValueDirect during class init unexpectedly threw"); + if (!b0) Console.WriteLine("SetValue after class init didn't throw as expected"); + if (!b1) Console.WriteLine("SetValueDirect after class init didn't throw as expected"); + Console.Write($"S is '{S}' "); + if (v) + { + Console.WriteLine(" as expected"); + } + else + { + Console.WriteLine($", should be '{S_Expected}'"); + } + bool ok = B0 && B1 && b0 && b1 && v; + + Console.WriteLine(ok ? "PASS" : "FAIL"); + return ok ? 100 : -1; + } +} + + diff --git a/tests/src/reflection/SetValue/TrySetReadonlyStaticField.csproj b/tests/src/reflection/SetValue/TrySetReadonlyStaticField.csproj new file mode 100644 index 0000000..d7d1899 --- /dev/null +++ b/tests/src/reflection/SetValue/TrySetReadonlyStaticField.csproj @@ -0,0 +1,21 @@ + + + + + Debug + AnyCPU + {58DB4A46-51BE-46A1-AEA1-0C32FBAF5562} + Exe + latest + true + + + + + + + + + + + -- 2.7.4