Add a test for creating a 0-sized buffer.

This is valid in WebGPU but causes validation errors in backends.

Also make it an OOM error on Metal to request a buffer close to
UINT32_MAX size because it would truncate the size, and could lead to
OOBs.

Bug: chromium:1069076
Change-Id: Ib961cb236cb7cabc0ae21203bf1d72ba82a56272
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/21060
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
This commit is contained in:
Corentin Wallez 2020-05-11 18:55:52 +00:00 committed by Commit Bot service account
parent 19b21c5b85
commit bf009f50c5
6 changed files with 46 additions and 9 deletions

View File

@ -84,7 +84,9 @@ namespace dawn_native { namespace d3d12 {
D3D12_RESOURCE_DESC resourceDescriptor;
resourceDescriptor.Dimension = D3D12_RESOURCE_DIMENSION_BUFFER;
resourceDescriptor.Alignment = 0;
resourceDescriptor.Width = GetSize();
// TODO(cwallez@chromium.org): Have a global "zero" buffer that can do everything instead
// of creating a new 4-byte buffer?
resourceDescriptor.Width = std::max(GetSize(), uint64_t(4u));
resourceDescriptor.Height = 1;
resourceDescriptor.DepthOrArraySize = 1;
resourceDescriptor.MipLevels = 1;

View File

@ -26,13 +26,14 @@ namespace dawn_native { namespace metal {
class Buffer : public BufferBase {
public:
Buffer(Device* device, const BufferDescriptor* descriptor);
static ResultOrError<Buffer*> Create(Device* device, const BufferDescriptor* descriptor);
id<MTLBuffer> GetMTLBuffer() const;
void OnMapCommandSerialFinished(uint32_t mapSerial, bool isWrite);
private:
using BufferBase::BufferBase;
MaybeError Initialize();
~Buffer() override;
// Dawn API
MaybeError MapReadAsyncImpl(uint32_t serial) override;

View File

@ -17,13 +17,21 @@
#include "common/Math.h"
#include "dawn_native/metal/DeviceMTL.h"
#include <limits>
namespace dawn_native { namespace metal {
// The size of uniform buffer and storage buffer need to be aligned to 16 bytes which is the
// largest alignment of supported data types
static constexpr uint32_t kMinUniformOrStorageBufferAlignment = 16u;
Buffer::Buffer(Device* device, const BufferDescriptor* descriptor)
: BufferBase(device, descriptor) {
// static
ResultOrError<Buffer*> Buffer::Create(Device* device, const BufferDescriptor* descriptor) {
Ref<Buffer> buffer = AcquireRef(new Buffer(device, descriptor));
DAWN_TRY(buffer->Initialize());
return buffer.Detach();
}
MaybeError Buffer::Initialize() {
MTLResourceOptions storageMode;
if (GetUsage() & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) {
storageMode = MTLResourceStorageModeShared;
@ -31,7 +39,14 @@ namespace dawn_native { namespace metal {
storageMode = MTLResourceStorageModePrivate;
}
uint32_t currentSize = GetSize();
if (GetSize() >
std::numeric_limits<uint64_t>::max() - kMinUniformOrStorageBufferAlignment) {
return DAWN_OUT_OF_MEMORY_ERROR("Buffer allocation is too large");
}
// TODO(cwallez@chromium.org): Have a global "zero" buffer that can do everything instead
// of creating a new 4-byte buffer?
uint32_t currentSize = std::max(GetSize(), uint64_t(4u));
// Metal validation layer requires the size of uniform buffer and storage buffer to be no
// less than the size of the buffer block defined in shader, and the overall size of the
// buffer must be aligned to the largest alignment of its members.
@ -39,7 +54,9 @@ namespace dawn_native { namespace metal {
currentSize = Align(currentSize, kMinUniformOrStorageBufferAlignment);
}
mMtlBuffer = [device->GetMTLDevice() newBufferWithLength:currentSize options:storageMode];
mMtlBuffer = [ToBackend(GetDevice())->GetMTLDevice() newBufferWithLength:currentSize
options:storageMode];
return {};
}
Buffer::~Buffer() {

View File

@ -110,7 +110,7 @@ namespace dawn_native { namespace metal {
return new BindGroupLayout(this, descriptor);
}
ResultOrError<BufferBase*> Device::CreateBufferImpl(const BufferDescriptor* descriptor) {
return new Buffer(this, descriptor);
return Buffer::Create(this, descriptor);
}
CommandBufferBase* Device::CreateCommandBuffer(CommandEncoder* encoder,
const CommandBufferDescriptor* descriptor) {

View File

@ -137,7 +137,9 @@ namespace dawn_native { namespace vulkan {
createInfo.sType = VK_STRUCTURE_TYPE_BUFFER_CREATE_INFO;
createInfo.pNext = nullptr;
createInfo.flags = 0;
createInfo.size = GetSize();
// TODO(cwallez@chromium.org): Have a global "zero" buffer that can do everything instead
// of creating a new 4-byte buffer?
createInfo.size = std::max(GetSize(), uint64_t(4u));
// Add CopyDst for non-mappable buffer initialization in CreateBufferMapped
// and robust resource initialization.
createInfo.usage = VulkanBufferUsage(GetUsage() | wgpu::BufferUsage::CopyDst);

View File

@ -581,3 +581,18 @@ DAWN_INSTANTIATE_TEST(CreateBufferMappedTests,
MetalBackend(),
OpenGLBackend(),
VulkanBackend());
class BufferTests : public DawnTest {};
TEST_P(BufferTests, ZeroSizedBuffer) {
wgpu::BufferDescriptor desc;
desc.size = 0;
desc.usage = wgpu::BufferUsage::CopyDst;
device.CreateBuffer(&desc);
}
DAWN_INSTANTIATE_TEST(BufferTests,
D3D12Backend(),
MetalBackend(),
OpenGLBackend(),
VulkanBackend());