Improve validation errors for encoders

Improves the validation messages in ComputePassEncoder.cpp,
ProgrammablePassEncoder.cpp, RenderBundleEncoder.cpp, and
EncodingContext.cpp/h to give them more contextual information.

Bug: dawn:563
Change-Id: I3807c30fb0de8e766fbc35b98ef9c059f9d441ee
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/67603
Commit-Queue: Brandon Jones <bajones@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
This commit is contained in:
Brandon Jones 2021-10-28 00:13:17 +00:00 committed by Dawn LUCI CQ
parent 5e2e2d863e
commit f33afcdba6
8 changed files with 164 additions and 81 deletions

View File

@ -891,8 +891,7 @@ namespace dawn_native {
if (GetDevice()->IsValidationEnabled()) {
DAWN_INVALID_IF(
mDebugGroupStackSize == 0,
"Every call to PopDebugGroup must be balanced by a corresponding call to "
"PushDebugGroup.");
"PopDebugGroup called when no debug groups are currently pushed.");
}
allocator->Allocate<PopDebugGroupCmd>(Command::PopDebugGroup);
mDebugGroupStackSize--;

View File

@ -26,18 +26,6 @@
namespace dawn_native {
namespace {
MaybeError ValidatePerDimensionDispatchSizeLimit(const DeviceBase* device, uint32_t size) {
if (size > device->GetLimits().v1.maxComputeWorkgroupsPerDimension) {
return DAWN_VALIDATION_ERROR("Dispatch size exceeds defined limits");
}
return {};
}
} // namespace
ComputePassEncoder::ComputePassEncoder(DeviceBase* device,
CommandEncoder* commandEncoder,
EncodingContext* encodingContext)
@ -85,9 +73,24 @@ namespace dawn_native {
[&](CommandAllocator* allocator) -> MaybeError {
if (IsValidationEnabled()) {
DAWN_TRY(mCommandBufferState.ValidateCanDispatch());
DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), x));
DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), y));
DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), z));
uint32_t workgroupsPerDimension =
GetDevice()->GetLimits().v1.maxComputeWorkgroupsPerDimension;
DAWN_INVALID_IF(
x > workgroupsPerDimension,
"Dispatch size X (%u) exceeds max compute workgroups per dimension (%u).",
x, workgroupsPerDimension);
DAWN_INVALID_IF(
y > workgroupsPerDimension,
"Dispatch size Y (%u) exceeds max compute workgroups per dimension (%u).",
y, workgroupsPerDimension);
DAWN_INVALID_IF(
z > workgroupsPerDimension,
"Dispatch size Z (%u) exceeds max compute workgroups per dimension (%u).",
z, workgroupsPerDimension);
}
// Record the synchronization scope for Dispatch, which is just the current
@ -117,21 +120,20 @@ namespace dawn_native {
// Indexed dispatches need a compute-shader based validation to check that the
// dispatch sizes aren't too big. Disallow them as unsafe until the validation
// is implemented.
if (GetDevice()->IsToggleEnabled(Toggle::DisallowUnsafeAPIs)) {
return DAWN_VALIDATION_ERROR(
"DispatchIndirect is disallowed because it doesn't validate that the "
"dispatch "
"size is valid yet.");
}
DAWN_INVALID_IF(
GetDevice()->IsToggleEnabled(Toggle::DisallowUnsafeAPIs),
"DispatchIndirect is disallowed because it doesn't validate that the "
"dispatch size is valid yet.");
if (indirectOffset % 4 != 0) {
return DAWN_VALIDATION_ERROR("Indirect offset must be a multiple of 4");
}
DAWN_INVALID_IF(indirectOffset % 4 != 0,
"Indirect offset (%u) is not a multiple of 4.", indirectOffset);
if (indirectOffset >= indirectBuffer->GetSize() ||
indirectOffset + kDispatchIndirectSize > indirectBuffer->GetSize()) {
return DAWN_VALIDATION_ERROR("Indirect offset out of bounds");
}
DAWN_INVALID_IF(
indirectOffset >= indirectBuffer->GetSize() ||
indirectOffset + kDispatchIndirectSize > indirectBuffer->GetSize(),
"Indirect offset (%u) and dispatch size (%u) exceeds the indirect buffer "
"size (%u).",
indirectOffset, kDispatchIndirectSize, indirectBuffer->GetSize());
}
// Record the synchronization scope for Dispatch, both the bindgroups and the

