diff --git a/layers/sync/sync_commandbuffer.h b/layers/sync/sync_commandbuffer.h index 92ab325bc62..9aa1deb8414 100644 --- a/layers/sync/sync_commandbuffer.h +++ b/layers/sync/sync_commandbuffer.h @@ -1,6 +1,6 @@ /* - * Copyright (c) 2019-2024 Valve Corporation - * Copyright (c) 2019-2024 LunarG, Inc. + * Copyright (c) 2019-2025 Valve Corporation + * Copyright (c) 2019-2025 LunarG, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,6 +17,7 @@ #pragma once #include "sync/sync_renderpass.h" +#include "sync/sync_reporting.h" #include "state_tracker/cmd_buffer_state.h" struct ReportKeyValues; @@ -33,6 +34,7 @@ class AlternateResourceUsage { using Record = std::unique_ptr; virtual Record MakeRecord() const = 0; virtual std::ostream &Format(std::ostream &out, const SyncValidator &sync_state) const = 0; + virtual vvl::Func GetCommand() const = 0; virtual ~RecordBase() {} }; @@ -46,6 +48,7 @@ class AlternateResourceUsage { FormatterState Formatter(const SyncValidator &sync_state) const { return FormatterState(sync_state, *this); }; std::ostream &Format(std::ostream &out, const SyncValidator &sync_state) const { return record_->Format(out, sync_state); }; + vvl::Func GetCommand() const { return record_->GetCommand(); } AlternateResourceUsage() = default; AlternateResourceUsage(const RecordBase &record) : record_(record.MakeRecord()) {} AlternateResourceUsage(const AlternateResourceUsage &other) : record_() { @@ -214,7 +217,6 @@ class CommandExecutionContext { virtual void AddUsageRecordExtraProperties(ResourceUsageTag tag, ReportKeyValues &extra_properties) const = 0; std::string FormatHazard(const HazardResult &hazard, ReportKeyValues &key_values) const; - std::string FormatHazard(const HazardResult &hazard) const; bool ValidForSyncOps() const; const SyncValidator &GetSyncState() const { return sync_state_; } @@ -263,6 +265,7 @@ class CommandBufferAccessContext : public CommandExecutionContext, DebugNameProv void Reset(); + ReportUsageInfo GetReportUsageInfo(ResourceUsageTagEx tag_ex) const; std::string FormatUsage(ResourceUsageTagEx tag_ex) const override; void AddUsageRecordExtraProperties(ResourceUsageTag tag, ReportKeyValues &extra_properties) const override; std::string FormatUsage(const char *usage_string, @@ -303,6 +306,7 @@ class CommandBufferAccessContext : public CommandExecutionContext, DebugNameProv void RecordExecutedCommandBuffer(const CommandBufferAccessContext &recorded_context); void ResolveExecutedCommandBuffer(const AccessContext &recorded_context, ResourceUsageTag offset); + // TODO: what about using queue_flags directly from base class? VkQueueFlags GetQueueFlags() const { return cb_state_ ? cb_state_->GetQueueFlags() : 0; } ExecutionType Type() const override { return kExecuted; } diff --git a/layers/sync/sync_error_messages.cpp b/layers/sync/sync_error_messages.cpp index a019e6ff598..1ab4784d117 100644 --- a/layers/sync/sync_error_messages.cpp +++ b/layers/sync/sync_error_messages.cpp @@ -19,6 +19,7 @@ #include "sync/sync_commandbuffer.h" #include "sync/sync_image.h" #include "sync/sync_reporting.h" +#include "sync/sync_validation.h" #include "state_tracker/buffer_state.h" #include "state_tracker/descriptor_sets.h" @@ -148,21 +149,53 @@ std::string ErrorMessages::BufferError(const HazardResult& hazard, VkBuffer buff } std::string ErrorMessages::BufferRegionError(const HazardResult& hazard, VkBuffer buffer, bool is_src_buffer, uint32_t region_index, - const CommandBufferAccessContext& cb_context) const { - const auto format = "Hazard %s for %s %s, region %" PRIu32 ". Access info %s."; - ReportKeyValues key_values; + const CommandBufferAccessContext& cb_context, const vvl::Func command) const { + // TEMP: will be part of more general code + const SyncAccessFlags write_barriers = hazard.State().access_state->GetWriteBarriers(); + const auto vk_protected_accesses = + ConvertSyncAccessesToCompactVkForm(write_barriers, cb_context.GetQueueFlags(), cb_context.GetSyncState().enabled_features, + cb_context.GetSyncState().extensions); + const auto& sync_access = syncAccessInfoByAccessIndex()[hazard.State().access_index]; + const bool barrier_protects_access = + std::find_if(vk_protected_accesses.begin(), vk_protected_accesses.end(), [&sync_access](const auto& protected_access) { + return (protected_access.second & sync_access.access_mask) != 0; + }) != vk_protected_accesses.end(); + + // TEMP: detect specific case to demo new direction of syncval error messages. + // This also will be replaced by more general implementation. + if (hazard.Hazard() == WRITE_AFTER_WRITE && !write_barriers.none() && barrier_protects_access) { + const ReportUsageInfo usage_info = cb_context.GetReportUsageInfo(hazard.TagEx()); + std::stringstream ss; + ss << string_SyncHazard(hazard.Hazard()) << " hazard detected. "; + ss << vvl::String(command) << " writes to " << validator_.FormatHandle(buffer); + ss << ", which was written earlier by "; + if (usage_info.command == command) { + ss << "another "; + } + ss << vvl::String(usage_info.command) << ". "; + ss << "The applied synchronization protects "; + ss << FormatSyncAccesses(write_barriers, cb_context.GetQueueFlags(), cb_context.GetSyncState().enabled_features, + cb_context.GetSyncState().extensions, false); + + ss << " but does not cover accesses at " << string_VkPipelineStageFlagBits2(sync_access.stage_mask) << " stage."; + + return ss.str(); + } else { + const auto format = "Hazard %s for %s %s, region %" PRIu32 ". Access info %s."; + ReportKeyValues key_values; - const std::string access_info = cb_context.FormatHazard(hazard, key_values); - const char* resource_parameter = is_src_buffer ? "srcBuffer" : "dstBuffer"; - std::string message = Format(format, string_SyncHazard(hazard.Hazard()), resource_parameter, - validator_.FormatHandle(buffer).c_str(), region_index, access_info.c_str()); - if (extra_properties_) { - key_values.Add(kPropertyMessageType, "BufferRegionError"); - key_values.Add(kPropertyResourceParameter, resource_parameter); - AddCbContextExtraProperties(cb_context, hazard.Tag(), key_values); - message += key_values.GetExtraPropertiesSection(pretty_print_extra_); + const std::string access_info = cb_context.FormatHazard(hazard, key_values); + const char* resource_parameter = is_src_buffer ? "srcBuffer" : "dstBuffer"; + std::string message = Format(format, string_SyncHazard(hazard.Hazard()), resource_parameter, + validator_.FormatHandle(buffer).c_str(), region_index, access_info.c_str()); + if (extra_properties_) { + key_values.Add(kPropertyMessageType, "BufferRegionError"); + key_values.Add(kPropertyResourceParameter, resource_parameter); + AddCbContextExtraProperties(cb_context, hazard.Tag(), key_values); + message += key_values.GetExtraPropertiesSection(pretty_print_extra_); + } + return message; } - return message; } std::string ErrorMessages::ImageRegionError(const HazardResult& hazard, VkImage image, bool is_src_image, uint32_t region_index, diff --git a/layers/sync/sync_error_messages.h b/layers/sync/sync_error_messages.h index 0e575b46c80..c7559b2a6b3 100644 --- a/layers/sync/sync_error_messages.h +++ b/layers/sync/sync_error_messages.h @@ -45,7 +45,7 @@ class ErrorMessages { const CommandBufferAccessContext& cb_context) const; std::string BufferRegionError(const HazardResult& hazard, VkBuffer buffer, bool is_src_buffer, uint32_t region_index, - const CommandBufferAccessContext& cb_context) const; + const CommandBufferAccessContext& cb_context, const vvl::Func command) const; std::string ImageRegionError(const HazardResult& hazard, VkImage image, bool is_src_image, uint32_t region_index, const CommandBufferAccessContext& cb_context) const; diff --git a/layers/sync/sync_reporting.cpp b/layers/sync/sync_reporting.cpp index 8fbdeea91d9..db22169ec13 100644 --- a/layers/sync/sync_reporting.cpp +++ b/layers/sync/sync_reporting.cpp @@ -143,7 +143,7 @@ static std::string FormatHandleRecord(const HandleRecord::FormatterState &format return out.str(); } -std::string FormatResourceUsageRecord(const ResourceUsageRecord::FormatterState &formatter) { +static std::string FormatResourceUsageRecord(const ResourceUsageRecord::FormatterState &formatter) { std::stringstream out; const ResourceUsageRecord &record = formatter.record; if (record.alt_usage) { @@ -235,11 +235,11 @@ static SyncAccessFlags FilterSyncAccessesByAllowedVkAccesses(const SyncAccessFla return filtered_accesses; } -static std::string FormatSyncAccesses(const SyncAccessFlags &sync_accesses, VkQueueFlags allowed_queue_flags, - const DeviceFeatures &features, const DeviceExtensions &device_extensions, - bool format_as_extra_property) { +std::vector> ConvertSyncAccessesToCompactVkForm( + const SyncAccessFlags &sync_accesses, VkQueueFlags allowed_queue_flags, const DeviceFeatures &features, + const DeviceExtensions &device_extensions) { if (sync_accesses.none()) { - return "0"; + return {}; } const VkPipelineStageFlags2 disabled_stages = sync_utils::DisabledPipelineStages(features, device_extensions); @@ -281,7 +281,7 @@ static std::string FormatSyncAccesses(const SyncAccessFlags &sync_accesses, VkQu } // Replace sequences of stages/accesses with more compact equivalent meta values where possible - std::vector> report_accesses; + std::vector> result; VkPipelineStageFlags2 stages_with_all_supported_accesses = 0; VkAccessFlags2 all_accesses = 0; // accesses for the above stages @@ -312,20 +312,29 @@ static std::string FormatSyncAccesses(const SyncAccessFlags &sync_accesses, VkQu sync_utils::ReplaceExpandBitsWithMetaMask(stages, all_transfer_expand_bits, VK_PIPELINE_STAGE_2_ALL_TRANSFER_BIT); sync_utils::ReplaceExpandBitsWithMetaMask(accesses, kShaderReadExpandBits, VK_ACCESS_2_SHADER_READ_BIT); - report_accesses.emplace_back(stages, accesses); + result.emplace_back(stages, accesses); } if (stages_with_all_supported_accesses) { if (IsSingleBitSet(stages_with_all_supported_accesses) && GetBitSetCount(all_accesses) <= 2) { // For simple configurations (1 stage and at most 2 accesses) don't use ALL accesses shortcut - report_accesses.emplace_back(stages_with_all_supported_accesses, all_accesses); + result.emplace_back(stages_with_all_supported_accesses, all_accesses); } else { sync_utils::ReplaceExpandBitsWithMetaMask(stages_with_all_supported_accesses, all_transfer_expand_bits, VK_PIPELINE_STAGE_2_ALL_TRANSFER_BIT); - report_accesses.emplace_back(stages_with_all_supported_accesses, sync_utils::kAllAccesses); + result.emplace_back(stages_with_all_supported_accesses, sync_utils::kAllAccesses); } } + return result; +} - // Create string with available accesses +std::string FormatSyncAccesses(const SyncAccessFlags &sync_accesses, VkQueueFlags allowed_queue_flags, + const DeviceFeatures &features, const DeviceExtensions &device_extensions, + bool format_as_extra_property) { + const auto report_accesses = + ConvertSyncAccessesToCompactVkForm(sync_accesses, allowed_queue_flags, features, device_extensions); + if (report_accesses.empty()) { + return "0"; + } std::stringstream out; bool first = true; for (const auto &[stages, accesses] : report_accesses) { @@ -403,9 +412,15 @@ std::string CommandExecutionContext::FormatHazard(const HazardResult &hazard, Re return out.str(); } -std::string CommandExecutionContext::FormatHazard(const HazardResult &hazard) const { - ReportKeyValues key_values; - return FormatHazard(hazard, key_values); +ReportUsageInfo CommandBufferAccessContext::GetReportUsageInfo(ResourceUsageTagEx tag_ex) const { + const ResourceUsageRecord &record = (*access_log_)[tag_ex.tag]; + ReportUsageInfo info; + if (record.alt_usage) { + info.command = record.alt_usage.GetCommand(); + } else { + info.command = record.command; + } + return info; } std::string CommandBufferAccessContext::FormatUsage(ResourceUsageTagEx tag_ex) const { diff --git a/layers/sync/sync_reporting.h b/layers/sync/sync_reporting.h index 4e4d6417d62..1407e80c612 100644 --- a/layers/sync/sync_reporting.h +++ b/layers/sync/sync_reporting.h @@ -17,7 +17,20 @@ #pragma once -#include "sync/sync_commandbuffer.h" +//#include "sync/sync_commandbuffer.h" +#include "generated/error_location_helper.h" +#include "generated/sync_validation_types.h" + +namespace vvl { +class CommandBuffer; +class StateObject; +class Image; +class Queue; +} +class DebugReport; +class SyncValidator; +struct DeviceFeatures; +struct DeviceExtensions; struct SyncNodeFormatter { const DebugReport *debug_report; @@ -44,6 +57,18 @@ struct ReportKeyValues { std::string GetExtraPropertiesSection(bool pretty_print) const; }; +struct ReportUsageInfo { + vvl::Func command = vvl::Func::Empty; +}; + +std::vector> ConvertSyncAccessesToCompactVkForm( + const SyncAccessFlags &sync_accesses, VkQueueFlags allowed_queue_flags, const DeviceFeatures &features, + const DeviceExtensions &device_extensions); + +std::string FormatSyncAccesses(const SyncAccessFlags &sync_accesses, VkQueueFlags allowed_queue_flags, + const DeviceFeatures &features, const DeviceExtensions &device_extensions, + bool format_as_extra_property); + inline constexpr const char *kPropertyMessageType = "message_type"; inline constexpr const char *kPropertyAccess = "access"; inline constexpr const char *kPropertyPriorAccess = "prior_access"; diff --git a/layers/sync/sync_submit.h b/layers/sync/sync_submit.h index 589bc633e81..9cec5fd58d6 100644 --- a/layers/sync/sync_submit.h +++ b/layers/sync/sync_submit.h @@ -265,6 +265,7 @@ class QueueBatchContext : public CommandExecutionContext, public std::enable_sha ~PresentResourceRecord() override {} PresentResourceRecord(const PresentedImageRecord &presented) : presented_(presented) {} std::ostream &Format(std::ostream &out, const SyncValidator &sync_state) const override; + vvl::Func GetCommand() const override { return vvl::Func::vkQueuePresentKHR; } private: PresentedImageRecord presented_; @@ -277,6 +278,7 @@ class QueueBatchContext : public CommandExecutionContext, public std::enable_sha AcquireResourceRecord(const PresentedImageRecord &presented, ResourceUsageTag tag, vvl::Func command) : presented_(presented), acquire_tag_(tag), command_(command) {} std::ostream &Format(std::ostream &out, const SyncValidator &sync_state) const override; + vvl::Func GetCommand() const override { return command_; } private: PresentedImageRecord presented_; diff --git a/layers/sync/sync_validation.cpp b/layers/sync/sync_validation.cpp index 66f3866f8f2..84f2fce879a 100644 --- a/layers/sync/sync_validation.cpp +++ b/layers/sync/sync_validation.cpp @@ -364,7 +364,8 @@ bool SyncValidator::PreCallValidateCmdCopyBuffer(VkCommandBuffer commandBuffer, auto hazard = context->DetectHazard(*src_buffer, SYNC_COPY_TRANSFER_READ, src_range); if (hazard.IsHazard()) { const LogObjectList objlist(commandBuffer, srcBuffer); - const auto error = error_messages_.BufferRegionError(hazard, srcBuffer, true, region, *cb_context); + const auto error = + error_messages_.BufferRegionError(hazard, srcBuffer, true, region, *cb_context, error_obj.location.function); skip |= SyncError(hazard.Hazard(), objlist, error_obj.location, error); } } @@ -373,7 +374,8 @@ bool SyncValidator::PreCallValidateCmdCopyBuffer(VkCommandBuffer commandBuffer, auto hazard = context->DetectHazard(*dst_buffer, SYNC_COPY_TRANSFER_WRITE, dst_range); if (hazard.IsHazard()) { const LogObjectList objlist(commandBuffer, dstBuffer); - const auto error = error_messages_.BufferRegionError(hazard, dstBuffer, false, region, *cb_context); + const auto error = + error_messages_.BufferRegionError(hazard, dstBuffer, false, region, *cb_context, error_obj.location.function); skip |= SyncError(hazard.Hazard(), objlist, error_obj.location, error); } } @@ -432,7 +434,8 @@ bool SyncValidator::PreCallValidateCmdCopyBuffer2(VkCommandBuffer commandBuffer, // TODO -- add tag information to log msg when useful. // TODO: there are no tests for this error const LogObjectList objlist(commandBuffer, pCopyBufferInfo->srcBuffer); - const auto error = error_messages_.BufferRegionError(hazard, pCopyBufferInfo->srcBuffer, true, region, *cb_context); + const auto error = error_messages_.BufferRegionError(hazard, pCopyBufferInfo->srcBuffer, true, region, *cb_context, + error_obj.location.function); skip |= SyncError(hazard.Hazard(), objlist, error_obj.location, error); } } @@ -442,8 +445,8 @@ bool SyncValidator::PreCallValidateCmdCopyBuffer2(VkCommandBuffer commandBuffer, if (hazard.IsHazard()) { // TODO: there are no tests for this error const LogObjectList objlist(commandBuffer, pCopyBufferInfo->dstBuffer); - const auto error = - error_messages_.BufferRegionError(hazard, pCopyBufferInfo->dstBuffer, false, region, *cb_context); + const auto error = error_messages_.BufferRegionError(hazard, pCopyBufferInfo->dstBuffer, false, region, *cb_context, + error_obj.location.function); skip |= SyncError(hazard.Hazard(), objlist, error_obj.location, error); } } @@ -1076,7 +1079,8 @@ bool SyncValidator::ValidateCmdCopyBufferToImage(VkCommandBuffer commandBuffer, if (hazard.IsHazard()) { // PHASE1 TODO -- add tag information to log msg when useful. const LogObjectList objlist(commandBuffer, srcBuffer); - const auto error = error_messages_.BufferRegionError(hazard, srcBuffer, true, region, *cb_access_context); + const auto error = + error_messages_.BufferRegionError(hazard, srcBuffer, true, region, *cb_access_context, loc.function); skip |= SyncError(hazard.Hazard(), objlist, loc, error); } } @@ -1210,7 +1214,8 @@ bool SyncValidator::ValidateCmdCopyImageToBuffer(VkCommandBuffer commandBuffer, hazard = context->DetectHazard(*dst_buffer, SYNC_COPY_TRANSFER_WRITE, dst_range); if (hazard.IsHazard()) { const LogObjectList objlist(commandBuffer, dstBuffer); - const auto error = error_messages_.BufferRegionError(hazard, dstBuffer, false, region, *cb_access_context); + const auto error = + error_messages_.BufferRegionError(hazard, dstBuffer, false, region, *cb_access_context, loc.function); skip |= SyncError(hazard.Hazard(), objlist, loc, error); } } diff --git a/tests/unit/sync_val.cpp b/tests/unit/sync_val.cpp index 22cb089cdcd..394f35dab5b 100644 --- a/tests/unit/sync_val.cpp +++ b/tests/unit/sync_val.cpp @@ -76,6 +76,36 @@ TEST_F(NegativeSyncVal, BufferCopy) { m_command_buffer.End(); } +TEST_F(NegativeSyncVal, BufferCopyWrongBarrier) { + TEST_DESCRIPTION("Buffer barrier does not specify proper dst stage/access"); + SetTargetApiVersion(VK_API_VERSION_1_3); + AddRequiredFeature(vkt::Feature::synchronization2); + RETURN_IF_SKIP(InitSyncVal()); + + vkt::Buffer buffer_a(*m_device, 256, VK_BUFFER_USAGE_TRANSFER_SRC_BIT); + vkt::Buffer buffer_b(*m_device, 256, VK_BUFFER_USAGE_TRANSFER_DST_BIT); + + VkBufferMemoryBarrier2 barrier = vku::InitStructHelper(); + barrier.srcStageMask = VK_PIPELINE_STAGE_2_TRANSFER_BIT; + barrier.srcAccessMask = VK_ACCESS_2_TRANSFER_WRITE_BIT; + barrier.dstStageMask = VK_PIPELINE_STAGE_2_CLEAR_BIT; + barrier.dstAccessMask = VK_ACCESS_2_TRANSFER_WRITE_BIT; + barrier.buffer = buffer_b; + barrier.size = 256; + + VkDependencyInfo dep_info = vku::InitStructHelper(); + dep_info.bufferMemoryBarrierCount = 1; + dep_info.pBufferMemoryBarriers = &barrier; + + m_command_buffer.Begin(); + m_command_buffer.Copy(buffer_a, buffer_b); + vk::CmdPipelineBarrier2(m_command_buffer, &dep_info); + m_errorMonitor->SetDesiredError("SYNC-HAZARD-WRITE-AFTER-WRITE"); + m_command_buffer.Copy(buffer_a, buffer_b); + m_errorMonitor->VerifyFound(); + m_command_buffer.End(); +} + TEST_F(NegativeSyncVal, BufferCopySecondary) { TEST_DESCRIPTION("Record buffer copy commands in secondary command buffers"); RETURN_IF_SKIP(InitSyncVal());