From 79a0be7032a2e751979aa4704fff4afd3182f3d3 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 8 Nov 2022 14:00:04 +0100 Subject: [PATCH 1/2] Fix recursive assembly spec parsing with textual PGO * Add thread local to skip textual PGO data if invoked recursively * Change AssemblyNameParser.ParseVersion to pass NumberFormatInfo.InvariantInfo in its ushort.TryParse invocation. This avoids initializing globalization from within this parsing, which was causing problems in the textual PGO scenario. Fix #77971 --- src/coreclr/vm/pgo.cpp | 229 ++++++++++-------- src/coreclr/vm/pgo.h | 1 + .../System/Reflection/AssemblyNameParser.cs | 4 +- 3 files changed, 134 insertions(+), 100 deletions(-) diff --git a/src/coreclr/vm/pgo.cpp b/src/coreclr/vm/pgo.cpp index 5a2e2225bda75..1dd4b8197e017 100644 --- a/src/coreclr/vm/pgo.cpp +++ b/src/coreclr/vm/pgo.cpp @@ -820,129 +820,160 @@ HRESULT PgoManager::getPgoInstrumentationResults(MethodDesc* pMD, BYTE** pAlloca // if (s_textFormatPgoData.GetCount() > 0) { - COUNT_T methodhash = pMD->GetStableHash(); - int codehash; - unsigned ilSize; - if (GetVersionResilientILCodeHashCode(pMD, &codehash, &ilSize)) + hr = getPgoInstrumentationResultsFromText(pMD, pAllocatedData, ppSchema, pCountSchemaItems, pInstrumentationData, pPgoSource); + } + + // If we didn't find any text format data, look for dynamic or static data. + // + if (FAILED(hr)) + { + PgoManager *mgr; + if (!pMD->IsDynamicMethod()) { - Header *found = s_textFormatPgoData.Lookup(CodeAndMethodHash(codehash, methodhash)); - if (found != NULL) - { - StackSArray schemaArray; + mgr = pMD->GetLoaderAllocator()->GetPgoManager(); + } + else + { + mgr = pMD->AsDynamicMethodDesc()->GetResolver()->GetDynamicPgoManager(); + } + + if (mgr != NULL) + { + hr = mgr->getPgoInstrumentationResultsInstance(pMD, pAllocatedData, ppSchema, pCountSchemaItems, pInstrumentationData, pPgoSource); + } + } + + return hr; +} + +HRESULT PgoManager::getPgoInstrumentationResultsFromText(MethodDesc* pMD, BYTE** pAllocatedData, ICorJitInfo::PgoInstrumentationSchema** ppSchema, UINT32* pCountSchemaItems, BYTE** pInstrumentationData, ICorJitInfo::PgoSource* pPgoSource) +{ + int codehash; + unsigned ilSize; + if (!GetVersionResilientILCodeHashCode(pMD, &codehash, &ilSize)) + { + return E_NOTIMPL; + } - if (ReadInstrumentationSchemaWithLayoutIntoSArray(found->GetData(), found->countsOffset, found->countsOffset, &schemaArray)) + COUNT_T methodhash = pMD->GetStableHash(); + Header* found = s_textFormatPgoData.Lookup(CodeAndMethodHash(codehash, methodhash)); + if (found == NULL) + { + return E_NOTIMPL; + } + + StackSArray schemaArray; + + if (!ReadInstrumentationSchemaWithLayoutIntoSArray(found->GetData(), found->countsOffset, found->countsOffset, &schemaArray)) + { + _ASSERTE(!"Unable to parse schema data"); + return E_NOTIMPL; + } + + HRESULT hr = E_NOTIMPL; + EX_TRY + { + for (unsigned iSchema = 0; iSchema < schemaArray.GetCount(); iSchema++) + { + ICorJitInfo::PgoInstrumentationSchema* schema = &(schemaArray)[iSchema]; + ICorJitInfo::PgoInstrumentationKind kind = schema->InstrumentationKind & ICorJitInfo::PgoInstrumentationKind::MarshalMask; + if ((kind == ICorJitInfo::PgoInstrumentationKind::TypeHandle) || (kind == ICorJitInfo::PgoInstrumentationKind::MethodHandle)) + { + for (int iEntry = 0; iEntry < schema->Count; iEntry++) { - EX_TRY + INT_PTR* handleValueAddress = (INT_PTR*)(found->GetData() + schema->Offset + iEntry * InstrumentationKindToSize(schema->InstrumentationKind)); + INT_PTR initialHandleValue = VolatileLoad(handleValueAddress); + + // TypeHandles can't reliably be loaded at ReadPGO time + // Instead, translate them before leaving this method. + // The ReadPgo method will place pointers to C style null + // terminated strings in the TypeHandle slots, and this will + // translate any of those into loaded TypeHandles as appropriate + if (((initialHandleValue & 1) == 1) && !ICorJitInfo::IsUnknownHandle(initialHandleValue)) { - // TypeHandles can't reliably be loaded at ReadPGO time - // Instead, translate them before leaving this method. - // The ReadPgo method will place pointers to C style null - // terminated strings in the TypeHandle slots, and this will - // translate any of those into loaded TypeHandles as appropriate + INT_PTR newPtr = 0; + char* string = ((char*)initialHandleValue) - 1; + + // Resolving types and methods here can invoke managed code where the + // JIT may recursively ask for PGO data. We do not support textual PGO + // for those cases. + static thread_local bool t_resolvingTypeOrMethod; - for (unsigned iSchema = 0; iSchema < schemaArray.GetCount(); iSchema++) + struct ResolveScope { - ICorJitInfo::PgoInstrumentationSchema *schema = &(schemaArray)[iSchema]; - ICorJitInfo::PgoInstrumentationKind kind = schema->InstrumentationKind & ICorJitInfo::PgoInstrumentationKind::MarshalMask; - if ((kind == ICorJitInfo::PgoInstrumentationKind::TypeHandle) || (kind == ICorJitInfo::PgoInstrumentationKind::MethodHandle)) + ResolveScope() { - for (int iEntry = 0; iEntry < schema->Count; iEntry++) - { - INT_PTR* handleValueAddress = (INT_PTR*)(found->GetData() + schema->Offset + iEntry * InstrumentationKindToSize(schema->InstrumentationKind)); - INT_PTR initialHandleValue = VolatileLoad(handleValueAddress); - if (((initialHandleValue & 1) == 1) && !ICorJitInfo::IsUnknownHandle(initialHandleValue)) - { - INT_PTR newPtr = 0; - char* string = ((char *)initialHandleValue) - 1; + t_resolvingTypeOrMethod = true; + } - // Don't attempt to load any types or methods until the EE is started - if (g_fEEStarted) - { - if (kind == ICorJitInfo::PgoInstrumentationKind::TypeHandle) - { - StackSString ts(SString::Utf8, string); - TypeHandle th = TypeName::GetTypeManaged(ts.GetUnicode(), NULL, FALSE, FALSE, FALSE, NULL, NULL); - newPtr = (INT_PTR)th.AsPtr(); - } - else - { - assert(kind == ICorJitInfo::PgoInstrumentationKind::MethodHandle); - // Format is: - // MethodName|@|fully_qualified_type_name - char* sep = strstr(string, "|@|"); - if (sep != nullptr) - { - StackSString typeString(SString::Utf8, sep + 3); - StackSString methodString(SString::Utf8, string, (COUNT_T)(sep - string)); - TypeHandle th = TypeName::GetTypeManaged(typeString.GetUnicode(), NULL, FALSE, FALSE, FALSE, NULL, NULL); - if (!th.IsNull()) - { - MethodDesc* pMD = MemberLoader::FindMethodByName(th.GetMethodTable(), methodString.GetUTF8()); - if (pMD != nullptr && !pMD->IsGenericMethodDefinition()) - { - newPtr = (INT_PTR)pMD; - } - } - } - } - } + ~ResolveScope() + { + t_resolvingTypeOrMethod = false; + } + }; - if (newPtr == 0) + // Don't attempt to load any types or methods until the EE is started + if (g_fEEStarted && !t_resolvingTypeOrMethod) + { + ResolveScope resolve; + + if (kind == ICorJitInfo::PgoInstrumentationKind::TypeHandle) + { + StackSString ts(SString::Utf8, string); + TypeHandle th = TypeName::GetTypeManaged(ts.GetUnicode(), NULL, FALSE, FALSE, FALSE, NULL, NULL); + newPtr = (INT_PTR)th.AsPtr(); + } + else + { + assert(kind == ICorJitInfo::PgoInstrumentationKind::MethodHandle); + // Format is: + // MethodName|@|fully_qualified_type_name + char* sep = strstr(string, "|@|"); + if (sep != nullptr) + { + StackSString typeString(SString::Utf8, sep + 3); + StackSString methodString(SString::Utf8, string, (COUNT_T)(sep - string)); + TypeHandle th = TypeName::GetTypeManaged(typeString.GetUnicode(), NULL, FALSE, FALSE, FALSE, NULL, NULL); + + if (!th.IsNull()) + { + MethodDesc* pMD = MemberLoader::FindMethodByName(th.GetMethodTable(), methodString.GetUTF8()); + if (pMD != nullptr && !pMD->IsGenericMethodDefinition()) { - newPtr = HashToPgoUnknownHandle(HashStringA(string)); + newPtr = (INT_PTR)pMD; } - - InterlockedCompareExchangeT(handleValueAddress, newPtr, initialHandleValue); } } } } - *pAllocatedData = new BYTE[schemaArray.GetCount() * sizeof(ICorJitInfo::PgoInstrumentationSchema)]; - memcpy(*pAllocatedData, schemaArray.OpenRawBuffer(), schemaArray.GetCount() * sizeof(ICorJitInfo::PgoInstrumentationSchema)); - schemaArray.CloseRawBuffer(); - *ppSchema = (ICorJitInfo::PgoInstrumentationSchema*)*pAllocatedData; - - *pCountSchemaItems = schemaArray.GetCount(); - *pInstrumentationData = found->GetData(); - *pPgoSource = ICorJitInfo::PgoSource::Text; + if (newPtr == 0) + { + newPtr = HashToPgoUnknownHandle(HashStringA(string)); + } - hr = S_OK; - } - EX_CATCH - { - hr = E_FAIL; + InterlockedCompareExchangeT(handleValueAddress, newPtr, initialHandleValue); } - EX_END_CATCH(RethrowTerminalExceptions) - } - else - { - _ASSERTE(!"Unable to parse schema data"); - hr = E_NOTIMPL; } } } - } - // If we didn't find any text format data, look for dynamic or static data. - // - if (FAILED(hr)) - { - PgoManager *mgr; - if (!pMD->IsDynamicMethod()) - { - mgr = pMD->GetLoaderAllocator()->GetPgoManager(); - } - else - { - mgr = pMD->AsDynamicMethodDesc()->GetResolver()->GetDynamicPgoManager(); - } + *pAllocatedData = new BYTE[schemaArray.GetCount() * sizeof(ICorJitInfo::PgoInstrumentationSchema)]; + memcpy(*pAllocatedData, schemaArray.OpenRawBuffer(), schemaArray.GetCount() * sizeof(ICorJitInfo::PgoInstrumentationSchema)); + schemaArray.CloseRawBuffer(); + *ppSchema = (ICorJitInfo::PgoInstrumentationSchema*)*pAllocatedData; - if (mgr != NULL) - { - hr = mgr->getPgoInstrumentationResultsInstance(pMD, pAllocatedData, ppSchema, pCountSchemaItems, pInstrumentationData, pPgoSource); - } + *pCountSchemaItems = schemaArray.GetCount(); + *pInstrumentationData = found->GetData(); + *pPgoSource = ICorJitInfo::PgoSource::Text; + + hr = S_OK; + } + EX_CATCH + { + hr = E_FAIL; } + EX_END_CATCH(RethrowTerminalExceptions) return hr; } diff --git a/src/coreclr/vm/pgo.h b/src/coreclr/vm/pgo.h index 38e6f4b89f33f..62db79d0636e9 100644 --- a/src/coreclr/vm/pgo.h +++ b/src/coreclr/vm/pgo.h @@ -157,6 +157,7 @@ class PgoManager private: static HRESULT ComputeOffsetOfActualInstrumentationData(const ICorJitInfo::PgoInstrumentationSchema* pSchema, UINT32 countSchemaItems, size_t headerInitialSize, UINT *offsetOfActualInstrumentationData); + static HRESULT PgoManager::getPgoInstrumentationResultsFromText(MethodDesc* pMD, BYTE** pAllocatedData, ICorJitInfo::PgoInstrumentationSchema** ppSchema, UINT32 *pCountSchemaItems, BYTE**pInstrumentationData, ICorJitInfo::PgoSource *pPgoSource); static void ReadPgoData(); static void WritePgoData(); diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/AssemblyNameParser.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/AssemblyNameParser.cs index 8ab6b644581f7..13c5a4ed4475d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/AssemblyNameParser.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/AssemblyNameParser.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; +using System.Globalization; using System.IO; using System.Runtime.CompilerServices; using System.Text; @@ -205,7 +206,8 @@ private Version ParseVersion(string attributeValue) if (!char.IsDigit(parts[i][j])) ThrowInvalidAssemblyName(); } - if (!(ushort.TryParse(parts[i], out versionNumbers[i]))) + + if (!ushort.TryParse(parts[i], NumberStyles.Integer, NumberFormatInfo.InvariantInfo, out versionNumbers[i])) { ThrowInvalidAssemblyName(); } From 0dc1d0c123e97cdc441f28e1454af5ca2eca787a Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 8 Nov 2022 18:46:13 +0100 Subject: [PATCH 2/2] Fix build --- src/coreclr/vm/pgo.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/pgo.h b/src/coreclr/vm/pgo.h index 62db79d0636e9..c9d5f8975fe3c 100644 --- a/src/coreclr/vm/pgo.h +++ b/src/coreclr/vm/pgo.h @@ -157,7 +157,7 @@ class PgoManager private: static HRESULT ComputeOffsetOfActualInstrumentationData(const ICorJitInfo::PgoInstrumentationSchema* pSchema, UINT32 countSchemaItems, size_t headerInitialSize, UINT *offsetOfActualInstrumentationData); - static HRESULT PgoManager::getPgoInstrumentationResultsFromText(MethodDesc* pMD, BYTE** pAllocatedData, ICorJitInfo::PgoInstrumentationSchema** ppSchema, UINT32 *pCountSchemaItems, BYTE**pInstrumentationData, ICorJitInfo::PgoSource *pPgoSource); + static HRESULT getPgoInstrumentationResultsFromText(MethodDesc* pMD, BYTE** pAllocatedData, ICorJitInfo::PgoInstrumentationSchema** ppSchema, UINT32 *pCountSchemaItems, BYTE**pInstrumentationData, ICorJitInfo::PgoSource *pPgoSource); static void ReadPgoData(); static void WritePgoData();