From 573abae522c4895adf028dce5b0c6e7d4e586324 Mon Sep 17 00:00:00 2001 From: Loko Kung Date: Thu, 14 Apr 2022 00:09:36 +0000 Subject: [PATCH] Reland "Add render pipeline cache key generation for Vulkan." - Fixes stack-buffer-overflow issue caused due to memcpy due to typing in recording fundamental array types. This is a reland of commit ca11c038241656fcd962439d0e506ae0dd9a4b99 Original change's description: > Add render pipeline cache key generation for Vulkan. > > Bug: dawn:549 > Change-Id: I0c0607984193ddcc2add05594517638e5370484d > Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/85863 > Reviewed-by: Austin Eng > Commit-Queue: Loko Kung Bug: dawn:549, dawn:1368 Change-Id: I6a62fa6e64873f2c487e9926190515cb5cc27bcf Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/86743 Reviewed-by: Austin Eng Kokoro: Kokoro Commit-Queue: Loko Kung --- src/dawn/native/CacheKey.h | 44 ++++- src/dawn/native/vulkan/CacheKeyVk.cpp | 168 +++++++++++++++++++- src/dawn/native/vulkan/CacheKeyVk.h | 22 ++- src/dawn/native/vulkan/RenderPipelineVk.cpp | 8 + 4 files changed, 229 insertions(+), 13 deletions(-) diff --git a/src/dawn/native/CacheKey.h b/src/dawn/native/CacheKey.h index 4772104195..2d58d76cb6 100644 --- a/src/dawn/native/CacheKey.h +++ b/src/dawn/native/CacheKey.h @@ -15,6 +15,9 @@ #ifndef SRC_DAWN_NATIVE_CACHEKEY_H_ #define SRC_DAWN_NATIVE_CACHEKEY_H_ +#include "dawn/common/TypedInteger.h" +#include "dawn/common/ityp_array.h" + #include #include #include @@ -66,6 +69,14 @@ namespace dawn::native { } return *this; } + template + CacheKey& RecordIterable(const ityp::array& iterable) { + Record(static_cast(iterable.size())); + for (auto it = iterable.begin(); it != iterable.end(); ++it) { + Record(*it); + } + return *this; + } template CacheKey& RecordIterable(const Ptr* ptr, size_t n) { Record(n); @@ -131,6 +142,15 @@ namespace dawn::native { } }; + // Specialized overload for TypedInteger. + template + class CacheKeySerializer<::detail::TypedIntegerImpl> { + public: + static void Serialize(CacheKey* key, const ::detail::TypedIntegerImpl t) { + CacheKeySerializer::Serialize(key, static_cast(t)); + } + }; + // Specialized overload for pointers. Since we are serializing for a cache key, we always // serialize via value, not by pointer. To handle nullptr scenarios, we always serialize whether // the pointer was nullptr followed by the contents if applicable. @@ -145,14 +165,28 @@ namespace dawn::native { } }; - // Specialized overload for string literals. - template - class CacheKeySerializer { + // Specialized overload for fixed arrays of primitives. + template + class CacheKeySerializer>> { public: - static void Serialize(CacheKey* key, const char (&t)[N]) { + static void Serialize(CacheKey* key, const T (&t)[N]) { static_assert(N > 0); key->Record(static_cast(N)); - key->insert(key->end(), t, t + N); + const char* it = reinterpret_cast(t); + key->insert(key->end(), it, it + sizeof(t)); + } + }; + + // Specialized overload for fixed arrays of non-primitives. + template + class CacheKeySerializer>> { + public: + static void Serialize(CacheKey* key, const T (&t)[N]) { + static_assert(N > 0); + key->Record(static_cast(N)); + for (size_t i = 0; i < N; i++) { + key->Record(t[i]); + } } }; diff --git a/src/dawn/native/vulkan/CacheKeyVk.cpp b/src/dawn/native/vulkan/CacheKeyVk.cpp index 9fffbd1b75..3930b54ca3 100644 --- a/src/dawn/native/vulkan/CacheKeyVk.cpp +++ b/src/dawn/native/vulkan/CacheKeyVk.cpp @@ -13,6 +13,7 @@ // limitations under the License. #include "dawn/native/vulkan/CacheKeyVk.h" +#include "dawn/native/vulkan/RenderPassCache.h" #include @@ -30,7 +31,7 @@ namespace dawn::native { CacheKey* key, const VkDescriptorSetLayoutCreateInfo& t) { key->Record(t.flags).RecordIterable(t.pBindings, t.bindingCount); - vulkan::SerializePnext<>(key, reinterpret_cast(&t)); + vulkan::SerializePnext<>(key, &t); } template <> @@ -46,7 +47,7 @@ namespace dawn::native { // The set layouts are not serialized here because they are pointers to backend objects. // They need to be cross-referenced with the frontend objects and serialized from there. key->Record(t.flags).RecordIterable(t.pPushConstantRanges, t.pushConstantRangeCount); - vulkan::SerializePnext<>(key, reinterpret_cast(&t)); + vulkan::SerializePnext<>(key, &t); } template <> @@ -78,8 +79,7 @@ namespace dawn::native { key->Record(t.flags, t.stage) .RecordIterable(t.pName, strlen(t.pName)) .Record(t.pSpecializationInfo); - vulkan::SerializePnext( - key, reinterpret_cast(&t)); + vulkan::SerializePnext(key, &t); } template <> @@ -94,4 +94,164 @@ namespace dawn::native { key->Record(t.flags, t.stage); } + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkVertexInputBindingDescription& t) { + key->Record(t.binding, t.stride, t.inputRate); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkVertexInputAttributeDescription& t) { + key->Record(t.location, t.binding, t.format, t.offset); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkPipelineVertexInputStateCreateInfo& t) { + key->Record(t.flags) + .RecordIterable(t.pVertexBindingDescriptions, t.vertexBindingDescriptionCount) + .RecordIterable(t.pVertexAttributeDescriptions, t.vertexAttributeDescriptionCount); + vulkan::SerializePnext<>(key, &t); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkPipelineInputAssemblyStateCreateInfo& t) { + key->Record(t.flags, t.topology, t.primitiveRestartEnable); + vulkan::SerializePnext<>(key, &t); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkPipelineTessellationStateCreateInfo& t) { + key->Record(t.flags, t.patchControlPoints); + vulkan::SerializePnext<>(key, &t); + } + + template <> + void CacheKeySerializer::Serialize(CacheKey* key, const VkViewport& t) { + key->Record(t.x, t.y, t.width, t.height, t.minDepth, t.maxDepth); + } + + template <> + void CacheKeySerializer::Serialize(CacheKey* key, const VkOffset2D& t) { + key->Record(t.x, t.y); + } + + template <> + void CacheKeySerializer::Serialize(CacheKey* key, const VkExtent2D& t) { + key->Record(t.width, t.height); + } + + template <> + void CacheKeySerializer::Serialize(CacheKey* key, const VkRect2D& t) { + key->Record(t.offset, t.extent); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkPipelineViewportStateCreateInfo& t) { + key->Record(t.flags) + .RecordIterable(t.pViewports, t.viewportCount) + .RecordIterable(t.pScissors, t.scissorCount); + vulkan::SerializePnext<>(key, &t); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkPipelineRasterizationStateCreateInfo& t) { + key->Record(t.flags, t.depthClampEnable, t.rasterizerDiscardEnable, t.polygonMode, + t.cullMode, t.frontFace, t.depthBiasEnable, t.depthBiasConstantFactor, + t.depthBiasClamp, t.depthBiasSlopeFactor, t.lineWidth); + vulkan::SerializePnext<>(key, &t); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkPipelineMultisampleStateCreateInfo& t) { + key->Record(t.flags, t.rasterizationSamples, t.sampleShadingEnable, t.minSampleShading, + t.pSampleMask, t.alphaToCoverageEnable, t.alphaToOneEnable); + vulkan::SerializePnext<>(key, &t); + } + + template <> + void CacheKeySerializer::Serialize(CacheKey* key, const VkStencilOpState& t) { + key->Record(t.failOp, t.passOp, t.depthFailOp, t.compareOp, t.compareMask, t.writeMask, + t.reference); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkPipelineDepthStencilStateCreateInfo& t) { + key->Record(t.flags, t.depthTestEnable, t.depthWriteEnable, t.depthCompareOp, + t.depthBoundsTestEnable, t.stencilTestEnable, t.front, t.back, t.minDepthBounds, + t.maxDepthBounds); + vulkan::SerializePnext<>(key, &t); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkPipelineColorBlendAttachmentState& t) { + key->Record(t.blendEnable, t.srcColorBlendFactor, t.dstColorBlendFactor, t.colorBlendOp, + t.srcAlphaBlendFactor, t.dstAlphaBlendFactor, t.alphaBlendOp, t.colorWriteMask); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkPipelineColorBlendStateCreateInfo& t) { + key->Record(t.flags, t.logicOpEnable, t.logicOp) + .RecordIterable(t.pAttachments, t.attachmentCount) + .Record(t.blendConstants); + vulkan::SerializePnext<>(key, &t); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkPipelineDynamicStateCreateInfo& t) { + key->Record(t.flags).RecordIterable(t.pDynamicStates, t.dynamicStateCount); + vulkan::SerializePnext<>(key, &t); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const vulkan::RenderPassCacheQuery& t) { + key->Record(t.colorMask.to_ulong(), t.resolveTargetMask.to_ulong()) + .RecordIterable(t.colorFormats) + .RecordIterable(t.colorLoadOp) + .RecordIterable(t.colorStoreOp) + .Record(t.hasDepthStencil, t.depthStencilFormat, t.depthLoadOp, t.depthStoreOp, + t.stencilLoadOp, t.stencilStoreOp, t.readOnlyDepthStencil, t.sampleCount); + } + + template <> + void CacheKeySerializer::Serialize( + CacheKey* key, + const VkGraphicsPipelineCreateInfo& t) { + // The pipeline layout and render pass are not serialized here because they are pointers to + // backend objects. They need to be cross-referenced with the frontend objects and + // serialized from there. The base pipeline information is also currently not recorded since + // we do not use them in our backend implementation. If we decide to use them later on, they + // also need to be cross-referenced from the frontend. + key->Record(t.flags) + .RecordIterable(t.pStages, t.stageCount) + .Record(t.pVertexInputState, t.pInputAssemblyState, t.pTessellationState, + t.pViewportState, t.pRasterizationState, t.pMultisampleState, + t.pDepthStencilState, t.pColorBlendState, t.pDynamicState, t.subpass); + vulkan::SerializePnext<>(key, &t); + } + } // namespace dawn::native diff --git a/src/dawn/native/vulkan/CacheKeyVk.h b/src/dawn/native/vulkan/CacheKeyVk.h index 0142a52def..0569fadf3d 100644 --- a/src/dawn/native/vulkan/CacheKeyVk.h +++ b/src/dawn/native/vulkan/CacheKeyVk.h @@ -70,18 +70,32 @@ namespace dawn::native::vulkan { SerializePnextImpl(key, root); } + template + const VkBaseOutStructure* ToVkBaseOutStructure(const VK_STRUCT_TYPE* t) { + // Sanity checks to ensure proper type safety. + static_assert( + offsetof(VK_STRUCT_TYPE, sType) == offsetof(VkBaseOutStructure, sType) && + offsetof(VK_STRUCT_TYPE, pNext) == offsetof(VkBaseOutStructure, pNext), + "Argument type is not a proper Vulkan structure type"); + return reinterpret_cast(t); + } + } // namespace detail - template - void SerializePnext(CacheKey* key, const VkBaseOutStructure* root) { + template 0)>> + void SerializePnext(CacheKey* key, const VK_STRUCT_TYPE* t) { + const VkBaseOutStructure* root = detail::ToVkBaseOutStructure(t); detail::ValidatePnextImpl(root); detail::SerializePnextImpl(key, root); } // Empty template specialization so that we can put this in to ensure failures occur if new // extensions are added without updating serialization. - template <> - void SerializePnext(CacheKey* key, const VkBaseOutStructure* root) { + template + void SerializePnext(CacheKey* key, const VK_STRUCT_TYPE* t) { + const VkBaseOutStructure* root = detail::ToVkBaseOutStructure(t); detail::ValidatePnextImpl<>(root); } diff --git a/src/dawn/native/vulkan/RenderPipelineVk.cpp b/src/dawn/native/vulkan/RenderPipelineVk.cpp index 9e97a192df..9b5349dcea 100644 --- a/src/dawn/native/vulkan/RenderPipelineVk.cpp +++ b/src/dawn/native/vulkan/RenderPipelineVk.cpp @@ -381,6 +381,9 @@ namespace dawn::native::vulkan { DAWN_ASSERT(stageCount < 2); shaderStages[stageCount] = shaderStage; stageCount++; + + // Record cache key for each shader since it will become inaccessible later on. + GetCacheKey()->Record(stage).RecordIterable(*spirv); } PipelineVertexInputStateCreateInfoTemporaryAllocations tempAllocations; @@ -528,6 +531,7 @@ namespace dawn::native::vulkan { query.SetSampleCount(GetSampleCount()); + GetCacheKey()->Record(query); DAWN_TRY_ASSIGN(renderPass, device->GetRenderPassCache()->GetRenderPass(query)); } @@ -555,6 +559,10 @@ namespace dawn::native::vulkan { createInfo.basePipelineHandle = VkPipeline{}; createInfo.basePipelineIndex = -1; + // Record cache key information now since createInfo is not stored. + GetCacheKey()->Record(createInfo, + static_cast(this)->GetLayout()->GetCacheKey()); + DAWN_TRY(CheckVkSuccess( device->fn.CreateGraphicsPipelines(device->GetVkDevice(), VkPipelineCache{}, 1, &createInfo, nullptr, &*mHandle),