From 45aed839e9c5647375bbc0249cc1b298f53ad190 Mon Sep 17 00:00:00 2001
From: Corentin Wallez <cwallez@chromium.org>
Date: Fri, 5 Jun 2020 15:44:03 +0000
Subject: [PATCH] CopyFromStagingToBuffer: ASSERT size is not 0

Zero-sized copies are invalid in a couple backends, and in follow up
CLs CreateBufferMapped will be change to handle zero-sized buffers
correctly.

Bug: chromium:1069076
Change-Id: Ieef62a13182bbe1e939a3847980c91339e42aa8f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/22460
Reviewed-by: Stephen White <senorblanco@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
---
 src/dawn_native/Buffer.cpp          |  4 ++++
 src/dawn_native/Queue.cpp           | 10 ++++++----
 src/dawn_native/metal/DeviceMTL.mm  |  7 +++----
 src/dawn_native/vulkan/DeviceVk.cpp | 10 ++++------
 4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp
index 2b738b2feb..46d00d1f0b 100644
--- a/src/dawn_native/Buffer.cpp
+++ b/src/dawn_native/Buffer.cpp
@@ -319,6 +319,10 @@ namespace dawn_native {
 
     MaybeError BufferBase::CopyFromStagingBuffer() {
         ASSERT(mStagingBuffer);
+        if (GetSize() == 0) {
+            return {};
+        }
+
         DAWN_TRY(GetDevice()->CopyFromStagingToBuffer(mStagingBuffer.get(), 0, this, 0, GetSize()));
 
         DynamicUploader* uploader = GetDevice()->GetDynamicUploader();
diff --git a/src/dawn_native/Queue.cpp b/src/dawn_native/Queue.cpp
index 81d6d61b53..3dcf2b0e46 100644
--- a/src/dawn_native/Queue.cpp
+++ b/src/dawn_native/Queue.cpp
@@ -113,6 +113,10 @@ namespace dawn_native {
                                           uint64_t bufferOffset,
                                           const void* data,
                                           size_t size) {
+        if (size == 0) {
+            return {};
+        }
+
         DeviceBase* device = GetDevice();
 
         UploadHandle uploadHandle;
@@ -122,10 +126,8 @@ namespace dawn_native {
 
         memcpy(uploadHandle.mappedBuffer, data, size);
 
-        DAWN_TRY(device->CopyFromStagingToBuffer(
-            uploadHandle.stagingBuffer, uploadHandle.startOffset, buffer, bufferOffset, size));
-
-        return {};
+        return device->CopyFromStagingToBuffer(uploadHandle.stagingBuffer, uploadHandle.startOffset,
+                                               buffer, bufferOffset, size);
     }
 
     MaybeError QueueBase::ValidateSubmit(uint32_t commandCount,
diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm
index 71f2ed7ea4..08431b90ca 100644
--- a/src/dawn_native/metal/DeviceMTL.mm
+++ b/src/dawn_native/metal/DeviceMTL.mm
@@ -247,10 +247,9 @@ namespace dawn_native { namespace metal {
                                                BufferBase* destination,
                                                uint64_t destinationOffset,
                                                uint64_t size) {
-        // Metal validation layers forbid  0-sized copies, skip it since it is a noop.
-        if (size == 0) {
-            return {};
-        }
+        // Metal validation layers forbid  0-sized copies, assert it is skipped prior to calling
+        // this function.
+        ASSERT(size != 0);
 
         id<MTLBuffer> uploadBuffer = ToBackend(source)->GetBufferHandle();
         id<MTLBuffer> buffer = ToBackend(destination)->GetMTLBuffer();
diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp
index ae623362dc..469ef23c85 100644
--- a/src/dawn_native/vulkan/DeviceVk.cpp
+++ b/src/dawn_native/vulkan/DeviceVk.cpp
@@ -541,12 +541,9 @@ namespace dawn_native { namespace vulkan {
                                                BufferBase* destination,
                                                uint64_t destinationOffset,
                                                uint64_t size) {
-        // It is a validation error to do a 0-sized copy in Vulkan skip it since it is a noop.
-        if (size == 0) {
-            return {};
-        }
-
-        CommandRecordingContext* recordingContext = GetPendingRecordingContext();
+        // It is a validation error to do a 0-sized copy in Vulkan, check it is skipped prior to
+        // calling this function.
+        ASSERT(size != 0);
 
         // Insert memory barrier to ensure host write operations are made visible before
         // copying from the staging buffer. However, this barrier can be removed (see note below).
@@ -557,6 +554,7 @@ namespace dawn_native { namespace vulkan {
 
         // Insert pipeline barrier to ensure correct ordering with previous memory operations on the
         // buffer.
+        CommandRecordingContext* recordingContext = GetPendingRecordingContext();
         ToBackend(destination)->TransitionUsageNow(recordingContext, wgpu::BufferUsage::CopyDst);
 
         VkBufferCopy copy;