Rewrite BufferReader to use std::span This prevents a UBU warning during read Also adds in tint::Copy to wrap/bounds check memcpy, because base::span::copy_from is not available in stand alone builds. Bug: 408010433 Change-Id: Ibb806aba009950631c5240b428401b9153f3ad89 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/299017 Reviewed-by: James Price <jrprice@google.com> Commit-Queue: James Price <jrprice@google.com> Reviewed-by: dan sinclair <dsinclair@chromium.org>
diff --git a/src/tint/utils/bytes/buffer_reader.cc b/src/tint/utils/bytes/buffer_reader.cc index 5c790dd..3c41de6 100644 --- a/src/tint/utils/bytes/buffer_reader.cc +++ b/src/tint/utils/bytes/buffer_reader.cc
@@ -30,24 +30,21 @@ #include <algorithm> #include "src/tint/utils/macros/compiler.h" +#include "src/tint/utils/memory/copy.h" namespace tint::bytes { BufferReader::~BufferReader() = default; -// TODO(408010433): Rewrite internals using span to avoid UBU during read -TINT_BEGIN_DISABLE_WARNING(UNSAFE_BUFFER_USAGE); size_t BufferReader::Read(std::byte* out, size_t count) { - size_t n = std::min(count, bytes_remaining_); - memcpy(out, data_, n); - data_ += n; - bytes_remaining_ -= n; + size_t n = std::min(count, data_.size()); + tint::Copy(out, count, data_.first(n)); + data_ = data_.subspan(n); return n; } -TINT_END_DISABLE_WARNING(UNSAFE_BUFFER_USAGE); bool BufferReader::IsEOF() const { - return bytes_remaining_ == 0; + return data_.empty(); } } // namespace tint::bytes
diff --git a/src/tint/utils/bytes/buffer_reader.h b/src/tint/utils/bytes/buffer_reader.h index f75c7f6..b8ee4f5 100644 --- a/src/tint/utils/bytes/buffer_reader.h +++ b/src/tint/utils/bytes/buffer_reader.h
@@ -40,25 +40,27 @@ /// BufferReader is an implementation of the Reader interface backed by a buffer. class BufferReader final : public Reader { public: - // Destructor + /// Destructor ~BufferReader() override; + // This constructor represents the boundary between unsafe and safe memory constructs, since a + // raw pointer is being converted to a std::span. There is no way to avoid this warning, because + // if the compiler could statically guarantee the raw pointer + size was valid here, there would + // be no need for std::span to exist. + TINT_BEGIN_DISABLE_WARNING(UNSAFE_BUFFER_USAGE_IN_CONTAINER); /// Constructor /// @param data the data to read from /// @param size the number of bytes in the buffer - BufferReader(const std::byte* data, size_t size) : data_(data), bytes_remaining_(size) { - TINT_ASSERT(data); - } + BufferReader(const std::byte* data, size_t size) : data_(data, size) { TINT_ASSERT(data); } + TINT_END_DISABLE_WARNING(UNSAFE_BUFFER_USAGE_IN_CONTAINER); /// Constructor /// @param str the string to read from - explicit BufferReader(std::string_view str) - : data_(reinterpret_cast<const std::byte*>(str.data())), bytes_remaining_(str.length()) {} + explicit BufferReader(std::string_view str) : data_(std::as_bytes(std::span{str})) {} /// Constructor /// @param span the byte span to read from - explicit BufferReader(std::span<const std::byte> span) - : data_(span.data()), bytes_remaining_(span.size()) { + explicit BufferReader(std::span<const std::byte> span) : data_(span) { TINT_ASSERT(span.data()); } @@ -70,10 +72,7 @@ private: /// The data to read from - const std::byte* data_ = nullptr; - - /// The number of bytes remaining - size_t bytes_remaining_ = 0; + std::span<const std::byte> data_; }; } // namespace tint::bytes
diff --git a/src/tint/utils/memory/BUILD.bazel b/src/tint/utils/memory/BUILD.bazel index be209de..3c4dc9f 100644 --- a/src/tint/utils/memory/BUILD.bazel +++ b/src/tint/utils/memory/BUILD.bazel
@@ -46,8 +46,10 @@ "bitcast.h", "block_allocator.h", "bump_allocator.h", + "copy.h", ], deps = [ + "//src/tint/utils/ice", "//src/tint/utils/macros", "//src/tint/utils/math", "//src/utils",
diff --git a/src/tint/utils/memory/BUILD.cmake b/src/tint/utils/memory/BUILD.cmake index 021881c..46edd77 100644 --- a/src/tint/utils/memory/BUILD.cmake +++ b/src/tint/utils/memory/BUILD.cmake
@@ -43,10 +43,12 @@ utils/memory/bitcast.h utils/memory/block_allocator.h utils/memory/bump_allocator.h + utils/memory/copy.h utils/memory/memory.cc ) tint_target_add_dependencies(tint_utils_memory lib + tint_utils_ice tint_utils_macros tint_utils_math )
diff --git a/src/tint/utils/memory/BUILD.gn b/src/tint/utils/memory/BUILD.gn index d83898f..ba17756 100644 --- a/src/tint/utils/memory/BUILD.gn +++ b/src/tint/utils/memory/BUILD.gn
@@ -49,10 +49,12 @@ "bitcast.h", "block_allocator.h", "bump_allocator.h", + "copy.h", "memory.cc", ] deps = [ "${dawn_root}/src/utils", + "${tint_src_dir}/utils/ice", "${tint_src_dir}/utils/macros", "${tint_src_dir}/utils/math", ]
diff --git a/src/tint/utils/memory/copy.h b/src/tint/utils/memory/copy.h new file mode 100644 index 0000000..fdc8dc6 --- /dev/null +++ b/src/tint/utils/memory/copy.h
@@ -0,0 +1,100 @@ +// Copyright 2026 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_TINT_UTILS_MEMORY_COPY_H_ +#define SRC_TINT_UTILS_MEMORY_COPY_H_ + +#include <cstring> +#include <span> + +#include "src/tint/utils/ice/ice.h" +#include "src/tint/utils/macros/compiler.h" + +// These wrappers around memcpy exist to localize the UNSAFE_BUFFER_USAGE warning intrinsic to the +// operation to one location. These warnings could be eliminated by using iteration to copy, but +// that would be less performant. std::ranges::copy does not guarantee bounds checking, so the +// warning still fires on some platforms. Chromium's //base has a more sophisticated implementation +// of this in the form of ::copy_from, but that is not available in standalone Dawn builds. +// +// It is intentional that the raw pointer -> std::span copy is not implemented, since that would +// involve crossing the boundary between unsafe -> safe memory operations. It is preferable for the +// call site where this is occurring be explicitly marked as unsafe, instead of trying to hide the +// warning. The idiomatic way to do this is to construct a std::span using the two-part constructor, +// which will need a warning suppression. And then call the std::span -> std::span version of Copy. + +namespace tint { + +/// Copy copies all elements from @p src to @p dst. +/// @param dst the destination span +/// @param src the source span +/// @tparam T the type of destination elements +/// @tparam N the size of the destination span +/// @tparam U the type of source elements +/// @tparam M the size of the source span +// The usage of std::memcpy will always trigger this warning +TINT_BEGIN_DISABLE_WARNING(UNSAFE_BUFFER_USAGE); +template <typename T, size_t N, typename U, size_t M> +inline void Copy(std::span<T, N> dst, std::span<U, M> src) + requires(N >= M) +{ + static_assert(sizeof(T) == sizeof(U)); + TINT_ASSERT(src.data()); + TINT_ASSERT(dst.data()); + if (!src.empty()) { + std::memcpy(dst.data(), src.data(), src.size_bytes()); + } +} +TINT_END_DISABLE_WARNING(UNSAFE_BUFFER_USAGE); + +/// Copy copies all elements from @p src to @p dst. +/// @param dst the destination pointer +/// @param dst_count the maximum number of elements that can be written to @p dst +/// @param src the source span +/// @tparam T the type of destination elements +/// @tparam U the type of source elements +/// @tparam M the size of the source span +// The usage of std::memcpy will always trigger this warning +TINT_BEGIN_DISABLE_WARNING(UNSAFE_BUFFER_USAGE); +template <typename T, typename U, size_t M> +inline void Copy(T* dst, size_t dst_count, std::span<U, M> src) { + static_assert(sizeof(T) == sizeof(U)); + TINT_ASSERT(src.data()); + TINT_ASSERT(dst); + TINT_ASSERT(dst_count >= src.size()); + if (!src.empty()) { + std::memcpy(dst, src.data(), src.size_bytes()); + } +} +TINT_END_DISABLE_WARNING(UNSAFE_BUFFER_USAGE); + +// template <typename T, typename U, size_t M> +// inline void Copy(std::span<T, M> dest, T* src, size_t src_count) +// is intentionally omitted, see above + +} // namespace tint + +#endif // SRC_TINT_UTILS_MEMORY_COPY_H_