From 750bbf2527b76d68d0940d6dcbc6a0d639ba71b9 Mon Sep 17 00:00:00 2001 From: Lei Shi Date: Wed, 31 Jan 2018 03:06:55 -0800 Subject: [PATCH] deps: update ChakraCore to Microsoft/ChakraCore@cc2aede32e [1.8>1.9] [MERGE #4619 @leirocks] fix issue unregistering function table Merge pull request #4619 from leirocks:xdata1 1. in between removing entry from SList and doing the unregistering, another thread might got the code address and doing registering. the fix is to make sure unregistering function table always happen before freeing the code address. 2. remove the code for x86 Reviewed-By: chakrabot --- .../core/lib/Backend/EmitBuffer.cpp | 7 ++++ .../lib/Backend/InterpreterThunkEmitter.cpp | 8 ----- .../core/lib/Backend/NativeCodeGenerator.cpp | 10 +++--- .../core/lib/Common/Common/Jobs.cpp | 14 ++++++-- deps/chakrashim/core/lib/Common/Common/Jobs.h | 8 +++++ .../core/lib/Common/Core/ConfigParser.cpp | 3 +- .../core/lib/Common/Core/DelayLoadLibrary.cpp | 7 ++-- .../Memory/DelayDeletingFunctionTable.cpp | 19 ++--------- .../core/lib/Common/Memory/XDataAllocator.h | 7 ++-- .../Common/Memory/amd64/XDataAllocator.cpp | 8 ++++- .../core/lib/Runtime/Base/FunctionBody.cpp | 32 +++++++++++-------- .../core/lib/Runtime/Base/ScriptContext.cpp | 4 ++- 12 files changed, 72 insertions(+), 55 deletions(-) diff --git a/deps/chakrashim/core/lib/Backend/EmitBuffer.cpp b/deps/chakrashim/core/lib/Backend/EmitBuffer.cpp index c8116a4fc58..94081478c07 100644 --- a/deps/chakrashim/core/lib/Backend/EmitBuffer.cpp +++ b/deps/chakrashim/core/lib/Backend/EmitBuffer.cpp @@ -55,7 +55,10 @@ template void EmitBufferManager::FreeAllocations(bool release) { +#if PDATA_ENABLED && defined(_WIN32) DelayDeletingFunctionTable::Clear(); +#endif + AutoRealOrFakeCriticalSection autoCs(&this->criticalSection); #if DBG_DUMP @@ -194,6 +197,10 @@ template bool EmitBufferManager::FreeAllocation(void* address) { +#if PDATA_ENABLED && defined(_WIN32) + DelayDeletingFunctionTable::Clear(); +#endif + AutoRealOrFakeCriticalSection autoCs(&this->criticalSection); #if _M_ARM diff --git a/deps/chakrashim/core/lib/Backend/InterpreterThunkEmitter.cpp b/deps/chakrashim/core/lib/Backend/InterpreterThunkEmitter.cpp index 66debff69d1..f0b2a0bc8e3 100644 --- a/deps/chakrashim/core/lib/Backend/InterpreterThunkEmitter.cpp +++ b/deps/chakrashim/core/lib/Backend/InterpreterThunkEmitter.cpp @@ -323,10 +323,6 @@ void* InterpreterThunkEmitter::ConvertToEntryPoint(PVOID dynamicInterpreterThunk bool InterpreterThunkEmitter::NewThunkBlock() { - // flush the function tables before allocating any new code - // to prevent old function table is referring to new code address - DelayDeletingFunctionTable::Clear(); - #ifdef ENABLE_OOP_NATIVE_CODEGEN if (CONFIG_FLAG(ForceStaticInterpreterThunk)) { @@ -401,10 +397,6 @@ bool InterpreterThunkEmitter::NewThunkBlock() #ifdef ENABLE_OOP_NATIVE_CODEGEN bool InterpreterThunkEmitter::NewOOPJITThunkBlock() { - // flush the function tables before allocating any new code - // to prevent old function table is referring to new code address - DelayDeletingFunctionTable::Clear(); - PSCRIPTCONTEXT_HANDLE remoteScriptContext = this->scriptContext->GetRemoteScriptAddr(); if (!JITManager::GetJITManager()->IsConnected()) { diff --git a/deps/chakrashim/core/lib/Backend/NativeCodeGenerator.cpp b/deps/chakrashim/core/lib/Backend/NativeCodeGenerator.cpp index 9498b10e170..36e493d0435 100644 --- a/deps/chakrashim/core/lib/Backend/NativeCodeGenerator.cpp +++ b/deps/chakrashim/core/lib/Backend/NativeCodeGenerator.cpp @@ -89,7 +89,9 @@ NativeCodeGenerator::~NativeCodeGenerator() { Assert(this->IsClosed()); +#if PDATA_ENABLED && defined(_WIN32) DelayDeletingFunctionTable::Clear(); +#endif #ifdef PROFILE_EXEC if (this->foregroundCodeGenProfiler != nullptr) @@ -902,10 +904,6 @@ void NativeCodeGenerator::CodeGen(PageAllocator* pageAllocator, CodeGenWorkItemI void NativeCodeGenerator::CodeGen(PageAllocator * pageAllocator, CodeGenWorkItem* workItem, const bool foreground) { - // flush the function tables before allocating any new code - // to prevent old function table is referring to new code address - DelayDeletingFunctionTable::Clear(); - if(foreground) { // Func::Codegen has a lot of things on the stack, so probe the stack here instead @@ -1117,7 +1115,6 @@ NativeCodeGenerator::CodeGen(PageAllocator * pageAllocator, CodeGenWorkItem* wor } #if defined(TARGET_64) - DelayDeletingFunctionTable::Clear(); XDataAllocation * xdataInfo = HeapNewZ(XDataAllocation); xdataInfo->address = (byte*)jitWriteData.xdataAddr; XDataAllocator::Register(xdataInfo, jitWriteData.codeAddress, jitWriteData.codeSize); @@ -3257,6 +3254,9 @@ NativeCodeGenerator::FreeNativeCodeGenAllocation(void* codeAddress) { if (JITManager::GetJITManager()->IsOOPJITEnabled()) { +#if PDATA_ENABLED && defined(_WIN32) + DelayDeletingFunctionTable::Clear(); +#endif ThreadContext * context = this->scriptContext->GetThreadContext(); HRESULT hr = JITManager::GetJITManager()->FreeAllocation(context->GetRemoteThreadContextAddr(), (intptr_t)codeAddress); JITManager::HandleServerCallResult(hr, RemoteCallType::MemFree); diff --git a/deps/chakrashim/core/lib/Common/Common/Jobs.cpp b/deps/chakrashim/core/lib/Common/Common/Jobs.cpp index 0ce5d944396..4f8af9c71af 100644 --- a/deps/chakrashim/core/lib/Common/Common/Jobs.cpp +++ b/deps/chakrashim/core/lib/Common/Common/Jobs.cpp @@ -615,8 +615,10 @@ namespace JsUtil threadId(GetCurrentThreadContextId()), threadService(threadService), threadCount(0), - maxThreadCount(0), - hasExtraWork(false) + maxThreadCount(0) +#if PDATA_ENABLED && defined(_WIN32) + ,hasExtraWork(false) +#endif { if (!threadService->HasCallback()) { @@ -678,7 +680,9 @@ namespace JsUtil //Wait for 1 sec on jobReady and shutdownBackgroundThread events. unsigned int result = WaitForMultipleObjectsEx(_countof(handles), handles, false, 1000, false); +#if PDATA_ENABLED && defined(_WIN32) DoExtraWork(); +#endif while (result == WAIT_TIMEOUT) { @@ -706,6 +710,7 @@ namespace JsUtil return result == WAIT_OBJECT_0; } +#if PDATA_ENABLED && defined(_WIN32) void BackgroundJobProcessor::DoExtraWork() { while (hasExtraWork) @@ -714,6 +719,7 @@ namespace JsUtil Sleep(50); } } +#endif bool BackgroundJobProcessor::WaitWithThreadForThreadStartedOrClosingEvent(ParallelThreadData *parallelThreadData, const unsigned int milliseconds) { @@ -1117,8 +1123,10 @@ namespace JsUtil } criticalSection.Leave(); +#if PDATA_ENABLED && defined(_WIN32) // flush the function tables in background thread after closed and before shutting down thread DelayDeletingFunctionTable::Clear(); +#endif EDGE_ETW_INTERNAL(EventWriteJSCRIPT_NATIVECODEGEN_STOP(this, 0)); } @@ -1420,6 +1428,7 @@ namespace JsUtil #endif } +#if PDATA_ENABLED && defined(_WIN32) void BackgroundJobProcessor::StartExtraWork() { hasExtraWork = true; @@ -1431,6 +1440,7 @@ namespace JsUtil { hasExtraWork = false; } +#endif #if DBG_DUMP //Just for debugging purpose diff --git a/deps/chakrashim/core/lib/Common/Common/Jobs.h b/deps/chakrashim/core/lib/Common/Common/Jobs.h index 148c1417061..8589f71a6ab 100644 --- a/deps/chakrashim/core/lib/Common/Common/Jobs.h +++ b/deps/chakrashim/core/lib/Common/Common/Jobs.h @@ -361,8 +361,10 @@ namespace JsUtil // Closes the job processor and closes the handle of background threads. virtual void Close(); +#if PDATA_ENABLED && defined(_WIN32) virtual void StartExtraWork() { }; virtual void EndExtraWork() { }; +#endif }; // ------------------------------------------------------------------------------------------------------------------------- @@ -460,7 +462,9 @@ namespace JsUtil unsigned int maxThreadCount; ParallelThreadData **parallelThreadData; +#if PDATA_ENABLED && defined(_WIN32) bool hasExtraWork; +#endif #if DBG_DUMP static char16 const * const DebugThreadNames[16]; @@ -470,8 +474,10 @@ namespace JsUtil BackgroundJobProcessor(AllocationPolicyManager* policyManager, ThreadService *threadService, bool disableParallelThreads); ~BackgroundJobProcessor(); +#if PDATA_ENABLED && defined(_WIN32) virtual void StartExtraWork() override; virtual void EndExtraWork() override; +#endif private: bool WaitWithThread(ParallelThreadData *parallelThreadData, const Event &e, const unsigned int milliseconds = INFINITE); @@ -489,7 +495,9 @@ namespace JsUtil void InitializeParallelThreadData(AllocationPolicyManager* policyManager, bool disableParallelThreads); void InitializeParallelThreadDataForThreadServiceCallBack(AllocationPolicyManager* policyManager); +#if PDATA_ENABLED && defined(_WIN32) void DoExtraWork(); +#endif public: virtual void AddManager(JobManager *const manager) override; diff --git a/deps/chakrashim/core/lib/Common/Core/ConfigParser.cpp b/deps/chakrashim/core/lib/Common/Core/ConfigParser.cpp index 9738730526f..0a16ee7cd96 100644 --- a/deps/chakrashim/core/lib/Common/Core/ConfigParser.cpp +++ b/deps/chakrashim/core/lib/Common/Core/ConfigParser.cpp @@ -600,8 +600,7 @@ HRESULT ConfigParser::SetOutputFile(const WCHAR* outputFile, const WCHAR* openMo _wcsicmp(fileName, _u("ByteCodeGenerator")) == 0 || _wcsicmp(fileName, _u("spartan")) == 0 || _wcsicmp(fileName, _u("spartan_edge")) == 0 || - _wcsicmp(fileName, _u("MicrosoftEdge")) == 0 || - _wcsicmp(fileName, _u("MicrosoftEdgeCP")) == 0) + _wcsnicmp(fileName, _u("MicrosoftEdge"), wcslen(_u("MicrosoftEdge"))) == 0) { // we need to output to %temp% directory in wwa. we don't have permission otherwise. diff --git a/deps/chakrashim/core/lib/Common/Core/DelayLoadLibrary.cpp b/deps/chakrashim/core/lib/Common/Core/DelayLoadLibrary.cpp index 20752028d91..aee807b52e5 100644 --- a/deps/chakrashim/core/lib/Common/Core/DelayLoadLibrary.cpp +++ b/deps/chakrashim/core/lib/Common/Core/DelayLoadLibrary.cpp @@ -89,9 +89,10 @@ DWORD NtdllLibrary::AddGrowableFunctionTable( _Out_ PVOID * DynamicTable, RangeBase, RangeEnd); #if _M_X64 - PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("Register: Begin: %llx, End: %x, Unwind: %llx, RangeBase: %llx, RangeEnd: %llx, table: %llx, Status: %x\n"), - FunctionTable->BeginAddress, FunctionTable->EndAddress, FunctionTable->UnwindInfoAddress, RangeBase, RangeEnd, *DynamicTable, status); + PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("Register: [%d] Begin: %llx, End: %x, Unwind: %llx, RangeBase: %llx, RangeEnd: %llx, table: %llx, Status: %x\n"), + GetCurrentThreadId(), FunctionTable->BeginAddress, FunctionTable->EndAddress, FunctionTable->UnwindInfoAddress, RangeBase, RangeEnd, *DynamicTable, status); #endif + Assert((status >= 0 && FunctionTable != nullptr) || status == 0xC000009A /*STATUS_INSUFFICIENT_RESOURCES*/); return status; } return 1; @@ -113,7 +114,7 @@ VOID NtdllLibrary::DeleteGrowableFunctionTable( _In_ PVOID DynamicTable ) } deleteGrowableFunctionTable(DynamicTable); - PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("UnRegister: table: %llx\n"), DynamicTable); + PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("UnRegister: [%d] table: %llx\n"), GetCurrentThreadId(), DynamicTable); } } diff --git a/deps/chakrashim/core/lib/Common/Memory/DelayDeletingFunctionTable.cpp b/deps/chakrashim/core/lib/Common/Memory/DelayDeletingFunctionTable.cpp index 9a58a55f830..fa18ddabf96 100644 --- a/deps/chakrashim/core/lib/Common/Memory/DelayDeletingFunctionTable.cpp +++ b/deps/chakrashim/core/lib/Common/Memory/DelayDeletingFunctionTable.cpp @@ -8,24 +8,20 @@ #if PDATA_ENABLED && defined(_WIN32) #include "Core/DelayLoadLibrary.h" #include -#endif PSLIST_HEADER DelayDeletingFunctionTable::Head = nullptr; DelayDeletingFunctionTable DelayDeletingFunctionTable::Instance; DelayDeletingFunctionTable::DelayDeletingFunctionTable() { -#if PDATA_ENABLED && defined(_WIN32) Head = (PSLIST_HEADER)_aligned_malloc(sizeof(SLIST_HEADER), MEMORY_ALLOCATION_ALIGNMENT); if (Head) { InitializeSListHead(Head); } -#endif } DelayDeletingFunctionTable::~DelayDeletingFunctionTable() { -#if PDATA_ENABLED && defined(_WIN32) Clear(); if (Head) { @@ -34,13 +30,11 @@ DelayDeletingFunctionTable::~DelayDeletingFunctionTable() _aligned_free(Head); Head = nullptr; } -#endif } bool DelayDeletingFunctionTable::AddEntry(FunctionTableHandle ft) { -#if PDATA_ENABLED && defined(_WIN32) if (Head) { FunctionTableNode* node = (FunctionTableNode*)_aligned_malloc(sizeof(FunctionTableNode), MEMORY_ALLOCATION_ALIGNMENT); @@ -51,13 +45,11 @@ bool DelayDeletingFunctionTable::AddEntry(FunctionTableHandle ft) return true; } } -#endif return false; } void DelayDeletingFunctionTable::Clear() { -#if PDATA_ENABLED && defined(_WIN32) if (Head) { SLIST_ENTRY* entry = InterlockedPopEntrySList(Head); @@ -69,21 +61,16 @@ void DelayDeletingFunctionTable::Clear() entry = InterlockedPopEntrySList(Head); } } -#endif } bool DelayDeletingFunctionTable::IsEmpty() { -#if PDATA_ENABLED && defined(_WIN32) return QueryDepthSList(Head) == 0; -#else - return true; -#endif } void DelayDeletingFunctionTable::DeleteFunctionTable(void* functionTable) { -#if PDATA_ENABLED && defined(_WIN32) NtdllLibrary::Instance->DeleteGrowableFunctionTable(functionTable); -#endif -} \ No newline at end of file +} + +#endif \ No newline at end of file diff --git a/deps/chakrashim/core/lib/Common/Memory/XDataAllocator.h b/deps/chakrashim/core/lib/Common/Memory/XDataAllocator.h index 8b9ba96d6b8..1329b61be65 100644 --- a/deps/chakrashim/core/lib/Common/Memory/XDataAllocator.h +++ b/deps/chakrashim/core/lib/Common/Memory/XDataAllocator.h @@ -12,13 +12,11 @@ #include "Memory/arm64/XDataAllocator.h" #endif +#if PDATA_ENABLED && defined(_WIN32) struct FunctionTableNode { -#ifdef _WIN32 SLIST_ENTRY itemEntry; FunctionTableHandle functionTable; - -#endif }; struct DelayDeletingFunctionTable @@ -33,4 +31,5 @@ struct DelayDeletingFunctionTable static void Clear(); static bool IsEmpty(); static void DeleteFunctionTable(void* functionTable); -}; \ No newline at end of file +}; +#endif \ No newline at end of file diff --git a/deps/chakrashim/core/lib/Common/Memory/amd64/XDataAllocator.cpp b/deps/chakrashim/core/lib/Common/Memory/amd64/XDataAllocator.cpp index d5591038dce..addea6f9359 100644 --- a/deps/chakrashim/core/lib/Common/Memory/amd64/XDataAllocator.cpp +++ b/deps/chakrashim/core/lib/Common/Memory/amd64/XDataAllocator.cpp @@ -127,6 +127,13 @@ void XDataAllocator::Register(XDataAllocation * xdataInfo, ULONG_PTR functionSta BOOLEAN success = FALSE; if (AutoSystemInfo::Data.IsWin8OrLater()) { +#if DBG + // Validate the PDATA was not registered or unregistered + ULONG64 imageBase = 0; + RUNTIME_FUNCTION *runtimeFunction = RtlLookupFunctionEntry((DWORD64)functionStart, &imageBase, nullptr); + Assert(runtimeFunction == NULL); +#endif + DWORD status = NtdllLibrary::Instance->AddGrowableFunctionTable(&xdataInfo->functionTable, &xdataInfo->pdata, /*MaxEntryCount*/ 1, @@ -134,7 +141,6 @@ void XDataAllocator::Register(XDataAllocation * xdataInfo, ULONG_PTR functionSta /*RangeBase*/ functionStart, /*RangeEnd*/ functionStart + functionSize); success = NT_SUCCESS(status); - Assert(success && xdataInfo->functionTable != nullptr); } else { diff --git a/deps/chakrashim/core/lib/Runtime/Base/FunctionBody.cpp b/deps/chakrashim/core/lib/Runtime/Base/FunctionBody.cpp index 72af470a1a0..5451d15aa30 100644 --- a/deps/chakrashim/core/lib/Runtime/Base/FunctionBody.cpp +++ b/deps/chakrashim/core/lib/Runtime/Base/FunctionBody.cpp @@ -8858,13 +8858,12 @@ namespace Js if (this->xdataInfo != nullptr) { #ifdef _WIN32 - if (this->xdataInfo->functionTable) + PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("EntryPointInfo::Cleanup: Freeing: function table: %llx, codeAddress: %%llx\n"), this->xdataInfo->functionTable, this->GetNativeEntrypoint()); + if (this->xdataInfo->functionTable + && !DelayDeletingFunctionTable::AddEntry(this->xdataInfo->functionTable)) { - if (!DelayDeletingFunctionTable::AddEntry(this->xdataInfo->functionTable)) - { - PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("EntryPointInfo::Cleanup: Failed to add to slist, table: %llx, address: %%llx\n"), this->xdataInfo->functionTable, this->GetNativeAddress()); - DelayDeletingFunctionTable::DeleteFunctionTable(this->xdataInfo->functionTable); - } + PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("EntryPointInfo::Cleanup: Failed to add to slist, table: %llx, address: %%llx\n"), this->xdataInfo->functionTable, this->GetNativeEntrypoint()); + DelayDeletingFunctionTable::DeleteFunctionTable(this->xdataInfo->functionTable); } #endif XDataAllocator::Unregister(this->xdataInfo); @@ -8876,7 +8875,8 @@ namespace Js } this->xdataInfo = nullptr; } -#endif +#endif //PDATA_ENABLED + this->OnCleanup(isShutdown); FreeJitTransferData(); @@ -9010,13 +9010,11 @@ namespace Js #if PDATA_ENABLED && defined(_WIN32) if (this->xdataInfo) { - if (this->xdataInfo->functionTable) + if (this->xdataInfo->functionTable + && !DelayDeletingFunctionTable::AddEntry(this->xdataInfo->functionTable)) { - if (!DelayDeletingFunctionTable::AddEntry(this->xdataInfo->functionTable)) - { - PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("EntryPointInfo::ResetOnLazyBailoutFailure: Failed to add to slist, table: %llx, address: %llx\n"), this->xdataInfo->functionTable, this->nativeAddress); - DelayDeletingFunctionTable::DeleteFunctionTable(this->xdataInfo->functionTable); - } + PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("EntryPointInfo::ResetOnLazyBailoutFailure: Failed to add to slist, table: %llx, address: %llx\n"), this->xdataInfo->functionTable, this->nativeAddress); + DelayDeletingFunctionTable::DeleteFunctionTable(this->xdataInfo->functionTable); } } #endif @@ -9181,6 +9179,14 @@ namespace Js { scriptContext->FreeFunctionEntryPoint((Js::JavascriptMethod)this->GetNativeAddress(), this->GetThunkAddress()); } +#if PDATA_ENABLED && defined(_WIN32) + else + { + // in case of debugger attaching, we have a new code generator and when deleting old code generator, + // the xData is not put in the delay list yet. clear the list now so the code addresses are ready to reuse + DelayDeletingFunctionTable::Clear(); + } +#endif } #ifdef PERF_COUNTERS diff --git a/deps/chakrashim/core/lib/Runtime/Base/ScriptContext.cpp b/deps/chakrashim/core/lib/Runtime/Base/ScriptContext.cpp index 2d759c43233..979334594a8 100644 --- a/deps/chakrashim/core/lib/Runtime/Base/ScriptContext.cpp +++ b/deps/chakrashim/core/lib/Runtime/Base/ScriptContext.cpp @@ -655,6 +655,7 @@ namespace Js if (hasFunctions) { #if ENABLE_NATIVE_CODEGEN +#if PDATA_ENABLED && defined(_WIN32) struct AutoReset { AutoReset(ThreadContext* threadContext) @@ -670,6 +671,7 @@ namespace Js ThreadContext* threadContext; } autoReset(this->GetThreadContext()); +#endif #endif // We still need to walk through all the function bodies and call cleanup @@ -3044,7 +3046,7 @@ namespace Js HRESULT hr = S_OK; BEGIN_TRANSLATE_OOM_TO_HRESULT_NESTED - this->nativeCodeGen = NewNativeCodeGenerator(this); + this->nativeCodeGen = NewNativeCodeGenerator(this); SetProfileModeNativeCodeGen(this->GetNativeCodeGenerator(), this->IsProfiling()); END_TRANSLATE_OOM_TO_HRESULT(hr);