From 4ff00cc4709877b30eee1f74e7180b3de707a7e7 Mon Sep 17 00:00:00 2001
From: Corentin Wallez <cwallez@chromium.org>
Date: Mon, 3 Oct 2022 23:07:23 +0000
Subject: [PATCH] Ensure no VVL message is ignored and doesn't cause test
 failures.

Messages could be ignored if:
 - They are not associated to any device (for example an issue around
   instance or adapter operations)
 - They happened between the last Tick() and device destruction.

Fix both cases to print the error to the dawn::ErrorLog and crash in
debug so that the errors are visible.

Bug: chromium:1258986
Change-Id: I9a88cd078c60b42deb2336da038902639f9a35ae
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/104360
Reviewed-by: Loko Kung <lokokung@google.com>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
---
 src/dawn/native/vulkan/BackendVk.cpp | 11 +++++++----
 src/dawn/native/vulkan/DeviceVk.cpp  | 21 +++++++++++++++++++++
 src/dawn/native/vulkan/DeviceVk.h    |  1 +
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/dawn/native/vulkan/BackendVk.cpp b/src/dawn/native/vulkan/BackendVk.cpp
index 42913ab1f4..870824f746 100644
--- a/src/dawn/native/vulkan/BackendVk.cpp
+++ b/src/dawn/native/vulkan/BackendVk.cpp
@@ -134,11 +134,10 @@ OnDebugUtilsCallback(VkDebugUtilsMessageSeverityFlagBitsEXT messageSeverity,
         return VK_FALSE;
     }
 
-    dawn::ErrorLog() << pCallbackData->pMessage;
-
     if (pUserData == nullptr) {
         return VK_FALSE;
     }
+    VulkanInstance* instance = reinterpret_cast<VulkanInstance*>(pUserData);
 
     // Look through all the object labels attached to the debug message and try to parse
     // a device debug prefix out of one of them. If a debug prefix is found and matches
@@ -150,12 +149,16 @@ OnDebugUtilsCallback(VkDebugUtilsMessageSeverityFlagBitsEXT messageSeverity,
             continue;
         }
 
-        VulkanInstance* instance = reinterpret_cast<VulkanInstance*>(pUserData);
         if (instance->HandleDeviceMessage(std::move(deviceDebugPrefix), pCallbackData->pMessage)) {
-            break;
+            return VK_FALSE;
         }
     }
 
+    // We get to this line if no device was associated with the message. Crash so that the failure
+    // is loud and makes tests fail in Debug.
+    dawn::ErrorLog() << pCallbackData->pMessage;
+    UNREACHABLE();
+
     return VK_FALSE;
 }
 
diff --git a/src/dawn/native/vulkan/DeviceVk.cpp b/src/dawn/native/vulkan/DeviceVk.cpp
index 826de69d24..9c99b62e2c 100644
--- a/src/dawn/native/vulkan/DeviceVk.cpp
+++ b/src/dawn/native/vulkan/DeviceVk.cpp
@@ -14,6 +14,7 @@
 
 #include "dawn/native/vulkan/DeviceVk.h"
 
+#include "dawn/common/Log.h"
 #include "dawn/common/NonCopyable.h"
 #include "dawn/common/Platform.h"
 #include "dawn/native/BackendConnection.h"
@@ -978,6 +979,21 @@ void Device::AppendDebugLayerMessages(ErrorData* error) {
     }
 }
 
+void Device::CheckDebugMessagesAfterDestruction() const {
+    if (!GetAdapter()->GetInstance()->IsBackendValidationEnabled() || mDebugMessages.empty()) {
+        return;
+    }
+
+    dawn::ErrorLog()
+        << "Some VVL messages were not handled before dawn::native::vulkan::Device destruction:";
+    for (const auto& message : mDebugMessages) {
+        dawn::ErrorLog() << " - " << message;
+    }
+
+    // Crash in debug
+    UNREACHABLE();
+}
+
 MaybeError Device::WaitForIdleForDestruction() {
     // Immediately tag the recording context as unused so we don't try to submit it in Tick.
     // Move the mRecordingContext.used to mUnusedCommands so it can be cleaned up in
@@ -1127,6 +1143,11 @@ void Device::DestroyImpl() {
     ASSERT(mVkDevice != VK_NULL_HANDLE);
     fn.DestroyDevice(mVkDevice, nullptr);
     mVkDevice = VK_NULL_HANDLE;
+
+    // No additonal Vulkan commands should be done by this device after this function. Check for any
+    // remaining Vulkan Validation Layer messages that may have been added during destruction or not
+    // handled prior to destruction.
+    CheckDebugMessagesAfterDestruction();
 }
 
 uint32_t Device::GetOptimalBytesPerRowAlignment() const {
diff --git a/src/dawn/native/vulkan/DeviceVk.h b/src/dawn/native/vulkan/DeviceVk.h
index fa27f82fd1..a3bc2e56ec 100644
--- a/src/dawn/native/vulkan/DeviceVk.h
+++ b/src/dawn/native/vulkan/DeviceVk.h
@@ -166,6 +166,7 @@ class Device final : public DeviceBase {
 
     MaybeError CheckDebugLayerAndGenerateErrors();
     void AppendDebugLayerMessages(ErrorData* error) override;
+    void CheckDebugMessagesAfterDestruction() const;
 
     void DestroyImpl() override;
     MaybeError WaitForIdleForDestruction() override;