View File

@ -24,7 +24,7 @@
namespace dawn_native {
EncodingContext::EncodingContext(DeviceBase* device, const ObjectBase* initialEncoder)
EncodingContext::EncodingContext(DeviceBase* device, const ApiObjectBase* initialEncoder)
: mDevice(device), mTopLevelEncoder(initialEncoder), mCurrentEncoder(initialEncoder) {
}
@ -87,7 +87,7 @@ namespace dawn_native {
}
}
void EncodingContext::EnterPass(const ObjectBase* passEncoder) {
void EncodingContext::EnterPass(const ApiObjectBase* passEncoder) {
// Assert we're at the top level.
ASSERT(mCurrentEncoder == mTopLevelEncoder);
ASSERT(passEncoder != nullptr);
@ -95,7 +95,7 @@ namespace dawn_native {
mCurrentEncoder = passEncoder;
}
MaybeError EncodingContext::ExitRenderPass(const ObjectBase* passEncoder,
MaybeError EncodingContext::ExitRenderPass(const ApiObjectBase* passEncoder,
RenderPassResourceUsageTracker usageTracker,
CommandEncoder* commandEncoder,
IndirectDrawMetadata indirectDrawMetadata) {
@ -121,7 +121,7 @@ namespace dawn_native {
return {};
}
void EncodingContext::ExitComputePass(const ObjectBase* passEncoder,
void EncodingContext::ExitComputePass(const ApiObjectBase* passEncoder,
ComputePassResourceUsage usages) {
ASSERT(mCurrentEncoder != mTopLevelEncoder);
ASSERT(mCurrentEncoder == passEncoder);
@ -130,6 +130,15 @@ namespace dawn_native {
mComputePassUsages.push_back(std::move(usages));
}
void EncodingContext::EnsurePassExited(const ApiObjectBase* passEncoder) {
if (mCurrentEncoder != mTopLevelEncoder && mCurrentEncoder == passEncoder) {
// The current pass encoder is being deleted. Implicitly end the pass with an error.
mCurrentEncoder = mTopLevelEncoder;
HandleError(DAWN_FORMAT_VALIDATION_ERROR(
"Command buffer recording ended before %s was ended.", passEncoder));
}
}
const RenderPassUsages& EncodingContext::GetRenderPassUsages() const {
ASSERT(!mWereRenderPassUsagesAcquired);
return mRenderPassUsages;
@ -161,12 +170,10 @@ namespace dawn_native {
}
MaybeError EncodingContext::Finish() {
if (IsFinished()) {
return DAWN_VALIDATION_ERROR("Command encoding already finished");
}
DAWN_INVALID_IF(IsFinished(), "Command encoding already finished.");
const void* currentEncoder = mCurrentEncoder;
const void* topLevelEncoder = mTopLevelEncoder;
const ApiObjectBase* currentEncoder = mCurrentEncoder;
const ApiObjectBase* topLevelEncoder = mTopLevelEncoder;
// Even if finish validation fails, it is now invalid to call any encoding commands,
// so we clear the encoders. Note: mTopLevelEncoder == nullptr is used as a flag for
@ -178,9 +185,8 @@ namespace dawn_native {
if (mError != nullptr) {
return std::move(mError);
}
if (currentEncoder != topLevelEncoder) {
return DAWN_VALIDATION_ERROR("Command buffer recording ended mid-pass");
}
DAWN_INVALID_IF(currentEncoder != topLevelEncoder,
"Command buffer recording ended before %s was ended.", currentEncoder);
return {};
}

View File

@ -28,13 +28,13 @@ namespace dawn_native {
class CommandEncoder;
class DeviceBase;
class ObjectBase;
class ApiObjectBase;
// Base class for allocating/iterating commands.
// It performs error tracking as well as encoding state for render/compute passes.
class EncodingContext {
public:
EncodingContext(DeviceBase* device, const ObjectBase* initialEncoder);
EncodingContext(DeviceBase* device, const ApiObjectBase* initialEncoder);
~EncodingContext();
CommandIterator AcquireCommands();
@ -70,14 +70,15 @@ namespace dawn_native {
return false;
}
inline bool CheckCurrentEncoder(const ObjectBase* encoder) {
inline bool CheckCurrentEncoder(const ApiObjectBase* encoder) {
if (DAWN_UNLIKELY(encoder != mCurrentEncoder)) {
if (mCurrentEncoder != mTopLevelEncoder) {
// The top level encoder was used when a pass encoder was current.
HandleError(DAWN_VALIDATION_ERROR("Command cannot be recorded inside a pass"));
HandleError(DAWN_FORMAT_VALIDATION_ERROR(
"Command cannot be recorded while %s is active.", mCurrentEncoder));
} else {
HandleError(DAWN_VALIDATION_ERROR(
"Recording in an error or already ended pass encoder"));
HandleError(DAWN_FORMAT_VALIDATION_ERROR(
"Recording in an error or already ended %s.", encoder));
}
return false;
}
@ -85,7 +86,7 @@ namespace dawn_native {
}
template <typename EncodeFunction>
inline bool TryEncode(const ObjectBase* encoder, EncodeFunction&& encodeFunction) {
inline bool TryEncode(const ApiObjectBase* encoder, EncodeFunction&& encodeFunction) {
if (!CheckCurrentEncoder(encoder)) {
return false;
}
@ -94,7 +95,7 @@ namespace dawn_native {
}
template <typename EncodeFunction, typename... Args>
inline bool TryEncode(const ObjectBase* encoder,
inline bool TryEncode(const ApiObjectBase* encoder,
EncodeFunction&& encodeFunction,
const char* formatStr,
const Args&... args) {
@ -111,14 +112,18 @@ namespace dawn_native {
void WillBeginRenderPass();
// Functions to set current encoder state
void EnterPass(const ObjectBase* passEncoder);
MaybeError ExitRenderPass(const ObjectBase* passEncoder,
void EnterPass(const ApiObjectBase* passEncoder);
MaybeError ExitRenderPass(const ApiObjectBase* passEncoder,
RenderPassResourceUsageTracker usageTracker,
CommandEncoder* commandEncoder,
IndirectDrawMetadata indirectDrawMetadata);
void ExitComputePass(const ObjectBase* passEncoder, ComputePassResourceUsage usages);
void ExitComputePass(const ApiObjectBase* passEncoder, ComputePassResourceUsage usages);
MaybeError Finish();
// Called when a pass encoder is deleted. Provides an opportunity to clean up if it's the
// mCurrentEncoder.
void EnsurePassExited(const ApiObjectBase* passEncoder);
const RenderPassUsages& GetRenderPassUsages() const;
const ComputePassUsages& GetComputePassUsages() const;
RenderPassUsages AcquireRenderPassUsages();
@ -138,12 +143,12 @@ namespace dawn_native {
// There can only be two levels of encoders. Top-level and render/compute pass.
// The top level encoder is the encoder the EncodingContext is created with.
// It doubles as flag to check if encoding has been Finished.
const ObjectBase* mTopLevelEncoder;
const ApiObjectBase* mTopLevelEncoder;
// The current encoder must be the same as the encoder provided to TryEncode,
// otherwise an error is produced. It may be nullptr if the EncodingContext is an error.
// The current encoder changes with Enter/ExitPass which should be called by
// CommandEncoder::Begin/EndPass.
const ObjectBase* mCurrentEncoder;
const ApiObjectBase* mCurrentEncoder;
RenderPassUsages mRenderPassUsages;
bool mWereRenderPassUsagesAcquired = false;

View File

@ -43,14 +43,21 @@ namespace dawn_native {
mValidationEnabled(device->IsValidationEnabled()) {
}
void ProgrammablePassEncoder::DeleteThis() {
// This must be called prior to the destructor because it may generate an error message
// which calls the virtual RenderPassEncoder->GetType() as part of it's formatting.
mEncodingContext->EnsurePassExited(this);
ApiObjectBase::DeleteThis();
}
bool ProgrammablePassEncoder::IsValidationEnabled() const {
return mValidationEnabled;
}
MaybeError ProgrammablePassEncoder::ValidateProgrammableEncoderEnd() const {
if (mDebugGroupStackSize != 0) {
return DAWN_VALIDATION_ERROR("Each Push must be balanced by a corresponding Pop.");
}
DAWN_INVALID_IF(mDebugGroupStackSize != 0,
"PushDebugGroup called %u time(s) without a corresponding PopDebugGroup.",
mDebugGroupStackSize);
return {};
}
@ -75,10 +82,9 @@ namespace dawn_native {
this,
[&](CommandAllocator* allocator) -> MaybeError {
if (IsValidationEnabled()) {
if (mDebugGroupStackSize == 0) {
return DAWN_VALIDATION_ERROR(
"Pop must be balanced by a corresponding Push.");
}
DAWN_INVALID_IF(
mDebugGroupStackSize == 0,
"PopDebugGroup called when no debug groups are currently pushed.");
}
allocator->Allocate<PopDebugGroupCmd>(Command::PopDebugGroup);
mDebugGroupStackSize--;
@ -115,18 +121,21 @@ namespace dawn_native {
const uint32_t* dynamicOffsetsIn) const {
DAWN_TRY(GetDevice()->ValidateObject(group));
if (index >= kMaxBindGroupsTyped) {
return DAWN_VALIDATION_ERROR("Setting bind group over the max");
}
DAWN_INVALID_IF(index >= kMaxBindGroupsTyped,
"Bind group index (%u) exceeds the maximum (%u).",
static_cast<uint32_t>(index), kMaxBindGroups);
ityp::span<BindingIndex, const uint32_t> dynamicOffsets(dynamicOffsetsIn,
BindingIndex(dynamicOffsetCountIn));
// Dynamic offsets count must match the number required by the layout perfectly.
const BindGroupLayoutBase* layout = group->GetLayout();
if (layout->GetDynamicBufferCount() != dynamicOffsets.size()) {
return DAWN_VALIDATION_ERROR("dynamicOffset count mismatch");
}
DAWN_INVALID_IF(
layout->GetDynamicBufferCount() != dynamicOffsets.size(),
"The number of dynamic offsets (%u) does not match the number of dynamic buffers (%u) "
"in %s.",
static_cast<uint32_t>(dynamicOffsets.size()),
static_cast<uint32_t>(layout->GetDynamicBufferCount()), layout);
for (BindingIndex i{0}; i < dynamicOffsets.size(); ++i) {
const BindingInfo& bindingInfo = layout->GetBindingInfo(i);
@ -150,9 +159,9 @@ namespace dawn_native {
UNREACHABLE();
}
if (!IsAligned(dynamicOffsets[i], requiredAlignment)) {
return DAWN_VALIDATION_ERROR("Dynamic Buffer Offset need to be aligned");
}
DAWN_INVALID_IF(!IsAligned(dynamicOffsets[i], requiredAlignment),
"Dynamic Offset[%u] (%u) is not %u byte aligned.",
static_cast<uint32_t>(i), dynamicOffsets[i], requiredAlignment);
BufferBinding bufferBinding = group->GetBindingAsBufferBinding(i);
@ -163,15 +172,20 @@ namespace dawn_native {
if ((dynamicOffsets[i] >
bufferBinding.buffer->GetSize() - bufferBinding.offset - bufferBinding.size)) {
if ((bufferBinding.buffer->GetSize() - bufferBinding.offset) ==
bufferBinding.size) {
return DAWN_VALIDATION_ERROR(
"Dynamic offset out of bounds. The binding goes to the end of the "
"buffer even with a dynamic offset of 0. Did you forget to specify "
"the binding's size?");
} else {
return DAWN_VALIDATION_ERROR("Dynamic offset out of bounds");
}
DAWN_INVALID_IF(
(bufferBinding.buffer->GetSize() - bufferBinding.offset) == bufferBinding.size,
"Dynamic Offset[%u] (%u) is out of bounds of %s with a size of %u and a bound "
"range of (offset: %u, size: %u). The binding goes to the end of the buffer "
"even with a dynamic offset of 0. Did you forget to specify "
"the binding's size?",
static_cast<uint32_t>(i), dynamicOffsets[i], bufferBinding.buffer,
bufferBinding.buffer->GetSize(), bufferBinding.offset, bufferBinding.size);
return DAWN_FORMAT_VALIDATION_ERROR(
"Dynamic Offset[%u] (%u) is out of bounds of "
"%s with a size of %u and a bound range of (offset: %u, size: %u).",
static_cast<uint32_t>(i), dynamicOffsets[i], bufferBinding.buffer,
bufferBinding.buffer->GetSize(), bufferBinding.offset, bufferBinding.size);
}
}

View File

@ -40,6 +40,8 @@ namespace dawn_native {
bool IsValidationEnabled() const;
MaybeError ValidateProgrammableEncoderEnd() const;
virtual void DeleteThis() override;
// Compute and render passes do different things on SetBindGroup. These are helper functions
// for the logic they have in common.
MaybeError ValidateSetBindGroup(BindGroupIndex index,

View File

@ -123,7 +123,8 @@ namespace dawn_native {
RenderBundleBase* RenderBundleEncoder::APIFinish(const RenderBundleDescriptor* descriptor) {
RenderBundleBase* result = nullptr;
if (GetDevice()->ConsumedError(FinishImpl(descriptor), &result)) {
if (GetDevice()->ConsumedError(FinishImpl(descriptor), &result, "calling Finish(%s).",
descriptor)) {
return RenderBundleBase::MakeError(GetDevice());
}

View File

@ -206,6 +206,60 @@ TEST_F(CommandBufferValidationTest, CallsAfterAFailedFinish) {
ASSERT_DEVICE_ERROR(encoder.CopyBufferToBuffer(copyBuffer, 0, copyBuffer, 0, 0));
}
// Test that passes which are de-referenced prior to ending still allow the correct errors to be
// produced.
TEST_F(CommandBufferValidationTest, PassDereferenced) {
DummyRenderPass dummyRenderPass(device);
// Control case, command buffer ended after the pass is ended.
{
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&dummyRenderPass);
pass.EndPass();
encoder.Finish();
}
// Error case, no reference is kept to a render pass.
{
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
encoder.BeginRenderPass(&dummyRenderPass);
ASSERT_DEVICE_ERROR(encoder.Finish());
}
// Error case, no reference is kept to a compute pass.
{
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
encoder.BeginComputePass();
ASSERT_DEVICE_ERROR(encoder.Finish());
}
// Error case, beginning a new pass after failing to end a de-referenced pass.
{
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
encoder.BeginRenderPass(&dummyRenderPass);
wgpu::ComputePassEncoder pass = encoder.BeginComputePass();
pass.EndPass();
ASSERT_DEVICE_ERROR(encoder.Finish());
}
// Error case, deleting the pass after finishing the commend encoder shouldn't generate an
// uncaptured error.
{
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
wgpu::ComputePassEncoder pass = encoder.BeginComputePass();
ASSERT_DEVICE_ERROR(encoder.Finish());
pass = nullptr;
}
// Valid case, command encoder is never finished so the de-referenced pass shouldn't generate an
// uncaptured error.
{
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
encoder.BeginComputePass();
}
}
// Test that calling inject validation error produces an error.
TEST_F(CommandBufferValidationTest, InjectValidationError) {
wgpu::CommandEncoder encoder = device.CreateCommandEncoder();