gpuav: Dont crash if trying Scoped GPU-AV with RT Pipelines

Author: spencer-lunarg

layers/gpuav/instrumentation/gpuav_shader_instrumentor.cpp

@@ -114,7 +114,7 @@ void GpuShaderInstrumentor::SetupClassicDescriptor(const Location& loc) {
     result = DispatchCreatePipelineLayout(device, &debug_pipeline_layout_info, nullptr,
                                           &instrumentation_pipeline_layout_[vvl::DescriptorModeClassic]);
     if (result != VK_SUCCESS) {
-        InternalError(device, loc, "vkCreateDescriptorSetLayout failed for internal pipeline layout");
+        InternalError(device, loc, "vkCreatePipelineLayout failed for internal pipeline layout");
         Cleanup();
         return;
     }
@@ -169,7 +169,7 @@ void GpuShaderInstrumentor::SetupDescriptorBuffers(const Location& loc) {
     result = DispatchCreatePipelineLayout(device, &debug_pipeline_layout_db_info, nullptr,
                                           &instrumentation_pipeline_layout_[vvl::DescriptorModeBuffer]);
     if (result != VK_SUCCESS) {
-        InternalError(device, loc, "vkCreateDescriptorSetLayout failed for internal pipeline layout for descriptor buffer");
+        InternalError(device, loc, "vkCreatePipelineLayout failed for internal pipeline layout for descriptor buffer");
         Cleanup();
         return;
     }
@@ -211,12 +211,12 @@ void GpuShaderInstrumentor::FinishDeviceSetup(const VkDeviceCreateInfo* pCreateI
     if (!modified_features.fragmentStoresAndAtomics) {
         InternalError(
             device, loc,
-            "GPU Shader Instrumentation requires fragmentStoresAndAtomics to allow witting out data inside the fragment shader.");
+            "GPU Shader Instrumentation requires fragmentStoresAndAtomics to allow writing out data inside the fragment shader.");
         return;
     }
     if (!modified_features.vertexPipelineStoresAndAtomics) {
         InternalError(device, loc,
-                      "GPU Shader Instrumentation requires vertexPipelineStoresAndAtomics to allow witting out data inside the "
+                      "GPU Shader Instrumentation requires vertexPipelineStoresAndAtomics to allow writing out data inside the "
                       "vertex shader.");
         return;
     }
@@ -227,7 +227,7 @@ void GpuShaderInstrumentor::FinishDeviceSetup(const VkDeviceCreateInfo* pCreateI
         return;
     }
     if (!modified_features.bufferDeviceAddress) {
-        InternalError(device, loc, "GPU Shader Instrumentation requires bufferDeviceAddress to manage witting out of the shader.");
+        InternalError(device, loc, "GPU Shader Instrumentation requires bufferDeviceAddress to manage writing out of the shader.");
         return;
     }
     if (!modified_features.scalarBlockLayout) {
@@ -377,9 +377,9 @@ void GpuShaderInstrumentor::PreCallRecordCreatePipelineLayout(VkDevice device, c
         if (chassis_state.modified_create_info.setLayoutCount > instrumentation_desc_set_bind_index_) {
             std::ostringstream strm;
             strm << "pCreateInfo::setLayoutCount (" << chassis_state.modified_create_info.setLayoutCount
-                 << ") will conflicts with validation's descriptor set at slot " << instrumentation_desc_set_bind_index_ << ". "
+                 << ") will conflict with validation's descriptor set at slot " << instrumentation_desc_set_bind_index_ << ". "
                  << "This Pipeline Layout has too many descriptor sets that will not allow GPU shader instrumentation to be setup "
-                    "for pipelines created with it, therefore no validation error will be repored for them by GPU-AV at runtime.";
+                    "for pipelines created with it, therefore no validation error will be reported for them by GPU-AV at runtime.";
             InternalWarning(device, record_obj.location, strm.str().c_str());
         } else {
             vvl::DescriptorMode mode = SelectDescriptorModeFromDSL(pCreateInfo->setLayoutCount, pCreateInfo->pSetLayouts);
@@ -452,11 +452,17 @@ void GpuShaderInstrumentor::PreCallRecordSetDebugUtilsObjectNameEXT(VkDevice dev
             }
         }
 
-        VkPipeline instrumented_pipeline = VK_NULL_HANDLE;
         // Can't instrument ray tracing pipeline post creation,
         // As corresponding shader binding tables may have already been created.
-        if (pipeline_state->linking_shaders == 0 &&
-            IsValueIn(pipeline_state->pipeline_type, {VK_PIPELINE_BIND_POINT_GRAPHICS, VK_PIPELINE_BIND_POINT_COMPUTE})) {
+        if (!IsValueIn(pipeline_state->pipeline_type, {VK_PIPELINE_BIND_POINT_GRAPHICS, VK_PIPELINE_BIND_POINT_COMPUTE})) {
+            InternalWarning(device, record_obj.location,
+                            "Only graphics and compute pipelines can be instrumented after they are created, this pipeline will "
+                            "not be instrumented. Set the name before creating the pipeline to have it selected.");
+            return;
+        }
+
+        VkPipeline instrumented_pipeline = VK_NULL_HANDLE;
+        if (pipeline_state->linking_shaders == 0) {
             std::vector<chassis::ShaderInstrumentationMetadata> shader_instrumentation_metadata;
             if (pipeline_state->pipeline_type == VK_PIPELINE_BIND_POINT_GRAPHICS) {
                 vku::safe_VkGraphicsPipelineCreateInfo new_pipeline_ci(pipeline_state->GraphicsCreateInfo());
@@ -703,9 +709,9 @@ void GpuShaderInstrumentor::PreCallRecordCreateShadersEXT(VkDevice device, uint3
         if (new_create_info.setLayoutCount > instrumentation_desc_set_bind_index_) {
             std::ostringstream strm;
             strm << "pCreateInfos[" << i << "]::setLayoutCount (" << new_create_info.setLayoutCount
-                 << ") will conflicts with validation's descriptor set at slot " << instrumentation_desc_set_bind_index_ << ". "
+                 << ") will conflict with validation's descriptor set at slot " << instrumentation_desc_set_bind_index_ << ". "
                  << "This Shader Object has too many descriptor sets that will not allow GPU shader instrumentation to be setup "
-                    "for VkShaderEXT created with it, therefore no validation error will be repored for them by GPU-AV at "
+                    "for VkShaderEXT created with it, therefore no validation error will be reported for them by GPU-AV at "
                     "runtime.";
             InternalWarning(device, record_obj.location, strm.str().c_str());
         }

tests/unit/gpu_av_ray_tracing.cpp

@@ -4400,3 +4400,45 @@ TEST_F(NegativeGpuAVRayTracing, OpReportIntersectionKHRHitKindOutOfRange) {
     ASSERT_GT(debug_buffer_ptr[0], 0u) << "Intersection shader was never invoked";
     debug_buffer.Memory().Unmap();
 }
+
+TEST_F(NegativeGpuAVRayTracing, SelectInstrumentedPipelineRegex) {
+    TEST_DESCRIPTION("Currently not possible, but need to ensure we do not crash");
+    SetTargetApiVersion(VK_API_VERSION_1_2);
+    AddRequiredExtensions(VK_EXT_DEBUG_UTILS_EXTENSION_NAME);
+    AddRequiredExtensions(VK_KHR_RAY_TRACING_PIPELINE_EXTENSION_NAME);
+    AddRequiredFeature(vkt::Feature::rayTracingPipeline);
+    AddRequiredFeature(vkt::Feature::accelerationStructure);
+    AddRequiredFeature(vkt::Feature::bufferDeviceAddress);
+
+    std::vector<VkLayerSettingEXT> layer_settings(2);
+    layer_settings[0] = {OBJECT_LAYER_NAME, "gpuav_select_instrumented_shaders", VK_LAYER_SETTING_TYPE_BOOL32_EXT, 1, &kVkTrue};
+    std::array<const char*, 1> shader_regexes = {{"ray_tracing_pipeline_foo"}};
+    layer_settings[1] = {OBJECT_LAYER_NAME, "gpuav_shaders_to_instrument", VK_LAYER_SETTING_TYPE_STRING_EXT, size32(shader_regexes),
+                         shader_regexes.data()};
+
+    VkLayerSettingsCreateInfoEXT layer_setting_ci = vku::InitStructHelper();
+    layer_setting_ci.settingCount = size32(layer_settings);
+    layer_setting_ci.pSettings = layer_settings.data();
+
+    VkValidationFeaturesEXT validation_features = GetGpuAvValidationFeatures();
+    validation_features.pNext = &layer_setting_ci;
+    RETURN_IF_SKIP(InitFrameworkForRayTracingTest(&validation_features));
+    if (!CanEnableGpuAV(*this)) {
+        GTEST_SKIP() << "Requirements for GPU-AV are not met";
+    }
+    RETURN_IF_SKIP(InitState());
+
+    vkt::rt::Pipeline pipeline(*this, m_device);
+    pipeline.SetGlslRayGenShader(kRayTracingMinimalGlsl);
+    pipeline.AddBinding(VK_DESCRIPTOR_TYPE_ACCELERATION_STRUCTURE_KHR, 0);
+    pipeline.CreateDescriptorSet();
+    pipeline.Build();
+
+    VkDebugUtilsObjectNameInfoEXT name_info = vku::InitStructHelper();
+    name_info.objectType = VK_OBJECT_TYPE_PIPELINE;
+    name_info.pObjectName = "ray_tracing_pipeline_foo";
+    name_info.objectHandle = uint64_t(pipeline.Handle().handle());
+    m_errorMonitor->SetDesiredWarning("Only graphics and compute pipelines can be instrumented after they are created");
+    vk::SetDebugUtilsObjectNameEXT(device(), &name_info);
+    m_errorMonitor->VerifyFound();
+}

tests/unit/gpu_av_shader_debug_info.cpp

@@ -2822,4 +2822,186 @@ TEST_F(NegativeGpuAVShaderDebugInfo, HeapMultipleDraws) {
     m_errorMonitor->SetDesiredError("Bad-Draw");
     m_default_queue->SubmitAndWait(m_command_buffer);
     m_errorMonitor->VerifyFound();
-}
+}
+
+TEST_F(NegativeGpuAVShaderDebugInfo, GraphicsPipelineLibraryReusedLibrary) {
+    TEST_DESCRIPTION("Make sure the source is found when a library already instrumented by a previous link is re-linked");
+    AddRequiredExtensions(VK_EXT_GRAPHICS_PIPELINE_LIBRARY_EXTENSION_NAME);
+    AddRequiredFeature(vkt::Feature::graphicsPipelineLibrary);
+    RETURN_IF_SKIP(InitGpuVUBufferDeviceAddress());
+    InitRenderTarget();
+
+    const char* vs_source = R"glsl(
+        #version 450
+        #extension GL_EXT_buffer_reference : enable
+        layout(buffer_reference, std430) readonly buffer IndexBuffer {
+            int indices[];
+        };
+        layout(set = 0, binding = 0) readonly buffer foo {
+            IndexBuffer data;
+            int x;
+        };
+        vec2 vertices[3];
+        void main() {
+            vertices[0] = vec2(-1.0, -1.0);
+            vertices[1] = vec2(1.0, -1.0);
+            vertices[2] = vec2(0.0, 1.0);
+            gl_Position = vec4(vertices[gl_VertexIndex % 3], 0.0, 1.0);
+            if (data.indices[0] == 42) {
+                gl_Position = vec4(0.0);
+            }
+        }
+    )glsl";
+
+    const char* fs_source = R"(
+               OpCapability Shader
+               OpCapability PhysicalStorageBufferAddresses
+          %2 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel PhysicalStorageBuffer64 GLSL450
+               OpEntryPoint Fragment %main "main" %uFragColor %_
+               OpExecutionMode %main OriginUpperLeft
+          %1 = OpString "bad.frag"
+               OpSource GLSL 450 %1 "#version 450
+#extension GL_EXT_buffer_reference : enable
+layout(buffer_reference, std430) readonly buffer IndexBuffer {
+    int indices[];
+};
+layout(set = 0, binding = 0) readonly buffer foo {
+    IndexBuffer data;
+    int x;
+};
+layout(location = 0) out vec4 uFragColor;
+void main() {
+    uFragColor = vec4(float(data.indices[16]));
+}
+"
+               OpSourceExtension "GL_EXT_buffer_reference"
+               OpName %main "main"
+               OpName %uFragColor "uFragColor"
+               OpName %foo "foo"
+               OpMemberName %foo 0 "data"
+               OpMemberName %foo 1 "x"
+               OpName %IndexBuffer "IndexBuffer"
+               OpMemberName %IndexBuffer 0 "indices"
+               OpName %_ ""
+               OpDecorate %uFragColor Location 0
+               OpDecorate %foo Block
+               OpMemberDecorate %foo 0 NonWritable
+               OpMemberDecorate %foo 0 Offset 0
+               OpMemberDecorate %foo 1 NonWritable
+               OpMemberDecorate %foo 1 Offset 8
+               OpDecorate %_runtimearr_int ArrayStride 4
+               OpDecorate %IndexBuffer Block
+               OpMemberDecorate %IndexBuffer 0 NonWritable
+               OpMemberDecorate %IndexBuffer 0 Offset 0
+               OpDecorate %_ NonWritable
+               OpDecorate %_ Binding 0
+               OpDecorate %_ DescriptorSet 0
+       %void = OpTypeVoid
+          %4 = OpTypeFunction %void
+      %float = OpTypeFloat 32
+    %v4float = OpTypeVector %float 4
+%_ptr_Output_v4float = OpTypePointer Output %v4float
+ %uFragColor = OpVariable %_ptr_Output_v4float Output
+               OpTypeForwardPointer %_ptr_PhysicalStorageBuffer_IndexBuffer PhysicalStorageBuffer
+        %int = OpTypeInt 32 1
+        %foo = OpTypeStruct %_ptr_PhysicalStorageBuffer_IndexBuffer %int
+%_runtimearr_int = OpTypeRuntimeArray %int
+%IndexBuffer = OpTypeStruct %_runtimearr_int
+%_ptr_PhysicalStorageBuffer_IndexBuffer = OpTypePointer PhysicalStorageBuffer %IndexBuffer
+%_ptr_StorageBuffer_foo = OpTypePointer StorageBuffer %foo
+          %_ = OpVariable %_ptr_StorageBuffer_foo StorageBuffer
+      %int_0 = OpConstant %int 0
+%_ptr_StorageBuffer__ptr_PhysicalStorageBuffer_IndexBuffer = OpTypePointer StorageBuffer %_ptr_PhysicalStorageBuffer_IndexBuffer
+     %int_16 = OpConstant %int 16
+%_ptr_PhysicalStorageBuffer_int = OpTypePointer PhysicalStorageBuffer %int
+               OpLine %1 11 11
+       %main = OpFunction %void None %4
+          %6 = OpLabel
+               OpLine %1 12 0
+         %20 = OpAccessChain %_ptr_StorageBuffer__ptr_PhysicalStorageBuffer_IndexBuffer %_ %int_0
+         %21 = OpLoad %_ptr_PhysicalStorageBuffer_IndexBuffer %20
+         %24 = OpAccessChain %_ptr_PhysicalStorageBuffer_int %21 %int_0 %int_16
+         %25 = OpLoad %int %24 Aligned 4
+         %26 = OpConvertSToF %float %25
+         %27 = OpCompositeConstruct %v4float %26 %26 %26 %26
+               OpStore %uFragColor %27
+               OpLine %1 13 0
+               OpReturn
+               OpFunctionEnd
+    )";
+
+    VkShaderObj vs(*m_device, vs_source, VK_SHADER_STAGE_VERTEX_BIT, SPV_ENV_VULKAN_1_2);
+    VkShaderObj good_fs(*m_device, kFragmentMinimalGlsl, VK_SHADER_STAGE_FRAGMENT_BIT);
+    VkShaderObj bad_fs(*m_device, fs_source, VK_SHADER_STAGE_FRAGMENT_BIT, SPV_ENV_VULKAN_1_2, SPV_SOURCE_ASM);
+
+    OneOffDescriptorSet descriptor_set(m_device, {{0, VK_DESCRIPTOR_TYPE_STORAGE_BUFFER, 1, VK_SHADER_STAGE_ALL, nullptr}});
+    const vkt::PipelineLayout pipeline_layout(*m_device, {&descriptor_set.layout_});
+
+    vkt::Buffer block_buffer(*m_device, 16, 0, vkt::device_address);
+    vkt::Buffer in_buffer(*m_device, 16, VK_BUFFER_USAGE_STORAGE_BUFFER_BIT, kHostVisibleMemProps);
+    auto data = static_cast<VkDeviceAddress*>(in_buffer.Memory().Map());
+    data[0] = block_buffer.Address();
+    descriptor_set.WriteDescriptorBufferInfo(0, in_buffer, 0, VK_WHOLE_SIZE, VK_DESCRIPTOR_TYPE_STORAGE_BUFFER);
+    descriptor_set.UpdateDescriptorSets();
+
+    CreatePipelineHelper vertex_input_lib(*this);
+    vertex_input_lib.InitVertexInputLibInfo();
+    vertex_input_lib.CreateGraphicsPipeline(false);
+
+    VkViewport viewport = {0, 0, 1, 1, 0, 1};
+    VkRect2D scissor = {{0, 0}, {1, 1}};
+
+    CreatePipelineHelper pre_raster_lib(*this);
+    pre_raster_lib.InitPreRasterLibInfo(&vs.GetStageCreateInfo());
+    pre_raster_lib.vp_state_ci_.pViewports = &viewport;
+    pre_raster_lib.vp_state_ci_.pScissors = &scissor;
+    pre_raster_lib.gp_ci_.layout = pipeline_layout;
+    pre_raster_lib.CreateGraphicsPipeline();
+
+    CreatePipelineHelper good_frag_shader_lib(*this);
+    good_frag_shader_lib.InitFragmentLibInfo(&good_fs.GetStageCreateInfo());
+    good_frag_shader_lib.gp_ci_.layout = pipeline_layout;
+    good_frag_shader_lib.CreateGraphicsPipeline(false);
+
+    CreatePipelineHelper bad_frag_shader_lib(*this);
+    bad_frag_shader_lib.InitFragmentLibInfo(&bad_fs.GetStageCreateInfo());
+    bad_frag_shader_lib.gp_ci_.layout = pipeline_layout;
+    bad_frag_shader_lib.CreateGraphicsPipeline(false);
+
+    CreatePipelineHelper frag_out_lib(*this);
+    frag_out_lib.InitFragmentOutputLibInfo();
+    frag_out_lib.CreateGraphicsPipeline(false);
+
+    VkPipeline good_libraries[4] = {vertex_input_lib, pre_raster_lib, good_frag_shader_lib, frag_out_lib};
+    VkPipelineLibraryCreateInfoKHR good_link_info = vku::InitStructHelper();
+    good_link_info.libraryCount = size32(good_libraries);
+    good_link_info.pLibraries = good_libraries;
+
+    VkGraphicsPipelineCreateInfo good_exe_pipe_ci = vku::InitStructHelper(&good_link_info);
+    good_exe_pipe_ci.layout = pipeline_layout;
+    vkt::Pipeline good_exe_pipe(*m_device, good_exe_pipe_ci);
+
+    VkPipeline bad_libraries[4] = {vertex_input_lib, pre_raster_lib, bad_frag_shader_lib, frag_out_lib};
+    VkPipelineLibraryCreateInfoKHR bad_link_info = vku::InitStructHelper();
+    bad_link_info.libraryCount = size32(bad_libraries);
+    bad_link_info.pLibraries = bad_libraries;
+
+    VkGraphicsPipelineCreateInfo bad_exe_pipe_ci = vku::InitStructHelper(&bad_link_info);
+    bad_exe_pipe_ci.layout = pipeline_layout;
+    vkt::Pipeline bad_exe_pipe(*m_device, bad_exe_pipe_ci);
+
+    m_command_buffer.Begin();
+    m_command_buffer.BeginRenderPass(m_renderPassBeginInfo);
+    vk::CmdBindPipeline(m_command_buffer, VK_PIPELINE_BIND_POINT_GRAPHICS, bad_exe_pipe);
+    vk::CmdBindDescriptorSets(m_command_buffer, VK_PIPELINE_BIND_POINT_GRAPHICS, pipeline_layout, 0, 1, &descriptor_set.set_, 0,
+                              nullptr);
+    vk::CmdDraw(m_command_buffer, 3, 1, 0, 0);
+    m_command_buffer.EndRenderPass();
+    m_command_buffer.End();
+
+    // VUID-RuntimeSpirv-PhysicalStorageBuffer64-11819
+    m_errorMonitor->SetDesiredError("Shader validation error occurred at bad.frag:12");
+    m_default_queue->SubmitAndWait(m_command_buffer);
+    m_errorMonitor->VerifyFound();
+}