gpuav: Vertex attributes OOB #9300
Conversation
|
CI Vulkan-ValidationLayers build queued with queue ID 354075. |
|
CI Vulkan-ValidationLayers build # 18835 running. |
| } attribute; | ||
| }; | ||
|
|
||
| struct InstrumentationErrorBlob { |
There was a problem hiding this comment.
Yes, "blob", it is about just as bad as "data" IMO, it is one more proof that we need to rework the error logging logic for shader instrumentation
arno-lunarg
force-pushed
the
arno-gpuav-improve-valcmd-perf
branch
from
6530258 to
73d4217
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 354093. |
|
CI Vulkan-ValidationLayers build # 18836 running. |
arno-lunarg
force-pushed
the
arno-gpuav-improve-valcmd-perf
branch
from
73d4217 to
4c7add4
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 354112. |
|
CI Vulkan-ValidationLayers build # 18837 running. |
| &stage_info_inst_it); | ||
|
|
||
| instrumentation_performed = true; | ||
| VVL_TracyPlot(__FUNCTION__, int64_t(1)); |
There was a problem hiding this comment.
can you remove, this feels like leaving a printf("1") here
There was a problem hiding this comment.
It's a way to count how many instrumentations were performed. Actually I will also add this to other instrumentation passes
There was a problem hiding this comment.
so if someone adds a new pass, who/how we checking they add this there, this doesn't seem like a good system to just hope these are scatter everywhere correctly all the time
There was a problem hiding this comment.
I would move this up into the Pass::Run() function
There was a problem hiding this comment.
Or honestly, it should be put in PrintDebugInfo because that is literally the purpose of it
| @@ -197,3 +197,74 @@ TEST_F(PositiveGpuAVIndexBuffer, IndexedIndirectRobustness) { | |||
| m_default_queue->Submit(m_command_buffer); | |||
| m_default_queue->Wait(); | |||
| } | |||
|
|
|||
| TEST_F(PositiveGpuAVIndexBuffer, NoShaderInputsVertexIndex16) { | |||
| TEST_DESCRIPTION("Vertex shader defines no vertex attributes - no OOB vertex fetch should be detected"); | |||
There was a problem hiding this comment.
Can we add a test where the vertex shader bounds a Location but not a 2nd one (so still no OOB)
There was a problem hiding this comment.
Sorry you mean a test with a vertex shader like so? :
char const *vsSource = R"glsl(
#version 450
layout(location=0) in vec3 pos;
void main() {
gl_Position = vec4(pos, 1.0);
}
)glsl";There was a problem hiding this comment.
yes, where Location 1would be the oob
|
CI Vulkan-ValidationLayers build # 18837 failed. |
arno-lunarg
force-pushed
the
arno-gpuav-improve-valcmd-perf
branch
from
4c7add4 to
df36dd8
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 354775. |
|
CI Vulkan-ValidationLayers build # 18845 running. |
arno-lunarg
force-pushed
the
arno-gpuav-improve-valcmd-perf
branch
from
df36dd8 to
6c220d9
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 354791. |
|
CI Vulkan-ValidationLayers build # 18846 running. |
|
CI Vulkan-ValidationLayers build # 18846 failed. |
arno-lunarg
force-pushed
the
arno-gpuav-improve-valcmd-perf
branch
from
6c220d9 to
7b63987
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 354926. |
|
CI Vulkan-ValidationLayers build # 18848 running. |
tests/unit/gpu_av_index_buffer.cpp
Outdated
| @@ -55,7 +54,7 @@ TEST_F(NegativeGpuAVIndexBuffer, IndexBufferOOB) { | |||
| m_errorMonitor->VerifyFound(); | |||
| } | |||
| // https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/9163 | |||
There was a problem hiding this comment.
can remove these links, these were just TODO, they serve no purpose anymore
| @@ -683,6 +683,22 @@ | |||
| ] | |||
| } | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
(This falls under the "gpu-av settings are hard" and not asking it to be moved in this PR)
At some level everything is going to just be "shader instrumentation" (I would have thought this would be under "buffer validation")
I just realized we have organized our setting by how we implement them and less how the user would try and find them... since this is mostly for vkconfig UX, we have some time before the next SDK to arrange as we find best
There was a problem hiding this comment.
Yes good point!
|
CI Vulkan-ValidationLayers build # 18848 failed. |
- Move vertex attributes OOB validation to instrumentation => faster, simpler - Handle VK_VERTEX_INPUT_RATE_INSTANCE
arno-lunarg
force-pushed
the
arno-gpuav-improve-valcmd-perf
branch
from
7b63987 to
7755a47
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 355400. |
|
CI Vulkan-ValidationLayers build # 18856 running. |
|
CI Vulkan-ValidationLayers build # 18856 passed. |
| out_vuid_msg = "VUID-vkCmdDrawIndexedIndirectCount-None-02721"; | ||
| break; | ||
| case vvl::Func::vkCmdDrawIndexedIndirect: | ||
| out_vuid_msg = "VUID-vkCmdDrawIndexedIndirect-None-02721"; |
There was a problem hiding this comment.
I would add a TODO to add VUID-vkCmdDrawMultiIndexedEXT-None-02721 here as well
| } | ||
|
|
||
| const auto [smallest_vbb_vertex_input_rate, smallest_vbb_instance_input_rate] = SmallestVertexAttributesCount(cb_state); | ||
| auto max_vertex_buffer_ptr = (uint32_t *)max_vertex_buffer.MapMemory(loc); |
There was a problem hiding this comment.
maybe at least a comment saying tied to MaxVertexIndex in GLSL
| VmaAllocationCreateInfo alloc_info = {}; | ||
| alloc_info.requiredFlags = VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_HOST_COHERENT_BIT; | ||
| // #ARNO_TODO - optimize buffer allocation as much as possible, | ||
| // alloc_info.pool = shared_copy_validation_resources.copy_regions_pool; |
| buffer_info.usage = VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT | VK_BUFFER_USAGE_STORAGE_BUFFER_BIT; | ||
| VmaAllocationCreateInfo alloc_info = {}; | ||
| alloc_info.requiredFlags = VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_HOST_COHERENT_BIT; | ||
| // #ARNO_TODO - optimize buffer allocation as much as possible, |
There was a problem hiding this comment.
not sure what this TODO is for, what is not optimized here, please add some more context or remove IMO
| @@ -276,6 +364,56 @@ void UpdateInstrumentationDescSet(Validator &gpuav, CommandBuffer &cb_state, VkD | |||
| desc_writes.emplace_back(wds); | |||
| } | |||
|
|
|||
| vko::Buffer max_vertex_buffer(gpuav); | |||
There was a problem hiding this comment.
the UpdateInstrumentationDescSet function is just here to call DispatchUpdateDescriptorSets
Adding all this other logic here feels very out of place, like why does vertex stuff need this but the other 5 buffers above didn't
Can you move this out to a dedicated function, like also if I turn off the vertex_attribute_fetch_oob feature, all of this stuff is happening for no reason
| @@ -165,8 +166,95 @@ static VkPipelineLayout CreateInstrumentationPipelineLayout(Validator &gpuav, Vk | |||
| } | |||
| } | |||
|
|
|||
| // Computes the smallest vertex attributes count among the set of bound vertex buffers. | |||
| // Used to detect out of bounds indices in index buffers. | |||
| // If no vertex buffer is bound, smallest_vertex_attributes_count is std::numeric_limits<uint32_t>::max() | |||
There was a problem hiding this comment.
can smallest_vertex_attributes_count ever be 0?
| VkDeviceSize smallest_vertex_attributes_count = std::numeric_limits<VkDeviceSize>::max(); | ||
| uint32_t binding = std::numeric_limits<uint32_t>::max(); | ||
| vvl::VertexBufferBinding binding_info{}; | ||
| struct Attribute { |
There was a problem hiding this comment.
Can we just use VkVertexInputAttributeDescription instead of redefining another struct here?
|
|
||
| void PreCallSetupShaderInstrumentationResources(Validator& gpuav, CommandBuffer& cb_state, VkPipelineBindPoint bind_point, | ||
| const Location& loc); | ||
|
|
||
| void PostCallSetupShaderInstrumentationResources(Validator& gpuav, CommandBuffer& cb_statee, VkPipelineBindPoint bind_point, | ||
| const Location& loc); | ||
|
|
||
| struct SmallestVertexBufferBinding { | ||
| VkDeviceSize smallest_vertex_attributes_count = std::numeric_limits<VkDeviceSize>::max(); | ||
| uint32_t binding = std::numeric_limits<uint32_t>::max(); |
There was a problem hiding this comment.
what is different between this and attribute.binding?
| }; | ||
|
|
||
| struct InstrumentationErrorBlob { | ||
| std::optional<SmallestVertexBufferBinding> smallest_vbb_vertex_input_rate{}; |
There was a problem hiding this comment.
instead of carrying around 2 of these, can't we just have a single std::optional<SmallestVertexBufferBinding> and put the VkVertexInputRate inside of it. Seems like there is a of logic that could be done as
bool SmallestVertexBufferBinding::IsInputRateVertex() { return input_rate == VK_VERTEX_INPUT_RATE_VERTEX; }
| } | ||
|
|
||
| for (const auto &[binding, vertex_binding_desc] : vertex_binding_descriptions) { | ||
| auto find_vbb = cb_state.current_vertex_buffer_binding_info.find(binding); |
There was a problem hiding this comment.
const auto *vbb = vvl::Find(cb_state.current_vertex_buffer_binding_info, binding);
if (!vertex_buffer) continue; // this is a validation error
| if (vertex_shader_used_locations.find(location) == vertex_shader_used_locations.end()) { | ||
| continue; | ||
| } | ||
| const VkDeviceSize attribute_size = vkuFormatElementSize(attrib.desc.format); |
There was a problem hiding this comment.
Use GetVertexInputFormatSize instead of vkuFormatElementSize, I just added it and vkuFormatElementSize is gonna be gone soon in future VUL update
| @@ -24,8 +24,8 @@ | |||
| #include "validation_cmd_trace_rays_rgen.h" | |||
|
|
|||
| // To view SPIR-V, copy contents of array and paste in https://www.khronos.org/spir/visualizer/ | |||
| [[maybe_unused]] const uint32_t validation_cmd_trace_rays_rgen_size = 1755; | |||
| [[maybe_unused]] const uint32_t validation_cmd_trace_rays_rgen[1755] = { | |||
| [[maybe_unused]] const uint32_t validation_cmd_trace_rays_rgen_size = 1740; | |||
There was a problem hiding this comment.
this one spooks me a bit, can you confirm what glslang version you were using?
VkVertexInputBindingDescription2EXT::divisor