Improve validation errors for Buffer

This required adding a version of device.ConsumedError that takes a
MaybeError and a formatted string context.

Also removes an unused method.

Bug: dawn:563
Change-Id: I7a2139cc47945d1f29bdfe926db3c932bf17c6d7
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/65564
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Brandon Jones <bajones@google.com>

src/dawn_native/Buffer.cpp

@@ -99,29 +99,28 @@ namespace dawn_native {
     }  // anonymous namespace
 
     MaybeError ValidateBufferDescriptor(DeviceBase*, const BufferDescriptor* descriptor) {
-        if (descriptor->nextInChain != nullptr) {
+        DAWN_INVALID_IF(descriptor->nextInChain != nullptr, "nextInChain must be nullptr");
-            return DAWN_VALIDATION_ERROR("nextInChain must be nullptr");
-        }
-
         DAWN_TRY(ValidateBufferUsage(descriptor->usage));
 
         wgpu::BufferUsage usage = descriptor->usage;
 
         const wgpu::BufferUsage kMapWriteAllowedUsages =
             wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopySrc;
-        if (usage & wgpu::BufferUsage::MapWrite && (usage & kMapWriteAllowedUsages) != usage) {
+        DAWN_INVALID_IF(
-            return DAWN_VALIDATION_ERROR("Only CopySrc is allowed with MapWrite");
+            usage & wgpu::BufferUsage::MapWrite && !IsSubset(usage, kMapWriteAllowedUsages),
-        }
+            "Buffer usages (%s) contains %s but is not a subset of %s.", usage,
+            wgpu::BufferUsage::MapWrite, kMapWriteAllowedUsages);
 
         const wgpu::BufferUsage kMapReadAllowedUsages =
             wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopyDst;
-        if (usage & wgpu::BufferUsage::MapRead && (usage & kMapReadAllowedUsages) != usage) {
+        DAWN_INVALID_IF(
-            return DAWN_VALIDATION_ERROR("Only CopyDst is allowed with MapRead");
+            usage & wgpu::BufferUsage::MapRead && !IsSubset(usage, kMapReadAllowedUsages),
-        }
+            "Buffer usages (%s) contains %s but is not a subset of %s.", usage,
+            wgpu::BufferUsage::MapRead, kMapReadAllowedUsages);
 
-        if (descriptor->mappedAtCreation && descriptor->size % 4 != 0) {
+        DAWN_INVALID_IF(descriptor->mappedAtCreation && descriptor->size % 4 != 0,
-            return DAWN_VALIDATION_ERROR("size must be aligned to 4 when mappedAtCreation is true");
+                        "Buffer is mapped at creation but its size (%u) is not a multiple of 4.",
-        }
+                        descriptor->size);
 
         return {};
     }
