From d03cced7d552ece168c6db92acb3a7c379aae0c0 Mon Sep 17 00:00:00 2001 From: Derek Mauro Date: Tue, 29 Nov 2022 09:19:04 -0800 Subject: CRC: Make crc32c_t as a class for explicit control of operators The motivation is to explicitly remove and document dangerous operations like adding crc32c_t to a set, because equality is not enough to guarantee uniqueness. PiperOrigin-RevId: 491656425 Change-Id: I7b4dadc1a59ea9861e6ec7a929d64b5746467832 --- absl/crc/crc32c.cc | 11 +++++---- absl/crc/crc32c.h | 46 +++++++++++++++++++++--------------- absl/crc/crc32c_benchmark.cc | 8 +++---- absl/crc/crc32c_test.cc | 22 ++++++++--------- absl/crc/internal/crc_memcpy.h | 4 ++-- absl/crc/internal/crc_memcpy_test.cc | 4 ++-- 6 files changed, 52 insertions(+), 43 deletions(-) diff --git a/absl/crc/crc32c.cc b/absl/crc/crc32c.cc index 82865df5..169826f5 100644 --- a/absl/crc/crc32c.cc +++ b/absl/crc/crc32c.cc @@ -55,7 +55,7 @@ crc32c_t ExtendCrc32cInternal(crc32c_t initial_crc, } // namespace crc_internal crc32c_t ComputeCrc32c(absl::string_view buf) { - return ExtendCrc32c(ToCrc32c(0), buf); + return ExtendCrc32c(crc32c_t{0}, buf); } crc32c_t ExtendCrc32cByZeroes(crc32c_t initial_crc, size_t length) { @@ -67,7 +67,7 @@ crc32c_t ExtendCrc32cByZeroes(crc32c_t initial_crc, size_t length) { crc32c_t ConcatCrc32c(crc32c_t lhs_crc, crc32c_t rhs_crc, size_t rhs_len) { uint32_t result = static_cast(lhs_crc); CrcEngine()->ExtendByZeroes(&result, rhs_len); - return static_cast(result) ^ rhs_crc; + return crc32c_t{result ^ static_cast(rhs_crc)}; } crc32c_t RemoveCrc32cPrefix(crc32c_t crc_a, crc32c_t crc_ab, size_t length_b) { @@ -89,9 +89,10 @@ crc32c_t MemcpyCrc32c(void* dest, const void* src, size_t count, // This operation has a runtime cost of O(log(`suffix_len`)) crc32c_t RemoveCrc32cSuffix(crc32c_t full_string_crc, crc32c_t suffix_crc, size_t suffix_len) { - crc32c_t crc_with_suffix_zeroed = - suffix_crc ^ full_string_crc ^ - ExtendCrc32cByZeroes(ToCrc32c(0), suffix_len); + crc32c_t crc_with_suffix_zeroed = crc32c_t{ + static_cast(suffix_crc) ^ + static_cast(full_string_crc) ^ + static_cast(ExtendCrc32cByZeroes(crc32c_t{0}, suffix_len))}; return crc_internal::UnextendCrc32cByZeroes( crc_with_suffix_zeroed, suffix_len); } diff --git a/absl/crc/crc32c.h b/absl/crc/crc32c.h index 8b030732..7be42be5 100644 --- a/absl/crc/crc32c.h +++ b/absl/crc/crc32c.h @@ -39,24 +39,32 @@ ABSL_NAMESPACE_BEGIN // crc32c_t //----------------------------------------------------------------------------- -// `crc32c_t` defines a strongly typed integer type for holding a CRC32C value. -enum class crc32c_t : uint32_t {}; - -// ToCrc32c() -// -// Converts a uint32_t value to crc32c_t. This API is necessary in C++14 -// and earlier. Code targeting C++17-or-later can instead use `crc32c_t{n}`. -inline crc32c_t ToCrc32c(uint32_t n) { - return static_cast(n); -} -// operator^ +// `crc32c_t` defines a strongly-typed integer for holding a CRC32C value. // -// Performs a bitwise XOR on two CRC32C values -inline crc32c_t operator^(crc32c_t lhs, crc32c_t rhs) { - const auto lhs_int = static_cast(lhs); - const auto rhs_int = static_cast(rhs); - return ToCrc32c(lhs_int ^ rhs_int); -} +// Some operators are intentionally omitted. Only equality operators are defined +// so that `crc32c_t` can be directly compared. Methods for putting `crc32c_t` +// directly into a set are omitted because this is bug-prone due to checksum +// collisions. Use an explicit conversion to the `uint32_t` space for operations +// that treat `crc32c_t` as an integer. +class crc32c_t final { + public: + crc32c_t() = default; + constexpr explicit crc32c_t(uint32_t crc) : crc_(crc) {} + + crc32c_t(const crc32c_t&) = default; + crc32c_t& operator=(const crc32c_t&) = default; + + explicit operator uint32_t() const { return crc_; } + + friend bool operator==(crc32c_t lhs, crc32c_t rhs) { + return static_cast(lhs) == static_cast(rhs); + } + + friend bool operator!=(crc32c_t lhs, crc32c_t rhs) { return !(lhs == rhs); } + + private: + uint32_t crc_; +}; namespace crc_internal { // Non-inline code path for `absl::ExtendCrc32c()`. Do not call directly. @@ -92,7 +100,7 @@ inline crc32c_t ExtendCrc32c(crc32c_t initial_crc, uint32_t crc = static_cast(initial_crc); if (crc_internal::ExtendCrc32cInline(&crc, buf_to_add.data(), buf_to_add.size())) { - return ToCrc32c(crc); + return crc32c_t{crc}; } } return crc_internal::ExtendCrc32cInternal(initial_crc, buf_to_add); @@ -116,7 +124,7 @@ crc32c_t ExtendCrc32cByZeroes(crc32c_t initial_crc, size_t length); // Using `MemcpyCrc32c()` is potentially faster than performing the `memcpy()` // and `ComputeCrc32c()` operations separately. crc32c_t MemcpyCrc32c(void* dest, const void* src, size_t count, - crc32c_t initial_crc = ToCrc32c(0)); + crc32c_t initial_crc = crc32c_t{0}); // ----------------------------------------------------------------------------- // CRC32C Arithmetic Functions diff --git a/absl/crc/crc32c_benchmark.cc b/absl/crc/crc32c_benchmark.cc index 2c7ac594..df99d5cf 100644 --- a/absl/crc/crc32c_benchmark.cc +++ b/absl/crc/crc32c_benchmark.cc @@ -44,7 +44,7 @@ BENCHMARK(BM_Calculate)->Arg(0)->Arg(1)->Arg(100)->Arg(10000)->Arg(500000); void BM_Extend(benchmark::State& state) { int len = state.range(0); std::string extension = TestString(len); - absl::crc32c_t base = absl::ToCrc32c(0xC99465AA); // CRC32C of "Hello World" + absl::crc32c_t base = absl::crc32c_t{0xC99465AA}; // CRC32C of "Hello World" for (auto s : state) { benchmark::DoNotOptimize(base); benchmark::DoNotOptimize(extension); @@ -55,7 +55,7 @@ void BM_Extend(benchmark::State& state) { BENCHMARK(BM_Extend)->Arg(0)->Arg(1)->Arg(100)->Arg(10000)->Arg(500000); void BM_ExtendByZeroes(benchmark::State& state) { - absl::crc32c_t base = absl::ToCrc32c(0xC99465AA); // CRC32C of "Hello World" + absl::crc32c_t base = absl::crc32c_t{0xC99465AA}; // CRC32C of "Hello World" int num_zeroes = state.range(0); for (auto s : state) { benchmark::DoNotOptimize(base); @@ -70,7 +70,7 @@ BENCHMARK(BM_ExtendByZeroes) ->Range(1, 1 << 20); void BM_UnextendByZeroes(benchmark::State& state) { - absl::crc32c_t base = absl::ToCrc32c(0xdeadbeef); + absl::crc32c_t base = absl::crc32c_t{0xdeadbeef}; int num_zeroes = state.range(0); for (auto s : state) { benchmark::DoNotOptimize(base); @@ -90,7 +90,7 @@ void BM_Concat(benchmark::State& state) { std::string string_b = TestString(string_b_len); // CRC32C of "Hello World" - absl::crc32c_t crc_a = absl::ToCrc32c(0xC99465AA); + absl::crc32c_t crc_a = absl::crc32c_t{0xC99465AA}; absl::crc32c_t crc_b = absl::ComputeCrc32c(string_b); for (auto s : state) { diff --git a/absl/crc/crc32c_test.cc b/absl/crc/crc32c_test.cc index 98e5fea3..0b9dc683 100644 --- a/absl/crc/crc32c_test.cc +++ b/absl/crc/crc32c_test.cc @@ -34,22 +34,22 @@ TEST(CRC32C, RFC3720) { // 32 bytes of ones. memset(data, 0, sizeof(data)); EXPECT_EQ(absl::ComputeCrc32c(absl::string_view(data, sizeof(data))), - absl::ToCrc32c(0x8a9136aa)); + absl::crc32c_t{0x8a9136aa}); // 32 bytes of ones. memset(data, 0xff, sizeof(data)); EXPECT_EQ(absl::ComputeCrc32c(absl::string_view(data, sizeof(data))), - absl::ToCrc32c(0x62a8ab43)); + absl::crc32c_t{0x62a8ab43}); // 32 incrementing bytes. for (int i = 0; i < 32; ++i) data[i] = static_cast(i); EXPECT_EQ(absl::ComputeCrc32c(absl::string_view(data, sizeof(data))), - absl::ToCrc32c(0x46dd794e)); + absl::crc32c_t{0x46dd794e}); // 32 decrementing bytes. for (int i = 0; i < 32; ++i) data[i] = static_cast(31 - i); EXPECT_EQ(absl::ComputeCrc32c(absl::string_view(data, sizeof(data))), - absl::ToCrc32c(0x113fdb5c)); + absl::crc32c_t{0x113fdb5c}); // An iSCSI - SCSI Read (10) Command PDU. constexpr uint8_t cmd[48] = { @@ -60,7 +60,7 @@ TEST(CRC32C, RFC3720) { }; EXPECT_EQ(absl::ComputeCrc32c(absl::string_view( reinterpret_cast(cmd), sizeof(cmd))), - absl::ToCrc32c(0xd9963a56)); + absl::crc32c_t{0xd9963a56}); } std::string TestString(size_t len) { @@ -73,8 +73,8 @@ std::string TestString(size_t len) { } TEST(CRC32C, Compute) { - EXPECT_EQ(absl::ComputeCrc32c(""), absl::ToCrc32c(0)); - EXPECT_EQ(absl::ComputeCrc32c("hello world"), absl::ToCrc32c(0xc99465aa)); + EXPECT_EQ(absl::ComputeCrc32c(""), absl::crc32c_t{0}); + EXPECT_EQ(absl::ComputeCrc32c("hello world"), absl::crc32c_t{0xc99465aa}); } TEST(CRC32C, Extend) { @@ -82,13 +82,13 @@ TEST(CRC32C, Extend) { std::string extension = "Extension String"; EXPECT_EQ( - absl::ExtendCrc32c(absl::ToCrc32c(base), extension), - absl::ToCrc32c(0xD2F65090)); // CRC32C of "Hello WorldExtension String" + absl::ExtendCrc32c(absl::crc32c_t{base}, extension), + absl::crc32c_t{0xD2F65090}); // CRC32C of "Hello WorldExtension String" } TEST(CRC32C, ExtendByZeroes) { std::string base = "hello world"; - absl::crc32c_t base_crc = absl::ToCrc32c(0xc99465aa); + absl::crc32c_t base_crc = absl::crc32c_t{0xc99465aa}; for (const size_t extend_by : {100, 10000, 100000}) { SCOPED_TRACE(extend_by); @@ -98,7 +98,7 @@ TEST(CRC32C, ExtendByZeroes) { } TEST(CRC32C, UnextendByZeroes) { - for (auto seed_crc : {absl::ToCrc32c(0), absl::ToCrc32c(0xc99465aa)}) { + for (auto seed_crc : {absl::crc32c_t{0}, absl::crc32c_t{0xc99465aa}}) { SCOPED_TRACE(seed_crc); for (const size_t size_1 : {2, 200, 20000, 200000, 20000000}) { for (const size_t size_2 : {0, 100, 10000, 100000, 10000000}) { diff --git a/absl/crc/internal/crc_memcpy.h b/absl/crc/internal/crc_memcpy.h index 8e728a6e..ae9cccad 100644 --- a/absl/crc/internal/crc_memcpy.h +++ b/absl/crc/internal/crc_memcpy.h @@ -40,7 +40,7 @@ class CrcMemcpy { public: static crc32c_t CrcAndCopy(void* __restrict dst, const void* __restrict src, std::size_t length, - crc32c_t initial_crc = ToCrc32c(0), + crc32c_t initial_crc = crc32c_t{0}, bool non_temporal = false) { static const ArchSpecificEngines engines = GetArchSpecificEngines(); auto* engine = non_temporal ? engines.non_temporal : engines.temporal; @@ -100,7 +100,7 @@ class CrcNonTemporalMemcpyAVXEngine : public CrcMemcpyEngine { // the generic fallback version. inline crc32c_t Crc32CAndCopy(void* __restrict dst, const void* __restrict src, std::size_t length, - crc32c_t initial_crc = ToCrc32c(0), + crc32c_t initial_crc = crc32c_t{0}, bool non_temporal = false) { return CrcMemcpy::CrcAndCopy(dst, src, length, initial_crc, non_temporal); } diff --git a/absl/crc/internal/crc_memcpy_test.cc b/absl/crc/internal/crc_memcpy_test.cc index 708e8666..bbdcd205 100644 --- a/absl/crc/internal/crc_memcpy_test.cc +++ b/absl/crc/internal/crc_memcpy_test.cc @@ -108,7 +108,7 @@ TEST_P(x86ParamTest, SmallCorrectnessCheckSourceAlignment) { static_cast(absl::Uniform(gen_)); } absl::crc32c_t initial_crc = - absl::ToCrc32c(absl::Uniform(gen_)); + absl::crc32c_t{absl::Uniform(gen_)}; absl::crc32c_t experiment_crc = engine_->Compute(destination_.get(), source_.get() + source_alignment, size, initial_crc); @@ -139,7 +139,7 @@ TEST_P(x86ParamTest, SmallCorrectnessCheckDestAlignment) { static_cast(absl::Uniform(gen_)); } absl::crc32c_t initial_crc = - absl::ToCrc32c(absl::Uniform(gen_)); + absl::crc32c_t{absl::Uniform(gen_)}; absl::crc32c_t experiment_crc = engine_->Compute(destination_.get() + dest_alignment, source_.get(), size, initial_crc); -- cgit v1.2.3