[1.8>1.9] [MERGE #4560 @leirocks] simplify xData deletion

Merge pull request #4560 from leirocks:ftfix

change to not using jit code deletion job to do the work, instead always do it in side channel and before new xData registration

lib/Backend/CodeGenWorkItem.cpp

@@ -205,7 +205,22 @@ void CodeGenWorkItem::OnWorkItemProcessFail(NativeCodeGenerator* codeGen)
 #if DBG
         this->allocation->allocation->isNotExecutableBecauseOOM = true;
 #endif
-        codeGen->FreeNativeCodeGenAllocation(this->allocation->allocation->address, nullptr);
+
+#if PDATA_ENABLED & defined(_WIN32)
+        if (this->entryPointInfo && this->entryPointInfo->GetXDataInfo()) 
+        {
+            void* functionTable = this->entryPointInfo->GetXDataInfo()->functionTable;
+            if (functionTable)
+            {
+                if (!DelayDeletingFunctionTable::AddEntry(functionTable))
+                {
+                    PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("OnWorkItemProcessFail: Failed to add to slist, table: %llx\n"), functionTable);
+                    DelayDeletingFunctionTable::DeleteFunctionTable(functionTable);
+                }
+            }
+        }
+#endif
+        codeGen->FreeNativeCodeGenAllocation(this->allocation->allocation->address);
     }
 }
 

lib/Backend/EmitBuffer.cpp

@@ -55,6 +55,7 @@ template <typename TAlloc, typename TPreReservedAlloc, class SyncObject>
 void
 EmitBufferManager<TAlloc, TPreReservedAlloc, SyncObject>::FreeAllocations(bool release)
 {
+    DelayDeletingFunctionTable::Clear();
     AutoRealOrFakeCriticalSection<SyncObject> autoCs(&this->criticalSection);
 
 #if DBG_DUMP
@@ -191,9 +192,10 @@ EmitBufferManager<TAlloc, TPreReservedAlloc, SyncObject>::NewAllocation(size_t b
 
 template <typename TAlloc, typename TPreReservedAlloc, class SyncObject>
 bool
-EmitBufferManager<TAlloc, TPreReservedAlloc, SyncObject>::FreeAllocation(void* address, void** functionTable)
+EmitBufferManager<TAlloc, TPreReservedAlloc, SyncObject>::FreeAllocation(void* address)
 {
     AutoRealOrFakeCriticalSection<SyncObject> autoCs(&this->criticalSection);
+
 #if _M_ARM
     address = (void*)((uintptr_t)address & ~0x1); // clear the thumb bit
 #endif
@@ -243,14 +245,6 @@ EmitBufferManager<TAlloc, TPreReservedAlloc, SyncObject>::FreeAllocation(void* a
             }
             VerboseHeapTrace(_u("Freeing 0x%p, allocation: 0x%p\n"), address, allocation->allocation->address);
 
-#if PDATA_ENABLED && defined(_WIN32)
-            if (functionTable && *functionTable)
-            {
-                NtdllLibrary::Instance->DeleteGrowableFunctionTable(*functionTable);
-                *functionTable = nullptr;
-            }
-#endif
-
             this->allocationHeap.Free(allocation->allocation);
             this->allocator->Free(allocation, sizeof(TEmitBufferAllocation));
 

lib/Backend/EmitBuffer.h

@@ -46,7 +46,7 @@ class EmitBufferManager
     bool ProtectBufferWithExecuteReadWriteForInterpreter(TEmitBufferAllocation* allocation);
     bool CommitBufferForInterpreter(TEmitBufferAllocation* allocation, _In_reads_bytes_(bufferSize) BYTE* pBuffer, _In_ size_t bufferSize);
     void CompletePreviousAllocation(TEmitBufferAllocation* allocation);
-    bool FreeAllocation(void* address, void** functionTable);
+    bool FreeAllocation(void* address);
     //Ends here
 
     bool IsInHeap(void* address);

lib/Backend/NativeCodeGenerator.cpp

@@ -1117,6 +1117,7 @@ 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);
@@ -3214,14 +3215,14 @@ NativeCodeGenerator::EnterScriptStart()
 }
 
 void
-FreeNativeCodeGenAllocation(Js::ScriptContext *scriptContext, Js::JavascriptMethod codeAddress, Js::JavascriptMethod thunkAddress, void** functionTable)
+FreeNativeCodeGenAllocation(Js::ScriptContext *scriptContext, Js::JavascriptMethod codeAddress, Js::JavascriptMethod thunkAddress)
 {
     if (!scriptContext->GetNativeCodeGenerator())
     { 
         return;
     }
 
-    scriptContext->GetNativeCodeGenerator()->QueueFreeNativeCodeGenAllocation((void*)codeAddress, (void*)thunkAddress, functionTable);
+    scriptContext->GetNativeCodeGenerator()->QueueFreeNativeCodeGenAllocation((void*)codeAddress, (void*)thunkAddress);
 }
 
 bool TryReleaseNonHiPriWorkItem(Js::ScriptContext* scriptContext, CodeGenWorkItem* workItem)
@@ -3252,30 +3253,22 @@ bool NativeCodeGenerator::TryReleaseNonHiPriWorkItem(CodeGenWorkItem* workItem)
 }
 
 void