@@ -265,10 +264,10 @@ namespace dawn_native {
 
         switch (mState) {
             case BufferState::Destroyed:
-                return DAWN_VALIDATION_ERROR("Destroyed buffer used in a submit");
+                return DAWN_FORMAT_VALIDATION_ERROR("%s used in submit while destroyed.", this);
             case BufferState::Mapped:
             case BufferState::MappedAtCreation:
-                return DAWN_VALIDATION_ERROR("Buffer used in a submit while mapped");
+                return DAWN_FORMAT_VALIDATION_ERROR("%s used in submit while mapped.", this);
             case BufferState::Unmapped:
                 return {};
         }
@@ -304,7 +303,9 @@ namespace dawn_native {
         }
 
         WGPUBufferMapAsyncStatus status;
-        if (GetDevice()->ConsumedError(ValidateMapAsync(mode, offset, size, &status))) {
+        if (GetDevice()->ConsumedError(ValidateMapAsync(mode, offset, size, &status),
+                                       "calling %s.MapAsync(%s, %u, %u, ...)", this, mode, offset,
+                                       size)) {
             if (callback) {
                 callback(status, userdata);
             }
@@ -360,7 +361,7 @@ namespace dawn_native {
             static_cast<ErrorBuffer*>(this)->ClearMappedData();
             mState = BufferState::Destroyed;
         }
-        if (GetDevice()->ConsumedError(ValidateDestroy())) {
+        if (GetDevice()->ConsumedError(ValidateDestroy(), "calling %s.Destroy()", this)) {
             return;
         }
         ASSERT(!IsError());
@@ -411,7 +412,7 @@ namespace dawn_native {
             static_cast<ErrorBuffer*>(this)->ClearMappedData();
             mState = BufferState::Unmapped;
         }
-        if (GetDevice()->ConsumedError(ValidateUnmap())) {
+        if (GetDevice()->ConsumedError(ValidateUnmap(), "calling %s.Unmap()", this)) {
             return;
         }
         ASSERT(!IsError());
@@ -439,32 +440,6 @@ namespace dawn_native {
         mState = BufferState::Unmapped;
     }
 
-    MaybeError BufferBase::ValidateMap(wgpu::BufferUsage requiredUsage,
-                                       WGPUBufferMapAsyncStatus* status) const {
-        *status = WGPUBufferMapAsyncStatus_DeviceLost;
-        DAWN_TRY(GetDevice()->ValidateIsAlive());
-
-        *status = WGPUBufferMapAsyncStatus_Error;
-        DAWN_TRY(GetDevice()->ValidateObject(this));
-
-        switch (mState) {
-            case BufferState::Mapped:
-            case BufferState::MappedAtCreation:
-                return DAWN_VALIDATION_ERROR("Buffer is already mapped");
-            case BufferState::Destroyed:
-                return DAWN_VALIDATION_ERROR("Buffer is destroyed");
-            case BufferState::Unmapped:
-                break;
-        }
-
-        if (!(mUsage & requiredUsage)) {
-            return DAWN_VALIDATION_ERROR("Buffer needs the correct map usage bit");
-        }
-
-        *status = WGPUBufferMapAsyncStatus_Success;
-        return {};
-    }
-
     MaybeError BufferBase::ValidateMapAsync(wgpu::MapMode mode,
                                             size_t offset,
                                             size_t size,
@@ -475,44 +450,37 @@ namespace dawn_native {
         *status = WGPUBufferMapAsyncStatus_Error;
         DAWN_TRY(GetDevice()->ValidateObject(this));
 
-        if (offset % 8 != 0) {
+        DAWN_INVALID_IF(offset % 8 != 0, "Offset (%u) must be a multiple of 8.", offset);
-            return DAWN_VALIDATION_ERROR("offset must be a multiple of 8");
+        DAWN_INVALID_IF(size % 4 != 0, "Size (%u) must be a multiple of 4.", size);
-        }
 
-        if (size % 4 != 0) {
+        DAWN_INVALID_IF(uint64_t(offset) > mSize || uint64_t(size) > mSize - uint64_t(offset),
-            return DAWN_VALIDATION_ERROR("size must be a multiple of 4");
+                        "Mapping range (offset:%u, size: %u) doesn't fit in the size (%u) of %s.",
-        }
+                        offset, size, mSize, this);
-
-        if (uint64_t(offset) > mSize || uint64_t(size) > mSize - uint64_t(offset)) {
-            return DAWN_VALIDATION_ERROR("size + offset must fit in the buffer");
-        }
 
         switch (mState) {
             case BufferState::Mapped:
             case BufferState::MappedAtCreation:
-                return DAWN_VALIDATION_ERROR("Buffer is already mapped");
+                return DAWN_FORMAT_VALIDATION_ERROR("%s is already mapped.", this);
             case BufferState::Destroyed:
-                return DAWN_VALIDATION_ERROR("Buffer is destroyed");
+                return DAWN_FORMAT_VALIDATION_ERROR("%s is destroyed.", this);
             case BufferState::Unmapped:
                 break;
         }
 
         bool isReadMode = mode & wgpu::MapMode::Read;
         bool isWriteMode = mode & wgpu::MapMode::Write;
-        if (!(isReadMode ^ isWriteMode)) {
+        DAWN_INVALID_IF(!(isReadMode ^ isWriteMode), "Map mode (%s) is not one of %s or %s.", mode,
-            return DAWN_VALIDATION_ERROR("Exactly one of Read or Write mode must be set");
+                        wgpu::MapMode::Write, wgpu::MapMode::Read);
-        }
 
         if (mode & wgpu::MapMode::Read) {
-            if (!(mUsage & wgpu::BufferUsage::MapRead)) {
+            DAWN_INVALID_IF(!(mUsage & wgpu::BufferUsage::MapRead),
-                return DAWN_VALIDATION_ERROR("The buffer must have the MapRead usage");
+                            "The buffer usages (%s) do not contain %s.", mUsage,
-            }
+                            wgpu::BufferUsage::MapRead);
         } else {
             ASSERT(mode & wgpu::MapMode::Write);
-
+            DAWN_INVALID_IF(!(mUsage & wgpu::BufferUsage::MapWrite),
-            if (!(mUsage & wgpu::BufferUsage::MapWrite)) {
+                            "The buffer usages (%s) do not contain %s.", mUsage,
-                return DAWN_VALIDATION_ERROR("The buffer must have the MapWrite usage");
+                            wgpu::BufferUsage::MapWrite);
-            }
         }
 
         *status = WGPUBufferMapAsyncStatus_Success;
@@ -569,9 +537,9 @@ namespace dawn_native {
                 // even if it did not have a mappable usage.
                 return {};
             case BufferState::Unmapped:
-                return DAWN_VALIDATION_ERROR("Buffer is unmapped");
+                return DAWN_FORMAT_VALIDATION_ERROR("%s is unmapped.", this);
             case BufferState::Destroyed:
-                return DAWN_VALIDATION_ERROR("Buffer is destroyed");
+                return DAWN_FORMAT_VALIDATION_ERROR("%s is destroyed.", this);
         }
         UNREACHABLE();
     }

src/dawn_native/Buffer.h

@@ -105,8 +105,6 @@ namespace dawn_native {
         MaybeError CopyFromStagingBuffer();
         void CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status);
 
-        MaybeError ValidateMap(wgpu::BufferUsage requiredUsage,
-                               WGPUBufferMapAsyncStatus* status) const;
         MaybeError ValidateMapAsync(wgpu::MapMode mode,
                                     size_t offset,
                                     size_t size,

src/dawn_native/Device.cpp

@@ -1160,7 +1160,8 @@ namespace dawn_native {
     ResultOrError<Ref<BufferBase>> DeviceBase::CreateBuffer(const BufferDescriptor* descriptor) {
         DAWN_TRY(ValidateIsAlive());
         if (IsValidationEnabled()) {
-            DAWN_TRY(ValidateBufferDescriptor(this, descriptor));
+            DAWN_TRY_CONTEXT(ValidateBufferDescriptor(this, descriptor), "validating %s",
+                             descriptor);
         }
 
         Ref<BufferBase> buffer;

src/dawn_native/Device.h

@@ -78,6 +78,23 @@ namespace dawn_native {
             return false;
         }
 
+        template <typename... Args>
+        bool ConsumedError(MaybeError maybeError, const char* formatStr, const Args&... args) {
+            if (DAWN_UNLIKELY(maybeError.IsError())) {
+                std::unique_ptr<ErrorData> error = maybeError.AcquireError();
+                if (error->GetType() == InternalErrorType::Validation) {
+                    std::string out;
+                    absl::UntypedFormatSpec format(formatStr);
+                    if (absl::FormatUntyped(&out, format, {absl::FormatArg(args)...})) {
+                        error->AppendContext(std::move(out));
+                    }
+                }
+                ConsumeError(std::move(error));
+                return true;
+            }
+            return false;
+        }
+
         template <typename T, typename... Args>
         bool ConsumedError(ResultOrError<T> resultOrError,
                            T* result,