sync: Wip

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<RecordBase>;
         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; }

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,

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;

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<std::pair<VkPipelineStageFlags2, VkAccessFlags2>> 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<std::pair<VkPipelineStageFlags2, VkAccessFlags2>> report_accesses;
+    std::vector<std::pair<VkPipelineStageFlags2, VkAccessFlags2>> 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 {

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<std::pair<VkPipelineStageFlags2, VkAccessFlags2>> 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";

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_;

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);
                 }
             }