-NativeCodeGenerator::FreeNativeCodeGenAllocation(void* codeAddress, void** functionTable)
+NativeCodeGenerator::FreeNativeCodeGenAllocation(void* codeAddress)
 {
     if (JITManager::GetJITManager()->IsOOPJITEnabled())
     {
-        // function table delete in content process
-#if PDATA_ENABLED && defined(_WIN32)
-        if (functionTable && *functionTable)
-        {
-            NtdllLibrary::Instance->DeleteGrowableFunctionTable(*functionTable);
-            *functionTable = nullptr;
-        }
-#endif
         ThreadContext * context = this->scriptContext->GetThreadContext();
         HRESULT hr = JITManager::GetJITManager()->FreeAllocation(context->GetRemoteThreadContextAddr(), (intptr_t)codeAddress);
         JITManager::HandleServerCallResult(hr, RemoteCallType::MemFree);
     }
     else if(this->backgroundAllocators)
     {
-        this->backgroundAllocators->emitBufferManager.FreeAllocation(codeAddress, functionTable);
+        this->backgroundAllocators->emitBufferManager.FreeAllocation(codeAddress);
     }
 }
 
 void
-NativeCodeGenerator::QueueFreeNativeCodeGenAllocation(void* codeAddress, void * thunkAddress, void** functionTable)
+NativeCodeGenerator::QueueFreeNativeCodeGenAllocation(void* codeAddress, void * thunkAddress)
 {
     ASSERT_THREAD();
 
@@ -3305,24 +3298,24 @@ NativeCodeGenerator::QueueFreeNativeCodeGenAllocation(void* codeAddress, void *
     // OOP JIT will always queue a job
 
     // The foreground allocators may have been used
-    if (this->foregroundAllocators && this->foregroundAllocators->emitBufferManager.FreeAllocation(codeAddress, functionTable))
+    if (this->foregroundAllocators && this->foregroundAllocators->emitBufferManager.FreeAllocation(codeAddress))
     {
         return;
     }
 
     // The background allocators were used. Queue a job to free the allocation from the background thread.
-    this->freeLoopBodyManager.QueueFreeLoopBodyJob(codeAddress, thunkAddress, functionTable);
+    this->freeLoopBodyManager.QueueFreeLoopBodyJob(codeAddress, thunkAddress);
 }
 
-void NativeCodeGenerator::FreeLoopBodyJobManager::QueueFreeLoopBodyJob(void* codeAddress, void * thunkAddress, void** functionTable)
+void NativeCodeGenerator::FreeLoopBodyJobManager::QueueFreeLoopBodyJob(void* codeAddress, void * thunkAddress)
 {
     Assert(!this->isClosed);
 
-    FreeLoopBodyJob* job = HeapNewNoThrow(FreeLoopBodyJob, this, codeAddress, thunkAddress, *functionTable);
+    FreeLoopBodyJob* job = HeapNewNoThrow(FreeLoopBodyJob, this, codeAddress, thunkAddress);
 
     if (job == nullptr)
     {
-        FreeLoopBodyJob stackJob(this, codeAddress, thunkAddress, *functionTable, false /* heapAllocated */);
+        FreeLoopBodyJob stackJob(this, codeAddress, thunkAddress, false /* heapAllocated */);
 
         {
             AutoOptionalCriticalSection lock(Processor()->GetCriticalSection());
@@ -3346,9 +3339,6 @@ void NativeCodeGenerator::FreeLoopBodyJobManager::QueueFreeLoopBodyJob(void* cod
             HeapDelete(job);
         }
     }
-
-    // function table successfully transferred to background job
-    *functionTable = nullptr;
 }
 
 #ifdef PROFILE_EXEC

lib/Backend/NativeCodeGenerator.h

@@ -104,10 +104,10 @@ void SetProfileMode(BOOL fSet);
     void UpdateQueueForDebugMode();
     bool IsBackgroundJIT() const;
     void EnterScriptStart();
-    void FreeNativeCodeGenAllocation(void* codeAddress, void** functionTable);
+    void FreeNativeCodeGenAllocation(void* codeAddress);
     bool TryReleaseNonHiPriWorkItem(CodeGenWorkItem* workItem);
 
-    void QueueFreeNativeCodeGenAllocation(void* codeAddress, void* thunkAddress, void** functionTable);
+    void QueueFreeNativeCodeGenAllocation(void* codeAddress, void* thunkAddress);
 
     bool IsClosed() { return isClosed; }
     void AddWorkItem(CodeGenWorkItem* workItem);
@@ -201,19 +201,17 @@ void SetProfileMode(BOOL fSet);
     class FreeLoopBodyJob: public JsUtil::Job
     {
     public:
-        FreeLoopBodyJob(JsUtil::JobManager *const manager, void* codeAddress, void* thunkAddress, void* functionTable, bool isHeapAllocated = true):
+        FreeLoopBodyJob(JsUtil::JobManager *const manager, void* codeAddress, void* thunkAddress, bool isHeapAllocated = true):
           JsUtil::Job(manager),
           codeAddress(codeAddress),
           thunkAddress(thunkAddress),
-          functionTable(functionTable),
           heapAllocated(isHeapAllocated)
         {
         }
 
         bool heapAllocated;
         void* codeAddress;
         void* thunkAddress;
-        void* functionTable;
     };
 
     class FreeLoopBodyJobManager sealed: public WaitableJobManager
@@ -281,9 +279,8 @@ void SetProfileMode(BOOL fSet);
         {
             FreeLoopBodyJob* freeLoopBodyJob = static_cast<FreeLoopBodyJob*>(job);
 
-            void* functionTable = freeLoopBodyJob->functionTable;
             // Free Loop Body
-            nativeCodeGen->FreeNativeCodeGenAllocation(freeLoopBodyJob->codeAddress, &functionTable);
+            nativeCodeGen->FreeNativeCodeGenAllocation(freeLoopBodyJob->codeAddress);
 
             return true;
         }
@@ -306,7 +303,7 @@ void SetProfileMode(BOOL fSet);
             }
         }
 
