From 7e39a00bd0a5fe0126109393a8aee5dc5c2295a7 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 24 Oct 2018 14:52:44 -0700 Subject: [PATCH 01/10] Allow jit to examine type of initonly static ref typed fields 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). Addresses #4108. --- .../superpmi-shared/icorjitinfoimpl.h | 3 + .../superpmi/superpmi-shared/lwmlist.h | 1 + .../superpmi-shared/methodcontext.cpp | 31 +++++++ .../superpmi/superpmi-shared/methodcontext.h | 12 ++- .../superpmi-shim-collector/icorjitinfo.cpp | 14 +++ .../superpmi-shim-counter/icorjitinfo.cpp | 7 ++ .../superpmi-shim-simple/icorjitinfo.cpp | 6 ++ src/ToolBox/superpmi/superpmi/icorjitinfo.cpp | 7 ++ src/inc/corinfo.h | 17 ++-- src/jit/compiler.h | 2 + src/jit/gentree.cpp | 88 ++++++++++++++++--- src/jit/jitconfigvalues.h | 1 + src/vm/jitinterface.cpp | 83 +++++++++++++++++ src/vm/jitinterface.h | 3 + src/zap/zapinfo.cpp | 10 +++ src/zap/zapinfo.h | 3 + 16 files changed, 272 insertions(+), 16 deletions(-) diff --git a/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h b/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h index e21f72b668ba..0d3b1583fd60 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 *isInitOnly); + // 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 42ffc0142350..0a7daabe8784 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 ea6dbd495359..ae9fe9aeb056 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 isInitOnly, CORINFO_CLASS_HANDLE result) +{ + if (GetStaticFieldCurrentClass == nullptr) + GetStaticFieldCurrentClass = new LightWeightMap(); + + Agnostic_GetStaticFieldCurrentClass value; + + value.classHandle = (DWORDLONG)result; + value.isInitOnly = isInitOnly; + + 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 isInitOnly-%u", key, value.classHandle, value.isInitOnly); +} +CORINFO_CLASS_HANDLE MethodContext::repGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isInitOnly) +{ + Agnostic_GetStaticFieldCurrentClass value = GetStaticFieldCurrentClass->Get((DWORDLONG) field); + + if (isInitOnly != nullptr) + { + *isInitOnly = value.isInitOnly; + } + + 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 fa7d699e62a5..9f6f72084b0d 100644 --- a/src/ToolBox/superpmi/superpmi-shared/methodcontext.h +++ b/src/ToolBox/superpmi/superpmi-shared/methodcontext.h @@ -174,6 +174,11 @@ class MethodContext DWORDLONG fieldAddress; DWORD fieldValue; }; + struct Agnostic_GetStaticFieldCurrentClass + { + DWORDLONG classHandle; + bool isInitOnly; + }; struct Agnostic_CORINFO_RESOLVED_TOKEN { Agnostic_CORINFO_RESOLVED_TOKENin inValue; @@ -923,6 +928,10 @@ class MethodContext void dmpGetFieldAddress(DWORDLONG key, const Agnostic_GetFieldAddress& value); void* repGetFieldAddress(CORINFO_FIELD_HANDLE field, void** ppIndirection); + void recGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool isInitOnly, CORINFO_CLASS_HANDLE result); + void dmpGetStaticFieldCurrentClass(DWORDLONG key, const Agnostic_GetStaticFieldCurrentClass& value); + CORINFO_CLASS_HANDLE repGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isInitOnly); + 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 @@ class MethodContext }; // ********************* 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 200152a046e2..4b018ef9b6ca 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 *isInitOnly) +{ + mc->cr->AddCall("getStaticFieldCurrentClass"); + bool localIsInitOnly = false; + CORINFO_CLASS_HANDLE result = original_ICorJitInfo->getStaticFieldCurrentClass(field, &localIsInitOnly); + mc->recGetStaticFieldCurrentClass(field, localIsInitOnly, result); + if (isInitOnly != nullptr) + { + *isInitOnly = localIsInitOnly; + } + 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 c2100e245fac..b33037f84776 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 *isInitOnly) +{ + mcs->AddCall("getStaticFieldCurrentClass"); + return original_ICorJitInfo->getStaticFieldCurrentClass(field, isInitOnly); +} + // 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 4ebd09a4f6b3..cea6af417a91 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 *isInitOnly) +{ + return original_ICorJitInfo->getStaticFieldCurrentClass(field, isInitOnly); +} + // 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 ac1abcc38d72..1f9ce352d9b0 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 *isInitOnly) +{ + jitInstance->mc->cr->AddCall("getStaticFieldCurrentClass"); + return jitInstance->mc->repGetStaticFieldCurrentClass(field, isInitOnly); +} + // 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/inc/corinfo.h b/src/inc/corinfo.h index f456bed6f4ff..3d4255363ef2 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,13 @@ class ICorDynamicInfo : public ICorStaticInfo void **ppIndirection = NULL ) = 0; + // For ref-class typed static readonly fields, return the class handle for value of the field + // if there is a unique location for the static and the class is already initialized. + virtual CORINFO_CLASS_HANDLE getStaticFieldCurrentClass( + CORINFO_FIELD_HANDLE field, + bool *isInitOnly + ) = 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 d7ba9709c5e1..f46f0b04309b 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -2612,6 +2612,8 @@ class Compiler 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* isExact, bool* IsNonNull); // 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 99109db1e1d1..13b99919d7ee 100644 --- a/src/jit/gentree.cpp +++ b/src/jit/gentree.cpp @@ -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, isExact, isNonNull); } break; @@ -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; } } } @@ -16543,6 +16541,76 @@ CORINFO_CLASS_HANDLE Compiler::gtGetArrayElementClassHandle(GenTree* array) return nullptr; } +//------------------------------------------------------------------------ +// gtGetFieldClassHandle: find class handle for a field +// +// Arguments: +// fieldHnd - field handle for field in question +// isExact - [OUT] true if type is known exactly +// isNonNull - [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* isExact, bool* isNonNull) +{ + 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 an initialized static init-only field? + bool isFieldInitOnly = false; + CORINFO_CLASS_HANDLE currentClass = + info.compCompHnd->getStaticFieldCurrentClass(fieldHnd, &isFieldInitOnly); + + if (currentClass != NO_CLASS_HANDLE) + { + // We know the current type -- can we rely on it? + if (isFieldInitOnly) + { + // Yes! We know the class exactly and can rely on this to always be true. + fieldClass = currentClass; + *isExact = true; + *isNonNull = true; + JITDUMP("Runtime reports field is init-only and has type %s\n", eeGetClassName(fieldClass)); + } + else + { + // No. We know the current type but it may change over time. + // + // We could use this type as an informed guess, someday, + // if it is a "better" type than the declared field type. + JITDUMP("Field is not init-only\n"); + } + } + 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 07f0ab15cecb..f238feeb6834 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 658e61017a3b..c96fe6a6278c 100644 --- a/src/vm/jitinterface.cpp +++ b/src/vm/jitinterface.cpp @@ -11756,6 +11756,79 @@ void* CEEJitInfo::getFieldAddress(CORINFO_FIELD_HANDLE fieldHnd, return result; } +/*********************************************************************/ +CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE fieldHnd, + bool* isInitOnly) +{ + CONTRACTL { + SO_TOLERANT; + THROWS; + GC_TRIGGERS; + MODE_PREEMPTIVE; + } CONTRACTL_END; + + CORINFO_CLASS_HANDLE result = NULL; + + if (isInitOnly != NULL) + { + *isInitOnly = false; + } + + // 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; + + // 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(); + + // We must not call here for statics of collectible types. + _ASSERTE(!pEnclosingMT->Collectible()); + + 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); + + 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; + } + } + } + + // If the field is init only, and the class is known, the class can't ever change. + if ((result != NULL) && (isInitOnly != NULL)) + { + DWORD fieldAttribs = field->GetAttributes(); + *isInitOnly = !!IsFdInitOnly(fieldAttribs); + } + + EE_TO_JIT_TRANSITION(); + + return result; +} + +/*********************************************************************/ static void *GetClassSync(MethodTable *pMT) { STANDARD_VM_CONTRACT; @@ -13865,6 +13938,16 @@ void* CEEInfo::getFieldAddress(CORINFO_FIELD_HANDLE fieldHnd, return (void *)0x10; } +CORINFO_CLASS_HANDLE CEEInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE fieldHnd, + bool* isInitOnly) +{ + LIMITED_METHOD_CONTRACT; + _ASSERTE(isVerifyOnly()); + if (isInitOnly != NULL) + *isInitOnly = false; + return NULL; +} + void* CEEInfo::getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, void **ppIndirection) { diff --git a/src/vm/jitinterface.h b/src/vm/jitinterface.h index d99a0ff2ecff..fa81545ef3f3 100644 --- a/src/vm/jitinterface.h +++ b/src/vm/jitinterface.h @@ -853,6 +853,8 @@ class CEEInfo : public ICorJitInfo void* getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection); + CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isInitOnly); + // ICorDebugInfo stuff void * allocateArray(ULONG cBytes); void freeArray(void *array); @@ -1492,6 +1494,7 @@ class CEEJitInfo : public CEEInfo 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* isInitOnly); void* getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, void **ppIndirection); void BackoutJitData(EEJitManager * jitMgr); diff --git a/src/zap/zapinfo.cpp b/src/zap/zapinfo.cpp index 486cc184067e..7512885fb677 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* isInitOnly) +{ + if (isInitOnly != NULL) + { + isInitOnly = false; + } + + return NULL; +} + DWORD ZapInfo::getFieldThreadLocalStoreID(CORINFO_FIELD_HANDLE field, void **ppIndirection) { diff --git a/src/zap/zapinfo.h b/src/zap/zapinfo.h index 70d633279065..0653969e98e5 100644 --- a/src/zap/zapinfo.h +++ b/src/zap/zapinfo.h @@ -417,6 +417,9 @@ class ZapInfo void * getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection); + CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, + bool* isInitO); + DWORD getFieldThreadLocalStoreID (CORINFO_FIELD_HANDLE field, void **ppIndirection); CORINFO_VARARGS_HANDLE getVarArgsHandle(CORINFO_SIG_INFO *sig, From c007a9fa7232cedbd774c08c5ee414aa4308b45f Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 8 Nov 2018 12:01:30 -0800 Subject: [PATCH 02/10] fix build issue --- src/zap/zapinfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zap/zapinfo.cpp b/src/zap/zapinfo.cpp index 7512885fb677..3d8bc35fed4a 100644 --- a/src/zap/zapinfo.cpp +++ b/src/zap/zapinfo.cpp @@ -2343,7 +2343,7 @@ CORINFO_CLASS_HANDLE ZapInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE fi { if (isInitOnly != NULL) { - isInitOnly = false; + *isInitOnly = false; } return NULL; From ccbbc71b149bd0ef5756d0a99e7e62423336a9d9 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 8 Nov 2018 14:19:05 -0800 Subject: [PATCH 03/10] ensure class is fully initialized Also remove assert for collectible classes and fix typo. --- src/vm/jitinterface.cpp | 14 ++++++++------ src/zap/zapinfo.h | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/vm/jitinterface.cpp b/src/vm/jitinterface.cpp index c96fe6a6278c..363e9b2650a7 100644 --- a/src/vm/jitinterface.cpp +++ b/src/vm/jitinterface.cpp @@ -11783,6 +11783,7 @@ CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE 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. @@ -11790,9 +11791,6 @@ CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE { MethodTable* pEnclosingMT = field->GetEnclosingMethodTable(); - // We must not call here for statics of collectible types. - _ASSERTE(!pEnclosingMT->Collectible()); - if (!pEnclosingMT->IsSharedByGenericInstantiations()) { // Allocate space for the local class if necessary, but don't trigger @@ -11805,6 +11803,9 @@ CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE OBJECTREF fieldObj = field->GetStaticOBJECTREF(); VALIDATEOBJECTREF(fieldObj); + // Check for initialization before looking at the value + isClassInitialized = !!pEnclosingMT->IsClassInited(); + if (fieldObj != NULL) { MethodTable *pObjMT = fieldObj->GetMethodTable(); @@ -11815,9 +11816,10 @@ CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE } } } - - // If the field is init only, and the class is known, the class can't ever change. - if ((result != NULL) && (isInitOnly != NULL)) + + // If the field is init only, and the class is known, and class initialization + // has finished, the class can't ever change. + if ((result != NULL) && isClassInitialized && (isInitOnly != NULL)) { DWORD fieldAttribs = field->GetAttributes(); *isInitOnly = !!IsFdInitOnly(fieldAttribs); diff --git a/src/zap/zapinfo.h b/src/zap/zapinfo.h index 0653969e98e5..05e1a6787df7 100644 --- a/src/zap/zapinfo.h +++ b/src/zap/zapinfo.h @@ -418,7 +418,7 @@ class ZapInfo void * getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection); CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, - bool* isInitO); + bool* isInitOnly); DWORD getFieldThreadLocalStoreID (CORINFO_FIELD_HANDLE field, void **ppIndirection); From 508577838259f4d8f82d7c50c53714170279d434 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 9 Nov 2018 10:37:18 -0800 Subject: [PATCH 04/10] isInitOnly -> isSpeculative Flip the sense of the return flag for getStaticFieldCurrentClass and update remainder of code accordingly. Change jit to not ask for speculative results. --- .../superpmi-shared/icorjitinfoimpl.h | 2 +- .../superpmi-shared/methodcontext.cpp | 12 +++---- .../superpmi/superpmi-shared/methodcontext.h | 6 ++-- .../superpmi-shim-collector/icorjitinfo.cpp | 12 +++---- .../superpmi-shim-counter/icorjitinfo.cpp | 4 +-- .../superpmi-shim-simple/icorjitinfo.cpp | 4 +-- src/ToolBox/superpmi/superpmi/icorjitinfo.cpp | 4 +-- src/inc/corinfo.h | 15 ++++++-- src/jit/gentree.cpp | 31 ++++++---------- src/vm/jitinterface.cpp | 35 +++++++++++++------ src/vm/jitinterface.h | 4 +-- src/zap/zapinfo.cpp | 6 ++-- src/zap/zapinfo.h | 2 +- 13 files changed, 75 insertions(+), 62 deletions(-) diff --git a/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h b/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h index 0d3b1583fd60..089fa03abefe 100644 --- a/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h +++ b/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h @@ -878,7 +878,7 @@ unsigned getClassDomainID(CORINFO_CLASS_HANDLE cls, void** ppIndirection = NULL) 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 *isInitOnly); +CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isSpeculative); // 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/methodcontext.cpp b/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp index ae9fe9aeb056..fcac0894598e 100644 --- a/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp @@ -3466,7 +3466,7 @@ void* MethodContext::repGetFieldAddress(CORINFO_FIELD_HANDLE field, void** ppInd return temp; } -void MethodContext::recGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool isInitOnly, CORINFO_CLASS_HANDLE result) +void MethodContext::recGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool isSpeculative, CORINFO_CLASS_HANDLE result) { if (GetStaticFieldCurrentClass == nullptr) GetStaticFieldCurrentClass = new LightWeightMap(); @@ -3474,22 +3474,22 @@ void MethodContext::recGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bo Agnostic_GetStaticFieldCurrentClass value; value.classHandle = (DWORDLONG)result; - value.isInitOnly = isInitOnly; + 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 isInitOnly-%u", key, value.classHandle, value.isInitOnly); + 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* isInitOnly) +CORINFO_CLASS_HANDLE MethodContext::repGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isSpeculative) { Agnostic_GetStaticFieldCurrentClass value = GetStaticFieldCurrentClass->Get((DWORDLONG) field); - if (isInitOnly != nullptr) + if (isSpeculative != nullptr) { - *isInitOnly = value.isInitOnly; + *isSpeculative = value.isSpeculative; } CORINFO_CLASS_HANDLE result = (CORINFO_CLASS_HANDLE) value.classHandle; diff --git a/src/ToolBox/superpmi/superpmi-shared/methodcontext.h b/src/ToolBox/superpmi/superpmi-shared/methodcontext.h index 9f6f72084b0d..a9610298e604 100644 --- a/src/ToolBox/superpmi/superpmi-shared/methodcontext.h +++ b/src/ToolBox/superpmi/superpmi-shared/methodcontext.h @@ -177,7 +177,7 @@ class MethodContext struct Agnostic_GetStaticFieldCurrentClass { DWORDLONG classHandle; - bool isInitOnly; + bool isSpeculative; }; struct Agnostic_CORINFO_RESOLVED_TOKEN { @@ -928,9 +928,9 @@ class MethodContext void dmpGetFieldAddress(DWORDLONG key, const Agnostic_GetFieldAddress& value); void* repGetFieldAddress(CORINFO_FIELD_HANDLE field, void** ppIndirection); - void recGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool isInitOnly, CORINFO_CLASS_HANDLE result); + 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* isInitOnly); + CORINFO_CLASS_HANDLE repGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isSpeculative); void recGetClassGClayout(CORINFO_CLASS_HANDLE cls, BYTE* gcPtrs, unsigned len, unsigned result); void dmpGetClassGClayout(DWORDLONG key, const Agnostic_GetClassGClayout& value); diff --git a/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp index 4b018ef9b6ca..54ee1c6a141c 100644 --- a/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp @@ -1817,15 +1817,15 @@ void* interceptor_ICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppInd } // return the class handle for the current value of a static field -CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isInitOnly) +CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isSpeculative) { mc->cr->AddCall("getStaticFieldCurrentClass"); - bool localIsInitOnly = false; - CORINFO_CLASS_HANDLE result = original_ICorJitInfo->getStaticFieldCurrentClass(field, &localIsInitOnly); - mc->recGetStaticFieldCurrentClass(field, localIsInitOnly, result); - if (isInitOnly != nullptr) + bool localIsSpeculative = false; + CORINFO_CLASS_HANDLE result = original_ICorJitInfo->getStaticFieldCurrentClass(field, &localIsSpeculative); + mc->recGetStaticFieldCurrentClass(field, localIsSpeculative, result); + if (isSpeculative != nullptr) { - *isInitOnly = localIsInitOnly; + *isSpeculative = localIsSpeculative; } return result; } diff --git a/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp index b33037f84776..2b227faf3b2f 100644 --- a/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp @@ -1404,10 +1404,10 @@ void* interceptor_ICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppInd } // return the class handle for the current value of a static field -CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isInitOnly) +CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isSpeculative) { mcs->AddCall("getStaticFieldCurrentClass"); - return original_ICorJitInfo->getStaticFieldCurrentClass(field, isInitOnly); + return original_ICorJitInfo->getStaticFieldCurrentClass(field, isSpeculative); } // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) diff --git a/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp index cea6af417a91..6514c2623e80 100644 --- a/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp @@ -1259,9 +1259,9 @@ void* interceptor_ICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppInd } // return the class handle for the current value of a static field -CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isInitOnly) +CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isSpeculative) { - return original_ICorJitInfo->getStaticFieldCurrentClass(field, isInitOnly); + return original_ICorJitInfo->getStaticFieldCurrentClass(field, isSpeculative); } // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) diff --git a/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp index 1f9ce352d9b0..2011c6455cec 100644 --- a/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp @@ -1517,10 +1517,10 @@ void* MyICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppIndirection) } // return the class handle for the current value of a static field -CORINFO_CLASS_HANDLE MyICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isInitOnly) +CORINFO_CLASS_HANDLE MyICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isSpeculative) { jitInstance->mc->cr->AddCall("getStaticFieldCurrentClass"); - return jitInstance->mc->repGetStaticFieldCurrentClass(field, isInitOnly); + return jitInstance->mc->repGetStaticFieldCurrentClass(field, isSpeculative); } // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) diff --git a/src/inc/corinfo.h b/src/inc/corinfo.h index 3d4255363ef2..d7dc72b53d69 100644 --- a/src/inc/corinfo.h +++ b/src/inc/corinfo.h @@ -3119,11 +3119,20 @@ class ICorDynamicInfo : public ICorStaticInfo void **ppIndirection = NULL ) = 0; - // For ref-class typed static readonly fields, return the class handle for value of the field - // if there is a unique location for the static and the class is already initialized. + // If isSpeculative 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 isSpeculative 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 *isSpeculative true if this type may change over time (field is not readonly or + // is readonly but class has not yet finished initialization). Set *isSpeculative false + // if this type will not change. virtual CORINFO_CLASS_HANDLE getStaticFieldCurrentClass( CORINFO_FIELD_HANDLE field, - bool *isInitOnly + bool *isSpeculative = NULL ) = 0; // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp index 13b99919d7ee..3c17f90c9ff0 100644 --- a/src/jit/gentree.cpp +++ b/src/jit/gentree.cpp @@ -16576,30 +16576,19 @@ CORINFO_CLASS_HANDLE Compiler::gtGetFieldClassHandle(CORINFO_FIELD_HANDLE fieldH eeGetClassName(fieldClass)); #endif // DEBUG - // Is this an initialized static init-only field? - bool isFieldInitOnly = false; - CORINFO_CLASS_HANDLE currentClass = - info.compCompHnd->getStaticFieldCurrentClass(fieldHnd, &isFieldInitOnly); + // 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) { - // We know the current type -- can we rely on it? - if (isFieldInitOnly) - { - // Yes! We know the class exactly and can rely on this to always be true. - fieldClass = currentClass; - *isExact = true; - *isNonNull = true; - JITDUMP("Runtime reports field is init-only and has type %s\n", eeGetClassName(fieldClass)); - } - else - { - // No. We know the current type but it may change over time. - // - // We could use this type as an informed guess, someday, - // if it is a "better" type than the declared field type. - JITDUMP("Field is not init-only\n"); - } + // Yes! We know the class exactly and can rely on this to always be true. + fieldClass = currentClass; + *isExact = true; + *isNonNull = true; + JITDUMP("Runtime reports field is init-only and initialized and has class %s\n", + eeGetClassName(fieldClass)); } else { diff --git a/src/vm/jitinterface.cpp b/src/vm/jitinterface.cpp index 363e9b2650a7..01ab49a6b0a5 100644 --- a/src/vm/jitinterface.cpp +++ b/src/vm/jitinterface.cpp @@ -11758,7 +11758,7 @@ void* CEEJitInfo::getFieldAddress(CORINFO_FIELD_HANDLE fieldHnd, /*********************************************************************/ CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE fieldHnd, - bool* isInitOnly) + bool* isSpeculative) { CONTRACTL { SO_TOLERANT; @@ -11769,9 +11769,9 @@ CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE CORINFO_CLASS_HANDLE result = NULL; - if (isInitOnly != NULL) + if (isSpeculative != NULL) { - *isInitOnly = false; + *isSpeculative = true; } // Only examine the field's value if we are producing jitted code. @@ -11817,12 +11817,27 @@ CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE } } - // If the field is init only, and the class is known, and class initialization - // has finished, the class can't ever change. - if ((result != NULL) && isClassInitialized && (isInitOnly != NULL)) + // Did we find a class? + if (result != NULL) { + // Figure out what to report back. DWORD fieldAttribs = field->GetAttributes(); - *isInitOnly = !!IsFdInitOnly(fieldAttribs); + bool isInitOnly = !!IsFdInitOnly(fieldAttribs); + bool isResultImmutable = isInitOnly && isClassInitialized; + + if (isSpeculative != NULL) + { + // Caller is ok with potentially mutable results. + *isSpeculative = !isResultImmutable; + } + else + { + // Caller only wants to see immutable results. + if (!isResultImmutable) + { + result = NULL; + } + } } EE_TO_JIT_TRANSITION(); @@ -13941,12 +13956,12 @@ void* CEEInfo::getFieldAddress(CORINFO_FIELD_HANDLE fieldHnd, } CORINFO_CLASS_HANDLE CEEInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE fieldHnd, - bool* isInitOnly) + bool* isSpeculative) { LIMITED_METHOD_CONTRACT; _ASSERTE(isVerifyOnly()); - if (isInitOnly != NULL) - *isInitOnly = false; + if (isSpeculative != NULL) + *isSpeculative = true; return NULL; } diff --git a/src/vm/jitinterface.h b/src/vm/jitinterface.h index fa81545ef3f3..dbce88c95c88 100644 --- a/src/vm/jitinterface.h +++ b/src/vm/jitinterface.h @@ -853,7 +853,7 @@ class CEEInfo : public ICorJitInfo void* getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection); - CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isInitOnly); + CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isSpeculative); // ICorDebugInfo stuff void * allocateArray(ULONG cBytes); @@ -1494,7 +1494,7 @@ class CEEJitInfo : public CEEInfo 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* isInitOnly); + CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isSpeculative); void* getMethodSync(CORINFO_METHOD_HANDLE ftnHnd, void **ppIndirection); void BackoutJitData(EEJitManager * jitMgr); diff --git a/src/zap/zapinfo.cpp b/src/zap/zapinfo.cpp index 3d8bc35fed4a..112587f464cf 100644 --- a/src/zap/zapinfo.cpp +++ b/src/zap/zapinfo.cpp @@ -2339,11 +2339,11 @@ void * ZapInfo::getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection return NULL; } -CORINFO_CLASS_HANDLE ZapInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isInitOnly) +CORINFO_CLASS_HANDLE ZapInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isSpeculative) { - if (isInitOnly != NULL) + if (isSpeculative != NULL) { - *isInitOnly = false; + *isSpeculative = true; } return NULL; diff --git a/src/zap/zapinfo.h b/src/zap/zapinfo.h index 05e1a6787df7..55656048e1d3 100644 --- a/src/zap/zapinfo.h +++ b/src/zap/zapinfo.h @@ -418,7 +418,7 @@ class ZapInfo void * getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection); CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, - bool* isInitOnly); + bool* isSpeculative); DWORD getFieldThreadLocalStoreID (CORINFO_FIELD_HANDLE field, void **ppIndirection); From 577d9248f9fdd3d2814a8e6524d2379343f283b2 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 9 Nov 2018 14:52:23 -0800 Subject: [PATCH 05/10] Review feedback Rename bool out parameters to have `p` prefix for clarity. Defer fetching field attributes if class is not initialized. --- .../superpmi-shared/icorjitinfoimpl.h | 2 +- .../superpmi-shared/methodcontext.cpp | 6 +- .../superpmi/superpmi-shared/methodcontext.h | 2 +- .../superpmi-shim-collector/icorjitinfo.cpp | 6 +- .../superpmi-shim-counter/icorjitinfo.cpp | 4 +- .../superpmi-shim-simple/icorjitinfo.cpp | 4 +- src/ToolBox/superpmi/superpmi/icorjitinfo.cpp | 4 +- src/inc/corinfo.h | 10 +-- src/jit/compiler.h | 6 +- src/jit/gentree.cpp | 86 +++++++++---------- src/vm/jitinterface.cpp | 20 ++--- src/vm/jitinterface.h | 4 +- src/zap/zapinfo.cpp | 6 +- src/zap/zapinfo.h | 2 +- 14 files changed, 80 insertions(+), 82 deletions(-) diff --git a/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h b/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h index 089fa03abefe..295bcb229fe7 100644 --- a/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h +++ b/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h @@ -878,7 +878,7 @@ unsigned getClassDomainID(CORINFO_CLASS_HANDLE cls, void** ppIndirection = NULL) 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 *isSpeculative); +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/methodcontext.cpp b/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp index fcac0894598e..6e839b504edb 100644 --- a/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp @@ -3483,13 +3483,13 @@ void MethodContext::dmpGetStaticFieldCurrentClass(DWORDLONG key, const Agnostic_ { 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* isSpeculative) +CORINFO_CLASS_HANDLE MethodContext::repGetStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative) { Agnostic_GetStaticFieldCurrentClass value = GetStaticFieldCurrentClass->Get((DWORDLONG) field); - if (isSpeculative != nullptr) + if (pIsSpeculative != nullptr) { - *isSpeculative = value.isSpeculative; + *pIsSpeculative = value.isSpeculative; } CORINFO_CLASS_HANDLE result = (CORINFO_CLASS_HANDLE) value.classHandle; diff --git a/src/ToolBox/superpmi/superpmi-shared/methodcontext.h b/src/ToolBox/superpmi/superpmi-shared/methodcontext.h index a9610298e604..a10ab65ae69a 100644 --- a/src/ToolBox/superpmi/superpmi-shared/methodcontext.h +++ b/src/ToolBox/superpmi/superpmi-shared/methodcontext.h @@ -930,7 +930,7 @@ class MethodContext 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* isSpeculative); + 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); diff --git a/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp index 54ee1c6a141c..c628f16d52a1 100644 --- a/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp @@ -1817,15 +1817,15 @@ void* interceptor_ICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppInd } // return the class handle for the current value of a static field -CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isSpeculative) +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 (isSpeculative != nullptr) + if (pIsSpeculative != nullptr) { - *isSpeculative = localIsSpeculative; + *pIsSpeculative = localIsSpeculative; } return result; } diff --git a/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp index 2b227faf3b2f..d6dbecdf0878 100644 --- a/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp @@ -1404,10 +1404,10 @@ void* interceptor_ICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppInd } // return the class handle for the current value of a static field -CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isSpeculative) +CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *pIsSpeculative) { mcs->AddCall("getStaticFieldCurrentClass"); - return original_ICorJitInfo->getStaticFieldCurrentClass(field, isSpeculative); + return original_ICorJitInfo->getStaticFieldCurrentClass(field, pIsSpeculative); } // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) diff --git a/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp index 6514c2623e80..04c0ba095056 100644 --- a/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp @@ -1259,9 +1259,9 @@ void* interceptor_ICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppInd } // return the class handle for the current value of a static field -CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isSpeculative) +CORINFO_CLASS_HANDLE interceptor_ICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *pIsSpeculative) { - return original_ICorJitInfo->getStaticFieldCurrentClass(field, isSpeculative); + return original_ICorJitInfo->getStaticFieldCurrentClass(field, pIsSpeculative); } // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) diff --git a/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp index 2011c6455cec..60cf36c1c8e2 100644 --- a/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp @@ -1517,10 +1517,10 @@ void* MyICJI::getFieldAddress(CORINFO_FIELD_HANDLE field, void** ppIndirection) } // return the class handle for the current value of a static field -CORINFO_CLASS_HANDLE MyICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *isSpeculative) +CORINFO_CLASS_HANDLE MyICJI::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool *pIsSpeculative) { jitInstance->mc->cr->AddCall("getStaticFieldCurrentClass"); - return jitInstance->mc->repGetStaticFieldCurrentClass(field, isSpeculative); + return jitInstance->mc->repGetStaticFieldCurrentClass(field, pIsSpeculative); } // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) diff --git a/src/inc/corinfo.h b/src/inc/corinfo.h index d7dc72b53d69..059dbbe0a704 100644 --- a/src/inc/corinfo.h +++ b/src/inc/corinfo.h @@ -3119,20 +3119,20 @@ class ICorDynamicInfo : public ICorStaticInfo void **ppIndirection = NULL ) = 0; - // If isSpeculative is NULL, return the class handle for the value of ref-class typed + // 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 isSpeculative is not NULL, fetch the class handle for the value of all ref-class + // 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 *isSpeculative true if this type may change over time (field is not readonly or - // is readonly but class has not yet finished initialization). Set *isSpeculative false + // 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 *isSpeculative = NULL + bool *pIsSpeculative = NULL ) = 0; // registers a vararg sig & returns a VM cookie for it (which can contain other stuff) diff --git a/src/jit/compiler.h b/src/jit/compiler.h index f46f0b04309b..ad555b812fd4 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -2603,9 +2603,9 @@ class Compiler // 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 @@ -2613,7 +2613,7 @@ class Compiler unsigned* runtimeLookupCount = nullptr, GenTree** handleTree = nullptr); // Get the class handle for a field - CORINFO_CLASS_HANDLE gtGetFieldClassHandle(CORINFO_FIELD_HANDLE fieldHnd, bool* isExact, bool* IsNonNull); + 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 3c17f90c9ff0..b685bf247943 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,7 +16173,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* isExact, bo if (fieldHnd != nullptr) { - objClass = gtGetFieldClassHandle(fieldHnd, isExact, isNonNull); + objClass = gtGetFieldClassHandle(fieldHnd, pIsExact, pIsNonNull); } break; @@ -16184,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; } @@ -16255,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 { @@ -16267,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; @@ -16281,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; @@ -16292,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; } @@ -16315,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) { @@ -16323,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) { @@ -16377,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; } @@ -16386,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; } @@ -16407,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); @@ -16435,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; } @@ -16479,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 @@ -16546,8 +16546,8 @@ CORINFO_CLASS_HANDLE Compiler::gtGetArrayElementClassHandle(GenTree* array) // // Arguments: // fieldHnd - field handle for field in question -// isExact - [OUT] true if type is known exactly -// isNonNull - [OUT] true if field value is not null +// 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 @@ -16555,7 +16555,7 @@ CORINFO_CLASS_HANDLE Compiler::gtGetArrayElementClassHandle(GenTree* array) // // May examine runtime state of static field instances. -CORINFO_CLASS_HANDLE Compiler::gtGetFieldClassHandle(CORINFO_FIELD_HANDLE fieldHnd, bool* isExact, bool* isNonNull) +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); @@ -16584,9 +16584,9 @@ CORINFO_CLASS_HANDLE Compiler::gtGetFieldClassHandle(CORINFO_FIELD_HANDLE fieldH if (currentClass != NO_CLASS_HANDLE) { // Yes! We know the class exactly and can rely on this to always be true. - fieldClass = currentClass; - *isExact = true; - *isNonNull = true; + fieldClass = currentClass; + *pIsExact = true; + *pIsNonNull = true; JITDUMP("Runtime reports field is init-only and initialized and has class %s\n", eeGetClassName(fieldClass)); } diff --git a/src/vm/jitinterface.cpp b/src/vm/jitinterface.cpp index 01ab49a6b0a5..efaa340151c3 100644 --- a/src/vm/jitinterface.cpp +++ b/src/vm/jitinterface.cpp @@ -11758,7 +11758,7 @@ void* CEEJitInfo::getFieldAddress(CORINFO_FIELD_HANDLE fieldHnd, /*********************************************************************/ CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE fieldHnd, - bool* isSpeculative) + bool* pIsSpeculative) { CONTRACTL { SO_TOLERANT; @@ -11769,9 +11769,9 @@ CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE CORINFO_CLASS_HANDLE result = NULL; - if (isSpeculative != NULL) + if (pIsSpeculative != NULL) { - *isSpeculative = true; + *pIsSpeculative = true; } // Only examine the field's value if we are producing jitted code. @@ -11821,14 +11821,12 @@ CORINFO_CLASS_HANDLE CEEJitInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE if (result != NULL) { // Figure out what to report back. - DWORD fieldAttribs = field->GetAttributes(); - bool isInitOnly = !!IsFdInitOnly(fieldAttribs); - bool isResultImmutable = isInitOnly && isClassInitialized; + bool isResultImmutable = isClassInitialized && IsFdInitOnly(field->GetAttributes()); - if (isSpeculative != NULL) + if (pIsSpeculative != NULL) { // Caller is ok with potentially mutable results. - *isSpeculative = !isResultImmutable; + *pIsSpeculative = !isResultImmutable; } else { @@ -13956,12 +13954,12 @@ void* CEEInfo::getFieldAddress(CORINFO_FIELD_HANDLE fieldHnd, } CORINFO_CLASS_HANDLE CEEInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE fieldHnd, - bool* isSpeculative) + bool* pIsSpeculative) { LIMITED_METHOD_CONTRACT; _ASSERTE(isVerifyOnly()); - if (isSpeculative != NULL) - *isSpeculative = true; + if (pIsSpeculative != NULL) + *pIsSpeculative = true; return NULL; } diff --git a/src/vm/jitinterface.h b/src/vm/jitinterface.h index dbce88c95c88..4ec732fdb060 100644 --- a/src/vm/jitinterface.h +++ b/src/vm/jitinterface.h @@ -853,7 +853,7 @@ class CEEInfo : public ICorJitInfo void* getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection); - CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isSpeculative); + CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative); // ICorDebugInfo stuff void * allocateArray(ULONG cBytes); @@ -1494,7 +1494,7 @@ class CEEJitInfo : public CEEInfo 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* isSpeculative); + 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/zap/zapinfo.cpp b/src/zap/zapinfo.cpp index 112587f464cf..8efeedded602 100644 --- a/src/zap/zapinfo.cpp +++ b/src/zap/zapinfo.cpp @@ -2339,11 +2339,11 @@ void * ZapInfo::getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection return NULL; } -CORINFO_CLASS_HANDLE ZapInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* isSpeculative) +CORINFO_CLASS_HANDLE ZapInfo::getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, bool* pIsSpeculative) { - if (isSpeculative != NULL) + if (pIsSpeculative != NULL) { - *isSpeculative = true; + *pIsSpeculative = true; } return NULL; diff --git a/src/zap/zapinfo.h b/src/zap/zapinfo.h index 55656048e1d3..ded4d53923bf 100644 --- a/src/zap/zapinfo.h +++ b/src/zap/zapinfo.h @@ -418,7 +418,7 @@ class ZapInfo void * getFieldAddress(CORINFO_FIELD_HANDLE field, void **ppIndirection); CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(CORINFO_FIELD_HANDLE field, - bool* isSpeculative); + bool* pIsSpeculative); DWORD getFieldThreadLocalStoreID (CORINFO_FIELD_HANDLE field, void **ppIndirection); From 64deef475a414adae26ccf954d11dd41d96e9df5 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 9 Nov 2018 16:19:10 -0800 Subject: [PATCH 06/10] add test case --- .../opt/Devirtualization/readonlystatic.cs | 51 +++++++++++++++++++ .../Devirtualization/readonlystatic.csproj | 34 +++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 tests/src/JIT/opt/Devirtualization/readonlystatic.cs create mode 100644 tests/src/JIT/opt/Devirtualization/readonlystatic.csproj diff --git a/tests/src/JIT/opt/Devirtualization/readonlystatic.cs b/tests/src/JIT/opt/Devirtualization/readonlystatic.cs new file mode 100644 index 000000000000..5cafa23acda2 --- /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 000000000000..592b35c501ef --- /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 From 2df3553959251942fd7421e1b5b75af5baa6e9c6 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 10 Nov 2018 07:33:57 -0800 Subject: [PATCH 07/10] block reflection from setting initonly static after class init --- src/vm/reflectioninvocation.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/vm/reflectioninvocation.cpp b/src/vm/reflectioninvocation.cpp index 6f5011ffc334..9e9d66cd95b5 100644 --- a/src/vm/reflectioninvocation.cpp +++ b/src/vm/reflectioninvocation.cpp @@ -337,6 +337,15 @@ FCIMPL7(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Ob FieldDesc* pFieldDesc = gc.refField->GetField(); + // 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())) + FCThrowVoid(kFieldAccessException); + } + HELPER_METHOD_FRAME_BEGIN_PROTECT(gc); //TODO: cleanup this function @@ -1714,6 +1723,11 @@ FCIMPL5(void, RuntimeFieldHandle::SetValueDirect, ReflectFieldObject *pFieldUNSA 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(attr)) + COMPlusThrow(kFieldAccessException,W("Acc_ReadOnly")); + // Verify the callee/caller access if (!pField->IsPublic() || (pEnclosingMT != NULL && !pEnclosingMT->IsExternallyVisible())) { From c32f9c870d59739ee9748aa7c2bccee8337a7bcf Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 10 Nov 2018 11:24:08 -0800 Subject: [PATCH 08/10] improve error message --- src/dlls/mscorrc/mscorrc.rc | 2 ++ src/dlls/mscorrc/resource.h | 2 ++ src/vm/reflectioninvocation.cpp | 29 +++++++++++++++++++---------- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/dlls/mscorrc/mscorrc.rc b/src/dlls/mscorrc/mscorrc.rc index 0e35c2906176..bb39c9ac6f79 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 512b22e7dedc..8c315e1abe63 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/vm/reflectioninvocation.cpp b/src/vm/reflectioninvocation.cpp index 9e9d66cd95b5..ae965a4a146f 100644 --- a/src/vm/reflectioninvocation.cpp +++ b/src/vm/reflectioninvocation.cpp @@ -337,17 +337,24 @@ FCIMPL7(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Ob FieldDesc* pFieldDesc = gc.refField->GetField(); + 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())) - FCThrowVoid(kFieldAccessException); + { + DefineFullyQualifiedNameForClassW(); + StackSString ssFieldName(SString::Utf8, pFieldDesc->GetName()); + COMPlusThrow(kFieldAccessException, + IDS_EE_CANNOT_SET_INITONLY_STATIC_FIELD, + ssFieldName.GetUnicode(), + GetFullyQualifiedNameForClassW(pEnclosingMT)); + } } - HELPER_METHOD_FRAME_BEGIN_PROTECT(gc); - //TODO: cleanup this function InvokeUtil::SetValidField(fieldType.GetSignatureCorElementType(), fieldType, pFieldDesc, &gc.target, &gc.value, declaringType, pDomainInitialized); @@ -1718,15 +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(attr)) - COMPlusThrow(kFieldAccessException,W("Acc_ReadOnly")); + if (pField->IsStatic() && pEnclosingMT->IsClassInited() && IsFdInitOnly(pField->GetAttributes())) + { + DefineFullyQualifiedNameForClassW(); + StackSString 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())) From d3b07c598c2f48bc3227839e811f23ba199dcb28 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sun, 11 Nov 2018 00:10:47 -0800 Subject: [PATCH 09/10] add test case, exclude corefx test --- tests/CoreFX/CoreFX.issues.json | 16 ++- .../SetValue/TrySetReadonlyStaticField.cs | 124 ++++++++++++++++++ .../SetValue/TrySetReadonlyStaticField.csproj | 21 +++ 3 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 tests/src/reflection/SetValue/TrySetReadonlyStaticField.cs create mode 100644 tests/src/reflection/SetValue/TrySetReadonlyStaticField.csproj diff --git a/tests/CoreFX/CoreFX.issues.json b/tests/CoreFX/CoreFX.issues.json index b784fbd63509..58923077152c 100644 --- a/tests/CoreFX/CoreFX.issues.json +++ b/tests/CoreFX/CoreFX.issues.json @@ -768,5 +768,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/reflection/SetValue/TrySetReadonlyStaticField.cs b/tests/src/reflection/SetValue/TrySetReadonlyStaticField.cs new file mode 100644 index 000000000000..7a5d726f37cb --- /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 000000000000..d7d18999cf9d --- /dev/null +++ b/tests/src/reflection/SetValue/TrySetReadonlyStaticField.csproj @@ -0,0 +1,21 @@ + + + + + Debug + AnyCPU + {58DB4A46-51BE-46A1-AEA1-0C32FBAF5562} + Exe + latest + true + + + + + + + + + + + From bf3c1a5467b84d437ff4f6d9b618afd614ae0f68 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 12 Nov 2018 07:46:18 -0800 Subject: [PATCH 10/10] use sstring --- src/vm/reflectioninvocation.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vm/reflectioninvocation.cpp b/src/vm/reflectioninvocation.cpp index ae965a4a146f..a8d2aa792daa 100644 --- a/src/vm/reflectioninvocation.cpp +++ b/src/vm/reflectioninvocation.cpp @@ -347,7 +347,7 @@ FCIMPL7(void, RuntimeFieldHandle::SetValue, ReflectFieldObject *pFieldUNSAFE, Ob if (pEnclosingMT->IsClassInited() && IsFdInitOnly(pFieldDesc->GetAttributes())) { DefineFullyQualifiedNameForClassW(); - StackSString ssFieldName(SString::Utf8, pFieldDesc->GetName()); + SString ssFieldName(SString::Utf8, pFieldDesc->GetName()); COMPlusThrow(kFieldAccessException, IDS_EE_CANNOT_SET_INITONLY_STATIC_FIELD, ssFieldName.GetUnicode(), @@ -1730,7 +1730,7 @@ FCIMPL5(void, RuntimeFieldHandle::SetValueDirect, ReflectFieldObject *pFieldUNSA if (pField->IsStatic() && pEnclosingMT->IsClassInited() && IsFdInitOnly(pField->GetAttributes())) { DefineFullyQualifiedNameForClassW(); - StackSString ssFieldName(SString::Utf8, pField->GetName()); + SString ssFieldName(SString::Utf8, pField->GetName()); COMPlusThrow(kFieldAccessException, IDS_EE_CANNOT_SET_INITONLY_STATIC_FIELD, ssFieldName.GetUnicode(),