From a9386f3060b4d1abaabfd4c1f3d4ffbc0c7b5627 Mon Sep 17 00:00:00 2001 From: Loko Kung Date: Tue, 22 Mar 2022 14:45:34 +0000 Subject: [PATCH] Fixes popErrorScope to match the specs. - Prepares for removal of unnecessary bool return, and just call callbacks appropriately. For now always returns true until all users are updated. - Removes PushErrorScope from handwritten commands now that we no longer need to do anything special. - Updates tests to reflect the change and make sure to set EXPECTs before calling functions to make tests easier to follow. Bug: dawn:1324, dawn:526 Change-Id: I90b09c54f9adbf2d6d50ad20dcedf68b5ed0b1fa Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/83942 Reviewed-by: Kai Ninomiya Commit-Queue: Austin Eng --- dawn_wire.json | 3 +- src/dawn/native/Device.cpp | 24 ++++-- .../validation/ErrorScopeValidationTests.cpp | 22 ++++- .../unittests/wire/WireErrorCallbackTests.cpp | 81 +++++++------------ src/dawn/wire/client/Device.cpp | 19 +---- src/dawn/wire/client/Device.h | 1 - src/dawn/wire/server/ServerDevice.cpp | 10 +-- 7 files changed, 67 insertions(+), 93 deletions(-) diff --git a/dawn_wire.json b/dawn_wire.json index 5c66ed6aa4..58ef049024 100644 --- a/dawn_wire.json +++ b/dawn_wire.json @@ -209,8 +209,7 @@ "BufferUnmap", "DeviceCreateErrorBuffer", "DeviceGetQueue", - "DeviceInjectError", - "DevicePushErrorScope" + "DeviceInjectError" ], "client_special_objects": [ "Adapter", diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp index d03a73fc20..0855ebe000 100644 --- a/src/dawn/native/Device.cpp +++ b/src/dawn/native/Device.cpp @@ -543,17 +543,25 @@ namespace dawn::native { } bool DeviceBase::APIPopErrorScope(wgpu::ErrorCallback callback, void* userdata) { + // TODO(crbug.com/dawn/1324) Remove return and make function void when users are updated. + bool returnValue = true; + if (callback == nullptr) { + static wgpu::ErrorCallback defaultCallback = [](WGPUErrorType, char const*, void*) {}; + callback = defaultCallback; + } + // TODO(crbug.com/dawn/1122): Call callbacks only on wgpuInstanceProcessEvents + if (IsLost()) { + callback(WGPUErrorType_DeviceLost, "GPU device disconnected", userdata); + return returnValue; + } if (mErrorScopeStack->Empty()) { - return false; + callback(WGPUErrorType_Unknown, "No error scopes to pop", userdata); + return returnValue; } ErrorScope scope = mErrorScopeStack->Pop(); - if (callback != nullptr) { - // TODO(crbug.com/dawn/1122): Call callbacks only on wgpuInstanceProcessEvents - callback(static_cast(scope.GetErrorType()), scope.GetErrorMessage(), - userdata); - } - - return true; + callback(static_cast(scope.GetErrorType()), scope.GetErrorMessage(), + userdata); + return returnValue; } PersistentCache* DeviceBase::GetPersistentCache() { diff --git a/src/dawn/tests/unittests/validation/ErrorScopeValidationTests.cpp b/src/dawn/tests/unittests/validation/ErrorScopeValidationTests.cpp index ca2dfe9bdd..808eba414b 100644 --- a/src/dawn/tests/unittests/validation/ErrorScopeValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/ErrorScopeValidationTests.cpp @@ -138,8 +138,11 @@ TEST_F(ErrorScopeValidationTest, UnhandledErrorsMatchUncapturedErrorCallback) { // Check that push/popping error scopes must be balanced. TEST_F(ErrorScopeValidationTest, PushPopBalanced) { // No error scopes to pop. - { EXPECT_FALSE(device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this)); } - + { + EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_Unknown, _, this)) + .Times(1); + device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this); + } // Too many pops { device.PushErrorScope(wgpu::ErrorFilter::Validation); @@ -149,7 +152,9 @@ TEST_F(ErrorScopeValidationTest, PushPopBalanced) { device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this + 1); FlushWire(); - EXPECT_FALSE(device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this + 2)); + EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_Unknown, _, this + 2)) + .Times(1); + device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this + 2); } } @@ -208,6 +213,17 @@ TEST_F(ErrorScopeValidationTest, DeviceDestroyedBeforeCallback) { } } +// If the device is destroyed, pop error scope should callback with device lost. +TEST_F(ErrorScopeValidationTest, DeviceDestroyedBeforePop) { + device.PushErrorScope(wgpu::ErrorFilter::Validation); + ExpectDeviceDestruction(); + device.Destroy(); + FlushWire(); + + EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_DeviceLost, _, this)).Times(1); + device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this); +} + // Regression test that on device shutdown, we don't get a recursion in O(pushed error scope) that // would lead to a stack overflow TEST_F(ErrorScopeValidationTest, ShutdownStackOverflow) { diff --git a/src/dawn/tests/unittests/wire/WireErrorCallbackTests.cpp b/src/dawn/tests/unittests/wire/WireErrorCallbackTests.cpp index 6ec0485f78..a9c55228ce 100644 --- a/src/dawn/tests/unittests/wire/WireErrorCallbackTests.cpp +++ b/src/dawn/tests/unittests/wire/WireErrorCallbackTests.cpp @@ -139,25 +139,21 @@ TEST_F(WireErrorCallbackTests, DeviceLoggingCallback) { // Test the return wire for error scopes. TEST_F(WireErrorCallbackTests, PushPopErrorScopeCallback) { - wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(1); - + wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); FlushClient(); - wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this); - WGPUErrorCallback callback; void* userdata; EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _)) .WillOnce(DoAll(SaveArg<1>(&callback), SaveArg<2>(&userdata), Return(true))); - + wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this); FlushClient(); - callback(WGPUErrorType_Validation, "Some error message", userdata); EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_Validation, StrEq("Some error message"), this)) .Times(1); - + callback(WGPUErrorType_Validation, "Some error message", userdata); FlushServer(); } @@ -165,15 +161,11 @@ TEST_F(WireErrorCallbackTests, PushPopErrorScopeCallback) { TEST_F(WireErrorCallbackTests, PopErrorScopeCallbackOrdering) { // Two error scopes are popped, and the first one returns first. { - wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); - wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(2); - + wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); + wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); FlushClient(); - wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this); - wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this + 1); - WGPUErrorCallback callback1; WGPUErrorCallback callback2; void* userdata1; @@ -181,33 +173,30 @@ TEST_F(WireErrorCallbackTests, PopErrorScopeCallbackOrdering) { EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _)) .WillOnce(DoAll(SaveArg<1>(&callback1), SaveArg<2>(&userdata1), Return(true))) .WillOnce(DoAll(SaveArg<1>(&callback2), SaveArg<2>(&userdata2), Return(true))); - + wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this); + wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this + 1); FlushClient(); - callback1(WGPUErrorType_Validation, "First error message", userdata1); EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_Validation, StrEq("First error message"), this)) .Times(1); + callback1(WGPUErrorType_Validation, "First error message", userdata1); FlushServer(); - callback2(WGPUErrorType_Validation, "Second error message", userdata2); EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_Validation, StrEq("Second error message"), this + 1)) .Times(1); + callback2(WGPUErrorType_Validation, "Second error message", userdata2); FlushServer(); } // Two error scopes are popped, and the second one returns first. { - wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); - wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(2); - + wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); + wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); FlushClient(); - wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this); - wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this + 1); - WGPUErrorCallback callback1; WGPUErrorCallback callback2; void* userdata1; @@ -215,36 +204,35 @@ TEST_F(WireErrorCallbackTests, PopErrorScopeCallbackOrdering) { EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _)) .WillOnce(DoAll(SaveArg<1>(&callback1), SaveArg<2>(&userdata1), Return(true))) .WillOnce(DoAll(SaveArg<1>(&callback2), SaveArg<2>(&userdata2), Return(true))); - + wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this); + wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this + 1); FlushClient(); - callback2(WGPUErrorType_Validation, "Second error message", userdata2); EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_Validation, StrEq("Second error message"), this + 1)) .Times(1); + callback2(WGPUErrorType_Validation, "Second error message", userdata2); FlushServer(); - callback1(WGPUErrorType_Validation, "First error message", userdata1); EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_Validation, StrEq("First error message"), this)) .Times(1); + callback1(WGPUErrorType_Validation, "First error message", userdata1); FlushServer(); } } // Test the return wire for error scopes in flight when the device is destroyed. -TEST_F(WireErrorCallbackTests, PopErrorScopeDeviceDestroyed) { - wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); +TEST_F(WireErrorCallbackTests, PopErrorScopeDeviceInFlightDestroy) { EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(1); - + wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); FlushClient(); - EXPECT_TRUE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this)); - EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _)).WillOnce(Return(true)); + wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this); FlushClient(); - // Incomplete callback called in Device destructor. + // Incomplete callback called in Device destructor. This is resolved after the end of this test. EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_Unknown, ValidStringMessage(), this)) .Times(1); @@ -253,12 +241,11 @@ TEST_F(WireErrorCallbackTests, PopErrorScopeDeviceDestroyed) { // Test that registering a callback then wire disconnect calls the callback with // DeviceLost. TEST_F(WireErrorCallbackTests, PopErrorScopeThenDisconnect) { - wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(1); + wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); - EXPECT_TRUE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this)); EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _)).WillOnce(Return(true)); - + wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this); FlushClient(); EXPECT_CALL(*mockDevicePopErrorScopeCallback, @@ -270,9 +257,8 @@ TEST_F(WireErrorCallbackTests, PopErrorScopeThenDisconnect) { // Test that registering a callback after wire disconnect calls the callback with // DeviceLost. TEST_F(WireErrorCallbackTests, PopErrorScopeAfterDisconnect) { - wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(1); - + wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); FlushClient(); GetWireClient()->Disconnect(); @@ -280,36 +266,23 @@ TEST_F(WireErrorCallbackTests, PopErrorScopeAfterDisconnect) { EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_DeviceLost, ValidStringMessage(), this)) .Times(1); - EXPECT_TRUE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this)); + wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this); } -// Test that PopErrorScope returns false if there are no error scopes. +// Empty stack (We are emulating the errors that would be callback-ed from native). TEST_F(WireErrorCallbackTests, PopErrorScopeEmptyStack) { - // Empty stack - { EXPECT_FALSE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this)); } - - // Pop too many times - { - wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation); - EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(1); - - EXPECT_TRUE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this)); - EXPECT_FALSE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this + 1)); - WGPUErrorCallback callback; void* userdata; EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _)) .WillOnce(DoAll(SaveArg<1>(&callback), SaveArg<2>(&userdata), Return(true))); - + wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this); FlushClient(); - callback(WGPUErrorType_Validation, "Some error message", userdata); EXPECT_CALL(*mockDevicePopErrorScopeCallback, - Call(WGPUErrorType_Validation, StrEq("Some error message"), this)) + Call(WGPUErrorType_Validation, StrEq("No error scopes to pop"), this)) .Times(1); - + callback(WGPUErrorType_Validation, "No error scopes to pop", userdata); FlushServer(); - } } // Test the return wire for device lost callback diff --git a/src/dawn/wire/client/Device.cpp b/src/dawn/wire/client/Device.cpp index c3866a322e..9378bd5b45 100644 --- a/src/dawn/wire/client/Device.cpp +++ b/src/dawn/wire/client/Device.cpp @@ -145,35 +145,18 @@ namespace dawn::wire::client { mDeviceLostUserdata = userdata; } - void Device::PushErrorScope(WGPUErrorFilter filter) { - mErrorScopeStackSize++; - - DevicePushErrorScopeCmd cmd; - cmd.self = ToAPI(this); - cmd.filter = filter; - - client->SerializeCommand(cmd); - } - bool Device::PopErrorScope(WGPUErrorCallback callback, void* userdata) { - if (mErrorScopeStackSize == 0) { - return false; - } - mErrorScopeStackSize--; - + // TODO(crbug.com/dawn/1324) Replace bool return with void when users are updated. if (client->IsDisconnected()) { callback(WGPUErrorType_DeviceLost, "GPU device disconnected", userdata); return true; } uint64_t serial = mErrorScopes.Add({callback, userdata}); - DevicePopErrorScopeCmd cmd; cmd.deviceId = this->id; cmd.requestSerial = serial; - client->SerializeCommand(cmd); - return true; } diff --git a/src/dawn/wire/client/Device.h b/src/dawn/wire/client/Device.h index bb6a96dc54..56e2af26ad 100644 --- a/src/dawn/wire/client/Device.h +++ b/src/dawn/wire/client/Device.h @@ -84,7 +84,6 @@ namespace dawn::wire::client { void* userdata = nullptr; }; RequestTracker mErrorScopes; - uint64_t mErrorScopeStackSize = 0; struct CreatePipelineAsyncRequest { WGPUCreateComputePipelineAsyncCallback createComputePipelineAsyncCallback = nullptr; diff --git a/src/dawn/wire/server/ServerDevice.cpp b/src/dawn/wire/server/ServerDevice.cpp index b04afc575f..45fb6b8472 100644 --- a/src/dawn/wire/server/ServerDevice.cpp +++ b/src/dawn/wire/server/ServerDevice.cpp @@ -89,13 +89,9 @@ namespace dawn::wire::server { userdata->requestSerial = requestSerial; userdata->device = ObjectHandle{deviceId, device->generation}; - ErrorScopeUserdata* unownedUserdata = userdata.release(); - bool success = mProcs.devicePopErrorScope( - device->handle, ForwardToServer<&Server::OnDevicePopErrorScope>, unownedUserdata); - if (!success) { - delete unownedUserdata; - } - return success; + mProcs.devicePopErrorScope(device->handle, ForwardToServer<&Server::OnDevicePopErrorScope>, + userdata.release()); + return true; } void Server::OnDevicePopErrorScope(ErrorScopeUserdata* userdata,