Make ConsumedError require handling the returned bool.
This change makes it harder to misuse ConsumedError which can cause
device loss. When it is a known error, instead use HandleError to
bypass the "unlikely" if clause.
Bug: dawn:1336
Change-Id: I3052db343fe4080b257f1c2f9535f743a0e78526
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/120384
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Loko Kung <lokokung@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/dawn/native/Buffer.cpp b/src/dawn/native/Buffer.cpp
index c7124e5..338d515 100644
--- a/src/dawn/native/Buffer.cpp
+++ b/src/dawn/native/Buffer.cpp
@@ -506,9 +506,7 @@
if (GetDevice()->ConsumedError(ValidateUnmap(), "calling %s.Unmap().", this)) {
return;
}
- if (GetDevice()->ConsumedError(Unmap(), "calling %s.Unmap().", this)) {
- return;
- }
+ DAWN_UNUSED(GetDevice()->ConsumedError(Unmap(), "calling %s.Unmap().", this));
}
MaybeError BufferBase::Unmap() {
diff --git a/src/dawn/native/ComputePassEncoder.cpp b/src/dawn/native/ComputePassEncoder.cpp
index d3c6a5c..63505c7 100644
--- a/src/dawn/native/ComputePassEncoder.cpp
+++ b/src/dawn/native/ComputePassEncoder.cpp
@@ -151,7 +151,7 @@
void ComputePassEncoder::APIEnd() {
if (mEnded && IsValidationEnabled()) {
- GetDevice()->ConsumedError(DAWN_VALIDATION_ERROR("%s was already ended.", this));
+ GetDevice()->HandleError(DAWN_VALIDATION_ERROR("%s was already ended.", this));
return;
}
diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp
index 28956d0..53356e1 100644
--- a/src/dawn/native/Device.cpp
+++ b/src/dawn/native/Device.cpp
@@ -469,6 +469,7 @@
void DeviceBase::HandleError(std::unique_ptr<ErrorData> error,
InternalErrorType additionalAllowedErrors,
WGPUDeviceLostReason lost_reason) {
+ AppendDebugLayerMessages(error.get());
InternalErrorType allowedErrors =
InternalErrorType::Validation | InternalErrorType::DeviceLost | additionalAllowedErrors;
InternalErrorType type = error->GetType();
@@ -545,7 +546,6 @@
void DeviceBase::ConsumeError(std::unique_ptr<ErrorData> error,
InternalErrorType additionalAllowedErrors) {
ASSERT(error != nullptr);
- AppendDebugLayerMessages(error.get());
HandleError(std::move(error), additionalAllowedErrors);
}
@@ -1203,21 +1203,24 @@
// The validation errors on BufferDescriptor should be prior to any OOM errors when
// MapppedAtCreation == false.
MaybeError maybeError = ValidateBufferDescriptor(this, &fakeDescriptor);
- if (maybeError.IsError()) {
- ConsumedError(maybeError.AcquireError(), InternalErrorType::OutOfMemory,
- "calling %s.CreateBuffer(%s).", this, desc);
- } else {
- const DawnBufferDescriptorErrorInfoFromWireClient* clientErrorInfo = nullptr;
- FindInChain(desc->nextInChain, &clientErrorInfo);
- if (clientErrorInfo != nullptr && clientErrorInfo->outOfMemory) {
- ConsumedError(DAWN_OUT_OF_MEMORY_ERROR("Failed to allocate memory for buffer mapping"),
- InternalErrorType::OutOfMemory);
- }
- }
// Set the size of the error buffer to 0 as this function is called only when an OOM happens at
// the client side.
fakeDescriptor.size = 0;
+
+ if (maybeError.IsError()) {
+ std::unique_ptr<ErrorData> error = maybeError.AcquireError();
+ error->AppendContext("calling %s.CreateBuffer(%s).", this, desc);
+ HandleError(std::move(error), InternalErrorType::OutOfMemory);
+ } else {
+ const DawnBufferDescriptorErrorInfoFromWireClient* clientErrorInfo = nullptr;
+ FindInChain(desc->nextInChain, &clientErrorInfo);
+ if (clientErrorInfo != nullptr && clientErrorInfo->outOfMemory) {
+ HandleError(DAWN_OUT_OF_MEMORY_ERROR("Failed to allocate memory for buffer mapping"),
+ InternalErrorType::OutOfMemory);
+ }
+ }
+
return BufferBase::MakeError(this, &fakeDescriptor);
}
@@ -1401,7 +1404,7 @@
}
void DeviceBase::APIValidateTextureDescriptor(const TextureDescriptor* desc) {
- ConsumedError(ValidateTextureDescriptor(this, desc));
+ DAWN_UNUSED(ConsumedError(ValidateTextureDescriptor(this, desc)));
}
QueueBase* DeviceBase::GetQueue() const {
diff --git a/src/dawn/native/Device.h b/src/dawn/native/Device.h
index ba3c7c7..0507bc2 100644
--- a/src/dawn/native/Device.h
+++ b/src/dawn/native/Device.h
@@ -76,8 +76,12 @@
InternalErrorType additionalAllowedErrors = InternalErrorType::None,
WGPUDeviceLostReason lost_reason = WGPUDeviceLostReason_Undefined);
- bool ConsumedError(MaybeError maybeError,
- InternalErrorType additionalAllowedErrors = InternalErrorType::None) {
+ // Variants of ConsumedError must use the returned boolean to handle failure cases since an
+ // error may cause a device loss and further execution may be undefined. This is especially
+ // true for the ResultOrError variants.
+ [[nodiscard]] bool ConsumedError(
+ MaybeError maybeError,
+ InternalErrorType additionalAllowedErrors = InternalErrorType::None) {
if (DAWN_UNLIKELY(maybeError.IsError())) {
ConsumeError(maybeError.AcquireError(), additionalAllowedErrors);
return true;
@@ -86,9 +90,10 @@
}
template <typename T>
- bool ConsumedError(ResultOrError<T> resultOrError,
- T* result,
- InternalErrorType additionalAllowedErrors = InternalErrorType::None) {
+ [[nodiscard]] bool ConsumedError(
+ ResultOrError<T> resultOrError,
+ T* result,
+ InternalErrorType additionalAllowedErrors = InternalErrorType::None) {
if (DAWN_UNLIKELY(resultOrError.IsError())) {
ConsumeError(resultOrError.AcquireError(), additionalAllowedErrors);
return true;
@@ -98,10 +103,10 @@
}
template <typename... Args>
- bool ConsumedError(MaybeError maybeError,
- InternalErrorType additionalAllowedErrors,
- const char* formatStr,
- const Args&... args) {
+ [[nodiscard]] bool ConsumedError(MaybeError maybeError,
+ InternalErrorType additionalAllowedErrors,
+ const char* formatStr,
+ const Args&... args) {
if (DAWN_UNLIKELY(maybeError.IsError())) {
std::unique_ptr<ErrorData> error = maybeError.AcquireError();
if (error->GetType() == InternalErrorType::Validation) {
@@ -114,17 +119,18 @@
}
template <typename... Args>
- bool ConsumedError(MaybeError maybeError, const char* formatStr, Args&&... args) {
- return ConsumedError(std::move(maybeError), InternalErrorType::None, formatStr,
- std::forward<Args>(args)...);
+ [[nodiscard]] bool ConsumedError(MaybeError maybeError,
+ const char* formatStr,
+ const Args&... args) {
+ return ConsumedError(std::move(maybeError), InternalErrorType::None, formatStr, args...);
}
template <typename T, typename... Args>
- bool ConsumedError(ResultOrError<T> resultOrError,
- T* result,
- InternalErrorType additionalAllowedErrors,
- const char* formatStr,
- const Args&... args) {
+ [[nodiscard]] bool ConsumedError(ResultOrError<T> resultOrError,
+ T* result,
+ InternalErrorType additionalAllowedErrors,
+ const char* formatStr,
+ const Args&... args) {
if (DAWN_UNLIKELY(resultOrError.IsError())) {
std::unique_ptr<ErrorData> error = resultOrError.AcquireError();
if (error->GetType() == InternalErrorType::Validation) {
@@ -138,12 +144,12 @@
}
template <typename T, typename... Args>
- bool ConsumedError(ResultOrError<T> resultOrError,
- T* result,
- const char* formatStr,
- Args&&... args) {
+ [[nodiscard]] bool ConsumedError(ResultOrError<T> resultOrError,
+ T* result,
+ const char* formatStr,
+ const Args&... args) {
return ConsumedError(std::move(resultOrError), result, InternalErrorType::None, formatStr,
- std::forward<Args>(args)...);
+ args...);
}
MaybeError ValidateObject(const ApiObjectBase* object) const;
diff --git a/src/dawn/native/Queue.cpp b/src/dawn/native/Queue.cpp
index a8d92e3..926e13a 100644
--- a/src/dawn/native/Queue.cpp
+++ b/src/dawn/native/Queue.cpp
@@ -281,7 +281,7 @@
uint64_t bufferOffset,
const void* data,
size_t size) {
- GetDevice()->ConsumedError(WriteBuffer(buffer, bufferOffset, data, size));
+ DAWN_UNUSED(GetDevice()->ConsumedError(WriteBuffer(buffer, bufferOffset, data, size)));
}
MaybeError QueueBase::WriteBuffer(BufferBase* buffer,
@@ -322,8 +322,8 @@
size_t dataSize,
const TextureDataLayout* dataLayout,
const Extent3D* writeSize) {
- GetDevice()->ConsumedError(
- WriteTextureInternal(destination, data, dataSize, *dataLayout, writeSize));
+ DAWN_UNUSED(GetDevice()->ConsumedError(
+ WriteTextureInternal(destination, data, dataSize, *dataLayout, writeSize)));
}
MaybeError QueueBase::WriteTextureInternal(const ImageCopyTexture* destination,
@@ -389,16 +389,16 @@
const ImageCopyTexture* destination,
const Extent3D* copySize,
const CopyTextureForBrowserOptions* options) {
- GetDevice()->ConsumedError(
- CopyTextureForBrowserInternal(source, destination, copySize, options));
+ DAWN_UNUSED(GetDevice()->ConsumedError(
+ CopyTextureForBrowserInternal(source, destination, copySize, options)));
}
void QueueBase::APICopyExternalTextureForBrowser(const ImageCopyExternalTexture* source,
const ImageCopyTexture* destination,
const Extent3D* copySize,
const CopyTextureForBrowserOptions* options) {
- GetDevice()->ConsumedError(
- CopyExternalTextureForBrowserInternal(source, destination, copySize, options));
+ DAWN_UNUSED(GetDevice()->ConsumedError(
+ CopyExternalTextureForBrowserInternal(source, destination, copySize, options)));
}
MaybeError QueueBase::CopyTextureForBrowserInternal(const ImageCopyTexture* source,
diff --git a/src/dawn/native/RenderPassEncoder.cpp b/src/dawn/native/RenderPassEncoder.cpp
index 3b5a062..5b3e409 100644
--- a/src/dawn/native/RenderPassEncoder.cpp
+++ b/src/dawn/native/RenderPassEncoder.cpp
@@ -137,7 +137,7 @@
void RenderPassEncoder::APIEnd() {
if (mEnded && IsValidationEnabled()) {
- GetDevice()->ConsumedError(DAWN_VALIDATION_ERROR("%s was already ended.", this));
+ GetDevice()->HandleError(DAWN_VALIDATION_ERROR("%s was already ended.", this));
return;
}
diff --git a/src/dawn/native/SwapChain.cpp b/src/dawn/native/SwapChain.cpp
index 3e1d3de..a43067f 100644
--- a/src/dawn/native/SwapChain.cpp
+++ b/src/dawn/native/SwapChain.cpp
@@ -35,16 +35,16 @@
wgpu::TextureUsage allowedUsage,
uint32_t width,
uint32_t height) override {
- GetDevice()->ConsumedError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
+ GetDevice()->HandleError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
}
TextureViewBase* APIGetCurrentTextureView() override {
- GetDevice()->ConsumedError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
+ GetDevice()->HandleError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
return TextureViewBase::MakeError(GetDevice());
}
void APIPresent() override {
- GetDevice()->ConsumedError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
+ GetDevice()->HandleError(DAWN_VALIDATION_ERROR("%s is an error swapchain.", this));
}
};
@@ -300,7 +300,7 @@
wgpu::TextureUsage allowedUsage,
uint32_t width,
uint32_t height) {
- GetDevice()->ConsumedError(
+ GetDevice()->HandleError(
DAWN_VALIDATION_ERROR("Configure is invalid for surface-based swapchains."));
}
diff --git a/src/dawn/tests/unittests/validation/DeviceValidationTests.cpp b/src/dawn/tests/unittests/validation/DeviceValidationTests.cpp
index e1c9e0b..041ca96 100644
--- a/src/dawn/tests/unittests/validation/DeviceValidationTests.cpp
+++ b/src/dawn/tests/unittests/validation/DeviceValidationTests.cpp
@@ -226,6 +226,6 @@
ExpectDeviceDestruction();
device.Destroy();
dawn::native::DeviceBase* nativeDevice = dawn::native::FromAPI(device.Get());
- ASSERT_DEVICE_ERROR(nativeDevice->ConsumedError(nativeDevice->Tick()),
+ ASSERT_DEVICE_ERROR(EXPECT_TRUE(nativeDevice->ConsumedError(nativeDevice->Tick())),
HasSubstr("[Device] is lost."));
}
