From 286061d810cbab144b5cc6a3f4c26c880dd271b2 Mon Sep 17 00:00:00 2001 From: Shrek Shao Date: Fri, 29 Apr 2022 18:12:17 +0000 Subject: [PATCH] Update default value for map range size to WGPU_WHOLE_MAP_SIZE Bug exposed by emscripten webgpu binding issue. It seems the spec updated after the last code updates. Update the validation part a bit (introduce rangeSize). Bug: dawn:1400 Change-Id: I0ddefd5c1a0976cc34102a44514bccd70f7a1ac0 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/88080 Reviewed-by: Austin Eng Kokoro: Kokoro Commit-Queue: Shrek Shao --- dawn.json | 4 +-- src/dawn/native/Buffer.cpp | 8 +++-- src/dawn/tests/end2end/BufferTests.cpp | 10 +++--- .../validation/BufferValidationTests.cpp | 34 +++++++++++++++++-- src/dawn/wire/client/Buffer.cpp | 8 +++-- 5 files changed, 49 insertions(+), 15 deletions(-) diff --git a/dawn.json b/dawn.json index 795ae27b8e..5463b92f4c 100644 --- a/dawn.json +++ b/dawn.json @@ -413,7 +413,7 @@ "returns": "void *", "args": [ {"name": "offset", "type": "size_t", "default": 0}, - {"name": "size", "type": "size_t", "default": 0} + {"name": "size", "type": "size_t", "default": "WGPU_WHOLE_MAP_SIZE"} ] }, { @@ -421,7 +421,7 @@ "returns": "void const *", "args": [ {"name": "offset", "type": "size_t", "default": 0}, - {"name": "size", "type": "size_t", "default": 0} + {"name": "size", "type": "size_t", "default": "WGPU_WHOLE_MAP_SIZE"} ] }, { diff --git a/src/dawn/native/Buffer.cpp b/src/dawn/native/Buffer.cpp index b4e2bd90b6..9161f11380 100644 --- a/src/dawn/native/Buffer.cpp +++ b/src/dawn/native/Buffer.cpp @@ -494,16 +494,18 @@ namespace dawn::native { } bool BufferBase::CanGetMappedRange(bool writable, size_t offset, size_t size) const { - if (offset % 8 != 0 || size % 4 != 0) { + if (offset % 8 != 0 || offset < mMapOffset || offset > mSize) { return false; } - if (size > mMapSize || offset < mMapOffset) { + size_t rangeSize = size == WGPU_WHOLE_MAP_SIZE ? mSize - offset : size; + + if (rangeSize % 4 != 0 || rangeSize > mMapSize) { return false; } size_t offsetInMappedRange = offset - mMapOffset; - if (offsetInMappedRange > mMapSize - size) { + if (offsetInMappedRange > mMapSize - rangeSize) { return false; } diff --git a/src/dawn/tests/end2end/BufferTests.cpp b/src/dawn/tests/end2end/BufferTests.cpp index 8b869ef2dc..9853495cf1 100644 --- a/src/dawn/tests/end2end/BufferTests.cpp +++ b/src/dawn/tests/end2end/BufferTests.cpp @@ -280,13 +280,13 @@ TEST_P(BufferMappingTests, MapWrite_TwicePreserve) { uint32_t data1 = 0x08090a0b; size_t offset1 = 8; - MapAsyncAndWait(buffer, wgpu::MapMode::Write, offset1, sizeof(data1)); + MapAsyncAndWait(buffer, wgpu::MapMode::Write, offset1, wgpu::kWholeMapSize); memcpy(buffer.GetMappedRange(offset1), &data1, sizeof(data1)); buffer.Unmap(); uint32_t data2 = 0x00010203; size_t offset2 = 0; - MapAsyncAndWait(buffer, wgpu::MapMode::Write, offset2, sizeof(data2)); + MapAsyncAndWait(buffer, wgpu::MapMode::Write, offset2, wgpu::kWholeMapSize); memcpy(buffer.GetMappedRange(offset2), &data2, sizeof(data2)); buffer.Unmap(); @@ -301,7 +301,7 @@ TEST_P(BufferMappingTests, MapWrite_TwiceRangeOverlap) { uint32_t data1[] = {0x01234567, 0x89abcdef}; size_t offset1 = 8; MapAsyncAndWait(buffer, wgpu::MapMode::Write, offset1, 8); - memcpy(buffer.GetMappedRange(offset1), data1, 8); + memcpy(buffer.GetMappedRange(offset1, 8), data1, 8); buffer.Unmap(); EXPECT_BUFFER_U32_EQ(0x00000000, buffer, 0); @@ -312,7 +312,7 @@ TEST_P(BufferMappingTests, MapWrite_TwiceRangeOverlap) { uint32_t data2[] = {0x01234567, 0x89abcdef, 0x55555555}; size_t offset2 = 0; MapAsyncAndWait(buffer, wgpu::MapMode::Write, offset2, 12); - memcpy(buffer.GetMappedRange(offset2), data2, 12); + memcpy(buffer.GetMappedRange(offset2, 12), data2, 12); buffer.Unmap(); EXPECT_BUFFER_U32_EQ(0x01234567, buffer, 0); @@ -354,7 +354,7 @@ TEST_P(BufferMappingTests, MapWrite_Large) { EXPECT_EQ(nullptr, buffer.GetMappedRange(0)); EXPECT_EQ(nullptr, buffer.GetMappedRange(8)); EXPECT_EQ(nullptr, buffer.GetMappedRange(16, kByteSize - 8)); - memcpy(buffer.GetMappedRange(16), myData.data(), kByteSize - 20); + memcpy(buffer.GetMappedRange(16, kByteSize - 20), myData.data(), kByteSize - 20); buffer.Unmap(); EXPECT_BUFFER_U32_RANGE_EQ(myData.data(), buffer, 16, kDataSize - 5); } diff --git a/src/dawn/tests/unittests/validation/BufferValidationTests.cpp b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp index 147e45ecc3..1c0abff8cf 100644 --- a/src/dawn/tests/unittests/validation/BufferValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp @@ -859,6 +859,14 @@ TEST_F(BufferValidationTest, GetMappedRange_OffsetSizeOOB) { EXPECT_NE(buffer.GetMappedRange(0, 8), nullptr); } + // Valid case: full range is ok with defaulted MapAsync size and defaulted GetMappedRangeSize + { + wgpu::Buffer buffer = CreateMapWriteBuffer(8); + buffer.MapAsync(wgpu::MapMode::Write, 0, wgpu::kWholeMapSize, nullptr, nullptr); + WaitForAllOperations(device); + EXPECT_NE(buffer.GetMappedRange(0, wgpu::kWholeMapSize), nullptr); + } + // Valid case: empty range at the end is ok { wgpu::Buffer buffer = CreateMapWriteBuffer(8); @@ -882,6 +890,17 @@ TEST_F(BufferValidationTest, GetMappedRange_OffsetSizeOOB) { WaitForAllOperations(device); EXPECT_EQ(buffer.GetMappedRange(9, 0), nullptr); EXPECT_EQ(buffer.GetMappedRange(16, 0), nullptr); + EXPECT_EQ(buffer.GetMappedRange(std::numeric_limits::max(), 0), nullptr); + } + + // Error case: offset is larger than the buffer size (even with size = 0) + { + wgpu::Buffer buffer = CreateMapWriteBuffer(16); + buffer.MapAsync(wgpu::MapMode::Write, 8, 8, nullptr, nullptr); + WaitForAllOperations(device); + EXPECT_EQ(buffer.GetMappedRange(16, 4), nullptr); + EXPECT_EQ(buffer.GetMappedRange(24, 0), nullptr); + EXPECT_EQ(buffer.GetMappedRange(std::numeric_limits::max(), 0), nullptr); } // Error case: offset + size is larger than the mapped range @@ -898,15 +917,26 @@ TEST_F(BufferValidationTest, GetMappedRange_OffsetSizeOOB) { wgpu::Buffer buffer = CreateMapWriteBuffer(12); buffer.MapAsync(wgpu::MapMode::Write, 0, 12, nullptr, nullptr); WaitForAllOperations(device); - EXPECT_EQ(buffer.GetMappedRange(8, std::numeric_limits::max()), nullptr); + // set size to (max - 1) to avoid being equal to kWholeMapSize + EXPECT_EQ(buffer.GetMappedRange(8, std::numeric_limits::max() - 1), nullptr); } - // Error case: offset is before the start of the range + // Error case: size is larger than the mapped range when using default kWholeMapSize + { + wgpu::Buffer buffer = CreateMapWriteBuffer(12); + buffer.MapAsync(wgpu::MapMode::Write, 0, 8, nullptr, nullptr); + WaitForAllOperations(device); + EXPECT_EQ(buffer.GetMappedRange(0), nullptr); + } + + // Error case: offset is before the start of the range (even with size = 0) { wgpu::Buffer buffer = CreateMapWriteBuffer(12); buffer.MapAsync(wgpu::MapMode::Write, 8, 4, nullptr, nullptr); WaitForAllOperations(device); EXPECT_EQ(buffer.GetMappedRange(7, 4), nullptr); EXPECT_EQ(buffer.GetMappedRange(0, 4), nullptr); + EXPECT_EQ(buffer.GetMappedRange(0, 12), nullptr); + EXPECT_EQ(buffer.GetMappedRange(0, 0), nullptr); } } diff --git a/src/dawn/wire/client/Buffer.cpp b/src/dawn/wire/client/Buffer.cpp index a490eb486a..e7033c12d4 100644 --- a/src/dawn/wire/client/Buffer.cpp +++ b/src/dawn/wire/client/Buffer.cpp @@ -377,16 +377,18 @@ namespace dawn::wire::client { } bool Buffer::CheckGetMappedRangeOffsetSize(size_t offset, size_t size) const { - if (offset % 8 != 0 || size % 4 != 0) { + if (offset % 8 != 0 || offset < mMapOffset || offset > mSize) { return false; } - if (size > mMapSize || offset < mMapOffset) { + size_t rangeSize = size == WGPU_WHOLE_MAP_SIZE ? mSize - offset : size; + + if (rangeSize % 4 != 0 || rangeSize > mMapSize) { return false; } size_t offsetInMappedRange = offset - mMapOffset; - return offsetInMappedRange <= mMapSize - size; + return offsetInMappedRange <= mMapSize - rangeSize; } void Buffer::FreeMappedData() {