From 0982dcea690069dc9a071a8252f36b37d87f820a Mon Sep 17 00:00:00 2001 From: Shrek Shao Date: Tue, 29 Nov 2022 23:05:30 +0000 Subject: [PATCH] Add DisallowDeprecatedPath toggle The runtime toggle is off by default. When turned on, the deprecation warning will be turned into validation error. Replace device->EmitDeprecationWarning with DAWN_MAKE_DEPRECATION_ERROR macro which make an internal validation error or make a MaybeError{} based on the toggle. The callsite can wrap it with a DAWN_TRY. Bug: dawn:1563, dawn:1525, dawn:1269, dawn:1602 Change-Id: I7fd6f4f8ffc2e054e5fc5fc4aaf23c47f5733847 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/111321 Reviewed-by: Austin Eng Commit-Queue: Shrek Shao Kokoro: Kokoro --- src/dawn/native/BindGroupLayout.cpp | 8 +-- src/dawn/native/Buffer.cpp | 8 +-- src/dawn/native/CommandEncoder.cpp | 36 +++++----- src/dawn/native/ComputePassEncoder.cpp | 18 +++-- src/dawn/native/Device.cpp | 6 +- src/dawn/native/Device.h | 2 +- src/dawn/native/Error.h | 9 +++ src/dawn/native/RenderPassEncoder.cpp | 5 +- src/dawn/native/Toggles.cpp | 7 ++ src/dawn/native/Toggles.h | 1 + src/dawn/tests/end2end/DeprecatedAPITests.cpp | 68 +++++++++++++------ 11 files changed, 112 insertions(+), 56 deletions(-) diff --git a/src/dawn/native/BindGroupLayout.cpp b/src/dawn/native/BindGroupLayout.cpp index ebd1bde981..abc1e4c761 100644 --- a/src/dawn/native/BindGroupLayout.cpp +++ b/src/dawn/native/BindGroupLayout.cpp @@ -115,11 +115,9 @@ MaybeError ValidateBindGroupLayoutEntry(DeviceBase* device, viewDimension, wgpu::TextureViewDimension::e2D); if (texture.multisampled && texture.sampleType == wgpu::TextureSampleType::Float) { - std::string warning = absl::StrFormat( - "Sample type %s for multisampled texture bindings is deprecated " - "and will be invalid in a future version.", - wgpu::TextureSampleType::Float); - device->EmitDeprecationWarning(warning.c_str()); + DAWN_TRY(DAWN_MAKE_DEPRECATION_ERROR( + device, "Sample type %s for multisampled texture bindings was %s.", + texture.sampleType, wgpu::TextureSampleType::Float)); } } diff --git a/src/dawn/native/Buffer.cpp b/src/dawn/native/Buffer.cpp index d2bdb62ef4..df7e758f30 100644 --- a/src/dawn/native/Buffer.cpp +++ b/src/dawn/native/Buffer.cpp @@ -124,12 +124,10 @@ MaybeError ValidateBufferDescriptor(DeviceBase* device, const BufferDescriptor* "Buffer is mapped at creation but its size (%u) is not a multiple of 4.", descriptor->size); - // TODO(dawn:1525): Change to validation error after the deprecation period. if (descriptor->size > device->GetLimits().v1.maxBufferSize) { - std::string warning = - absl::StrFormat("Buffer size (%u) exceeds the max buffer size limit (%u).", - descriptor->size, device->GetLimits().v1.maxBufferSize); - device->EmitDeprecationWarning(warning.c_str()); + DAWN_TRY(DAWN_MAKE_DEPRECATION_ERROR( + device, "Buffer size (%u) exceeds the max buffer size limit (%u).", descriptor->size, + device->GetLimits().v1.maxBufferSize)); } return {}; diff --git a/src/dawn/native/CommandEncoder.cpp b/src/dawn/native/CommandEncoder.cpp index 649fcb1f32..13c8431caa 100644 --- a/src/dawn/native/CommandEncoder.cpp +++ b/src/dawn/native/CommandEncoder.cpp @@ -241,9 +241,10 @@ MaybeError ValidateRenderPassColorAttachment(DeviceBase* device, bool useClearColor = HasDeprecatedColor(colorAttachment); const dawn::native::Color& clearValue = useClearColor ? colorAttachment.clearColor : colorAttachment.clearValue; + if (useClearColor) { - device->EmitDeprecationWarning( - "clearColor is deprecated, prefer using clearValue instead."); + DAWN_TRY(DAWN_MAKE_DEPRECATION_ERROR( + device, "clearColor is deprecated, prefer using clearValue instead.")); } if (colorAttachment.loadOp == wgpu::LoadOp::Clear) { @@ -306,11 +307,12 @@ MaybeError ValidateRenderPassDepthStencilAttachment( !IsSubset(Aspect::Depth, attachment->GetAspects())) { if (depthStencilAttachment->depthLoadOp == wgpu::LoadOp::Load && depthStencilAttachment->depthStoreOp == wgpu::StoreOp::Store) { - // TODO(dawn:1269): Remove this branch after the deprecation period. - device->EmitDeprecationWarning( - "Setting depthLoadOp and depthStoreOp when " - "the attachment has no depth aspect or depthReadOnly is true is " - "deprecated."); + DAWN_TRY(DAWN_MAKE_DEPRECATION_ERROR( + device, + "depthLoadOp is (%s) and depthStoreOp is (%s) " + "when depthReadOnly (%u) or the attachment (%s) has no depth aspect.", + depthStencilAttachment->depthLoadOp, depthStencilAttachment->depthStoreOp, + depthStencilAttachment->depthReadOnly, attachment)); } else { DAWN_INVALID_IF(depthStencilAttachment->depthLoadOp != wgpu::LoadOp::Undefined, "depthLoadOp (%s) must not be set if the attachment (%s) has " @@ -341,11 +343,12 @@ MaybeError ValidateRenderPassDepthStencilAttachment( !IsSubset(Aspect::Stencil, attachment->GetAspects())) { if (depthStencilAttachment->stencilLoadOp == wgpu::LoadOp::Load && depthStencilAttachment->stencilStoreOp == wgpu::StoreOp::Store) { - // TODO(dawn:1269): Remove this branch after the deprecation period. - device->EmitDeprecationWarning( - "Setting stencilLoadOp and stencilStoreOp when " - "the attachment has no stencil aspect or stencilReadOnly is true is " - "deprecated."); + DAWN_TRY(DAWN_MAKE_DEPRECATION_ERROR( + device, + "stencilLoadOp is (%s) and stencilStoreOp is (%s) " + "when stencilReadOnly (%u) or the attachment (%s) has no stencil aspect.", + depthStencilAttachment->stencilLoadOp, depthStencilAttachment->stencilStoreOp, + depthStencilAttachment->stencilReadOnly, attachment)); } else { DAWN_INVALID_IF( depthStencilAttachment->stencilLoadOp != wgpu::LoadOp::Undefined, @@ -376,8 +379,8 @@ MaybeError ValidateRenderPassDepthStencilAttachment( } if (!std::isnan(depthStencilAttachment->clearDepth)) { - // TODO(dawn:1269): Remove this branch after the deprecation period. - device->EmitDeprecationWarning("clearDepth is deprecated, prefer depthClearValue instead."); + DAWN_TRY(DAWN_MAKE_DEPRECATION_ERROR( + device, "clearDepth is deprecated, prefer depthClearValue instead.")); DAWN_INVALID_IF( depthStencilAttachment->clearDepth < 0.0f || depthStencilAttachment->clearDepth > 1.0f, "clearDepth is not between 0.0 and 1.0"); @@ -390,11 +393,10 @@ MaybeError ValidateRenderPassDepthStencilAttachment( "depthClearValue is not between 0.0 and 1.0"); } - // TODO(dawn:1269): Remove after the deprecation period. if (depthStencilAttachment->stencilClearValue == 0 && depthStencilAttachment->clearStencil != 0) { - device->EmitDeprecationWarning( - "clearStencil is deprecated, prefer stencilClearValue instead."); + DAWN_TRY(DAWN_MAKE_DEPRECATION_ERROR( + device, "clearStencil is deprecated, prefer stencilClearValue instead.")); } // *sampleCount == 0 must only happen when there is no color attachment. In that case we diff --git a/src/dawn/native/ComputePassEncoder.cpp b/src/dawn/native/ComputePassEncoder.cpp index 2b9a8cecea..0480753063 100644 --- a/src/dawn/native/ComputePassEncoder.cpp +++ b/src/dawn/native/ComputePassEncoder.cpp @@ -167,15 +167,20 @@ void ComputePassEncoder::APIEnd() { } void ComputePassEncoder::APIEndPass() { - GetDevice()->EmitDeprecationWarning("endPass() has been deprecated. Use end() instead."); + if (GetDevice()->ConsumedError(DAWN_MAKE_DEPRECATION_ERROR( + GetDevice(), "endPass() has been deprecated. Use end() instead."))) { + return; + } APIEnd(); } void ComputePassEncoder::APIDispatch(uint32_t workgroupCountX, uint32_t workgroupCountY, uint32_t workgroupCountZ) { - GetDevice()->EmitDeprecationWarning( - "dispatch() has been deprecated. Use dispatchWorkgroups() instead."); + if (GetDevice()->ConsumedError(DAWN_MAKE_DEPRECATION_ERROR( + GetDevice(), "dispatch() has been deprecated. Use dispatchWorkgroups() instead."))) { + return; + } APIDispatchWorkgroups(workgroupCountX, workgroupCountY, workgroupCountZ); } @@ -313,8 +318,11 @@ ComputePassEncoder::TransformIndirectDispatchBuffer(Ref indirectBuff } void ComputePassEncoder::APIDispatchIndirect(BufferBase* indirectBuffer, uint64_t indirectOffset) { - GetDevice()->EmitDeprecationWarning( - "dispatchIndirect() has been deprecated. Use dispatchWorkgroupsIndirect() instead."); + if (GetDevice()->ConsumedError(DAWN_MAKE_DEPRECATION_ERROR( + GetDevice(), + "dispatchIndirect() has been deprecated. Use dispatchWorkgroupsIndirect() instead."))) { + return; + } APIDispatchWorkgroupsIndirect(indirectBuffer, indirectOffset); } diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp index 61f7162789..c713c26f7d 100644 --- a/src/dawn/native/Device.cpp +++ b/src/dawn/native/Device.cpp @@ -1333,10 +1333,10 @@ size_t DeviceBase::GetDeprecationWarningCountForTesting() { return mDeprecationWarnings->count; } -void DeviceBase::EmitDeprecationWarning(const char* warning) { +void DeviceBase::EmitDeprecationWarning(const std::string& message) { mDeprecationWarnings->count++; - if (mDeprecationWarnings->emitted.insert(warning).second) { - dawn::WarningLog() << warning; + if (mDeprecationWarnings->emitted.insert(message).second) { + dawn::WarningLog() << message; } } diff --git a/src/dawn/native/Device.h b/src/dawn/native/Device.h index a7f96d1bb7..bfba10956f 100644 --- a/src/dawn/native/Device.h +++ b/src/dawn/native/Device.h @@ -342,7 +342,7 @@ class DeviceBase : public RefCountedWithExternalCount { size_t GetLazyClearCountForTesting(); void IncrementLazyClearCountForTesting(); size_t GetDeprecationWarningCountForTesting(); - void EmitDeprecationWarning(const char* warning); + void EmitDeprecationWarning(const std::string& warning); void EmitLog(const char* message); void EmitLog(WGPULoggingType loggingType, const char* message); void APIForceLoss(wgpu::DeviceLostReason reason, const char* message); diff --git a/src/dawn/native/Error.h b/src/dawn/native/Error.h index 1f12864681..6ab93c788c 100644 --- a/src/dawn/native/Error.h +++ b/src/dawn/native/Error.h @@ -22,6 +22,7 @@ #include "absl/strings/str_format.h" #include "dawn/common/Result.h" #include "dawn/native/ErrorData.h" +#include "dawn/native/Toggles.h" #include "dawn/native/webgpu_absl_format.h" namespace dawn::native { @@ -81,6 +82,14 @@ using ResultOrError = Result; for (;;) \ break +// DAWN_MAKE_DEPRECATION_ERROR is used at deprecation paths. It returns a MaybeError. +// When the disallow_deprecated_path toggle is on, it creates an internal validation error. +// Otherwise it returns {} and emits a deprecation warning, and moves on. +#define DAWN_MAKE_DEPRECATION_ERROR(device, ...) \ + device->IsToggleEnabled(Toggle::DisallowDeprecatedAPIs) \ + ? MaybeError(DAWN_VALIDATION_ERROR(__VA_ARGS__)) \ + : (device->EmitDeprecationWarning(absl::StrFormat(__VA_ARGS__)), MaybeError{}) + // DAWN_DEVICE_LOST_ERROR means that there was a real unrecoverable native device lost error. // We can't even do a graceful shutdown because the Device is gone. #define DAWN_DEVICE_LOST_ERROR(MESSAGE) DAWN_MAKE_ERROR(InternalErrorType::DeviceLost, MESSAGE) diff --git a/src/dawn/native/RenderPassEncoder.cpp b/src/dawn/native/RenderPassEncoder.cpp index 8967c91ac0..018081e109 100644 --- a/src/dawn/native/RenderPassEncoder.cpp +++ b/src/dawn/native/RenderPassEncoder.cpp @@ -160,7 +160,10 @@ void RenderPassEncoder::APIEnd() { } void RenderPassEncoder::APIEndPass() { - GetDevice()->EmitDeprecationWarning("endPass() has been deprecated. Use end() instead."); + if (GetDevice()->ConsumedError(DAWN_MAKE_DEPRECATION_ERROR( + GetDevice(), "endPass() has been deprecated. Use end() instead."))) { + return; + } APIEnd(); } diff --git a/src/dawn/native/Toggles.cpp b/src/dawn/native/Toggles.cpp index 74afc50ee4..869b5dce9f 100644 --- a/src/dawn/native/Toggles.cpp +++ b/src/dawn/native/Toggles.cpp @@ -325,6 +325,13 @@ static constexpr ToggleEnumAndInfoList kToggleNameAndInfoList = {{ "resources. This toggle is enabled by default on D3D12 backends using Intel Gen9.5 and Gen11 " "GPUs due to a driver issue on Intel D3D12 driver.", "https://crbug.com/1237175"}}, + {Toggle::DisallowDeprecatedAPIs, + {"disallow_deprecated_apis", + "Disallow all deprecated paths by changing the deprecation warnings to validation error for " + "these paths." + "This toggle is off by default. It is expected to turn on or get removed when WebGPU V1 " + "ships and stays stable.", + "https://crbug.com/dawn/1563"}}, // Comment to separate the }} so it is clearer what to copy-paste to add a toggle. }}; } // anonymous namespace diff --git a/src/dawn/native/Toggles.h b/src/dawn/native/Toggles.h index 9a5d7920e9..d80138f94c 100644 --- a/src/dawn/native/Toggles.h +++ b/src/dawn/native/Toggles.h @@ -82,6 +82,7 @@ enum class Toggle { MetalUseMockBlitEncoderForWriteTimestamp, VulkanSplitCommandBufferOnDepthStencilComputeSampleAfterRenderPass, D3D12Allocate2DTexturewithCopyDstAsCommittedResource, + DisallowDeprecatedAPIs, EnumCount, InvalidEnum = EnumCount, diff --git a/src/dawn/tests/end2end/DeprecatedAPITests.cpp b/src/dawn/tests/end2end/DeprecatedAPITests.cpp index 3a00744de6..a015a5875d 100644 --- a/src/dawn/tests/end2end/DeprecatedAPITests.cpp +++ b/src/dawn/tests/end2end/DeprecatedAPITests.cpp @@ -25,6 +25,17 @@ #include "dawn/utils/ComboRenderPipelineDescriptor.h" #include "dawn/utils/WGPUHelpers.h" +constexpr char kDisallowDeprecatedAPIsToggleName[] = "disallow_deprecated_apis"; + +#define EXPECT_DEPRECATION_ERROR_OR_WARNING(statement) \ + if (HasToggleEnabled(kDisallowDeprecatedAPIsToggleName)) { \ + ASSERT_DEVICE_ERROR(statement); \ + } else { \ + EXPECT_DEPRECATION_WARNING(statement); \ + } \ + for (;;) \ + break + class DeprecationTests : public DawnTest { protected: void SetUp() override { @@ -36,9 +47,9 @@ class DeprecationTests : public DawnTest { // Test that setting attachment rather than view for render pass color and depth/stencil attachments // is deprecated. -TEST_P(DeprecationTests, ReadOnlyDepthStencilStoreLoadOpsAttachment) { +// TODO(dawn:1602): validation implementations need updating +TEST_P(DeprecationTests, DISABLED_ReadOnlyDepthStencilStoreLoadOpsAttachment) { utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 1, 1); - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); wgpu::RenderPassEncoder pass; // Check that setting load/store ops with read only depth/stencil attachments gives a warning. @@ -61,21 +72,28 @@ TEST_P(DeprecationTests, ReadOnlyDepthStencilStoreLoadOpsAttachment) { depthAttachment->depthLoadOp = wgpu::LoadOp::Load; depthAttachment->depthStoreOp = wgpu::StoreOp::Store; - EXPECT_DEPRECATION_WARNING(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo)); + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + EXPECT_DEPRECATION_ERROR_OR_WARNING( + pass = encoder.BeginRenderPass(&renderPass.renderPassInfo)); + } depthAttachment->depthLoadOp = wgpu::LoadOp::Undefined; depthAttachment->depthStoreOp = wgpu::StoreOp::Undefined; depthAttachment->stencilLoadOp = wgpu::LoadOp::Load; depthAttachment->stencilStoreOp = wgpu::StoreOp::Store; - EXPECT_DEPRECATION_WARNING(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo)); - - pass.End(); + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + EXPECT_DEPRECATION_ERROR_OR_WARNING( + pass = encoder.BeginRenderPass(&renderPass.renderPassInfo)); + } } // Test that setting the clearColor, clearDepth, or clearStencil values for render pass attachments // is deprecated. (dawn:1269) -TEST_P(DeprecationTests, AttachmentClearColor) { +// TODO(dawn:1602): validation implementations need updating +TEST_P(DeprecationTests, DISABLED_AttachmentClearColor) { utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 1, 1); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); wgpu::RenderPassEncoder pass; @@ -103,22 +121,19 @@ TEST_P(DeprecationTests, AttachmentClearColor) { depthAttachment->clearStencil = 1; - EXPECT_DEPRECATION_WARNING(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo)); - pass.End(); + EXPECT_DEPRECATION_ERROR_OR_WARNING(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo)); depthAttachment->clearStencil = 0; depthAttachment->depthClearValue = 0.0f; depthAttachment->clearDepth = 1.0f; - EXPECT_DEPRECATION_WARNING(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo)); - pass.End(); + EXPECT_DEPRECATION_ERROR_OR_WARNING(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo)); renderPass.renderPassInfo.depthStencilAttachment = nullptr; renderPass.renderPassInfo.cColorAttachments[0].clearColor = {1.0, 2.0, 3.0, 4.0}; renderPass.renderPassInfo.cColorAttachments[0].clearValue = {5.0, 4.0, 3.0, 2.0}; - EXPECT_DEPRECATION_WARNING(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo)); - pass.End(); + EXPECT_DEPRECATION_ERROR_OR_WARNING(pass = encoder.BeginRenderPass(&renderPass.renderPassInfo)); } // Test that endPass() is deprecated for both render and compute passes. @@ -129,13 +144,13 @@ TEST_P(DeprecationTests, EndPass) { utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 1, 1); wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo); - EXPECT_DEPRECATION_WARNING(pass.EndPass()); + EXPECT_DEPRECATION_ERROR_OR_WARNING(pass.EndPass()); } { wgpu::ComputePassEncoder pass = encoder.BeginComputePass(); - EXPECT_DEPRECATION_WARNING(pass.EndPass()); + EXPECT_DEPRECATION_ERROR_OR_WARNING(pass.EndPass()); } } @@ -161,9 +176,9 @@ TEST_P(DeprecationTests, Dispatch) { wgpu::ComputePassEncoder pass = encoder.BeginComputePass(); pass.SetPipeline(pipeline); - EXPECT_DEPRECATION_WARNING(pass.Dispatch(1)); + EXPECT_DEPRECATION_ERROR_OR_WARNING(pass.Dispatch(1)); - EXPECT_DEPRECATION_WARNING(pass.DispatchIndirect(indirectBuffer, 0)); + EXPECT_DEPRECATION_ERROR_OR_WARNING(pass.DispatchIndirect(indirectBuffer, 0)); pass.End(); } @@ -181,7 +196,16 @@ TEST_P(DeprecationTests, MaxBufferSizeValidation) { device.CreateBuffer(&descriptor); descriptor.size = GetSupportedLimits().limits.maxBufferSize + 1; - EXPECT_DEPRECATION_WARNING(device.CreateBuffer(&descriptor)); + EXPECT_DEPRECATION_ERROR_OR_WARNING(device.CreateBuffer(&descriptor)); +} + +// Test that multisampled texture with sampleType == float should be deprecated. +TEST_P(DeprecationTests, MultisampledTextureSampleType) { + EXPECT_DEPRECATION_ERROR_OR_WARNING(utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::TextureSampleType::Float, + wgpu::TextureViewDimension::e2D, true}, + })); } DAWN_INSTANTIATE_TEST(DeprecationTests, @@ -190,4 +214,10 @@ DAWN_INSTANTIATE_TEST(DeprecationTests, NullBackend(), OpenGLBackend(), OpenGLESBackend(), - VulkanBackend()); + VulkanBackend(), + D3D12Backend({kDisallowDeprecatedAPIsToggleName}), + MetalBackend({kDisallowDeprecatedAPIsToggleName}), + NullBackend({kDisallowDeprecatedAPIsToggleName}), + OpenGLBackend({kDisallowDeprecatedAPIsToggleName}), + OpenGLESBackend({kDisallowDeprecatedAPIsToggleName}), + VulkanBackend({kDisallowDeprecatedAPIsToggleName}));