From 725337df266c8f1ab049215c2c78fefa41680722 Mon Sep 17 00:00:00 2001 From: baldurk Date: Wed, 22 Nov 2017 14:55:24 +0000 Subject: [PATCH] Clarify some iterator checks (where asserts were used before) * Reported by Coverity Scan - mostly this is just adding error checking where there was previously just an assert before a use of an invalid iterator. --- renderdoc/driver/d3d11/d3d11_context_wrap.cpp | 13 +- .../driver/d3d12/d3d12_command_list_wrap.cpp | 108 +++++---- renderdoc/driver/d3d12/d3d12_commands.cpp | 13 +- renderdoc/driver/d3d12/d3d12_counters.cpp | 18 +- renderdoc/driver/d3d12/d3d12_device.cpp | 5 +- .../driver/shaders/dxbc/dxbc_inspect.cpp | 16 +- renderdoc/driver/vulkan/vk_core.cpp | 19 +- renderdoc/driver/vulkan/vk_counters.cpp | 18 +- .../driver/vulkan/wrappers/vk_draw_funcs.cpp | 221 +++++++++--------- 9 files changed, 247 insertions(+), 184 deletions(-) diff --git a/renderdoc/driver/d3d11/d3d11_context_wrap.cpp b/renderdoc/driver/d3d11/d3d11_context_wrap.cpp index 341512aa6..6fbcd897b 100644 --- a/renderdoc/driver/d3d11/d3d11_context_wrap.cpp +++ b/renderdoc/driver/d3d11/d3d11_context_wrap.cpp @@ -6929,9 +6929,16 @@ bool WrappedID3D11DeviceContext::Serialise_Unmap(SerialiserType &ser, ID3D11Reso // locate the intercept data and remove it from the open maps list auto it = m_OpenMaps.find(mapIdx); - RDCASSERT(it != m_OpenMaps.end()); - intercept = it->second; - m_OpenMaps.erase(it); + + if(it != m_OpenMaps.end()) + { + intercept = it->second; + m_OpenMaps.erase(it); + } + else + { + RDCERR("Couldn't find map for %llu/%u in open maps list", mapIdx.resource, mapIdx.subresource); + } MapWrittenData = (byte *)intercept.app.pData; diff --git a/renderdoc/driver/d3d12/d3d12_command_list_wrap.cpp b/renderdoc/driver/d3d12/d3d12_command_list_wrap.cpp index e9a624c70..33ef39f68 100644 --- a/renderdoc/driver/d3d12/d3d12_command_list_wrap.cpp +++ b/renderdoc/driver/d3d12/d3d12_command_list_wrap.cpp @@ -3614,61 +3614,67 @@ void WrappedID3D12GraphicsCommandList::ReplayExecuteIndirect(ID3D12GraphicsComma D3D12CommandData::DrawcallUse use(m_Cmd->m_CurChunkOffset, 0); auto it = std::lower_bound(m_Cmd->m_DrawcallUses.begin(), m_Cmd->m_DrawcallUses.end(), use); - RDCASSERT(it != m_Cmd->m_DrawcallUses.end()); - - uint32_t baseEventID = it->eventID; - - // TODO when re-recording all, we should submit every drawcall individually - if(m_Cmd->m_DrawcallCallback && m_Cmd->m_DrawcallCallback->RecordAllCmds()) + if(it == m_Cmd->m_DrawcallUses.end()) { - firstCommand = 0; - firstArg = 0; - lastArg = ~0U; - } - // To add the execute, we made an event N that is the 'parent' marker, then - // N+1, N+2, N+3, ... for each of the arguments. If the first sub-argument is selected - // then we'll replay up to N but not N+1, so just do nothing - we DON'T want to draw - // the first sub-draw in that range. - else if(m_Cmd->m_LastEventID > baseEventID) - { - if(m_Cmd->m_FirstEventID <= 1) - { - // one event per arg, and N args per command - uint32_t numArgs = m_Cmd->m_LastEventID - baseEventID; - - // play all commands up to the one we want - firstCommand = 0; - - // how many commands? - uint32_t numCmds = numArgs / sigSize + 1; - count = RDCMIN(count, numCmds); - - // play all args in the fnial commmad up to the one we want - firstArg = 0; - - // how many args in the final command - if(numCmds > count) - lastArg = ~0U; - else - lastArg = numArgs % sigSize; - } - else - { - // note we'll never be asked to do e.g. 3rd-7th commands of an execute. Only ever 0th-nth or - // a single argument. - uint32_t argIdx = (curEID - baseEventID - 1); - - firstCommand = argIdx / sigSize; - count = RDCMIN(count, firstCommand + 1); - - firstArg = argIdx % sigSize; - lastArg = firstArg + 1; - } + RDCERR("Unexpected drawcall not found in uses vector, offset %llu", m_Cmd->m_CurChunkOffset); } else { - // don't do anything, we've selected the base event - count = 0; + uint32_t baseEventID = it->eventID; + + // TODO when re-recording all, we should submit every drawcall individually + if(m_Cmd->m_DrawcallCallback && m_Cmd->m_DrawcallCallback->RecordAllCmds()) + { + firstCommand = 0; + firstArg = 0; + lastArg = ~0U; + } + // To add the execute, we made an event N that is the 'parent' marker, then + // N+1, N+2, N+3, ... for each of the arguments. If the first sub-argument is selected + // then we'll replay up to N but not N+1, so just do nothing - we DON'T want to draw + // the first sub-draw in that range. + else if(m_Cmd->m_LastEventID > baseEventID) + { + if(m_Cmd->m_FirstEventID <= 1) + { + // one event per arg, and N args per command + uint32_t numArgs = m_Cmd->m_LastEventID - baseEventID; + + // play all commands up to the one we want + firstCommand = 0; + + // how many commands? + uint32_t numCmds = numArgs / sigSize + 1; + count = RDCMIN(count, numCmds); + + // play all args in the fnial commmad up to the one we want + firstArg = 0; + + // how many args in the final command + if(numCmds > count) + lastArg = ~0U; + else + lastArg = numArgs % sigSize; + } + else + { + // note we'll never be asked to do e.g. 3rd-7th commands of an execute. Only ever 0th-nth + // or + // a single argument. + uint32_t argIdx = (curEID - baseEventID - 1); + + firstCommand = argIdx / sigSize; + count = RDCMIN(count, firstCommand + 1); + + firstArg = argIdx % sigSize; + lastArg = firstArg + 1; + } + } + else + { + // don't do anything, we've selected the base event + count = 0; + } } } diff --git a/renderdoc/driver/d3d12/d3d12_commands.cpp b/renderdoc/driver/d3d12/d3d12_commands.cpp index 1f8ad8744..5684b236a 100644 --- a/renderdoc/driver/d3d12/d3d12_commands.cpp +++ b/renderdoc/driver/d3d12/d3d12_commands.cpp @@ -933,7 +933,12 @@ uint32_t D3D12CommandData::HandlePreCallback(ID3D12GraphicsCommandList *list, bo // look up the EID this drawcall came from DrawcallUse use(m_CurChunkOffset, 0); auto it = std::lower_bound(m_DrawcallUses.begin(), m_DrawcallUses.end(), use); - RDCASSERT(it != m_DrawcallUses.end()); + + if(it == m_DrawcallUses.end()) + { + RDCERR("Couldn't find drawcall use entry for %llu", m_CurChunkOffset); + return 0; + } uint32_t eventID = it->eventID; @@ -1003,7 +1008,11 @@ ID3D12GraphicsCommandList *D3D12CommandData::RerecordCmdList(ResourceId cmdid, { auto it = m_RerecordCmds.find(cmdid); - RDCASSERT(it != m_RerecordCmds.end()); + if(it == m_RerecordCmds.end()) + { + RDCERR("Didn't generate re-record command for %llu", cmdid); + return NULL; + } return it->second; } diff --git a/renderdoc/driver/d3d12/d3d12_counters.cpp b/renderdoc/driver/d3d12/d3d12_counters.cpp index 91578c4ce..3d2e79adc 100644 --- a/renderdoc/driver/d3d12/d3d12_counters.cpp +++ b/renderdoc/driver/d3d12/d3d12_counters.cpp @@ -453,12 +453,18 @@ vector D3D12Replay::FetchCounters(const vector &count // find the result we're aliasing auto it = std::find(ret.begin(), ret.end(), search); - RDCASSERT(it != ret.end()); - - // duplicate the result and append - CounterResult aliased = *it; - aliased.eventID = cb.m_AliasEvents[i].second; - ret.push_back(aliased); + if(it != ret.end()) + { + // duplicate the result and append + CounterResult aliased = *it; + aliased.eventID = cb.m_AliasEvents[i].second; + ret.push_back(aliased); + } + else + { + RDCERR("Expected to find alias-target result for EID %u counter %u, but didn't", + search.eventID, search.counterID); + } } } diff --git a/renderdoc/driver/d3d12/d3d12_device.cpp b/renderdoc/driver/d3d12/d3d12_device.cpp index 75703e388..d8bfabbcf 100644 --- a/renderdoc/driver/d3d12/d3d12_device.cpp +++ b/renderdoc/driver/d3d12/d3d12_device.cpp @@ -1723,7 +1723,10 @@ void WrappedID3D12Device::AddDebugMessage(MessageCategory c, MessageSeverity sv, auto it = std::lower_bound(cmd.m_DrawcallUses.begin(), cmd.m_DrawcallUses.end(), use); RDCASSERT(it != cmd.m_DrawcallUses.end()); - msg.eventID = it->eventID; + if(it != cmd.m_DrawcallUses.end()) + msg.eventID = it->eventID; + else + RDCERR("Couldn't locate drawcall use for current chunk offset %llu", cmd.m_CurChunkOffset); } AddDebugMessage(msg); diff --git a/renderdoc/driver/shaders/dxbc/dxbc_inspect.cpp b/renderdoc/driver/shaders/dxbc/dxbc_inspect.cpp index 2b4c77f5b..2751ed04f 100644 --- a/renderdoc/driver/shaders/dxbc/dxbc_inspect.cpp +++ b/renderdoc/driver/shaders/dxbc/dxbc_inspect.cpp @@ -689,12 +689,15 @@ DXBCFile::DXBCFile(const void *ByteCode, size_t ByteCodeLength) for(vector *arr : {&m_SRVs, &m_UAVs, &m_Samplers}) { vector &resArray = *arr; - for(size_t i = 0; i < resArray.size();) + for(auto it = resArray.begin(); it != resArray.end();) { - if(resArray[i].bindCount > 1) + if(it->bindCount > 1) { - ShaderInputBind desc = resArray[i]; - resArray.erase(resArray.begin() + i); + // copy off the array item description + ShaderInputBind desc = *it; + + // remove the array item, and get the iterator to the next item to process + it = resArray.erase(it); string rname = desc.name; uint32_t arraySize = desc.bindCount; @@ -708,12 +711,11 @@ DXBCFile::DXBCFile(const void *ByteCode, size_t ByteCodeLength) desc.reg++; } - // continue from the i'th element again since - // we just removed it. continue; } - i++; + // just move on if this item wasn't arrayed + it++; } } } diff --git a/renderdoc/driver/vulkan/vk_core.cpp b/renderdoc/driver/vulkan/vk_core.cpp index c52fe2cd1..378a1830c 100644 --- a/renderdoc/driver/vulkan/vk_core.cpp +++ b/renderdoc/driver/vulkan/vk_core.cpp @@ -345,7 +345,12 @@ uint32_t WrappedVulkan::HandlePreCallback(VkCommandBuffer commandBuffer, DrawFla // look up the EID this drawcall came from DrawcallUse use(m_CurChunkOffset, 0); auto it = std::lower_bound(m_DrawcallUses.begin(), m_DrawcallUses.end(), use); - RDCASSERT(it != m_DrawcallUses.end()); + + if(it == m_DrawcallUses.end()) + { + RDCERR("Couldn't find drawcall use entry for %llu", m_CurChunkOffset); + return 0; + } uint32_t eventID = it->eventID; @@ -2342,9 +2347,11 @@ void WrappedVulkan::AddDebugMessage(MessageCategory c, MessageSeverity sv, Messa // look up the EID this drawcall came from DrawcallUse use(m_CurChunkOffset, 0); auto it = std::lower_bound(m_DrawcallUses.begin(), m_DrawcallUses.end(), use); - RDCASSERT(it != m_DrawcallUses.end()); - msg.eventID = it->eventID; + if(it != m_DrawcallUses.end()) + msg.eventID = it->eventID; + else + RDCERR("Couldn't locate drawcall use for current chunk offset %llu", m_CurChunkOffset); } msg.messageID = 0; msg.source = src; @@ -2500,7 +2507,11 @@ VkCommandBuffer WrappedVulkan::RerecordCmdBuf(ResourceId cmdid, PartialReplayInd { auto it = m_RerecordCmds.find(cmdid); - RDCASSERT(it != m_RerecordCmds.end()); + if(it == m_RerecordCmds.end()) + { + RDCERR("Didn't generate re-record command for %llu", cmdid); + return NULL; + } return it->second; } diff --git a/renderdoc/driver/vulkan/vk_counters.cpp b/renderdoc/driver/vulkan/vk_counters.cpp index 5cab36503..80fb67ed2 100644 --- a/renderdoc/driver/vulkan/vk_counters.cpp +++ b/renderdoc/driver/vulkan/vk_counters.cpp @@ -442,12 +442,18 @@ vector VulkanReplay::FetchCounters(const vector &coun // find the result we're aliasing auto it = std::find(ret.begin(), ret.end(), search); - RDCASSERT(it != ret.end()); - - // duplicate the result and append - CounterResult aliased = *it; - aliased.eventID = cb.m_AliasEvents[i].second; - ret.push_back(aliased); + if(it != ret.end()) + { + // duplicate the result and append + CounterResult aliased = *it; + aliased.eventID = cb.m_AliasEvents[i].second; + ret.push_back(aliased); + } + else + { + RDCERR("Expected to find alias-target result for EID %u counter %u, but didn't", + search.eventID, search.counterID); + } } } diff --git a/renderdoc/driver/vulkan/wrappers/vk_draw_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_draw_funcs.cpp index 863fb2316..998ac76e3 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_draw_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_draw_funcs.cpp @@ -325,68 +325,74 @@ bool WrappedVulkan::Serialise_vkCmdDrawIndirect(SerialiserType &ser, VkCommandBu DrawcallUse use(m_CurChunkOffset, 0); auto it = std::lower_bound(m_DrawcallUses.begin(), m_DrawcallUses.end(), use); - RDCASSERT(it != m_DrawcallUses.end()); - - uint32_t baseEventID = it->eventID; - - // when re-recording all, submit every drawcall individually to the callback - if(m_DrawcallCallback && m_DrawcallCallback->RecordAllCmds() && IsDrawInRenderPass()) + if(it == m_DrawcallUses.end()) { - for(uint32_t i = 0; i < count; i++) + RDCERR("Unexpected drawcall not found in uses vector, offset %llu", m_CurChunkOffset); + } + else + { + uint32_t baseEventID = it->eventID; + + // when re-recording all, submit every drawcall individually to the callback + if(m_DrawcallCallback && m_DrawcallCallback->RecordAllCmds() && IsDrawInRenderPass()) { - uint32_t eventID = HandlePreCallback(commandBuffer, DrawFlags::Drawcall, i + 1); - - ObjDisp(commandBuffer) - ->CmdDrawIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, 1, stride); - - if(eventID && m_DrawcallCallback->PostDraw(eventID, commandBuffer)) + for(uint32_t i = 0; i < count; i++) { + uint32_t eventID = HandlePreCallback(commandBuffer, DrawFlags::Drawcall, i + 1); + ObjDisp(commandBuffer) ->CmdDrawIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, 1, stride); - m_DrawcallCallback->PostRedraw(eventID, commandBuffer); + + if(eventID && m_DrawcallCallback->PostDraw(eventID, commandBuffer)) + { + ObjDisp(commandBuffer) + ->CmdDrawIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, 1, stride); + m_DrawcallCallback->PostRedraw(eventID, commandBuffer); + } + + offset += stride; + } + } + // To add the multidraw, we made an event N that is the 'parent' marker, then + // N+1, N+2, N+3, ... for each of the sub-draws. If the first sub-draw is selected + // then we'll replay up to N but not N+1, so just do nothing - we DON'T want to draw + // the first sub-draw in that range. + else if(m_LastEventID > baseEventID) + { + uint32_t drawidx = 0; + + if(m_FirstEventID <= 1) + { + // if we're replaying part-way into a multidraw, we can replay the first part + // 'easily' + // by just reducing the Count parameter to however many we want to replay. This only + // works if we're replaying from the first multidraw to the nth (n less than Count) + count = RDCMIN(count, m_LastEventID - baseEventID); + } + else + { + // otherwise we do the 'hard' case, draw only one multidraw + // note we'll never be asked to do e.g. 3rd-7th of a multidraw. Only ever 0th-nth or + // a single draw. + drawidx = (curEID - baseEventID - 1); + + offset += stride * drawidx; + count = 1; } - offset += stride; - } - } - // To add the multidraw, we made an event N that is the 'parent' marker, then - // N+1, N+2, N+3, ... for each of the sub-draws. If the first sub-draw is selected - // then we'll replay up to N but not N+1, so just do nothing - we DON'T want to draw - // the first sub-draw in that range. - else if(m_LastEventID > baseEventID) - { - uint32_t drawidx = 0; - - if(m_FirstEventID <= 1) - { - // if we're replaying part-way into a multidraw, we can replay the first part 'easily' - // by just reducing the Count parameter to however many we want to replay. This only - // works if we're replaying from the first multidraw to the nth (n less than Count) - count = RDCMIN(count, m_LastEventID - baseEventID); - } - else - { - // otherwise we do the 'hard' case, draw only one multidraw - // note we'll never be asked to do e.g. 3rd-7th of a multidraw. Only ever 0th-nth or - // a single draw. - drawidx = (curEID - baseEventID - 1); - - offset += stride * drawidx; - count = 1; - } - - if(IsDrawInRenderPass()) - { - uint32_t eventID = HandlePreCallback(commandBuffer, DrawFlags::Drawcall, drawidx + 1); - - ObjDisp(commandBuffer) - ->CmdDrawIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, count, stride); - - if(eventID && m_DrawcallCallback->PostDraw(eventID, commandBuffer)) + if(IsDrawInRenderPass()) { + uint32_t eventID = HandlePreCallback(commandBuffer, DrawFlags::Drawcall, drawidx + 1); + ObjDisp(commandBuffer) ->CmdDrawIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, count, stride); - m_DrawcallCallback->PostRedraw(eventID, commandBuffer); + + if(eventID && m_DrawcallCallback->PostDraw(eventID, commandBuffer)) + { + ObjDisp(commandBuffer) + ->CmdDrawIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, count, stride); + m_DrawcallCallback->PostRedraw(eventID, commandBuffer); + } } } } @@ -602,69 +608,76 @@ bool WrappedVulkan::Serialise_vkCmdDrawIndexedIndirect(SerialiserType &ser, DrawcallUse use(m_CurChunkOffset, 0); auto it = std::lower_bound(m_DrawcallUses.begin(), m_DrawcallUses.end(), use); - RDCASSERT(it != m_DrawcallUses.end()); - - uint32_t baseEventID = it->eventID; - - // when re-recording all, submit every drawcall individually to the callback - if(m_DrawcallCallback && m_DrawcallCallback->RecordAllCmds() && IsDrawInRenderPass()) + if(it == m_DrawcallUses.end()) { - for(uint32_t i = 0; i < count; i++) + RDCERR("Unexpected drawcall not found in uses vector, offset %llu", m_CurChunkOffset); + } + else + { + uint32_t baseEventID = it->eventID; + + // when re-recording all, submit every drawcall individually to the callback + if(m_DrawcallCallback && m_DrawcallCallback->RecordAllCmds() && IsDrawInRenderPass()) { - uint32_t eventID = HandlePreCallback(commandBuffer, DrawFlags::Drawcall, i + 1); - - ObjDisp(commandBuffer) - ->CmdDrawIndexedIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, 1, stride); - - if(eventID && m_DrawcallCallback->PostDraw(eventID, commandBuffer)) + for(uint32_t i = 0; i < count; i++) { + uint32_t eventID = HandlePreCallback(commandBuffer, DrawFlags::Drawcall, i + 1); + ObjDisp(commandBuffer) ->CmdDrawIndexedIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, 1, stride); - m_DrawcallCallback->PostRedraw(eventID, commandBuffer); + + if(eventID && m_DrawcallCallback->PostDraw(eventID, commandBuffer)) + { + ObjDisp(commandBuffer) + ->CmdDrawIndexedIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, 1, + stride); + m_DrawcallCallback->PostRedraw(eventID, commandBuffer); + } + + offset += stride; + } + } + // To add the multidraw, we made an event N that is the 'parent' marker, then + // N+1, N+2, N+3, ... for each of the sub-draws. If the first sub-draw is selected + // then we'll replay up to N but not N+1, so just do nothing - we DON'T want to draw + // the first sub-draw in that range. + else if(m_LastEventID > baseEventID) + { + uint32_t drawidx = 0; + + if(m_FirstEventID <= 1) + { + // if we're replaying part-way into a multidraw, we can replay the first part + // 'easily' + // by just reducing the Count parameter to however many we want to replay. This only + // works if we're replaying from the first multidraw to the nth (n less than Count) + count = RDCMIN(count, m_LastEventID - baseEventID); + } + else + { + // otherwise we do the 'hard' case, draw only one multidraw + // note we'll never be asked to do e.g. 3rd-7th of a multidraw. Only ever 0th-nth or + // a single draw. + drawidx = (curEID - baseEventID - 1); + + offset += stride * drawidx; + count = 1; } - offset += stride; - } - } - // To add the multidraw, we made an event N that is the 'parent' marker, then - // N+1, N+2, N+3, ... for each of the sub-draws. If the first sub-draw is selected - // then we'll replay up to N but not N+1, so just do nothing - we DON'T want to draw - // the first sub-draw in that range. - else if(m_LastEventID > baseEventID) - { - uint32_t drawidx = 0; - - if(m_FirstEventID <= 1) - { - // if we're replaying part-way into a multidraw, we can replay the first part 'easily' - // by just reducing the Count parameter to however many we want to replay. This only - // works if we're replaying from the first multidraw to the nth (n less than Count) - count = RDCMIN(count, m_LastEventID - baseEventID); - } - else - { - // otherwise we do the 'hard' case, draw only one multidraw - // note we'll never be asked to do e.g. 3rd-7th of a multidraw. Only ever 0th-nth or - // a single draw. - drawidx = (curEID - baseEventID - 1); - - offset += stride * drawidx; - count = 1; - } - - if(IsDrawInRenderPass()) - { - uint32_t eventID = HandlePreCallback(commandBuffer, DrawFlags::Drawcall, drawidx + 1); - - ObjDisp(commandBuffer) - ->CmdDrawIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, count, stride); - - if(eventID && m_DrawcallCallback->PostDraw(eventID, commandBuffer)) + if(IsDrawInRenderPass()) { + uint32_t eventID = HandlePreCallback(commandBuffer, DrawFlags::Drawcall, drawidx + 1); + ObjDisp(commandBuffer) ->CmdDrawIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, count, stride); - m_DrawcallCallback->PostRedraw(eventID, commandBuffer); + + if(eventID && m_DrawcallCallback->PostDraw(eventID, commandBuffer)) + { + ObjDisp(commandBuffer) + ->CmdDrawIndirect(Unwrap(commandBuffer), Unwrap(buffer), offset, count, stride); + m_DrawcallCallback->PostRedraw(eventID, commandBuffer); + } } } }