From 43ad7c7eb48620659581e2de1a917382c5f2c66a Mon Sep 17 00:00:00 2001 From: Del Myers Date: Tue, 1 Dec 2020 15:34:29 -0800 Subject: [PATCH] Reduce overhead of retargeting branches. (#327) * Add back-links to branches from targets. Makes branch targets track what branches target them. This reduces the overhead of retargeting branches after an insert because it means that the entire graph does not need to be searched for branches. Instead the branch target holds its own list of branches that target it, and that list can be iterated over instead. This reduces the cost of InsertBeforeAndRetargetOffsets from O(G) to O(B) where B is the number of branches that target the Instruction being inserted before. The amortized cost should therefore be near O(1) because the average instruction is not a branch target, and most branch targets will have only 1 branch tareting them. I have not done the math to be certain of this, though. The list of branches targeting a single instruction is stored as a CDataItem so that we don't have to add additional pointers to instructions where it isn't necessary. The list is a list of raw pointers because every pointer in the list should be kept alive by the graph, and using CComPtrs would increase overhead while iterating through the list. * Reduce overhead of GetInstructionSize * Remove message logging from Instruction.cpp Message logging is taking up a lot of time during iteration over instructions. It doesn't add a lot of useful info to the logs, so I'm removing it. * Added debugwait to profiler helpers * code review comments. Fix memory leak. * fix merge problems * Fix linux build --- .../DataContainer.cpp | 42 ++- src/InstrumentationEngine.Lib/DataContainer.h | 2 +- .../BranchTargetInfo.cpp | 148 +++++++++ src/InstrumentationEngine/BranchTargetInfo.h | 47 +++ src/InstrumentationEngine/CMakeLists.txt | 1 + src/InstrumentationEngine/ExceptionClause.cpp | 64 +++- src/InstrumentationEngine/ExceptionClause.h | 10 +- src/InstrumentationEngine/Instruction.cpp | 297 +++--------------- src/InstrumentationEngine/Instruction.h | 207 +++++++----- .../InstructionFactory.cpp | 9 +- .../InstructionGraph.cpp | 94 ++---- src/InstrumentationEngine/InstructionGraph.h | 5 +- .../InstrumentationEngine.vcxproj | 1 + .../InstrEngineTests/ProfilerHelpers.cs | 14 +- 14 files changed, 492 insertions(+), 449 deletions(-) create mode 100644 src/InstrumentationEngine/BranchTargetInfo.cpp create mode 100644 src/InstrumentationEngine/BranchTargetInfo.h diff --git a/src/InstrumentationEngine.Lib/DataContainer.cpp b/src/InstrumentationEngine.Lib/DataContainer.cpp index 6032c6fe..2ae62540 100644 --- a/src/InstrumentationEngine.Lib/DataContainer.cpp +++ b/src/InstrumentationEngine.Lib/DataContainer.cpp @@ -4,7 +4,7 @@ #include "stdafx.h" #include "DataContainer.h" -MicrosoftInstrumentationEngine::CDataContainer::CDataContainer() +MicrosoftInstrumentationEngine::CDataContainer::CDataContainer() : m_dataContainerMap(1) { DEFINE_REFCOUNT_NAME(CDataContainer); @@ -32,27 +32,51 @@ MicrosoftInstrumentationEngine::CDataContainer::~CDataContainer() HRESULT MicrosoftInstrumentationEngine::CDataContainer::SetDataItem( _In_ const GUID* pComponentId, _In_ const GUID* pObjectGuid, - _In_ IUnknown* pDataItem + _In_opt_ IUnknown* pDataItem ) { HRESULT hr = S_OK; CCriticalSectionHolder lock(&m_cs); - CAtlMap>* pMap; + CAtlMap>* pMap = nullptr; if (m_dataContainerMap.Lookup(*pComponentId, pMap) == false) { - pMap = new CAtlMap>; - if (!pMap) + // Only create the new map if we aren't setting the + // data item to null. + if (pDataItem != nullptr) { - return E_OUTOFMEMORY; - } + pMap = new CAtlMap>(1); + if (!pMap) + { + return E_OUTOFMEMORY; + } - m_dataContainerMap.SetAt(*pComponentId, pMap); + m_dataContainerMap.SetAt(*pComponentId, pMap); + } } - pMap->SetAt(*pObjectGuid, CComPtr(pDataItem)); + if (pMap != nullptr) + { + // if the data item is null, free memory. + if (pDataItem == nullptr) + { + pMap->RemoveKey(*pObjectGuid); + + if (pMap->GetCount() == 0) + { + m_dataContainerMap.RemoveKey(*pComponentId); + + // need to manually delete the map. + delete pMap; + } + } + else + { + pMap->SetAt(*pObjectGuid, CComPtr(pDataItem)); + } + } return hr; } diff --git a/src/InstrumentationEngine.Lib/DataContainer.h b/src/InstrumentationEngine.Lib/DataContainer.h index a26f0fef..6772c08c 100644 --- a/src/InstrumentationEngine.Lib/DataContainer.h +++ b/src/InstrumentationEngine.Lib/DataContainer.h @@ -23,7 +23,7 @@ namespace MicrosoftInstrumentationEngine virtual HRESULT __stdcall SetDataItem( _In_ const GUID* componentId, _In_ const GUID* objectGuid, - _In_ IUnknown* pDataItem + _In_opt_ IUnknown* pDataItem ); virtual HRESULT __stdcall GetDataItem( diff --git a/src/InstrumentationEngine/BranchTargetInfo.cpp b/src/InstrumentationEngine/BranchTargetInfo.cpp new file mode 100644 index 00000000..791df0b9 --- /dev/null +++ b/src/InstrumentationEngine/BranchTargetInfo.cpp @@ -0,0 +1,148 @@ +#include "stdafx.h" +#include "refcount.h" +#include "Instruction.h" +#include "BranchTargetInfo.h" + +namespace MicrosoftInstrumentationEngine +{ + // Note: we construct m_branches with only one initial bucket to save memory and because + // most instructions will not be targeted by that many branches. + CBranchTargetInfo::CBranchTargetInfo(_In_ CInstruction* pInstruction) : m_pInstruction(pInstruction), m_branches(1) + { + } + + HRESULT CBranchTargetInfo::GetInstance(_In_ CInstruction* pInstruction, _Outptr_ CBranchTargetInfo** ppResult) + { + IfNullRet(pInstruction); + IfNullRet(ppResult); + + *ppResult = nullptr; + + const GUID* uuid = &__uuidof(CBranchTargetInfo); + CComPtr pUnknown; + HRESULT hr; + // Don't assert, it is common for there to not be target info. + IfFailRetNoLog(pInstruction->GetDataItem(uuid, uuid, &pUnknown)); + CComPtr pTargetInfo; + IfFailRet(pUnknown.QueryInterface(&pTargetInfo)); + + IfFalseRet(pTargetInfo != nullptr, E_UNEXPECTED); + *ppResult = pTargetInfo.Detach(); + + return S_OK; + } + + HRESULT CBranchTargetInfo::GetOrCreateInstance(_In_ CInstruction* pInstruction, _Out_ CBranchTargetInfo** ppResult) + { + IfNullRet(pInstruction); + IfNullRet(ppResult); + if (!SUCCEEDED(GetInstance(pInstruction, ppResult))) + { + CComPtr pResult; + pResult.Attach(new CBranchTargetInfo(pInstruction)); + const GUID* uuid = &__uuidof(CBranchTargetInfo); + pInstruction->SetDataItem(uuid, uuid, pResult); + *ppResult = pResult.Detach(); + } + + return S_OK; + } + + HRESULT CBranchTargetInfo::SetBranchTarget(_In_ CInstruction* pBranch, _In_opt_ CInstruction* pTarget, _In_opt_ CInstruction* pOldTarget) + { + IfNullRet(pBranch); + + HRESULT hr; + + if (pOldTarget != nullptr) + { + CComPtr pOldInfo; + + // There may not be an instance of CBranchTargetInfo if we are in the + // middle of retargetting a branch. That is OK, just ignore it. + if (SUCCEEDED(CBranchTargetInfo::GetInstance(pOldTarget, &pOldInfo))) + { + pOldInfo->Remove(pBranch); + } + } + + if (pTarget != nullptr) + { + CComPtr pInfo; + IfFailRet(CBranchTargetInfo::GetOrCreateInstance(pTarget, &pInfo)); + pInfo.p->m_branches.emplace(pBranch); + } + + return S_OK; + } + + HRESULT CBranchTargetInfo::RetargetBranches(_In_ CInstruction* pOriginalInstruction, _In_ CInstruction* pNewInstruction) + { + IfNullRet(pOriginalInstruction); + IfNullRet(pNewInstruction); + + CComPtr pTargetInfo; + if (SUCCEEDED(CBranchTargetInfo::GetInstance(pOriginalInstruction, &pTargetInfo))) + { + return pTargetInfo->Retarget(pNewInstruction); + } + return S_OK; + } + + HRESULT CBranchTargetInfo::Retarget(_In_ CInstruction* pNewInstruction) + { + HRESULT hr = S_OK; + + // Remove this CBranchTargetInfo from the data container. If a new one + // is needed, it will be regenerated when branch targets are set. + Disconnect(); + for (CInstruction* pInstr : m_branches) + { + if (pInstr->GetIsBranchInternal()) + { + CComPtr pBranch; + IfFailRet(pInstr->QueryInterface(__uuidof(CBranchInstruction), (void**)&pBranch)); + + // This indicates that the new instruction targets this instruction. + // If we were to retarget it, we would cause an infinite loop. + if (pBranch != pNewInstruction) + { + IfFailRet(pBranch->SetBranchTarget(pNewInstruction)); + } + else + { + // reset the branch target, since the disconnection + // above will have removed it. + IfFailRet(pBranch->SetBranchTarget(m_pInstruction)); + } + } + else if (pInstr->GetIsSwitchInternal()) + { + CComPtr pSwitch; + IfFailRet(pInstr->QueryInterface(__uuidof(ISwitchInstruction), (void**)&pSwitch)); + IfFalseRet(pSwitch != nullptr, E_UNEXPECTED); + IfFailRet(pSwitch->ReplaceBranchTarget(m_pInstruction, pNewInstruction)); + } + } + + return S_OK; + } + + void CBranchTargetInfo::Remove(_In_ CInstruction* pOldBranch) + { + m_branches.erase(pOldBranch); + if (m_branches.empty()) + { + Disconnect(); + } + } + + void CBranchTargetInfo::Disconnect() + { + const GUID* uuid = &__uuidof(CBranchTargetInfo); + + // break cycles. + m_pInstruction->SetDataItem(uuid, uuid, nullptr); + } +} + diff --git a/src/InstrumentationEngine/BranchTargetInfo.h b/src/InstrumentationEngine/BranchTargetInfo.h new file mode 100644 index 00000000..60039812 --- /dev/null +++ b/src/InstrumentationEngine/BranchTargetInfo.h @@ -0,0 +1,47 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +#pragma once + + +namespace MicrosoftInstrumentationEngine +{ + class __declspec(uuid("0E23A44A-6700-4810-889C-8067638C512E")) + CBranchTargetInfo : public IUnknown, virtual CModuleRefCount + { + private: + + // This is a raw pointer to avoid circular references. + CInstruction* m_pInstruction; + + // Using raw pointers for speed. The graph should keep these pointers + // alive. + std::unordered_set m_branches; + + public: + DEFINE_DELEGATED_REFCOUNT_ADDREF(CBranchTargetInfo); + DEFINE_DELEGATED_REFCOUNT_RELEASE(CBranchTargetInfo); + STDMETHOD(QueryInterface)(_In_ REFIID riid, _Out_ void** ppvObject) override + { + return ImplQueryInterface( + static_cast(this), + this, + riid, + ppvObject + ); + } + + public: + static HRESULT GetInstance(_In_ CInstruction* pInstruction, _Outptr_ CBranchTargetInfo** ppResult); + static HRESULT GetOrCreateInstance(_In_ CInstruction* pInstruction, _Out_ CBranchTargetInfo** ppResult); + static HRESULT SetBranchTarget(_In_ CInstruction* pBranch, _In_opt_ CInstruction* pNewTarget, _In_opt_ CInstruction* pOldTarget); + static HRESULT RetargetBranches(_In_ CInstruction* pOriginalInstruction, _In_ CInstruction* pNewInstruction); + void Disconnect(); + + virtual ~CBranchTargetInfo() = default; + + CBranchTargetInfo(_In_ CInstruction* pInstruction); + HRESULT Retarget(_In_ CInstruction* pNewInstruction); + void Remove(_In_ CInstruction* pOldBranch); + }; +} \ No newline at end of file diff --git a/src/InstrumentationEngine/CMakeLists.txt b/src/InstrumentationEngine/CMakeLists.txt index d13f489c..2e7709e1 100644 --- a/src/InstrumentationEngine/CMakeLists.txt +++ b/src/InstrumentationEngine/CMakeLists.txt @@ -20,6 +20,7 @@ set(src_files ./AssemblyInfo.cpp #./AssemblyInjector.cpp #./AtlModule.cpp + ./BranchTargetInfo.cpp ./CompositeType.cpp ./CorHeaders.cpp ./CorMethodMalloc.cpp diff --git a/src/InstrumentationEngine/ExceptionClause.cpp b/src/InstrumentationEngine/ExceptionClause.cpp index cf2f3737..c65c62de 100644 --- a/src/InstrumentationEngine/ExceptionClause.cpp +++ b/src/InstrumentationEngine/ExceptionClause.cpp @@ -24,11 +24,11 @@ HRESULT MicrosoftInstrumentationEngine::CExceptionClause::InitializeFromSmall( m_flags = static_cast(pSmallClause->Flags); #endif - IfFailRet(pInstructionGraph->GetInstructionAtOffset(pSmallClause->TryOffset, &m_pTryFirstInstruction)); + IfFailRet(pInstructionGraph->GetInstructionAtOffsetInternal(pSmallClause->TryOffset, &m_pTryFirstInstruction)); IfFailRet(pInstructionGraph->GetInstructionAtEndOffset(pSmallClause->TryOffset + pSmallClause->TryLength, &m_pTryLastInstruction)); // Is it legal to not have a handler for a filter? Not doing if fail ret here. - pInstructionGraph->GetInstructionAtOffset(pSmallClause->HandlerOffset, &m_pHandlerFirstInstruction); + pInstructionGraph->GetInstructionAtOffsetInternal(pSmallClause->HandlerOffset, &m_pHandlerFirstInstruction); pInstructionGraph->GetInstructionAtEndOffset(pSmallClause->HandlerOffset + pSmallClause->HandlerLength, &m_pHandlerLastInstruction); if (m_flags == COR_ILEXCEPTION_CLAUSE_NONE) @@ -39,7 +39,7 @@ HRESULT MicrosoftInstrumentationEngine::CExceptionClause::InitializeFromSmall( else if (m_flags == COR_ILEXCEPTION_CLAUSE_FILTER) { //filter block - get the filter first instruction - IfFailRet(pInstructionGraph->GetInstructionAtOffset(pSmallClause->FilterOffset, &m_pFilterFirstInstruction)); + IfFailRet(pInstructionGraph->GetInstructionAtOffsetInternal(pSmallClause->FilterOffset, &m_pFilterFirstInstruction)); } CLogging::LogMessage(_T("End CExceptionClause::InitializeFromSmall")); @@ -57,11 +57,11 @@ HRESULT MicrosoftInstrumentationEngine::CExceptionClause::InitializeFromFat( m_flags = pFatClause->Flags; - IfFailRet(pInstructionGraph->GetInstructionAtOffset(pFatClause->TryOffset, &m_pTryFirstInstruction)); + IfFailRet(pInstructionGraph->GetInstructionAtOffsetInternal(pFatClause->TryOffset, &m_pTryFirstInstruction)); IfFailRet(pInstructionGraph->GetInstructionAtEndOffset(pFatClause->TryOffset + pFatClause->TryLength, &m_pTryLastInstruction)); // Is it legal to not have a handler for a filter? Not doing if fail ret here. - pInstructionGraph->GetInstructionAtOffset(pFatClause->HandlerOffset, &m_pHandlerFirstInstruction); + pInstructionGraph->GetInstructionAtOffsetInternal(pFatClause->HandlerOffset, &m_pHandlerFirstInstruction); pInstructionGraph->GetInstructionAtEndOffset(pFatClause->HandlerOffset + pFatClause->HandlerLength, &m_pHandlerLastInstruction); if (m_flags == COR_ILEXCEPTION_CLAUSE_NONE) @@ -72,7 +72,7 @@ HRESULT MicrosoftInstrumentationEngine::CExceptionClause::InitializeFromFat( else if (m_flags == COR_ILEXCEPTION_CLAUSE_FILTER) { //filter block - get the filter first instruction - IfFailRet(pInstructionGraph->GetInstructionAtOffset(pFatClause->FilterOffset, &m_pFilterFirstInstruction)); + IfFailRet(pInstructionGraph->GetInstructionAtOffsetInternal(pFatClause->FilterOffset, &m_pFilterFirstInstruction)); } CLogging::LogMessage(_T("End CExceptionClause::InitializeFromSmall")); @@ -95,18 +95,15 @@ HRESULT MicrosoftInstrumentationEngine::CExceptionClause::RenderExceptionClause( DWORD firstTryOffset = 0; DWORD lastTryOffset = 0; - DWORD lastTrySize = 0; + DWORD lastTrySize = m_pTryLastInstruction->GetInstructionSize();; IfFailRet(m_pTryFirstInstruction->GetOffset(&firstTryOffset)); IfFailRet(m_pTryLastInstruction->GetOffset(&lastTryOffset)); - IfFailRet(CInstruction::GetInstructionSize(m_pTryLastInstruction, &lastTrySize)); DWORD firstHandlerOffset = 0; DWORD lastHandlerOffset = 0; - DWORD lastHandlerSize = 0; + DWORD lastHandlerSize = m_pHandlerLastInstruction->GetInstructionSize(); IfFailRet(m_pHandlerFirstInstruction->GetOffset(&firstHandlerOffset)); IfFailRet(m_pHandlerLastInstruction->GetOffset(&lastHandlerOffset)); - IfFailRet(CInstruction::GetInstructionSize(m_pHandlerLastInstruction, &lastHandlerSize)); - pEHClause->TryOffset = firstTryOffset; pEHClause->TryLength = (lastTryOffset + lastTrySize) - firstTryOffset; @@ -308,7 +305,14 @@ HRESULT MicrosoftInstrumentationEngine::CExceptionClause::SetTryFirstInstruction HRESULT hr = S_OK; CLogging::LogMessage(_T("Starting CExceptionClause::SetTryFirstInstruction")); - m_pTryFirstInstruction = pInstruction; + if (pInstruction != nullptr) + { + IfFailRet(pInstruction->QueryInterface(__uuidof(CInstruction), (void**)&m_pTryFirstInstruction)); + } + else + { + m_pTryFirstInstruction = nullptr; + } CLogging::LogMessage(_T("End CExceptionClause::SetTryFirstInstruction")); return hr; @@ -319,7 +323,14 @@ HRESULT MicrosoftInstrumentationEngine::CExceptionClause::SetTryLastInstruction( HRESULT hr = S_OK; CLogging::LogMessage(_T("Starting CExceptionClause::SetTryLastInstruction")); - m_pTryLastInstruction = pInstruction; + if (pInstruction != nullptr) + { + IfFailRet(pInstruction->QueryInterface(__uuidof(CInstruction), (void**)&m_pTryLastInstruction)); + } + else + { + m_pTryLastInstruction = nullptr; + } CLogging::LogMessage(_T("End CExceptionClause::SetTryLastInstruction")); return hr; @@ -330,7 +341,14 @@ HRESULT MicrosoftInstrumentationEngine::CExceptionClause::SetHandlerFirstInstruc HRESULT hr = S_OK; CLogging::LogMessage(_T("Starting CExceptionClause::SetHandlerFirstInstruction")); - m_pHandlerFirstInstruction = pInstruction; + if (pInstruction != nullptr) + { + IfFailRet(pInstruction->QueryInterface(__uuidof(CInstruction), (void**)&m_pHandlerFirstInstruction)); + } + else + { + m_pHandlerFirstInstruction = nullptr; + } CLogging::LogMessage(_T("End CExceptionClause::SetHandlerFirstInstruction")); return hr; @@ -341,7 +359,14 @@ HRESULT MicrosoftInstrumentationEngine::CExceptionClause::SetHandlerLastInstruct HRESULT hr = S_OK; CLogging::LogMessage(_T("Starting CExceptionClause::SetHandlerLastInstruction")); - m_pHandlerLastInstruction = pInstruction; + if (pInstruction != nullptr) + { + IfFailRet(pInstruction->QueryInterface(__uuidof(CInstruction), (void**)&m_pHandlerLastInstruction)); + } + else + { + m_pHandlerLastInstruction = nullptr; + } CLogging::LogMessage(_T("End CExceptionClause::SetHandlerLastInstruction")); return hr; @@ -352,7 +377,14 @@ HRESULT MicrosoftInstrumentationEngine::CExceptionClause::SetFilterFirstInstruct HRESULT hr = S_OK; CLogging::LogMessage(_T("Starting CExceptionClause::SetFilterFirstInstruction")); - m_pFilterFirstInstruction = pInstruction; + if (pInstruction != nullptr) + { + IfFailRet(pInstruction->QueryInterface(__uuidof(CInstruction), (void**)&m_pFilterFirstInstruction)); + } + else + { + m_pFilterFirstInstruction = nullptr; + } CLogging::LogMessage(_T("End CExceptionClause::SetFilterFirstInstruction")); return hr; diff --git a/src/InstrumentationEngine/ExceptionClause.h b/src/InstrumentationEngine/ExceptionClause.h index 3cb263ed..dbe4c4b2 100644 --- a/src/InstrumentationEngine/ExceptionClause.h +++ b/src/InstrumentationEngine/ExceptionClause.h @@ -18,11 +18,11 @@ namespace MicrosoftInstrumentationEngine { private: CorExceptionFlag m_flags; - CComPtr m_pTryFirstInstruction; - CComPtr m_pTryLastInstruction; - CComPtr m_pHandlerFirstInstruction; - CComPtr m_pHandlerLastInstruction; - CComPtr m_pFilterFirstInstruction; + CComPtr m_pTryFirstInstruction; + CComPtr m_pTryLastInstruction; + CComPtr m_pHandlerFirstInstruction; + CComPtr m_pHandlerLastInstruction; + CComPtr m_pFilterFirstInstruction; mdToken m_ExceptionHandlerType; diff --git a/src/InstrumentationEngine/Instruction.cpp b/src/InstrumentationEngine/Instruction.cpp index 5258275f..ef9093e6 100644 --- a/src/InstrumentationEngine/Instruction.cpp +++ b/src/InstrumentationEngine/Instruction.cpp @@ -3,6 +3,7 @@ #include "stdafx.h" #include "Instruction.h" +#include "BranchTargetInfo.h" HRESULT MicrosoftInstrumentationEngine::CInstruction::LogInstruction(bool ignoreTest) { @@ -111,9 +112,6 @@ HRESULT MicrosoftInstrumentationEngine::CInstruction::InstructionFromBytes( { HRESULT hr = S_OK; *ppInstruction = NULL; - - CLogging::LogMessage(_T("Starting CInstruction::InstructionFromBytes")); - ILOrdinalOpcode opcode; CInstruction::OrdinalOpcodeFromBytes(pCode, pEndOfCode, &opcode); @@ -193,15 +191,11 @@ HRESULT MicrosoftInstrumentationEngine::CInstruction::InstructionFromBytes( default: { - CLogging::LogMessage(_T("CInstruction::InstructionFromBytes - Incorrect operand type")); return E_FAIL; } } } } - - CLogging::LogMessage(_T("End CInstruction::InstructionFromBytes")); - return S_OK; } @@ -362,56 +356,37 @@ HRESULT MicrosoftInstrumentationEngine::CInstruction::SetIsRemoved() HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOffset(_Out_ DWORD* pdwOffset) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetOffset")); - IfNullRetPointer(pdwOffset); IfFailRet(EnsureGraphUpdated()); *pdwOffset = m_offset; - - CLogging::LogMessage(_T("End CInstruction::GetOffset")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOriginalOffset(_Out_ DWORD* pdwOffset) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetOriginalOffset")); - IfNullRetPointer(pdwOffset); IfFailRet(EnsureGraphUpdated()); *pdwOffset = m_origOffset; - - CLogging::LogMessage(_T("End CInstruction::GetOriginalOffset")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOpCodeName(_Out_ BSTR* pbstrName) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetOpCodeName")); - IfNullRetPointer(pbstrName); CComBSTR bstrOpCodeName = s_ilOpcodeInfo[m_opcode].m_name; *pbstrName = bstrOpCodeName.Detach(); - - CLogging::LogMessage(_T("End CInstruction::GetOpCodeName")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOpCode(_Out_ ILOrdinalOpcode* pOpCode) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetOpCode")); IfNullRetPointer(pOpCode); *pOpCode = m_opcode; - - CLogging::LogMessage(_T("End CInstruction::GetOpCode")); - return S_OK; } @@ -419,78 +394,54 @@ HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOpCodeName(_Out_ BSTR* HRESULT MicrosoftInstrumentationEngine::CInstruction::GetAlternateOrdinalOpcode(_Out_ ILOrdinalOpcode* pAlternative) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetAlternateOrdinalOpcode")); IfNullRetPointer(pAlternative); *pAlternative = s_ilOpcodeInfo[m_opcode].m_alternate; - - CLogging::LogMessage(_T("End CInstruction::GetAlternateOrdinalOpcode")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOpcodeFlags(_Out_ ILOpcodeFlags* pFlags) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetAlternateOrdinalOpcode")); IfNullRetPointer(pFlags); *pFlags = s_ilOpcodeInfo[m_opcode].m_flags; - - CLogging::LogMessage(_T("End CInstruction::GetAlternateOrdinalOpcode")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetInstructionLength(_Out_ DWORD* pdwLength) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetInstructionLength ")); IfNullRetPointer(pdwLength); *pdwLength = s_ilOpcodeInfo[m_opcode].m_opcodeLength + s_ilOpcodeInfo[m_opcode].m_operandLength; - - CLogging::LogMessage(_T("End CInstruction::GetInstructionLength ")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOpcodeLength(_Out_ DWORD* pdwLength) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetOpcodeLength")); IfNullRetPointer(pdwLength); *pdwLength = s_ilOpcodeInfo[m_opcode].m_opcodeLength; - - CLogging::LogMessage(_T("End CInstruction::GetOpcodeLength")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOperandType(_Out_ ILOperandType* pType) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetOpcodeLength")); IfNullRetPointer(pType); *pType = s_ilOpcodeInfo[m_opcode].m_type; - - CLogging::LogMessage(_T("End CInstruction::GetOpcodeLength")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOperandLength(_Out_ DWORD* pdwLength) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetOpcodeLength")); IfNullRetPointer(pdwLength); *pdwLength = s_ilOpcodeInfo[m_opcode].m_operandLength; - - CLogging::LogMessage(_T("End CInstruction::GetOpcodeLength")); - return S_OK; } @@ -498,13 +449,9 @@ HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOperandLength(_Out_ DWO HRESULT MicrosoftInstrumentationEngine::CInstruction::GetIsNew(_Out_ BOOL* pbValue) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetIsNew")); IfNullRetPointer(pbValue); *pbValue = m_instructionGeneration == InstructionGeneration::Generation_New ? TRUE : FALSE; - - CLogging::LogMessage(_T("End CInstruction::GetIsNew")); - return S_OK; } @@ -512,77 +459,44 @@ HRESULT MicrosoftInstrumentationEngine::CInstruction::GetIsNew(_Out_ BOOL* pbVal HRESULT MicrosoftInstrumentationEngine::CInstruction::GetIsRemoved(_Out_ BOOL* pbValue) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetIsRemoved")); IfNullRetPointer(pbValue); *pbValue = m_bIsRemoved; - - CLogging::LogMessage(_T("End CInstruction::GetIsRemoved")); - - return S_OK; -} - -HRESULT MicrosoftInstrumentationEngine::CInstruction::GetInstructionSize(_In_ IInstruction* pInstruction, _Out_ DWORD* pdwSize) -{ - HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetInstructionSize")); - IfNullRetPointer(pInstruction); - IfNullRetPointer(pdwSize); - - DWORD dwOpCodeLength = 0; - IfFailRet(pInstruction->GetOpcodeLength(&dwOpCodeLength)); - - DWORD dwOperandLength = 0; - IfFailRet(pInstruction->GetOperandLength(&dwOperandLength)); - - *pdwSize = dwOpCodeLength + dwOperandLength; - - CLogging::LogMessage(_T("End CInstruction::GetInstructionSize")); - return S_OK; } -HRESULT MicrosoftInstrumentationEngine::CInstruction::GetInstructionSize(_Out_ DWORD* pdwSize) +DWORD MicrosoftInstrumentationEngine::CInstruction::GetInstructionSize() { - return GetInstructionSize(this, pdwSize); + // Optimization. GetInstructionSize() is not implemented as a combination of GetOpCodeLength() + GetOperandLength() because + // it is called many times inside tight loops. Removing the additional virtual function call reduces overhead, and + // allows better optimization on return values. + return s_ilOpcodeInfo[m_opcode].m_opcodeLength + s_ilOpcodeInfo[m_opcode].m_operandLength; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetIsBranch(_Out_ BOOL* pbValue) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetIsBranch")); IfNullRetPointer(pbValue); *pbValue = GetIsBranchInternal(); - - CLogging::LogMessage(_T("End CInstruction::GetIsBranch")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetIsSwitch(_Out_ BOOL* pbValue) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetIsSwitch")); IfNullRetPointer(pbValue); *pbValue = GetIsSwitchInternal(); - - CLogging::LogMessage(_T("End CInstruction::GetIsSwitch")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetIsCallInstruction(_Out_ BOOL* pbValue) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetIsCallInstruction")); IfNullRetPointer(pbValue); *pbValue = (m_opcode == Cee_Call || m_opcode == Cee_Calli || m_opcode == Cee_Callvirt); - - CLogging::LogMessage(_T("End CInstruction::GetIsCallInstruction")); - return S_OK; } @@ -674,42 +588,32 @@ HRESULT MicrosoftInstrumentationEngine::CInstruction::GetIsFallThrough(_Out_ BOO HRESULT MicrosoftInstrumentationEngine::CInstruction::GetNextInstruction(_Out_ IInstruction** ppNextInstruction) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetNextInstruction")); IfNullRetPointer(ppNextInstruction); *ppNextInstruction = NULL; if (m_pNextInstruction == NULL) { - CLogging::LogMessage(_T("CInstruction::GetNextInstruction - no next instruction")); return E_FAIL; } *ppNextInstruction = (IInstruction*)(m_pNextInstruction.p); (*ppNextInstruction)->AddRef(); - - CLogging::LogMessage(_T("End CInstruction::GetNextInstruction")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetPreviousInstruction(_Out_ IInstruction** ppPrevInstruction) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetPreviousInstruction")); IfNullRetPointer(ppPrevInstruction); *ppPrevInstruction = NULL; if (m_pPreviousInstruction == NULL) { - CLogging::LogMessage(_T("CInstruction::GetPreviousInstruction - no previous instruction")); return E_FAIL; } *ppPrevInstruction = (IInstruction*)(m_pPreviousInstruction.p); (*ppPrevInstruction)->AddRef(); - - CLogging::LogMessage(_T("End CInstruction::GetPreviousInstruction")); - return S_OK; } @@ -717,112 +621,77 @@ HRESULT MicrosoftInstrumentationEngine::CInstruction::GetPreviousInstruction(_Ou HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOriginalNextInstruction(_Out_ IInstruction** ppNextInstruction) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetOriginalNextInstruction")); IfNullRetPointer(ppNextInstruction); *ppNextInstruction = NULL; if (m_pOriginalNextInstruction == NULL) { - CLogging::LogMessage(_T("CInstruction::GetOriginalNextInstruction - no original next instruction")); return E_FAIL; } *ppNextInstruction = (IInstruction*)(m_pOriginalNextInstruction.p); (*ppNextInstruction)->AddRef(); - - CLogging::LogMessage(_T("End CInstruction::GetOriginalNextInstruction")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::GetOriginalPreviousInstruction(_Out_ IInstruction** ppPrevInstruction) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::GetOriginalPreviousInstruction")); IfNullRetPointer(ppPrevInstruction); *ppPrevInstruction = NULL; if (m_pOriginalPreviousInstruction == NULL) { - CLogging::LogMessage(_T("CInstruction::GetOriginalPreviousInstruction - no original previous instruction")); return E_FAIL; } *ppPrevInstruction = (IInstruction*)(m_pOriginalPreviousInstruction.p); (*ppPrevInstruction)->AddRef(); - - CLogging::LogMessage(_T("End CInstruction::GetOriginalPreviousInstruction")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::SetNextInstruction(_In_opt_ CInstruction* pInstruction, _In_ bool setOrig) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::SetNextInstruction")); - m_pNextInstruction = pInstruction; if (m_pOriginalNextInstruction == NULL && setOrig) { m_pOriginalNextInstruction = m_pNextInstruction; } - - CLogging::LogMessage(_T("End CInstruction::SetNextInstruction")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::SetPreviousInstruction(_In_opt_ CInstruction* pInstruction, _In_ bool setOrig) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::SetPreviousInstruction")); - m_pPreviousInstruction = pInstruction; if (m_pOriginalPreviousInstruction == NULL && setOrig) { m_pOriginalPreviousInstruction = m_pPreviousInstruction; } - - CLogging::LogMessage(_T("End CInstruction::SetPreviousInstruction")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::SetOriginalOffset(_In_ ULONG offset) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::SetOriginalOffset offset is %04x"), offset); - m_origOffset = offset; - - CLogging::LogMessage(_T("End CInstruction::SetOriginalOffset")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::SetOffset(_In_ ULONG offset) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::SetOffset offset is %04x"), offset); - m_offset = offset; - - CLogging::LogMessage(_T("End CInstruction::SetOffset")); - return S_OK; } HRESULT MicrosoftInstrumentationEngine::CInstruction::SetInstructionGeneration(_In_ InstructionGeneration instructionGeneration) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstruction::SetInstructionGeneration")); - m_instructionGeneration = instructionGeneration; - - CLogging::LogMessage(_T("End CInstruction::SetInstructionGeneration")); - return S_OK; } @@ -918,14 +787,9 @@ HRESULT MicrosoftInstrumentationEngine::COperandInstruction::InitializeFromBytes HRESULT MicrosoftInstrumentationEngine::COperandInstruction::GetOperandType(_Out_ enum ILOperandType* pType) { HRESULT hr = S_OK; - - CLogging::LogMessage(_T("Starting COperandInstruction::GetOperandType")); IfNullRetPointer(pType); *pType = s_ilOpcodeInfo[m_opcode].m_type; - - CLogging::LogMessage(_T("End COperandInstruction::GetOperandType")); - return S_OK; } @@ -936,8 +800,6 @@ HRESULT MicrosoftInstrumentationEngine::COperandInstruction::GetOperandValue( ) { HRESULT hr = S_OK; - - CLogging::LogMessage(_T("Starting COperandInstruction::GetOperandValue")); IfNullRetPointer(pBytes); if (dwSize < s_ilOpcodeInfo[m_opcode].m_operandLength) @@ -975,9 +837,6 @@ HRESULT MicrosoftInstrumentationEngine::COperandInstruction::GetOperandValue( return E_FAIL; } } - - CLogging::LogMessage(_T("End COperandInstruction::GetOperandValue")); - return hr; } @@ -987,8 +846,6 @@ HRESULT MicrosoftInstrumentationEngine::COperandInstruction::SetOperandValue( ) { HRESULT hr = S_OK; - - CLogging::LogMessage(_T("Starting COperandInstruction::SetOperandValue")); IfNullRetPointer(pBytes); if (dwSize != s_ilOpcodeInfo[m_opcode].m_operandLength) @@ -1026,9 +883,6 @@ HRESULT MicrosoftInstrumentationEngine::COperandInstruction::SetOperandValue( return E_FAIL; } } - - CLogging::LogMessage(_T("End COperandInstruction::SetOperandValue")); - return S_OK; } @@ -1092,13 +946,9 @@ HRESULT MicrosoftInstrumentationEngine::CBranchInstruction::InitializeFromBytes( HRESULT MicrosoftInstrumentationEngine::CBranchInstruction::IsShortBranch(_Out_ BOOL* pbValue) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CBranchInstruction::IsShortBranch")); IfNullRetPointer(pbValue); *pbValue = (m_opcode < Cee_Br || m_opcode == Cee_Leave_S); - - CLogging::LogMessage(_T("End CBranchInstruction::IsShortBranch")); - return hr; } @@ -1107,8 +957,6 @@ HRESULT MicrosoftInstrumentationEngine::CBranchInstruction::IsShortBranch(_Out_ HRESULT MicrosoftInstrumentationEngine::CBranchInstruction::ExpandBranch() { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CBranchInstruction::ExpandBranch")); - if (CInstruction::s_ilOpcodeInfo[m_opcode].m_operandLength == 1) { //Expand the branch to the long form @@ -1122,9 +970,6 @@ HRESULT MicrosoftInstrumentationEngine::CBranchInstruction::ExpandBranch() m_opcode = (ILOrdinalOpcode) (m_opcode + (Cee_Br - Cee_Br_S)); } } - - CLogging::LogMessage(_T("End CBranchInstruction::ExpandBranch")); - return S_OK; } @@ -1132,23 +977,16 @@ HRESULT MicrosoftInstrumentationEngine::CBranchInstruction::ExpandBranch() HRESULT MicrosoftInstrumentationEngine::CBranchInstruction::GetBranchTarget(_Out_ IInstruction** ppTarget) { HRESULT hr = S_OK; - - CLogging::LogMessage(_T("Starting CBranchInstruction::GetTargetOffset")); IfNullRetPointer(ppTarget); *ppTarget = (IInstruction*)m_pBranchTarget; (*ppTarget)->AddRef(); - - CLogging::LogMessage(_T("End CBranchInstruction::GetTargetOffset")); - return hr; } HRESULT MicrosoftInstrumentationEngine::CBranchInstruction::GetTargetOffset(_Out_ DWORD* pOffset) { HRESULT hr = S_OK; - - CLogging::LogMessage(_T("Starting CBranchInstruction::GetTargetOffset")); IfNullRetPointer(pOffset); *pOffset = 0; @@ -1160,20 +998,18 @@ HRESULT MicrosoftInstrumentationEngine::CBranchInstruction::GetTargetOffset(_Out { *pOffset = m_decodedTargetOffset; } - - CLogging::LogMessage(_T("End CBranchInstruction::GetTargetOffset")); - return hr; } HRESULT MicrosoftInstrumentationEngine::CBranchInstruction::SetBranchTarget(_In_ IInstruction* pInstruction) { HRESULT hr = S_OK; - - CLogging::LogMessage(_T("Starting CBranchInstruction::SetBranchTarget")); IfNullRetPointer(pInstruction); - m_pBranchTarget = pInstruction; + CComPtr pOldInstruction = m_pBranchTarget; + m_pBranchTarget.Release(); + IfFailRet(CInstruction::CastTo(pInstruction, &m_pBranchTarget)); + IfFailRet(CBranchTargetInfo::SetBranchTarget(this, m_pBranchTarget, pOldInstruction)); if (m_pBranchTarget == nullptr) { @@ -1185,36 +1021,17 @@ HRESULT MicrosoftInstrumentationEngine::CBranchInstruction::SetBranchTarget(_In_ { m_pOrigBranchTarget = m_pBranchTarget; } - - CLogging::LogMessage(_T("End CBranchInstruction::SetBranchTarget")); - return hr; } -MicrosoftInstrumentationEngine::CSwitchInstruction::CSwitchInstruction( - _In_ ILOrdinalOpcode opcode, - _In_ bool isNew - ) : CInstruction(opcode, isNew) -{ - -} - MicrosoftInstrumentationEngine::CSwitchInstruction::CSwitchInstruction( _In_ ILOrdinalOpcode opcode, _In_ bool isNew, - _In_ DWORD cBranchTargets, - _In_reads_(cBranchTargets) IInstruction** ppBranchTargets - ) : CInstruction(opcode, isNew) + _In_ DWORD initialCount + ) : CInstruction(opcode, isNew), m_branchTargets(initialCount) { - m_branchTargets.reserve(cBranchTargets); - - for (DWORD i = 0; i < cBranchTargets; i++) - { - CComPtr pTarget = ppBranchTargets[i]; - m_branchTargets[i] = pTarget; - } } HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::InitializeFromBytes( @@ -1223,9 +1040,6 @@ HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::InitializeFromBytes( ) { HRESULT hr = S_OK; - - CLogging::LogMessage(_T("Starting CSwitchInstruction::Initialize")); - const ULONG* p = (ULONG*)(pCode + CInstruction::s_ilOpcodeInfo[m_opcode].m_opcodeLength); ULONG count = *p; // First U4 integer is the count of branch deltas p++; @@ -1241,9 +1055,6 @@ HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::InitializeFromBytes( m_origBranchTargetOffsets.push_back(*p); p++; } - - CLogging::LogMessage(_T("End CSwitchInstruction::Initialize")); - return hr; } @@ -1257,19 +1068,22 @@ HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::GetOperandLength(_Ou return hr; } +DWORD MicrosoftInstrumentationEngine::CSwitchInstruction::GetInstructionSize() +{ + // Optimization. GetInstructionSize() is not implemented as a combination of GetOpCodeLength() + GetOperandLength() because + // it is called many times inside tight loops. Removing the additional virtual function call reduces overhead, and + // allows better optimization on return values. + return s_ilOpcodeInfo[m_opcode].m_opcodeLength + (DWORD)((m_branchTargets.size() + 1) * sizeof(DWORD)); +} + HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::GetBranchTarget(_In_ DWORD index, _Out_ IInstruction** ppTarget) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CSwitchInstruction::GetBranchTarget")); - *ppTarget = GetBranchTargetInternal(index); if (*ppTarget != NULL) { (*ppTarget)->AddRef(); } - - CLogging::LogMessage(_T("End CSwitchInstruction::GetBranchTarget")); - return hr; } @@ -1281,7 +1095,7 @@ IInstruction* MicrosoftInstrumentationEngine::CSwitchInstruction::GetBranchTarge return NULL; } - const CComPtr& pInstruction = m_branchTargets[index]; + const CComPtr& pInstruction = m_branchTargets[index]; if (pInstruction == NULL) { CLogging::LogError(_T("CSwitchInstruction::GetBranchTarget - branch target at index is null")); @@ -1297,9 +1111,12 @@ HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::SetBranchTarget(_In_ HRESULT hr = S_OK; CLogging::LogMessage(_T("Starting CSwitchInstruction::SetBranchTarget")); - m_branchTargets[index] = pTarget; + CComPtr oldTarget = m_branchTargets[index]; + CComPtr pNewTarget; + IfFailRet(CInstruction::CastTo(pTarget, &pNewTarget)); - CLogging::LogMessage(_T("End CSwitchInstruction::SetBranchTarget")); + m_branchTargets[index] = pNewTarget; + IfFailRet(CBranchTargetInfo::SetBranchTarget(this, pNewTarget, oldTarget)); return hr; } @@ -1307,21 +1124,14 @@ HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::SetBranchTarget(_In_ HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::RemoveBranchTargetAt(_In_ DWORD index) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CSwitchInstruction::RemoveBranchTargetAt")); - m_branchTargets.erase(m_branchTargets.begin() + index); - - CLogging::LogMessage(_T("End CSwitchInstruction::RemoveBranchTargetAt")); - return hr; } HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::RemoveBranchTarget(_In_ IInstruction* pTarget) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CSwitchInstruction::RemoveBranchTarget")); - - for(std::vector>::iterator iter = m_branchTargets.begin(); iter != m_branchTargets.end(); ++iter) + for(std::vector>::iterator iter = m_branchTargets.begin(); iter != m_branchTargets.end(); ++iter) { if(*iter == pTarget) { @@ -1329,9 +1139,6 @@ HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::RemoveBranchTarget(_ } } - CLogging::LogMessage(_T("End CSwitchInstruction::RemoveBranchTarget")); - - return hr; } @@ -1340,19 +1147,14 @@ HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::RemoveBranchTarget(_ HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::ReplaceBranchTarget(_In_ IInstruction* pOriginal, _In_ IInstruction *pNew) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CSwitchInstruction::SetBranchTarget")); - for(ULONG i = 0; i < m_branchTargets.size(); i++) { if(m_branchTargets[i] == pOriginal) { - SetBranchTarget(i, pNew); + IfFailRet(SetBranchTarget(i, pNew)); //Need to continue as several branches of the switch might point to the same location } } - - CLogging::LogMessage(_T("End CSwitchInstruction::SetBranchTarget")); - return hr; } @@ -1360,24 +1162,17 @@ HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::ReplaceBranchTarget( HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::GetBranchCount(_Out_ DWORD* pBranchCount) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CSwitchInstruction::GetBranchCount")); - IfNullRetPointer(pBranchCount); *pBranchCount = (DWORD)(m_branchTargets.size()); - - CLogging::LogMessage(_T("End CSwitchInstruction::GetBranchCount")); - return hr; } HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::GetBranchOffset(_In_ DWORD index, _In_ DWORD* pdwOffset) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CSwitchInstruction::GetBranchCount")); - IfNullRetPointer(pdwOffset); - CComPtr pInstruction = m_branchTargets[index]; + CComPtr pInstruction = m_branchTargets[index]; if (pInstruction != NULL) { IfFailRet(pInstruction->GetOffset(pdwOffset)); @@ -1387,16 +1182,11 @@ HRESULT MicrosoftInstrumentationEngine::CSwitchInstruction::GetBranchOffset(_In_ // The offsets encoded in the instruction stream are relative to this instruction *pdwOffset = m_origBranchTargetOffsets[index]; } - - CLogging::LogMessage(_T("End CSwitchInstruction::GetBranchCount")); - return hr; } MicrosoftInstrumentationEngine::CLoadConstInstruction::CLoadConstInstruction(_In_ int value) : COperandInstruction(Cee_Ldc_I4, TRUE) { - CLogging::LogMessage(_T("Starting CLoadConstInstruction::CLoadConstInstruction")); - // Check if the instruction can can be made a one byte // where the value of the constant is implied by the opcode if (value >= -1 && value <= 8) @@ -1413,16 +1203,12 @@ MicrosoftInstrumentationEngine::CLoadConstInstruction::CLoadConstInstruction(_In { m_value.i = value; } - - CLogging::LogMessage(_T("End CLoadConstInstruction::CLoadConstInstruction")); } MicrosoftInstrumentationEngine::CLoadLocalInstruction::CLoadLocalInstruction( _In_ USHORT index ) : COperandInstruction(Cee_Ldloc, TRUE) { - CLogging::LogMessage(_T("Starting CLoadLocalInstruction::CLoadLocalInstruction")); - // Check if the instruction can can be made a one byte if (index < 4) { @@ -1439,8 +1225,6 @@ MicrosoftInstrumentationEngine::CLoadLocalInstruction::CLoadLocalInstruction( m_opcode = Cee_Ldloc; m_value.i = index; } - - CLogging::LogMessage(_T("End CLoadLocalInstruction::CLoadLocalInstruction")); } @@ -1449,8 +1233,6 @@ MicrosoftInstrumentationEngine::CLoadLocalAddrInstruction::CLoadLocalAddrInstruc _In_ USHORT index ) : COperandInstruction(Cee_Ldloca, TRUE) { - CLogging::LogMessage(_T("Starting CLoadLocalAddrInstruction::CLoadLocalAddrInstruction")); - // Check if the instruction can can be made a one byte if (index <= 0xff) { @@ -1462,16 +1244,12 @@ MicrosoftInstrumentationEngine::CLoadLocalAddrInstruction::CLoadLocalAddrInstruc m_opcode = Cee_Ldloca; m_value.i = index; } - - CLogging::LogMessage(_T("End CLoadLocalAddrInstruction::CLoadLocalAddrInstruction")); } MicrosoftInstrumentationEngine::CStoreLocalInstruction::CStoreLocalInstruction( _In_ USHORT index ) : COperandInstruction(Cee_Stloc, TRUE) { - CLogging::LogMessage(_T("Starting CLoadLocalAddrInstruction::CStoreLocalInstruction")); - // Check if the instruction can can be made a one byte if (index < 4) { @@ -1488,8 +1266,6 @@ MicrosoftInstrumentationEngine::CStoreLocalInstruction::CStoreLocalInstruction( m_opcode = Cee_Stloc; m_value.i = index; } - - CLogging::LogMessage(_T("End CLoadLocalAddrInstruction::CStoreLocalInstruction")); } @@ -1497,8 +1273,6 @@ MicrosoftInstrumentationEngine::CLoadArgInstruction::CLoadArgInstruction( _In_ USHORT index ) : COperandInstruction(Cee_Ldarg, TRUE) { - CLogging::LogMessage(_T("Starting CLoadLocalAddrInstruction::CLoadArgInstruction")); - // Check if the instruction can can be made a one byte if (index < 4) { @@ -1515,16 +1289,12 @@ MicrosoftInstrumentationEngine::CLoadArgInstruction::CLoadArgInstruction( m_opcode = Cee_Ldarg; m_value.i = (BYTE) index; } - - CLogging::LogMessage(_T("End CLoadLocalAddrInstruction::CLoadArgInstruction")); } MicrosoftInstrumentationEngine::CLoadArgAddrInstruction::CLoadArgAddrInstruction( _In_ USHORT index ) : COperandInstruction(Cee_Ldarga, TRUE) { - CLogging::LogMessage(_T("Starting CLoadLocalAddrInstruction::CLoadArgAddrInstruction")); - // Check if the instruction can can be 2 bytes if (index <= 0xff) { @@ -1536,8 +1306,6 @@ MicrosoftInstrumentationEngine::CLoadArgAddrInstruction::CLoadArgAddrInstruction m_opcode = Cee_Ldarga; m_value.i = (BYTE) index; } - - CLogging::LogMessage(_T("End CLoadLocalAddrInstruction::CLoadArgAddrInstruction")); } // Return the instruction's impact on the execution stack. @@ -1796,6 +1564,13 @@ HRESULT MicrosoftInstrumentationEngine::CInstruction::Disconnect() { m_pOriginalNextInstruction.Release(); } + + CComPtr pBranchTargetInfo; + if (SUCCEEDED(CBranchTargetInfo::GetInstance(this, &pBranchTargetInfo))) + { + pBranchTargetInfo->Disconnect(); + } + return S_OK; } diff --git a/src/InstrumentationEngine/Instruction.h b/src/InstrumentationEngine/Instruction.h index a4dfabce..116d1d3c 100644 --- a/src/InstrumentationEngine/Instruction.h +++ b/src/InstrumentationEngine/Instruction.h @@ -35,6 +35,7 @@ namespace MicrosoftInstrumentationEngine // CONSIDER: Store the original length as well? The graph immediately changes short branches etc... ILOrdinalOpcode m_opcode; + DWORD m_offset; DWORD m_origOffset; InstructionGeneration m_instructionGeneration; @@ -68,11 +69,16 @@ namespace MicrosoftInstrumentationEngine DEFINE_DELEGATED_REFCOUNT_RELEASE(CInstruction); STDMETHOD(QueryInterface)(_In_ REFIID riid, _Out_ void **ppvObject) override { - return QueryInterfaceInternal( - riid, - ppvObject, - static_cast(this), - static_cast(this)); + if (FAILED(QueryBaseType(riid, ppvObject))) + { + return ImplQueryInterface( + static_cast(this), + static_cast(this), + riid, + ppvObject + ); + } + return S_OK; } public: @@ -85,8 +91,6 @@ namespace MicrosoftInstrumentationEngine HRESULT SetGraph(_In_ CInstructionGraph* pGraph); constexpr CInstructionGraph* GetGraph() { return m_pGraph; } - static HRESULT GetInstructionSize(_In_ IInstruction* pInstruction, _Out_ DWORD* pdwSize); - public: virtual HRESULT __stdcall GetOffset(_Out_ DWORD* pdwOffset) override; virtual HRESULT __stdcall GetOriginalOffset(_Out_ DWORD* pdwOffset) override; @@ -112,8 +116,6 @@ namespace MicrosoftInstrumentationEngine // set to true of this instruction has been removed or replaced from in the instruction stream. virtual HRESULT __stdcall GetIsRemoved(_Out_ BOOL* pbValue) override; - virtual HRESULT __stdcall GetInstructionSize(_Out_ DWORD* pdwSize); - virtual HRESULT __stdcall GetIsBranch(_Out_ BOOL* pbValue) override; virtual HRESULT __stdcall GetIsSwitch(_Out_ BOOL* pbValue) override; virtual HRESULT __stdcall GetIsCallInstruction(_Out_ BOOL* pbValue) override; @@ -135,6 +137,8 @@ namespace MicrosoftInstrumentationEngine public: // NOTE: these do not fixup the rest of hte graph. Call the versions on instruction graph if you want // previous, and next to be udpated correctly + + virtual DWORD GetInstructionSize(); HRESULT SetNextInstruction(_In_opt_ CInstruction* pInstruction, _In_ bool setOrig); HRESULT SetPreviousInstruction(_In_opt_ CInstruction* pInstruction, _In_ bool setOrig); @@ -146,7 +150,6 @@ namespace MicrosoftInstrumentationEngine static HRESULT InstructionFromBytes(_In_ LPCBYTE pCode, _In_ LPCBYTE pEndOfCode, _Out_ CInstruction** ppInstruction); static HRESULT OrdinalOpcodeFromBytes(_In_reads_to_ptr_(pEndOfCode) LPCBYTE pCode, _In_ LPCBYTE pEndOfCode, _Out_ ILOrdinalOpcode* pOpcode); - HRESULT EmitIL(_In_reads_bytes_(dwcbILBuffer) BYTE* pILBuffer, _In_ DWORD dwcbILBuffer); HRESULT LogInstruction(bool ignoreTest); @@ -158,26 +161,40 @@ namespace MicrosoftInstrumentationEngine constexpr bool GetIsSwitchInternal() const { return m_opcode == Cee_Switch; } bool GetIsBranchInternal() const { return IsFlagSet(s_ilOpcodeInfo[m_opcode].m_flags, ILOpcodeFlag_Branch); } + // Attempt to cast the given IInstruction to a more specific TCast type. pInstruction may + // be null, in which case, the result will also be null. + template + static HRESULT CastTo(_In_opt_ IInstruction* pInstruction, _Outptr_result_maybenull_ TCast** pResult) + { + IfNullRet(pResult); + if (pInstruction == nullptr) + { + *pResult = nullptr; + } + + return pInstruction->QueryInterface(pResult); + } + protected: - template - HRESULT QueryInterfaceInternal(_In_ REFIID riid, _Out_ void** ppvObject, Args... pThisArgs) + + template + HRESULT QueryBaseType(_In_ REFIID riid, _Out_ void** ppvObject) { // Workaround for the diamond inheritence problem. // Both CDataContainer and IInstruction inherit from IUnknown and // ImplQueryInterface() doesn't know which one to pick when casting CInstruction* to IUnknown*. - // A more comprehensive fix will be covered by https://github.com/microsoft/CLRInstrumentationEngine/issues/311 - if (__uuidof(CInstruction) == riid && ppvObject != nullptr) + // A more comprehensive fix will be covered by https://github.com/microsoft/CLRInstrumentationEngine/issues/311 + + IfNullRet(ppvObject); + + if (__uuidof(TBase) == riid) { - *ppvObject = static_cast(this); + *ppvObject = static_cast(this); AddRef(); return S_OK; } - return ImplQueryInterface( - std::forward(pThisArgs)..., - riid, - ppvObject - ); + return E_NOINTERFACE; } private: @@ -241,12 +258,18 @@ namespace MicrosoftInstrumentationEngine DEFINE_DELEGATED_REFCOUNT_RELEASE(COperandInstruction); STDMETHOD(QueryInterface)(_In_ REFIID riid, _Out_ void **ppvObject) override { - return QueryInterfaceInternal( - riid, - ppvObject, - static_cast(this), - static_cast(this), - static_cast(this)); + if (FAILED(QueryBaseType(riid, ppvObject))) + { + if FAILED(CInstruction::QueryInterface(riid, ppvObject)) + { + return ImplQueryInterface( + static_cast(this), + riid, + ppvObject + ); + } + } + return S_OK; } // IOperandInstruction methods @@ -278,12 +301,12 @@ namespace MicrosoftInstrumentationEngine DEFINE_DELEGATED_REFCOUNT_RELEASE(CLoadConstInstruction); STDMETHOD(QueryInterface)(_In_ REFIID riid, _Out_ void **ppvObject) override { - return QueryInterfaceInternal( - riid, - ppvObject, - static_cast((COperandInstruction*)this), - static_cast((CInstruction*)this), - static_cast(this)); + if (FAILED(QueryBaseType(riid, ppvObject))) + { + return COperandInstruction::QueryInterface(riid, ppvObject); + } + + return S_OK; } }; @@ -296,16 +319,16 @@ namespace MicrosoftInstrumentationEngine ); public: - DEFINE_DELEGATED_REFCOUNT_ADDREF(CLoadConstInstruction); - DEFINE_DELEGATED_REFCOUNT_RELEASE(CLoadConstInstruction); + DEFINE_DELEGATED_REFCOUNT_ADDREF(CLoadLocalInstruction); + DEFINE_DELEGATED_REFCOUNT_RELEASE(CLoadLocalInstruction); STDMETHOD(QueryInterface)(_In_ REFIID riid, _Out_ void **ppvObject) override { - return QueryInterfaceInternal( - riid, - ppvObject, - static_cast((COperandInstruction*)this), - static_cast((CInstruction*)this), - static_cast(this)); + if (FAILED(QueryBaseType(riid, ppvObject))) + { + return COperandInstruction::QueryInterface(riid, ppvObject); + } + + return S_OK; } }; @@ -322,12 +345,12 @@ namespace MicrosoftInstrumentationEngine DEFINE_DELEGATED_REFCOUNT_RELEASE(CLoadLocalAddrInstruction); STDMETHOD(QueryInterface)(_In_ REFIID riid, _Out_ void **ppvObject) override { - return QueryInterfaceInternal( - riid, - ppvObject, - static_cast((COperandInstruction*)this), - static_cast((CInstruction*)this), - static_cast(this)); + if (FAILED(QueryBaseType(riid, ppvObject))) + { + return COperandInstruction::QueryInterface(riid, ppvObject); + } + + return S_OK; } }; @@ -345,12 +368,12 @@ namespace MicrosoftInstrumentationEngine DEFINE_DELEGATED_REFCOUNT_RELEASE(CStoreLocalInstruction); STDMETHOD(QueryInterface)(_In_ REFIID riid, _Out_ void **ppvObject) override { - return QueryInterfaceInternal( - riid, - ppvObject, - static_cast((COperandInstruction*)this), - static_cast((CInstruction*)this), - static_cast(this)); + if (FAILED(QueryBaseType(riid, ppvObject))) + { + return COperandInstruction::QueryInterface(riid, ppvObject); + } + + return S_OK; } }; @@ -367,12 +390,12 @@ namespace MicrosoftInstrumentationEngine DEFINE_DELEGATED_REFCOUNT_RELEASE(CLoadArg); STDMETHOD(QueryInterface)(_In_ REFIID riid, _Out_ void **ppvObject) override { - return QueryInterfaceInternal( - riid, - ppvObject, - static_cast((COperandInstruction*)this), - static_cast((CInstruction*)this), - static_cast(this)); + if (FAILED(QueryBaseType(riid, ppvObject))) + { + return COperandInstruction::QueryInterface(riid, ppvObject); + } + + return S_OK; } }; @@ -389,12 +412,12 @@ namespace MicrosoftInstrumentationEngine DEFINE_DELEGATED_REFCOUNT_RELEASE(CLoadArgAddrInstruction); STDMETHOD(QueryInterface)(_In_ REFIID riid, _Out_ void **ppvObject) override { - return QueryInterfaceInternal( - riid, - ppvObject, - static_cast((COperandInstruction*)this), - static_cast((CInstruction*)this), - static_cast(this)); + if (FAILED(QueryBaseType(riid, ppvObject))) + { + return COperandInstruction::QueryInterface(riid, ppvObject); + } + + return S_OK; } }; @@ -405,8 +428,8 @@ namespace MicrosoftInstrumentationEngine // NOTE: This is the target offset for the original decoded instruction. // It should not be used if m_pBranchTarget has been set. DWORD m_decodedTargetOffset; - CComPtr m_pBranchTarget; - CComPtr m_pOrigBranchTarget; + CComPtr m_pBranchTarget; + CComPtr m_pOrigBranchTarget; public: CBranchInstruction( @@ -424,12 +447,20 @@ namespace MicrosoftInstrumentationEngine DEFINE_DELEGATED_REFCOUNT_RELEASE(CILBranch); STDMETHOD(QueryInterface)(_In_ REFIID riid, _Out_ void **ppvObject) override { - return QueryInterfaceInternal( - riid, - ppvObject, - static_cast(this), - static_cast((CInstruction*)this), - static_cast(this)); + if (FAILED(QueryBaseType(riid, ppvObject))) + { + if (FAILED(CInstruction::QueryInterface(riid, ppvObject))) + { + return ImplQueryInterface( + static_cast(this), + riid, + ppvObject + ); + } + + } + + return S_OK; } // IBranchInstruction methods @@ -464,19 +495,13 @@ namespace MicrosoftInstrumentationEngine // Vector of actual branch targets. Offsets etc... should be read from here // after the initialization phase is over. - std::vector> m_branchTargets; + std::vector> m_branchTargets; public: - CSwitchInstruction( - _In_ ILOrdinalOpcode opcode, - _In_ bool isNew - ); - CSwitchInstruction( _In_ ILOrdinalOpcode opcode, _In_ bool isNew, - _In_ DWORD cBranchTargets, - _In_reads_(cBranchTargets) IInstruction** ppBranchTargets + _In_ DWORD initialCount = 0 ); virtual HRESULT InitializeFromBytes( @@ -484,17 +509,27 @@ namespace MicrosoftInstrumentationEngine _In_ LPCBYTE pEndOfCode ) override; + virtual DWORD GetInstructionSize() override; + public: DEFINE_DELEGATED_REFCOUNT_ADDREF(CSwitchInstruction); DEFINE_DELEGATED_REFCOUNT_RELEASE(CSwitchInstruction); STDMETHOD(QueryInterface)(_In_ REFIID riid, _Out_ void **ppvObject) override { - return QueryInterfaceInternal( - riid, - ppvObject, - static_cast(this), - static_cast((CInstruction*)this), - static_cast(this)); + if (FAILED(QueryBaseType(riid, ppvObject))) + { + if (FAILED(CInstruction::QueryInterface(riid, ppvObject))) + { + return ImplQueryInterface( + static_cast(this), + riid, + ppvObject + ); + } + + } + + return S_OK; } public: diff --git a/src/InstrumentationEngine/InstructionFactory.cpp b/src/InstrumentationEngine/InstructionFactory.cpp index dadd4156..8e9bac81 100644 --- a/src/InstrumentationEngine/InstructionFactory.cpp +++ b/src/InstrumentationEngine/InstructionFactory.cpp @@ -219,12 +219,19 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionFactory::CreateSwitchInstruc IfNullRetPointer(ppInstruction); CComPtr pInstruction; - pInstruction.Attach(new CSwitchInstruction(opcode, TRUE, cBranchTargets, ppBranchTargets)); + pInstruction.Attach(new CSwitchInstruction(opcode, TRUE, cBranchTargets)); if (pInstruction == NULL) { return E_OUTOFMEMORY; } + for (DWORD i = 0; i < cBranchTargets; i++) + { + CComPtr pTarget; + IfFailRet(ppBranchTargets[i]->QueryInterface(&pTarget)); + IfFailRet(pInstruction->SetBranchTarget(i, ppBranchTargets[i])); + } + *ppInstruction = (IInstruction*)(pInstruction.p); (*ppInstruction)->AddRef(); diff --git a/src/InstrumentationEngine/InstructionGraph.cpp b/src/InstrumentationEngine/InstructionGraph.cpp index b588a7c8..59adbae5 100644 --- a/src/InstrumentationEngine/InstructionGraph.cpp +++ b/src/InstrumentationEngine/InstructionGraph.cpp @@ -4,6 +4,7 @@ #include "stdafx.h" #include "Instruction.h" #include "InstructionGraph.h" +#include "BranchTargetInfo.h" MicrosoftInstrumentationEngine::CInstructionGraph::CInstructionGraph() : m_pMethodInfo(NULL), m_bHasBaselineBeenSet(false), m_bAreInstructionsStale(true) { @@ -265,9 +266,7 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::DecodeInstructions(_I pPrevious = pNewInstr.p; // Jump to the next instruction - DWORD instructionSize = 0; - IfFailRet(pNewInstr->GetInstructionSize(&instructionSize)); - pCode += instructionSize; + pCode += pNewInstr->GetInstructionSize(); if (pCode >= pEndOfCode) { @@ -382,8 +381,7 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::EncodeIL( CInstruction* pInstruction = m_pFirstInstruction; while (pInstruction != NULL) { - DWORD dwInstructionSize = 0; - IfFailRet(pInstruction->GetInstructionSize(&dwInstructionSize)); + DWORD dwInstructionSize = pInstruction->GetInstructionSize(); BOOL bIsNewInstruction = FALSE; pInstruction->GetIsNew(&bIsNewInstruction); @@ -490,7 +488,7 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::EncodeIL( return S_OK; } -HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::GetInstructionAtEndOffset(_In_ DWORD offset, _Out_ IInstruction** ppInstruction) +HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::GetInstructionAtEndOffset(_In_ DWORD offset, _Out_ CInstruction** ppInstruction) { HRESULT hr = S_OK; CCriticalSectionHolder lock(&m_cs); @@ -501,8 +499,7 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::GetInstructionAtEndOf DWORD currOffset = 0; IfFailRet(pCurrent->GetOffset(&currOffset)); - DWORD currSize = 0; - IfFailRet(pCurrent->GetInstructionSize(&currSize)); + DWORD currSize = pCurrent->GetInstructionSize(); if ((currOffset + currSize) == offset) { @@ -533,7 +530,7 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::CalculateInstructionO if (pPrev != NULL) { IfFailRet(pPrev->GetOffset(&dwPrevOffset)); - IfFailRet(pPrev->GetInstructionSize(&dwPrevLength)); + dwPrevLength = pPrev->GetInstructionSize(); } IfFailRet(pCurrent->SetOffset(dwPrevOffset + dwPrevLength)); @@ -705,9 +702,18 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::GetUninstrumentedLast } HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::GetInstructionAtOffset(_In_ DWORD offset, _Out_ IInstruction** ppInstruction) +{ + HRESULT hr; + IfNullRetPointer(ppInstruction); + CComPtr pResult; + IfFailRet(GetInstructionAtOffsetInternal(offset, &pResult)); + *ppInstruction = pResult.Detach(); + return S_OK; +} + +HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::GetInstructionAtOffsetInternal(_In_ DWORD offset, _Out_ CInstruction** ppInstruction) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstructionGraph::GetInstructionAtOffset")); CCriticalSectionHolder lock(&m_cs); IfNullRetPointer(ppInstruction); @@ -729,15 +735,22 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::GetInstructionAtOffse pCurrInstruction = pCurrInstruction->NextInstructionInternal(); } - CLogging::LogMessage(_T("End CInstructionGraph::GetInstructionAtOffset")); - return E_FAIL; } HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::GetInstructionAtOriginalOffset(_In_ DWORD offset, _Out_ IInstruction** ppInstruction) +{ + IfNullRetPointer(ppInstruction); + HRESULT hr = S_OK; + CComPtr pResult; + IfFailRet(GetInstructionAtOriginalOffsetInternal(offset, &pResult)); + *ppInstruction = pResult.Detach(); + return S_OK; +} + +HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::GetInstructionAtOriginalOffsetInternal(_In_ DWORD offset, _Out_ CInstruction** ppInstruction) { HRESULT hr = S_OK; - CLogging::LogMessage(_T("Starting CInstructionGraph::GetInstructionAtOriginalOffset")); CCriticalSectionHolder lock(&m_cs); IfNullRetPointer(ppInstruction); @@ -767,8 +780,6 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::GetInstructionAtOrigi } } - CLogging::LogMessage(_T("End CInstructionGraph::GetInstructionAtOriginalOffset")); - return E_FAIL; } @@ -924,40 +935,7 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::InsertBeforeAndRetarg // Search the graph for any branch whose branch target is the old instruction. Set the branch target // to the new instruction. - CInstruction* pCurr = m_pFirstInstruction; - while (pCurr != nullptr) - { - bool isBranch = pCurr->GetIsBranchInternal(); - - if (isBranch) - { - CComPtr pBranch = (CBranchInstruction*)pCurr; - - const IInstruction* pBranchTarget = pBranch->GetBranchTargetInternal(); - - // If the branch target is the original instruction AND the branch is not the new instruction, - // update it. Note the second condition is important because for things like leave instructions, - // often the next instruction is the branch target and resetting this would create an infinite loop. - if (pBranchTarget == pInstructionOrig && pCurr != pInstructionNew) - { - IfFailRet(pBranch->SetBranchTarget(pInstructionNew)); - } - } - else - { - bool isSwitch = pCurr->GetIsSwitchInternal(); - - if (isSwitch) - { - CComPtr pSwitch; - IfFailRet(pCurr->QueryInterface(__uuidof(ISwitchInstruction), (LPVOID*)&pSwitch)); - IfFailRet(pSwitch->ReplaceBranchTarget(pInstructionOrig, pInstructionNew)); - } - - } - - pCurr = pCurr->NextInstructionInternal(); - } + IfFailRet(CBranchTargetInfo::RetargetBranches(pInstrOrig, pInstrNew)); // Search the exception clauses for anything that points to the old instruction // Set that target to the new instruction @@ -1123,28 +1101,14 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::RemoveAll() CLogging::LogMessage(_T("Starting CInstructionGraph::RemoveAll")); CCriticalSectionHolder lock(&m_cs); - CInstruction* pInstr = (CInstruction*)m_pFirstInstruction; + CInstruction* pInstr = m_pFirstInstruction.p; while (pInstr != NULL) { pInstr->SetIsRemoved(); - - CInstruction* pPreviousInstruction = pInstr->PreviousInstructionInternal(); CInstruction* pNextInstruction = pInstr->NextInstructionInternal(); - - if (pPreviousInstruction != NULL) - { - IfFailRet(pPreviousInstruction->SetNextInstruction(NULL, false)); - } - - if (pNextInstruction != NULL) - { - IfFailRet(pNextInstruction->SetPreviousInstruction(NULL, false)); - } - IfFailRet(pInstr->SetNextInstruction(NULL, false)); IfFailRet(pInstr->SetPreviousInstruction(NULL, false)); - pInstr = pNextInstruction; } @@ -1178,7 +1142,7 @@ HRESULT MicrosoftInstrumentationEngine::CInstructionGraph::CalculateMaxStack(_Ou // is not taken into account. It makes a linear pass over the instructions looking at // the stack impact of each instruction and maintaining a theoretical maximum. // The actual maximum may be slightly smaller. - CInstruction* pInstr = (CInstruction*)m_pFirstInstruction; + CInstruction* pInstr = m_pFirstInstruction.p; while (pInstr != NULL) { CInstruction* pNextInstruction = pInstr->NextInstructionInternal(); diff --git a/src/InstrumentationEngine/InstructionGraph.h b/src/InstrumentationEngine/InstructionGraph.h index cc843649..b8e68c32 100644 --- a/src/InstrumentationEngine/InstructionGraph.h +++ b/src/InstrumentationEngine/InstructionGraph.h @@ -71,12 +71,15 @@ namespace MicrosoftInstrumentationEngine HRESULT CalculateInstructionOffsets(); - HRESULT GetInstructionAtEndOffset(_In_ DWORD offset, _Out_ IInstruction** ppInstruction); + HRESULT GetInstructionAtEndOffset(_In_ DWORD offset, _Out_ CInstruction** ppInstruction); HRESULT CalculateMaxStack(_Out_ DWORD* pMaxStack); constexpr CInstruction* FirstInstructionInternal() { return m_pFirstInstruction.p; } constexpr CInstruction* OriginalFirstInstructionInternal() { return m_pOrigFirstInstruction.p; } + HRESULT GetInstructionAtOffsetInternal(_In_ DWORD offset, _Out_ CInstruction** ppInstruction); + HRESULT GetInstructionAtOriginalOffsetInternal(_In_ DWORD offset, _Out_ CInstruction** ppInstruction); + HRESULT RefreshInstructions(); // IInstructionGraph methods diff --git a/src/InstrumentationEngine/InstrumentationEngine.vcxproj b/src/InstrumentationEngine/InstrumentationEngine.vcxproj index f61f3f66..31dfe37e 100644 --- a/src/InstrumentationEngine/InstrumentationEngine.vcxproj +++ b/src/InstrumentationEngine/InstrumentationEngine.vcxproj @@ -117,6 +117,7 @@ + diff --git a/tests/InstrEngineTests/InstrEngineTests/ProfilerHelpers.cs b/tests/InstrEngineTests/InstrEngineTests/ProfilerHelpers.cs index b58dea61..7888998b 100644 --- a/tests/InstrEngineTests/InstrEngineTests/ProfilerHelpers.cs +++ b/tests/InstrEngineTests/InstrEngineTests/ProfilerHelpers.cs @@ -11,6 +11,7 @@ using System.Text; using System.Text.RegularExpressions; using System.Xml; +using System.Runtime.InteropServices; namespace InstrEngineTests { @@ -46,6 +47,8 @@ internal class ProfilerHelpers // startup to allow attaching a debugger. private static bool ThrowMessageBoxAtStartup = false; + private static bool WaitForDebugger = false; + private static bool BinaryRecompiled = false; // Enable ref recording to track down memory leaks. For debug only. @@ -102,6 +105,11 @@ public static void LaunchAppUnderProfiler(string testApp, string testScript, str psi.EnvironmentVariables.Add("MicrosoftInstrumentationEngine_MessageboxAtAttach", @"1"); } + if (WaitForDebugger) + { + psi.EnvironmentVariables.Add("MicrosoftInstrumentationEngine_DebugWait", @"1"); + } + if (TestParameters.DisableMethodSignatureValidation) { psi.EnvironmentVariables.Add("MicrosoftInstrumentationEngine_DisableCodeSignatureValidation", @"1"); @@ -417,10 +425,8 @@ private static void SetBitness(bool isWow64) flag |= COMPLUS_ENABLE_64BIT; } - if(!NativeMethods.SetComPlusPackageInstallStatus(flag)) - { - throw new InvalidOperationException(); - } + // safely ignore the result. + _ = NativeMethods.SetComPlusPackageInstallStatus(flag); } } }