Move zero-size copy skips from the frontend to the backend
Bug: chromium:1217741
Change-Id: Ib4884b5aa80124ed9da48262fcae11cf6d605034
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/53883
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Jiawei Shao <jiawei.shao@intel.com>
diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp
index 8f9db69..7676a49 100644
--- a/src/dawn_native/CommandEncoder.cpp
+++ b/src/dawn_native/CommandEncoder.cpp
@@ -633,16 +633,13 @@
mTopLevelBuffers.insert(destination);
}
- // Skip noop copies. Some backends validation rules disallow them.
- if (size != 0) {
- CopyBufferToBufferCmd* copy =
- allocator->Allocate<CopyBufferToBufferCmd>(Command::CopyBufferToBuffer);
- copy->source = source;
- copy->sourceOffset = sourceOffset;
- copy->destination = destination;
- copy->destinationOffset = destinationOffset;
- copy->size = size;
- }
+ CopyBufferToBufferCmd* copy =
+ allocator->Allocate<CopyBufferToBufferCmd>(Command::CopyBufferToBuffer);
+ copy->source = source;
+ copy->sourceOffset = sourceOffset;
+ copy->destination = destination;
+ copy->destinationOffset = destinationOffset;
+ copy->size = size;
return {};
});
@@ -682,23 +679,18 @@
ApplyDefaultTextureDataLayoutOptions(&srcLayout, blockInfo, *copySize);
- // Skip noop copies.
- if (copySize->width != 0 && copySize->height != 0 &&
- copySize->depthOrArrayLayers != 0) {
- // Record the copy command.
- CopyBufferToTextureCmd* copy =
- allocator->Allocate<CopyBufferToTextureCmd>(Command::CopyBufferToTexture);
- copy->source.buffer = source->buffer;
- copy->source.offset = srcLayout.offset;
- copy->source.bytesPerRow = srcLayout.bytesPerRow;
- copy->source.rowsPerImage = srcLayout.rowsPerImage;
- copy->destination.texture = destination->texture;
- copy->destination.origin = destination->origin;
- copy->destination.mipLevel = destination->mipLevel;
- copy->destination.aspect =
- ConvertAspect(destination->texture->GetFormat(), destination->aspect);
- copy->copySize = *copySize;
- }
+ CopyBufferToTextureCmd* copy =
+ allocator->Allocate<CopyBufferToTextureCmd>(Command::CopyBufferToTexture);
+ copy->source.buffer = source->buffer;
+ copy->source.offset = srcLayout.offset;
+ copy->source.bytesPerRow = srcLayout.bytesPerRow;
+ copy->source.rowsPerImage = srcLayout.rowsPerImage;
+ copy->destination.texture = destination->texture;
+ copy->destination.origin = destination->origin;
+ copy->destination.mipLevel = destination->mipLevel;
+ copy->destination.aspect =
+ ConvertAspect(destination->texture->GetFormat(), destination->aspect);
+ copy->copySize = *copySize;
return {};
});
@@ -738,22 +730,17 @@
ApplyDefaultTextureDataLayoutOptions(&dstLayout, blockInfo, *copySize);
- // Skip noop copies.
- if (copySize->width != 0 && copySize->height != 0 &&
- copySize->depthOrArrayLayers != 0) {
- // Record the copy command.
- CopyTextureToBufferCmd* copy =
- allocator->Allocate<CopyTextureToBufferCmd>(Command::CopyTextureToBuffer);
- copy->source.texture = source->texture;
- copy->source.origin = source->origin;
- copy->source.mipLevel = source->mipLevel;
- copy->source.aspect = ConvertAspect(source->texture->GetFormat(), source->aspect);
- copy->destination.buffer = destination->buffer;
- copy->destination.offset = dstLayout.offset;
- copy->destination.bytesPerRow = dstLayout.bytesPerRow;
- copy->destination.rowsPerImage = dstLayout.rowsPerImage;
- copy->copySize = *copySize;
- }
+ CopyTextureToBufferCmd* copy =
+ allocator->Allocate<CopyTextureToBufferCmd>(Command::CopyTextureToBuffer);
+ copy->source.texture = source->texture;
+ copy->source.origin = source->origin;
+ copy->source.mipLevel = source->mipLevel;
+ copy->source.aspect = ConvertAspect(source->texture->GetFormat(), source->aspect);
+ copy->destination.buffer = destination->buffer;
+ copy->destination.offset = dstLayout.offset;
+ copy->destination.bytesPerRow = dstLayout.bytesPerRow;
+ copy->destination.rowsPerImage = dstLayout.rowsPerImage;
+ copy->copySize = *copySize;
return {};
});
@@ -783,22 +770,18 @@
mTopLevelTextures.insert(destination->texture);
}
- // Skip noop copies.
- if (copySize->width != 0 && copySize->height != 0 &&
- copySize->depthOrArrayLayers != 0) {
- CopyTextureToTextureCmd* copy =
- allocator->Allocate<CopyTextureToTextureCmd>(Command::CopyTextureToTexture);
- copy->source.texture = source->texture;
- copy->source.origin = source->origin;
- copy->source.mipLevel = source->mipLevel;
- copy->source.aspect = ConvertAspect(source->texture->GetFormat(), source->aspect);
- copy->destination.texture = destination->texture;
- copy->destination.origin = destination->origin;
- copy->destination.mipLevel = destination->mipLevel;
- copy->destination.aspect =
- ConvertAspect(destination->texture->GetFormat(), destination->aspect);
- copy->copySize = *copySize;
- }
+ CopyTextureToTextureCmd* copy =
+ allocator->Allocate<CopyTextureToTextureCmd>(Command::CopyTextureToTexture);
+ copy->source.texture = source->texture;
+ copy->source.origin = source->origin;
+ copy->source.mipLevel = source->mipLevel;
+ copy->source.aspect = ConvertAspect(source->texture->GetFormat(), source->aspect);
+ copy->destination.texture = destination->texture;
+ copy->destination.origin = destination->origin;
+ copy->destination.mipLevel = destination->mipLevel;
+ copy->destination.aspect =
+ ConvertAspect(destination->texture->GetFormat(), destination->aspect);
+ copy->copySize = *copySize;
return {};
});
diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.cpp b/src/dawn_native/d3d12/CommandBufferD3D12.cpp
index a2dca9c..29b3a98 100644
--- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp
+++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp
@@ -626,6 +626,10 @@
case Command::CopyBufferToBuffer: {
CopyBufferToBufferCmd* copy = mCommands.NextCommand<CopyBufferToBufferCmd>();
+ if (copy->size == 0) {
+ // Skip no-op copies.
+ break;
+ }
Buffer* srcBuffer = ToBackend(copy->source.Get());
Buffer* dstBuffer = ToBackend(copy->destination.Get());
@@ -646,6 +650,11 @@
case Command::CopyBufferToTexture: {
CopyBufferToTextureCmd* copy = mCommands.NextCommand<CopyBufferToTextureCmd>();
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
Buffer* buffer = ToBackend(copy->source.buffer.Get());
Texture* texture = ToBackend(copy->destination.texture.Get());
@@ -676,6 +685,11 @@
case Command::CopyTextureToBuffer: {
CopyTextureToBufferCmd* copy = mCommands.NextCommand<CopyTextureToBufferCmd>();
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
Texture* texture = ToBackend(copy->source.texture.Get());
Buffer* buffer = ToBackend(copy->destination.buffer.Get());
@@ -700,7 +714,11 @@
case Command::CopyTextureToTexture: {
CopyTextureToTextureCmd* copy =
mCommands.NextCommand<CopyTextureToTextureCmd>();
-
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
Texture* source = ToBackend(copy->source.texture.Get());
Texture* destination = ToBackend(copy->destination.texture.Get());
diff --git a/src/dawn_native/metal/CommandBufferMTL.mm b/src/dawn_native/metal/CommandBufferMTL.mm
index a9f1896..b390184 100644
--- a/src/dawn_native/metal/CommandBufferMTL.mm
+++ b/src/dawn_native/metal/CommandBufferMTL.mm
@@ -704,6 +704,10 @@
case Command::CopyBufferToBuffer: {
CopyBufferToBufferCmd* copy = mCommands.NextCommand<CopyBufferToBufferCmd>();
+ if (copy->size == 0) {
+ // Skip no-op copies.
+ break;
+ }
ToBackend(copy->source)->EnsureDataInitialized(commandContext);
ToBackend(copy->destination)
@@ -721,6 +725,11 @@
case Command::CopyBufferToTexture: {
CopyBufferToTextureCmd* copy = mCommands.NextCommand<CopyBufferToTextureCmd>();
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
auto& src = copy->source;
auto& dst = copy->destination;
auto& copySize = copy->copySize;
@@ -739,6 +748,11 @@
case Command::CopyTextureToBuffer: {
CopyTextureToBufferCmd* copy = mCommands.NextCommand<CopyTextureToBufferCmd>();
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
auto& src = copy->source;
auto& dst = copy->destination;
auto& copySize = copy->copySize;
@@ -814,6 +828,11 @@
case Command::CopyTextureToTexture: {
CopyTextureToTextureCmd* copy =
mCommands.NextCommand<CopyTextureToTextureCmd>();
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
Texture* srcTexture = ToBackend(copy->source.texture.Get());
Texture* dstTexture = ToBackend(copy->destination.texture.Get());
diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp
index 40eb26b..085a8f0 100644
--- a/src/dawn_native/opengl/CommandBufferGL.cpp
+++ b/src/dawn_native/opengl/CommandBufferGL.cpp
@@ -612,6 +612,10 @@
case Command::CopyBufferToBuffer: {
CopyBufferToBufferCmd* copy = mCommands.NextCommand<CopyBufferToBufferCmd>();
+ if (copy->size == 0) {
+ // Skip no-op copies.
+ break;
+ }
ToBackend(copy->source)->EnsureDataInitialized();
ToBackend(copy->destination)
@@ -630,6 +634,11 @@
case Command::CopyBufferToTexture: {
CopyBufferToTextureCmd* copy = mCommands.NextCommand<CopyBufferToTextureCmd>();
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
auto& src = copy->source;
auto& dst = copy->destination;
Buffer* buffer = ToBackend(src.buffer.Get());
@@ -664,6 +673,11 @@
case Command::CopyTextureToBuffer: {
CopyTextureToBufferCmd* copy = mCommands.NextCommand<CopyTextureToBufferCmd>();
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
auto& src = copy->source;
auto& dst = copy->destination;
auto& copySize = copy->copySize;
@@ -771,6 +785,11 @@
case Command::CopyTextureToTexture: {
CopyTextureToTextureCmd* copy =
mCommands.NextCommand<CopyTextureToTextureCmd>();
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
auto& src = copy->source;
auto& dst = copy->destination;
diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp
index 2383e90..6409079 100644
--- a/src/dawn_native/vulkan/CommandBufferVk.cpp
+++ b/src/dawn_native/vulkan/CommandBufferVk.cpp
@@ -516,6 +516,10 @@
switch (type) {
case Command::CopyBufferToBuffer: {
CopyBufferToBufferCmd* copy = mCommands.NextCommand<CopyBufferToBufferCmd>();
+ if (copy->size == 0) {
+ // Skip no-op copies.
+ break;
+ }
Buffer* srcBuffer = ToBackend(copy->source.Get());
Buffer* dstBuffer = ToBackend(copy->destination.Get());
@@ -540,6 +544,11 @@
case Command::CopyBufferToTexture: {
CopyBufferToTextureCmd* copy = mCommands.NextCommand<CopyBufferToTextureCmd>();
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
auto& src = copy->source;
auto& dst = copy->destination;
@@ -578,6 +587,11 @@
case Command::CopyTextureToBuffer: {
CopyTextureToBufferCmd* copy = mCommands.NextCommand<CopyTextureToBufferCmd>();
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
auto& src = copy->source;
auto& dst = copy->destination;
@@ -610,6 +624,11 @@
case Command::CopyTextureToTexture: {
CopyTextureToTextureCmd* copy =
mCommands.NextCommand<CopyTextureToTextureCmd>();
+ if (copy->copySize.width == 0 || copy->copySize.height == 0 ||
+ copy->copySize.depthOrArrayLayers == 0) {
+ // Skip no-op copies.
+ continue;
+ }
TextureCopy& src = copy->source;
TextureCopy& dst = copy->destination;
SubresourceRange srcRange = GetSubresourcesAffectedByCopy(src, copy->copySize);
diff --git a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp
index dbbc3327..c75c50d 100644
--- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp
+++ b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp
@@ -226,6 +226,22 @@
}
}
+// Test a successful B2B copy where the last external reference is dropped.
+// This is a regression test for crbug.com/1217741 where submitting a command
+// buffer with dropped resources when the copy size is 0 was a use-after-free.
+TEST_F(CopyCommandTest_B2B, DroppedBuffer) {
+ wgpu::Buffer source = CreateBuffer(16, wgpu::BufferUsage::CopySrc);
+ wgpu::Buffer destination = CreateBuffer(16, wgpu::BufferUsage::CopyDst);
+
+ wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+ encoder.CopyBufferToBuffer(source, 0, destination, 0, 0);
+ wgpu::CommandBuffer commandBuffer = encoder.Finish();
+
+ source = nullptr;
+ destination = nullptr;
+ device.GetQueue().Submit(1, &commandBuffer);
+}
+
// Test B2B copies with OOB
TEST_F(CopyCommandTest_B2B, OutOfBounds) {
wgpu::Buffer source = CreateBuffer(16, wgpu::BufferUsage::CopySrc);
