[wire] Spanify BufferConsumer and it's callsites. - This change spanifies the APIs provided via BufferConsumer. This is an Dawn internal helper class that needs to be spanified to facilitate more spanification of the Dawn wire API. - Removes BufferConsumer_impl.h since separating the templates and the implementations of them was making it harder to update them, and the implementations aren't too big, so the compilation gains should be marginal. - A lot of the wire transfer structs needed volatile constructors to satisfy std::span iterator requirements. Since we don't actually expect these constructors to be called (they are needed for some algorithms that need to copy or duplicate elements), we just trivially implement them with a DAWN_UNREACHABLE(). Bug: 528027992 Change-Id: I60cfa340df03104057c51c292776942fd17a897f Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/320696 Commit-Queue: Kai Ninomiya <kainino@chromium.org> Commit-Queue: Loko Kung <lokokung@google.com> Reviewed-by: Kai Ninomiya <kainino@chromium.org> Auto-Submit: Loko Kung <lokokung@google.com>
diff --git a/generator/templates/dawn/wire/WireCmd.cpp b/generator/templates/dawn/wire/WireCmd.cpp index 86c5ed9..7da1bab 100644 --- a/generator/templates/dawn/wire/WireCmd.cpp +++ b/generator/templates/dawn/wire/WireCmd.cpp
@@ -28,8 +28,8 @@ #include "dawn/wire/WireCmd_autogen.h" #include "dawn/wire/Wire.h" +#include "src/dawn/common/Enumerator.h" #include "src/dawn/common/Numeric.h" -#include "src/dawn/wire/BufferConsumer_impl.h" #include "src/utils/assert.h" #include "src/utils/log.h" @@ -129,11 +129,12 @@ {%- set Return = "Return" if is_return_command else "" -%} {%- set Cmd = "Cmd" if is_cmd else "" -%} {%- set Inherits = " : CmdHeader" if is_cmd else "" %} + {%- set TransferStructName = Return + name + "Transfer" -%} //* Structure for the wire format of each of the records. Members that are values //* are embedded directly in the structure. Other members are assumed to be in the //* memory directly following the structure in the buffer. - struct {{Return}}{{name}}Transfer{{Inherits}} { + struct {{TransferStructName}}{{Inherits}} { static_assert({{[is_cmd, record.extensible, record.chained].count(True)}} <= 1, "Record must be at most one of is_cmd, extensible, and chained."); {% if record.extensible %} @@ -156,14 +157,35 @@ WGPUBool has_{{as_varName(member.name)}}; {% endif %} {% endfor %} + + {{TransferStructName}}() = default; + {{TransferStructName}}(const {{TransferStructName}}&) = default; + {{TransferStructName}}({{TransferStructName}}&&) = default; + + //* Volatile constructors and assignment operators are never expected to be called at + //* runtime when handling wire commands. They exist solely to satisfy C++20 iterator and + //* std::span requirements (e.g. std::indirectly_readable) when constructing views over + //* volatile shared memory buffers. + [[noreturn]] {{TransferStructName}}(const volatile {{TransferStructName}}& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] {{TransferStructName}}(volatile {{TransferStructName}}&& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] {{TransferStructName}}(const volatile {{TransferStructName}}&& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] {{TransferStructName}}& operator=(const volatile {{TransferStructName}}& other) { + DAWN_UNREACHABLE(); + } }; {% if is_cmd %} - static_assert(offsetof({{Return}}{{name}}Transfer, commandSize) == 0); + static_assert(offsetof({{TransferStructName}}, commandSize) == 0); {% endif -%} {% if record.chained %} - static_assert(offsetof({{Return}}{{name}}Transfer, chain) == 0); + static_assert(offsetof({{TransferStructName}}, chain) == 0); {% endif %} //* Returns the required transfer size for `record` in addition to the transfer structure. @@ -233,7 +255,7 @@ //* and `provider` to serialize objects. [[maybe_unused]] WireResult {{Return}}{{name}}Serialize( const {{Return}}{{name}}{{Cmd}}& record, - {{Return}}{{name}}Transfer* transfer, + {{TransferStructName}}* transfer, [[maybe_unused]] SerializeBuffer* buffer {%- if record.may_have_dawn_object -%} , const ObjectIdProvider& provider @@ -309,22 +331,24 @@ {% endif %} auto memberLength = {{member_length(member, "record.")}}; - {{member_transfer_type(member.type)}}* memberBuffer; + Span<{{member_transfer_type(member.type)}}> memberBuffer; WIRE_TRY(buffer->NextN(memberLength, &memberBuffer)); {% if member.type.is_wire_transparent %} - //* memcpy is not defined for null pointers, even when the length is zero. - //* conflicts with the common practice to use (nullptr, 0) to represent an - //* span. Guard memcpy with a zero check to work around this language bug. if (memberLength != 0) { - memcpy( - memberBuffer, record.{{memberName}}, - {{member_transfer_sizeof(member.type)}} * memberLength); + // TODO(https://crbug.com/524406299): Use Span::CopyFrom. + std::ranges::copy( + // TODO(https://crbug.com/530019520): Use deduction guides to avoid explicit templating. + // TODO(https://crbug.com/528027992): Spanify the record members. + SpanAsBytes(DAWN_UNSAFE_TODO(Span<const {{as_cType(member.type.name)}}>(record.{{memberName}}, memberLength))), + SpanAsWritableBytes(memberBuffer).begin() + ); } {% else %} //* This loop cannot overflow because it iterates up to |memberLength|. Even if //* memberLength were the maximum integer value, |i| would become equal to it //* just before exiting the loop, but not increment past or wrap around. + //* TODO(https://crbug.com/528027992): Spanify the record members. for (decltype(memberLength) i = 0; i < memberLength; ++i) { {{serialize_member(member.type, member.array_element_optional, "record." + memberName + "[i]", "memberBuffer[i]" )}} } @@ -341,7 +365,7 @@ //* Ids to actual objects. [[maybe_unused]] WireResult {{Return}}{{name}}Deserialize( {{Return}}{{name}}{{Cmd}}* record, - const volatile {{Return}}{{name}}Transfer* transfer, + const volatile {{TransferStructName}}* transfer, DeserializeBuffer* deserializeBuffer, [[maybe_unused]] DeserializeAllocator* allocator {%- if record.may_have_dawn_object -%} @@ -374,7 +398,7 @@ WIRE_TRY(deserializeBuffer->Read(&chainTransfer)); {{CType}}* typedOutStruct; - WIRE_TRY(GetSpace(allocator, 1u, &typedOutStruct)); + WIRE_TRY(GetSpace(allocator, &typedOutStruct)); typedOutStruct->chain.sType = sType; typedOutStruct->chain.next = nullptr; WIRE_TRY({{CType}}Deserialize(typedOutStruct, chainTransfer, @@ -427,9 +451,8 @@ {% else %} { {% endif %} - auto memberLength = {{member_length(member, "record->")}}; - const volatile {{member_transfer_type(member.type)}}* memberBuffer; - WIRE_TRY(deserializeBuffer->ReadN(memberLength, &memberBuffer)); + Span<const volatile {{member_transfer_type(member.type)}}> memberBuffer; + WIRE_TRY(deserializeBuffer->ReadN({{member_length(member, "record->")}}, &memberBuffer)); //* For data-only members (e.g. "data" in WriteBuffer and WriteTexture), they are //* not security sensitive so we can directly refer the data inside the transfer @@ -438,35 +461,24 @@ //* dawn_native must be a copy of what's in the wire. {% if member.json_data["wire_is_data_only"] %} record->{{memberName}} = - const_cast<const {{member_transfer_type(member.type)}}*>(memberBuffer); + const_cast<const {{member_transfer_type(member.type)}}*>(memberBuffer.data()); {% else %} - {{as_cType(member.type.name)}}* copiedMembers; - WIRE_TRY(GetSpace(allocator, memberLength, &copiedMembers)); - record->{{memberName}} = copiedMembers; + Span<{{as_cType(member.type.name)}}> copiedMembers; + WIRE_TRY(GetSpace(allocator, memberBuffer.size(), &copiedMembers)); + record->{{memberName}} = copiedMembers.data(); {% if member.type.is_wire_transparent %} - //* memcpy is not defined for null pointers, even when the length is zero. - //* conflicts with the common practice to use (nullptr, 0) to represent an - //* span. Guard memcpy with a zero check to work around this language bug. - if (memberLength != 0) { - //* memcpy is not allowed to copy from volatile objects. However, these - //* arrays are just used as plain data, and don't impact control flow. - //* So if the underlying data were changed while the copy was still - //* executing, we would get different data - but it wouldn't cause - //* unexpected downstream effects. - memcpy( - copiedMembers, - const_cast<const {{member_transfer_type(member.type)}}*>(memberBuffer), - {{member_transfer_sizeof(member.type)}} * memberLength); + if (!memberBuffer.empty()) { + // TODO(https://crbug.com/524406299): Use Span::CopyFrom. + std::ranges::copy( + SpanAsBytes(memberBuffer), + SpanAsWritableBytes(copiedMembers).begin() + ); } {% else %} - //* This loop cannot overflow because it iterates up to |memberLength|. Even - //* if memberLength were the maximum integer value, |i| would become equal - //* to it just before exiting the loop, but not increment past or wrap - //* around. - for (decltype(memberLength) i = 0; i < memberLength; ++i) { - {{deserialize_member(member.type, member.array_element_optional, "memberBuffer[i]", "copiedMembers[i]")}} + for (auto [i, member] : Enumerate(memberBuffer)) { + {{deserialize_member(member.type, member.array_element_optional, "member", "copiedMembers[i]" )}} } {% endif %} {% endif %} @@ -482,8 +494,9 @@ {% set Return = "Return" if is_return else "" %} {% set Name = Return + command.name.CamelCase() %} {% set Cmd = Name + "Cmd" %} + {% set TransferStructName = Name + "Transfer" %} size_t {{Cmd}}::GetRequiredSize() const { - return WireAlignSizeof<{{Name}}Transfer>() + {{Name}}GetExtraRequiredSize(*this); + return WireAlignSizeof<{{TransferStructName}}>() + {{Name}}GetExtraRequiredSize(*this); } {% if command.may_have_dawn_object %} @@ -491,7 +504,7 @@ size_t commandSize, SerializeBuffer* serializeBuffer, const ObjectIdProvider& provider) const { - {{Name}}Transfer* transfer; + {{TransferStructName}}* transfer; WIRE_TRY(serializeBuffer->Next(&transfer)); transfer->commandSize = commandSize; return ({{Name}}Serialize(*this, transfer, serializeBuffer, provider)); @@ -505,7 +518,7 @@ DeserializeBuffer* deserializeBuffer, DeserializeAllocator* allocator, const ObjectIdResolver& resolver) { - const volatile {{Name}}Transfer* transfer; + const volatile {{TransferStructName}}* transfer; WIRE_TRY(deserializeBuffer->Read(&transfer)); return {{Name}}Deserialize(this, transfer, deserializeBuffer, allocator, resolver); } @@ -515,7 +528,7 @@ } {% else %} WireResult {{Cmd}}::Serialize(size_t commandSize, SerializeBuffer* serializeBuffer) const { - {{Name}}Transfer* transfer; + {{TransferStructName}}* transfer; WIRE_TRY(serializeBuffer->Next(&transfer)); transfer->commandSize = commandSize; return ({{Name}}Serialize(*this, transfer, serializeBuffer)); @@ -528,7 +541,7 @@ } WireResult {{Cmd}}::Deserialize(DeserializeBuffer* deserializeBuffer, DeserializeAllocator* allocator) { - const volatile {{Name}}Transfer* transfer; + const volatile {{TransferStructName}}* transfer; WIRE_TRY(deserializeBuffer->Read(&transfer)); return {{Name}}Deserialize(this, transfer, deserializeBuffer, allocator); } @@ -547,8 +560,8 @@ // Allocates enough space from allocator to countain T[count] and return it in out. // Return FatalError if the allocator couldn't allocate the memory. // Always writes to |out| on success. -template <typename T, typename N> -WireResult GetSpace(DeserializeAllocator* allocator, N count, T** out) { +template <typename T> +WireResult GetSpace(DeserializeAllocator* allocator, size_t count, Span<T>* out) { // Because we use this function extensively when `count` == 1, we can optimize the // size computations a bit more for those cases via constexpr version of the // alignment computation. @@ -565,17 +578,48 @@ size = *sizeN; } - *out = static_cast<T*>(allocator->GetSpace(size)); - if (*out == nullptr) { + // TODO(https://crbug.com/528027992): Spanify DeserializeAllocator::GetSpace. + auto* ptr = static_cast<T*>(allocator->GetSpace(size)); + if (ptr == nullptr) { return WireResult::FatalError; } + // SAFETY: Size and alignment is checked above. + *out = DAWN_UNSAFE_BUFFERS(Span<T>(ptr, count)); + return WireResult::Success; +} +template <typename T> +WireResult GetSpace(DeserializeAllocator* allocator, T** out) { + Span<T> span; + WIRE_TRY(GetSpace(allocator, 1, &span)); + *out = span.data(); return WireResult::Success; } struct WGPUChainedStructTransfer { WGPUSType sType; bool hasNext; + + WGPUChainedStructTransfer() = default; + WGPUChainedStructTransfer(const WGPUChainedStructTransfer&) = default; + WGPUChainedStructTransfer(WGPUChainedStructTransfer&&) = default; + + //* Volatile constructors and assignment operators are never expected to be called at + //* runtime when handling wire commands. They exist solely to satisfy C++20 iterator and + //* std::span requirements (e.g. std::indirectly_readable) when constructing views over + //* volatile shared memory buffers. + [[noreturn]] WGPUChainedStructTransfer(const volatile WGPUChainedStructTransfer& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] WGPUChainedStructTransfer(volatile WGPUChainedStructTransfer&& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] WGPUChainedStructTransfer(const volatile WGPUChainedStructTransfer&& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] WGPUChainedStructTransfer& operator=(const volatile WGPUChainedStructTransfer& other) { + DAWN_UNREACHABLE(); + } }; //* Structs that need special handling for [de]serialization code generation. @@ -590,6 +634,27 @@ struct WGPUStringViewTransfer { bool has_data; uint64_t length; + + WGPUStringViewTransfer() = default; + WGPUStringViewTransfer(const WGPUStringViewTransfer&) = default; + WGPUStringViewTransfer(WGPUStringViewTransfer&&) = default; + + //* Volatile constructors and assignment operators are never expected to be called at + //* runtime when handling wire commands. They exist solely to satisfy C++20 iterator and + //* std::span requirements (e.g. std::indirectly_readable) when constructing views over + //* volatile shared memory buffers. + [[noreturn]] WGPUStringViewTransfer(const volatile WGPUStringViewTransfer& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] WGPUStringViewTransfer(volatile WGPUStringViewTransfer&& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] WGPUStringViewTransfer(const volatile WGPUStringViewTransfer&& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] WGPUStringViewTransfer& operator=(const volatile WGPUStringViewTransfer& other) { + DAWN_UNREACHABLE(); + } }; size_t WGPUStringViewGetExtraRequiredSize(const WGPUStringView& record) { @@ -626,9 +691,11 @@ length = std::strlen(record.data); } if (length > 0) { - char* memberBuffer; + Span<char> memberBuffer; WIRE_TRY(buffer->NextN(length, &memberBuffer)); - memcpy(memberBuffer, record.data, length); + // TODO(https://crbug.com/524406299): Use Span::CopyFrom. + // TODO(https://crbug.com/528027992): Spanify the record members. + std::ranges::copy(record.data, record.data + length, memberBuffer.begin()); } transfer->length = length; return WireResult::Success; @@ -668,14 +735,15 @@ return WireResult::FatalError; } size_t stringLength = static_cast<size_t>(length); - const volatile char* stringInBuffer; + Span<const volatile char> stringInBuffer; WIRE_TRY(deserializeBuffer->ReadN(stringLength, &stringInBuffer)); - char* copiedString; + Span<char> copiedString; WIRE_TRY(GetSpace(allocator, stringLength, &copiedString)); - memcpy(copiedString, const_cast<const char*>(stringInBuffer), stringLength); + // TODO(https://crbug.com/524406299): Use Span::CopyFrom. + std::ranges::copy(stringInBuffer, copiedString.begin()); - record->data = copiedString; + record->data = copiedString.data(); record->length = stringLength; return WireResult::Success; }
diff --git a/generator/templates/dawn/wire/WireCmd.h b/generator/templates/dawn/wire/WireCmd.h index bd84478..f1a265b 100644 --- a/generator/templates/dawn/wire/WireCmd.h +++ b/generator/templates/dawn/wire/WireCmd.h
@@ -83,6 +83,27 @@ struct CmdHeader { uint64_t commandSize; WireCmd commandId; + + CmdHeader() = default; + CmdHeader(const CmdHeader&) = default; + CmdHeader(CmdHeader&&) = default; + + // Volatile constructors and assignment operators are never expected to be called at + // runtime when handling wire commands. They exist solely to satisfy C++20 iterator and + // std::span requirements (e.g. std::indirectly_readable) when constructing views over + // volatile shared memory buffers. + [[noreturn]] CmdHeader(const volatile CmdHeader& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] CmdHeader(volatile CmdHeader&& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] CmdHeader(const volatile CmdHeader&& other) { + DAWN_UNREACHABLE(); + } + [[noreturn]] CmdHeader& operator=(const volatile CmdHeader& other) { + DAWN_UNREACHABLE(); + } }; {% macro write_command_struct(command, is_return_command) %}
diff --git a/generator/templates/dawn/wire/client/ClientHandlers.cpp b/generator/templates/dawn/wire/client/ClientHandlers.cpp index 0fa80cb..c19520f 100644 --- a/generator/templates/dawn/wire/client/ClientHandlers.cpp +++ b/generator/templates/dawn/wire/client/ClientHandlers.cpp
@@ -62,12 +62,12 @@ {% endfor %} const volatile char* Client::HandleCommands(const volatile char* commands, size_t size) { - DeserializeBuffer deserializeBuffer(commands, size); + // TODO(https://crbug.com/528027992): Spanify the input commands. + DeserializeBuffer deserializeBuffer(SpanAsBytes(DAWN_UNSAFE_TODO(Span<const volatile char>(commands, size)))); - while (deserializeBuffer.AvailableSize() >= sizeof(CmdHeader)) { - WireCmd cmdId = static_cast<const volatile CmdHeader*>( - static_cast<const volatile void*>(deserializeBuffer.Buffer())) - ->commandId; + const volatile CmdHeader* cmdHeader; + while (deserializeBuffer.Peek(&cmdHeader) != WireResult::FatalError) { + WireCmd cmdId = cmdHeader->commandId; WireResult result = WireResult::FatalError; switch (cmdId) { {% for command in cmd_records["special command"] %} @@ -92,7 +92,7 @@ mAllocator.Reset(); } - if (deserializeBuffer.AvailableSize() != 0) { + if (!deserializeBuffer.Empty()) { return nullptr; }
diff --git a/generator/templates/dawn/wire/server/ServerHandlers.cpp b/generator/templates/dawn/wire/server/ServerHandlers.cpp index c807a86..434e9ca 100644 --- a/generator/templates/dawn/wire/server/ServerHandlers.cpp +++ b/generator/templates/dawn/wire/server/ServerHandlers.cpp
@@ -88,12 +88,12 @@ {% endfor %} const volatile char* Server::HandleCommands(const volatile char* commands, size_t size) { - DeserializeBuffer deserializeBuffer(commands, size); + // TODO(https://crbug.com/528027992): Spanify the input commands. + DeserializeBuffer deserializeBuffer(SpanAsBytes(DAWN_UNSAFE_TODO(Span<const volatile char>(commands, size)))); - while (deserializeBuffer.AvailableSize() >= sizeof(CmdHeader)) { - WireCmd cmdId = static_cast<const volatile CmdHeader*>( - static_cast<const volatile void*>(deserializeBuffer.Buffer())) - ->commandId; + const volatile CmdHeader* cmdHeader; + while (deserializeBuffer.Peek(&cmdHeader) != WireResult::FatalError) { + WireCmd cmdId = cmdHeader->commandId; WireResult result; switch (cmdId) { {% for command in cmd_records["special command"] + cmd_records["command"] %} @@ -120,7 +120,7 @@ } } - if (deserializeBuffer.AvailableSize() != 0) { + if (!deserializeBuffer.Empty()) { return nullptr; }
diff --git a/src/dawn/tests/unittests/wire/WireSpecificCommandTests.cpp b/src/dawn/tests/unittests/wire/WireSpecificCommandTests.cpp index 8cb2019..af9b69d 100644 --- a/src/dawn/tests/unittests/wire/WireSpecificCommandTests.cpp +++ b/src/dawn/tests/unittests/wire/WireSpecificCommandTests.cpp
@@ -82,7 +82,9 @@ toIntercept(); size_t endOffset = c2sBuf->GetOffsetForTesting(); std::span<const char> subrange = c2sBuf->GetContentSubrange(startOffset, endOffset); - dawn::wire::DeserializeBuffer deserializeBuffer(subrange.data(), subrange.size()); + // TODO(https://crbug.com/528027992): Make TerribleCommandBuffer return Span instead of + // std::span. + dawn::wire::DeserializeBuffer deserializeBuffer(SpanAsBytes(Span<const char>(subrange))); EXPECT_EQ(WireResult::Success, cmd.Deserialize(&deserializeBuffer, &mAllocator, *GetWireServer()->GetImplForTesting()));
diff --git a/src/dawn/wire/BUILD.gn b/src/dawn/wire/BUILD.gn index 5de57df..35f54b3 100644 --- a/src/dawn/wire/BUILD.gn +++ b/src/dawn/wire/BUILD.gn
@@ -89,7 +89,6 @@ sources = get_target_outputs(":gen") sources += [ "BufferConsumer.h", - "BufferConsumer_impl.h", "ChunkedCommandHandler.cpp", "ChunkedCommandHandler.h", "ChunkedCommandSerializer.cpp",
diff --git a/src/dawn/wire/BufferConsumer.h b/src/dawn/wire/BufferConsumer.h index 7cf03ae..e309c18 100644 --- a/src/dawn/wire/BufferConsumer.h +++ b/src/dawn/wire/BufferConsumer.h
@@ -29,21 +29,26 @@ #define SRC_DAWN_WIRE_BUFFERCONSUMER_H_ #include <cstddef> +#include <new> #include "partition_alloc/pointers/raw_ptr.h" #include "src/dawn/common/Constants.h" #include "src/dawn/common/Math.h" #include "src/dawn/wire/WireResult.h" +#include "src/utils/compiler.h" +#include "src/utils/span.h" namespace dawn::wire { // Wire specific alignment helpers. template <typename T> constexpr size_t WireAlignSizeof() { + static_assert(alignof(T) <= kWireBufferAlignment, "T must be 8-byte alignable for the wire"); return AlignSizeof<T, kWireBufferAlignment>(); } template <typename T> std::optional<size_t> WireAlignSizeofN(size_t n) { + static_assert(alignof(T) <= kWireBufferAlignment, "T must be 8-byte alignable for the wire"); return AlignSizeofN<T, kWireBufferAlignment>(n); } @@ -54,45 +59,81 @@ // BufferConsumer also contains bounds checks to prevent reading out-of-bounds. template <typename BufferT> class BufferConsumer { - static_assert(sizeof(BufferT) == 1, - "BufferT must be 1-byte, but may have const/volatile qualifiers."); + static_assert(std::is_same_v<std::remove_cv_t<BufferT>, std::byte>, + "BufferT must be std::byte, but may have const/volatile qualifiers."); public: - BufferConsumer(BufferT* buffer, size_t size) : mBuffer(buffer), mSize(size) {} + explicit BufferConsumer(Span<BufferT> data) : mData(data) {} - BufferT* Buffer() const { return mBuffer; } - size_t AvailableSize() const { return mSize; } + bool Empty() const { return mData.empty(); } protected: - template <typename T, typename N> - WireResult NextN(N count, T** data); + template <typename T> + WireResult NextN(size_t count, Span<T>* out) { + size_t byteCount = 0; + WIRE_TRY(PeekN(count, out, &byteCount)); + mData = mData.subspan(byteCount); + return WireResult::Success; + } template <typename T> - WireResult Next(T** data); + WireResult Next(T** data) { + Span<T> out; + WIRE_TRY(NextN(1u, &out)); + *data = out.data(); + return WireResult::Success; + } + // Reads `count` objects of type T from the buffer and returns a span to them. The number of + // bytes that would be read is also optionally returned in |byteCount|. template <typename T> - WireResult Peek(T** data); + WireResult PeekN(size_t count, Span<T>* out, size_t* byteCount = nullptr) const { + // If size is nullopt then it indicates an overflow. + auto size = WireAlignSizeofN<T>(count); + if (!size || *size > mData.size()) { + return WireResult::FatalError; + } + DAWN_CHECK(*size >= sizeof(T) * count); + if (byteCount) { + *byteCount = *size; + } - private: - raw_ptr<BufferT, AllowPtrArithmetic> mBuffer; - size_t mSize; + // TODO(https://crbug.com/528027992): Use ReinterpretSpan once we fix alignment check that + // currently fails on Android AMD. This should be solvable by ensuring that + // dawn::wire::CommandSerializer::GetCmdSpace returns std::max_align_t aligned memory + // address. + *out = DAWN_UNSAFE_TODO( + Span<T>(reinterpret_cast<T*>(mData.first(sizeof(T) * count).data()), count)); + return WireResult::Success; + } + + // TODO(https://crbug.com/526537224): Use RawSpan instead of Span. + Span<BufferT> mData = {}; }; -class SerializeBuffer : public BufferConsumer<char> { +class SerializeBuffer : public BufferConsumer<std::byte> { public: using BufferConsumer::BufferConsumer; + using BufferConsumer::Next; using BufferConsumer::NextN; }; -class DeserializeBuffer : public BufferConsumer<const volatile char> { +class DeserializeBuffer : public BufferConsumer<const volatile std::byte> { public: using BufferConsumer::BufferConsumer; - using BufferConsumer::Peek; - template <typename T, typename N> - WireResult ReadN(N count, const volatile T** data) { - return NextN(count, data); + template <typename T> + WireResult Peek(const volatile T** data) const { + Span<const volatile T> out; + WIRE_TRY(PeekN(1u, &out)); + *data = out.data(); + return WireResult::Success; + } + + template <typename T> + WireResult ReadN(size_t count, Span<const volatile T>* out) { + return NextN(count, out); } template <typename T>
diff --git a/src/dawn/wire/BufferConsumer_impl.h b/src/dawn/wire/BufferConsumer_impl.h deleted file mode 100644 index 5476e31..0000000 --- a/src/dawn/wire/BufferConsumer_impl.h +++ /dev/null
@@ -1,83 +0,0 @@ -// Copyright 2021 The Dawn & Tint Authors -// -// Redistribution and use in source and binary forms, with or without -// modification, are permitted provided that the following conditions are met: -// -// 1. Redistributions of source code must retain the above copyright notice, this -// list of conditions and the following disclaimer. -// -// 2. Redistributions in binary form must reproduce the above copyright notice, -// this list of conditions and the following disclaimer in the documentation -// and/or other materials provided with the distribution. -// -// 3. Neither the name of the copyright holder nor the names of its -// contributors may be used to endorse or promote products derived from -// this software without specific prior written permission. -// -// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" -// AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE -// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE -// DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE -// FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL -// DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR -// SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER -// CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, -// OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - -#ifndef SRC_DAWN_WIRE_BUFFERCONSUMER_IMPL_H_ -#define SRC_DAWN_WIRE_BUFFERCONSUMER_IMPL_H_ - -#include <limits> -#include <type_traits> - -#include "src/dawn/wire/BufferConsumer.h" -#include "src/utils/compiler.h" - -namespace dawn::wire { - -template <typename BufferT> -template <typename T> -WireResult BufferConsumer<BufferT>::Peek(T** data) { - if (sizeof(T) > mSize) { - return WireResult::FatalError; - } - - *data = reinterpret_cast<T*>(mBuffer.get()); - return WireResult::Success; -} - -template <typename BufferT> -template <typename T> -WireResult BufferConsumer<BufferT>::Next(T** data) { - constexpr size_t kSize = WireAlignSizeof<T>(); - if (kSize > mSize) { - return WireResult::FatalError; - } - - *data = reinterpret_cast<T*>(mBuffer.get()); - DAWN_UNSAFE_TODO(mBuffer += kSize); - mSize -= kSize; - return WireResult::Success; -} - -template <typename BufferT> -template <typename T, typename N> -WireResult BufferConsumer<BufferT>::NextN(N count, T** data) { - static_assert(std::is_unsigned<N>::value, "|count| argument of NextN must be unsigned."); - - // If size is zero then it indicates an overflow. - auto size = WireAlignSizeofN<T>(count); - if (!size || *size > mSize) { - return WireResult::FatalError; - } - - *data = reinterpret_cast<T*>(mBuffer.get()); - DAWN_UNSAFE_TODO(mBuffer += *size); - mSize -= *size; - return WireResult::Success; -} - -} // namespace dawn::wire - -#endif // SRC_DAWN_WIRE_BUFFERCONSUMER_IMPL_H_
diff --git a/src/dawn/wire/CMakeLists.txt b/src/dawn/wire/CMakeLists.txt index bdada2b..1aa417d 100644 --- a/src/dawn/wire/CMakeLists.txt +++ b/src/dawn/wire/CMakeLists.txt
@@ -41,7 +41,6 @@ set(private_headers "${DAWN_WIRE_GEN_HEADERS}" - "BufferConsumer_impl.h" "BufferConsumer.h" "ChunkedCommandHandler.h" "ChunkedCommandSerializer.h"
diff --git a/src/dawn/wire/ChunkedCommandSerializer.cpp b/src/dawn/wire/ChunkedCommandSerializer.cpp index b3c0b74..8f1e8f2 100644 --- a/src/dawn/wire/ChunkedCommandSerializer.cpp +++ b/src/dawn/wire/ChunkedCommandSerializer.cpp
@@ -47,25 +47,23 @@ mSerializer->Flush(); } -void ChunkedCommandSerializer::SerializeChunkedCommand(const char* allocatedBuffer, - size_t totalSize) { +void ChunkedCommandSerializer::SerializeChunkedCommand(Span<const std::byte> allocatedBuffer) { // Constant regarding the size of the WireChunkedCommandCmd that can be computed once. static size_t kWireChunkedCmdPrefixSize = ChunkedCommandCmd{0, 0, nullptr, 0}.GetRequiredSize(); + // TODO(https://crbug.com/528027992): Spanify the ChunkedCommandCmd struct. ChunkedCommandCmd cmd; cmd.id = mNextChunkedCommandId++; - cmd.size = totalSize; + cmd.size = allocatedBuffer.size(); - size_t remainingSize = totalSize; - while (remainingSize > 0) { - cmd.chunkData = allocatedBuffer; - cmd.chunkSize = std::min(remainingSize, mMaxAllocationSize - kWireChunkedCmdPrefixSize); + while (!allocatedBuffer.empty()) { + cmd.chunkData = reinterpret_cast<const char*>(allocatedBuffer.data()); + cmd.chunkSize = + std::min(allocatedBuffer.size(), mMaxAllocationSize - kWireChunkedCmdPrefixSize); DAWN_ASSERT(cmd.GetRequiredSize() <= mMaxAllocationSize); SerializeCommand(cmd); - - DAWN_UNSAFE_TODO(allocatedBuffer += cmd.chunkSize); - remainingSize -= cmd.chunkSize; + allocatedBuffer = allocatedBuffer.subspan(cmd.chunkSize); } }
diff --git a/src/dawn/wire/ChunkedCommandSerializer.h b/src/dawn/wire/ChunkedCommandSerializer.h index 81bfd14..0167e25 100644 --- a/src/dawn/wire/ChunkedCommandSerializer.h +++ b/src/dawn/wire/ChunkedCommandSerializer.h
@@ -63,11 +63,9 @@ WireResult SerializeCommandExtension(SerializeBuffer* serializeBuffer, Extension&& e, Extensions&&... es) { - std::byte* buffer; + Span<std::byte> buffer; WIRE_TRY(serializeBuffer->NextN(e.size, &buffer)); - - // TODO(https://crbug.com/492456046): Spanify NextN to return a Span. - e.serialize(DAWN_UNSAFE_TODO(Span<std::byte>(buffer, e.size))); + e.serialize(buffer); WIRE_TRY(SerializeCommandExtension(serializeBuffer, std::forward<Extensions>(es)...)); return WireResult::Success; @@ -125,9 +123,11 @@ size_t requiredSize = (Align(extensions.size, kWireBufferAlignment) + ... + commandSize); if (requiredSize <= mMaxAllocationSize) { - char* allocatedBuffer = static_cast<char*>(mSerializer->GetCmdSpace(requiredSize)); - if (allocatedBuffer != nullptr) { - SerializeBuffer serializeBuffer(allocatedBuffer, requiredSize); + // TODO(https://crbug.com/528027992): Spanify CommandSerializer::GetCmdSpace. + Span<std::byte> allocatedBuffer = DAWN_UNSAFE_TODO(Span<std::byte>( + static_cast<std::byte*>(mSerializer->GetCmdSpace(requiredSize)), requiredSize)); + if (allocatedBuffer.data() != nullptr) { + SerializeBuffer serializeBuffer(allocatedBuffer); WireResult rCmd = SerializeCmd(cmd, requiredSize, &serializeBuffer); WireResult rExts = detail::SerializeCommandExtension(&serializeBuffer, extensions...); @@ -140,19 +140,21 @@ // Allocate as zero-initialized because padding won't get initialized during command // serialization (and this whole buffer is sent raw to the other end of the wire). - auto cmdSpace = std::unique_ptr<char[]>(new char[requiredSize]{}); + auto cmdSpaceAlloc = std::unique_ptr<std::byte[]>(new std::byte[requiredSize]{}); + // TODO(https://crbug.com/512465980): Use dawn::HeapArray above. + Span<std::byte> DAWN_UNSAFE_TODO(cmdSpace{cmdSpaceAlloc.get(), requiredSize}); - SerializeBuffer serializeBuffer(cmdSpace.get(), requiredSize); + SerializeBuffer serializeBuffer(cmdSpace); WireResult rCmd = SerializeCmd(cmd, requiredSize, &serializeBuffer); WireResult rExts = detail::SerializeCommandExtension(&serializeBuffer, extensions...); if (rCmd != WireResult::Success || rExts != WireResult::Success) [[unlikely]] { mSerializer->OnSerializeError(); return; } - SerializeChunkedCommand(cmdSpace.get(), requiredSize); + SerializeChunkedCommand(cmdSpace); } - void SerializeChunkedCommand(const char* allocatedBuffer, size_t totalSize); + void SerializeChunkedCommand(Span<const std::byte> allocatedBuffer); raw_ptr<CommandSerializer> mSerializer; size_t mMaxAllocationSize;
diff --git a/src/dawn/wire/client/Buffer.cpp b/src/dawn/wire/client/Buffer.cpp index 93c3db4..22cbfc7 100644 --- a/src/dawn/wire/client/Buffer.cpp +++ b/src/dawn/wire/client/Buffer.cpp
@@ -35,7 +35,6 @@ #include "dawn/wire/WireCmd_autogen.h" #include "partition_alloc/pointers/raw_ptr.h" #include "src/dawn/common/StringViewUtils.h" -#include "src/dawn/wire/BufferConsumer_impl.h" #include "src/dawn/wire/client/Client.h" #include "src/dawn/wire/client/Device.h" #include "src/dawn/wire/client/EventManager.h"
diff --git a/src/dawn/wire/client/Queue.cpp b/src/dawn/wire/client/Queue.cpp index 17fcdf1..6b16c39 100644 --- a/src/dawn/wire/client/Queue.cpp +++ b/src/dawn/wire/client/Queue.cpp
@@ -34,7 +34,6 @@ #include "partition_alloc/pointers/raw_ptr.h" #include "src/dawn/common/Atomic.h" #include "src/dawn/common/StringViewUtils.h" -#include "src/dawn/wire/BufferConsumer_impl.h" #include "src/dawn/wire/client/Client.h" #include "src/dawn/wire/client/EventManager.h" #include "src/utils/compiler.h"
diff --git a/src/dawn/wire/server/ServerBuffer.cpp b/src/dawn/wire/server/ServerBuffer.cpp index 053f39d..52cf9a5 100644 --- a/src/dawn/wire/server/ServerBuffer.cpp +++ b/src/dawn/wire/server/ServerBuffer.cpp
@@ -31,7 +31,7 @@ #include "dawn/wire/WireCmd_autogen.h" #include "src/dawn/common/StringViewUtils.h" -#include "src/dawn/wire/BufferConsumer_impl.h" +#include "src/dawn/wire/BufferConsumer.h" #include "src/dawn/wire/WireResult.h" #include "src/dawn/wire/server/Server.h" #include "src/utils/assert.h"