-        void QueueFreeLoopBodyJob(void* codeAddress, void* thunkAddress, void** functionTable);
+        void QueueFreeLoopBodyJob(void* codeAddress, void* thunkAddress);
 
     private:
         NativeCodeGenerator* nativeCodeGen;

lib/Backend/PDataManager.cpp

@@ -46,7 +46,7 @@ void PDataManager::UnregisterPdata(RUNTIME_FUNCTION* pdata)
     if (AutoSystemInfo::Data.IsWin8OrLater())
     {
         // TODO: need to move to background?
-        NtdllLibrary::Instance->DeleteGrowableFunctionTable(pdata);
+        DelayDeletingFunctionTable::DeleteFunctionTable(pdata);
     }
     else
     {

lib/Common/BackendApi.h

@@ -62,7 +62,7 @@ void UpdateNativeCodeGeneratorForDebugMode(NativeCodeGenerator* nativeCodeGen);
 CriticalSection *GetNativeCodeGenCriticalSection(NativeCodeGenerator *pNativeCodeGen);
 bool TryReleaseNonHiPriWorkItem(Js::ScriptContext* scriptContext, CodeGenWorkItem* workItem);
 void NativeCodeGenEnterScriptStart(NativeCodeGenerator * nativeCodeGen);
-void FreeNativeCodeGenAllocation(Js::ScriptContext* scriptContext, Js::JavascriptMethod codeAddress, Js::JavascriptMethod thunkAddress, void** functionTable);
+void FreeNativeCodeGenAllocation(Js::ScriptContext* scriptContext, Js::JavascriptMethod codeAddress, Js::JavascriptMethod thunkAddress);
 InProcCodeGenAllocators* GetForegroundAllocator(NativeCodeGenerator * nativeCodeGen, PageAllocator* pageallocator);
 void GenerateFunction(NativeCodeGenerator * nativeCodeGen, Js::FunctionBody * functionBody, Js::ScriptFunction * function = NULL);
 void GenerateLoopBody(NativeCodeGenerator * nativeCodeGen, Js::FunctionBody * functionBody, Js::LoopHeader * loopHeader, Js::EntryPointInfo* entryPointInfo, uint localCount, Js::Var localSlots[]);

lib/Common/ConfigFlagsList.h

@@ -371,6 +371,7 @@ PHASE(All)
         PHASE(DeferSourceLoad)
         PHASE(ObjectMutationBreakpoint)
         PHASE(NativeCodeData)
+        PHASE(XData)
 #undef PHASE
 #endif
 

lib/Common/Core/DelayLoadLibrary.cpp

@@ -82,12 +82,17 @@ DWORD NtdllLibrary::AddGrowableFunctionTable( _Out_ PVOID * DynamicTable,
                 return 1;
             }
         }
-        return addGrowableFunctionTable(DynamicTable,
+        DWORD status = addGrowableFunctionTable(DynamicTable,
             FunctionTable,
             EntryCount,
             MaximumEntryCount,
             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);
+#endif
+        return status;
     }
     return 1;
 }
@@ -107,6 +112,8 @@ VOID NtdllLibrary::DeleteGrowableFunctionTable( _In_ PVOID DynamicTable )
             }
         }
         deleteGrowableFunctionTable(DynamicTable);
+
+        PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("UnRegister: table: %llx\n"), DynamicTable);
     }
 }
 

lib/Common/Memory/DelayDeletingFunctionTable.cpp

@@ -64,7 +64,7 @@ void DelayDeletingFunctionTable::Clear()
         while (entry)
         {
             FunctionTableNode* list = (FunctionTableNode*)entry;
-            NtdllLibrary::Instance->DeleteGrowableFunctionTable(list->functionTable);
+            DeleteFunctionTable(list->functionTable);
             _aligned_free(entry);
             entry = InterlockedPopEntrySList(Head);
         }
@@ -79,4 +79,11 @@ bool DelayDeletingFunctionTable::IsEmpty()
 #else
     return true;
 #endif
+}
+
+void DelayDeletingFunctionTable::DeleteFunctionTable(void* functionTable)
+{
+#if PDATA_ENABLED && defined(_WIN32)
+    NtdllLibrary::Instance->DeleteGrowableFunctionTable(functionTable);
+#endif
 }

lib/Common/Memory/XDataAllocator.h

@@ -32,4 +32,5 @@ struct DelayDeletingFunctionTable
     static bool AddEntry(FunctionTableHandle ft);
     static void Clear();
     static bool IsEmpty();
+    static void DeleteFunctionTable(void* functionTable);
 };

lib/Common/Memory/amd64/XDataAllocator.cpp

@@ -134,10 +134,7 @@ void XDataAllocator::Register(XDataAllocation * xdataInfo, ULONG_PTR functionSta
             /*RangeBase*/ functionStart,
             /*RangeEnd*/ functionStart + functionSize);
         success = NT_SUCCESS(status);
-        if (success)
-        {
-            Assert(xdataInfo->functionTable != nullptr);
-        }
+        Assert(success && xdataInfo->functionTable != nullptr);
     }
     else
     {

lib/JITServer/JITServer.cpp

@@ -676,7 +676,7 @@ ServerFreeAllocation(
 
     return ServerCallWrapper(context, [&]()->HRESULT
     {
-        context->GetCodeGenAllocators()->emitBufferManager.FreeAllocation((void*)codeAddress, nullptr);
+        context->GetCodeGenAllocators()->emitBufferManager.FreeAllocation((void*)codeAddress);
         return S_OK;
     });
 }