From e72e773ad0684626ba4ed79f97b7f36cd8e5c923 Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Thu, 14 Jun 2018 14:41:22 -0400 Subject: remove SkTCast SkTCast is functionally equivalent to reinterpret_cast. The comment about SkTCast helping to avoid strict alising issues is not true. Dereferencing a pointer cast to a pointer of an unrelated type is always undefined, even if smuggled through a union like in SkTCast. To really avoid aliasing issues, you need to make a union[1] of the two value types, or better, memcpy between values. I've had to fix MatrixText.cpp where switching to reinterpret_cast actually let Clang notice and warn that we're exploiting undefined behavior, and GrSwizzle.h and SkCamera.cpp caught by GCC. I've switched SkTLList over to use SkAlignedSTStorage, which seems to help convince some GCC versions that fObj is used in a sound way. [1] The union punning trick is non-standard in C++, but GCC and MSVC both explicitly support it. I believe Clang does not officially explicitly support it, but probably does quietly for GCC compatibility. Change-Id: I71822e82c962f9aaac8be24d3c0f39f4f8b05026 Reviewed-on: https://skia-review.googlesource.com/134947 Commit-Queue: Mike Klein Commit-Queue: Brian Salomon Auto-Submit: Mike Klein Reviewed-by: Brian Salomon --- bench/MathBench.cpp | 4 ++-- bench/RectBench.cpp | 4 ++-- include/core/SkTLazy.h | 6 +++--- include/core/SkTypes.h | 14 -------------- include/private/GrSwizzle.h | 8 ++++++-- src/core/SkDraw.cpp | 4 ++-- src/core/SkScan_AntiPath.cpp | 2 +- src/core/SkTLList.h | 22 +++++++++++----------- src/gpu/GrProgramDesc.cpp | 2 +- src/utils/SkCamera.cpp | 8 ++++---- tests/MatrixTest.cpp | 14 ++++++++++---- tests/PointTest.cpp | 4 ++-- tests/RectangleTextureTest.cpp | 4 ++-- 13 files changed, 46 insertions(+), 50 deletions(-) diff --git a/bench/MathBench.cpp b/bench/MathBench.cpp index 7f3408e8e4..74b89a3204 100644 --- a/bench/MathBench.cpp +++ b/bench/MathBench.cpp @@ -76,8 +76,8 @@ protected: int count) = 0; void performTest(float* SK_RESTRICT dst, const float* SK_RESTRICT src, int count) override { - uint32_t* d = SkTCast(dst); - const uint32_t* s = SkTCast(src); + uint32_t* d = reinterpret_cast(dst); + const uint32_t* s = reinterpret_cast(src); this->performITest(d, s, count); } private: diff --git a/bench/RectBench.cpp b/bench/RectBench.cpp index 386fb43dfb..407c3a976f 100644 --- a/bench/RectBench.cpp +++ b/bench/RectBench.cpp @@ -185,7 +185,7 @@ protected: for (size_t i = 0; i < sizes; i++) { paint.setStrokeWidth(gSizes[i]); this->setupPaint(&paint); - canvas->drawPoints(fMode, N * 2, SkTCast(fRects), paint); + canvas->drawPoints(fMode, N * 2, reinterpret_cast(fRects), paint); paint.setColor(fColors[i % N]); } } @@ -263,7 +263,7 @@ protected: this->setupPaint(&paint); paint.setColor(color); paint.setAlpha(alpha); - canvas->drawPoints(fMode, N * 2, SkTCast(fRects), paint); + canvas->drawPoints(fMode, N * 2, reinterpret_cast(fRects), paint); } } } diff --git a/include/core/SkTLazy.h b/include/core/SkTLazy.h index a166f05a74..26ee922985 100644 --- a/include/core/SkTLazy.h +++ b/include/core/SkTLazy.h @@ -61,7 +61,7 @@ public: if (this->isValid()) { fPtr->~T(); } - fPtr = new (SkTCast(fStorage.get())) T(std::forward(args)...); + fPtr = new (reinterpret_cast(fStorage.get())) T(std::forward(args)...); return fPtr; } @@ -75,7 +75,7 @@ public: if (this->isValid()) { *fPtr = src; } else { - fPtr = new (SkTCast(fStorage.get())) T(src); + fPtr = new (reinterpret_cast(fStorage.get())) T(src); } return fPtr; } @@ -84,7 +84,7 @@ public: if (this->isValid()) { *fPtr = std::move(src); } else { - fPtr = new (SkTCast(fStorage.get())) T(std::move(src)); + fPtr = new (reinterpret_cast(fStorage.get())) T(std::move(src)); } return fPtr; } diff --git a/include/core/SkTypes.h b/include/core/SkTypes.h index 262002bbda..e001bb1068 100644 --- a/include/core/SkTypes.h +++ b/include/core/SkTypes.h @@ -247,20 +247,6 @@ enum class SkBackingFit { kExact }; -/////////////////////////////////////////////////////////////////////////////// - -/** - * Use to cast a pointer to a different type, and maintaining strict-aliasing - */ -template Dst SkTCast(const void* ptr) { - union { - const void* src; - Dst dst; - } data; - data.src = ptr; - return data.dst; -} - ////////////////////////////////////////////////////////////////////////////// /** \class SkNoncopyable diff --git a/include/private/GrSwizzle.h b/include/private/GrSwizzle.h index db645dbe49..da5d3e0f16 100644 --- a/include/private/GrSwizzle.h +++ b/include/private/GrSwizzle.h @@ -38,8 +38,12 @@ private: , fKey((CToI(c[0]) << 0) | (CToI(c[1]) << 2) | (CToI(c[2]) << 4) | (CToI(c[3]) << 6)) {} GR_STATIC_ASSERT(sizeof(char[4]) == sizeof(uint32_t)); - uint32_t* asUIntPtr() { return SkTCast(fSwiz); } - uint32_t asUInt() const { return *SkTCast(fSwiz); } + uint32_t* asUIntPtr() { return reinterpret_cast(fSwiz); } + uint32_t asUInt() const { + uint32_t v; + memcpy(&v, fSwiz, 4); + return v; + } public: GrSwizzle() { *this = RGBA(); } diff --git a/src/core/SkDraw.cpp b/src/core/SkDraw.cpp index 2c8eb84845..097569fab2 100644 --- a/src/core/SkDraw.cpp +++ b/src/core/SkDraw.cpp @@ -723,11 +723,11 @@ SkDraw::RectType SkDraw::ComputeRectType(const SkPaint& paint, } static const SkPoint* rect_points(const SkRect& r) { - return SkTCast(&r); + return reinterpret_cast(&r); } static SkPoint* rect_points(SkRect& r) { - return SkTCast(&r); + return reinterpret_cast(&r); } static void draw_rect_as_path(const SkDraw& orig, const SkRect& prePaintRect, diff --git a/src/core/SkScan_AntiPath.cpp b/src/core/SkScan_AntiPath.cpp index 9c64f32496..13ca3c8b9a 100644 --- a/src/core/SkScan_AntiPath.cpp +++ b/src/core/SkScan_AntiPath.cpp @@ -497,7 +497,7 @@ static void add_aa_span(uint8_t* alpha, U8CPU startAlpha, int middleCount, if (middleCount >= MIN_COUNT_FOR_QUAD_LOOP) { // loop until we're quad-byte aligned - while (SkTCast(alpha) & 0x3) { + while (reinterpret_cast(alpha) & 0x3) { alpha[0] = SkToU8(alpha[0] + maxValue); alpha += 1; middleCount -= 1; diff --git a/src/core/SkTLList.h b/src/core/SkTLList.h index b865538c27..8680ea2e45 100644 --- a/src/core/SkTLList.h +++ b/src/core/SkTLList.h @@ -8,9 +8,9 @@ #ifndef SkTLList_DEFINED #define SkTLList_DEFINED -#include "SkTInternalLList.h" - #include "SkMalloc.h" +#include "SkTInternalLList.h" +#include "SkTemplates.h" #include "SkTypes.h" #include #include @@ -32,7 +32,7 @@ template class SkTLList : SkNoncopyable { private: struct Block; struct Node { - char fObj[sizeof(T)]; + SkAlignedSTStorage<1, T> fObj; SK_DECLARE_INTERNAL_LLIST_INTERFACE(Node); Block* fBlock; // owning block. }; @@ -51,7 +51,7 @@ public: typename NodeList::Iter iter; Node* node = iter.init(fList, Iter::kHead_IterStart); while (node) { - SkTCast(node->fObj)->~T(); + reinterpret_cast(node->fObj.get())->~T(); Block* block = node->fBlock; node = iter.next(); if (0 == --block->fNodesInUse) { @@ -71,7 +71,7 @@ public: Node* node = this->createNode(); fList.addToHead(node); this->validate(); - return new (node->fObj) T(std::forward(args)...); + return new (node->fObj.get()) T(std::forward(args)...); } /** Adds a new element to the list at the tail. */ @@ -80,7 +80,7 @@ public: Node* node = this->createNode(); fList.addToTail(node); this->validate(); - return new (node->fObj) T(std::forward(args)...); + return new (node->fObj.get()) T(std::forward(args)...); } /** Adds a new element to the list before the location indicated by the iterator. If the @@ -90,7 +90,7 @@ public: Node* node = this->createNode(); fList.addBefore(node, location.getNode()); this->validate(); - return new (node->fObj) T(std::forward(args)...); + return new (node->fObj.get()) T(std::forward(args)...); } /** Adds a new element to the list after the location indicated by the iterator. If the @@ -100,7 +100,7 @@ public: Node* node = this->createNode(); fList.addAfter(node, location.getNode()); this->validate(); - return new (node->fObj) T(std::forward(args)...); + return new (node->fObj.get()) T(std::forward(args)...); } /** Convenience methods for getting an iterator initialized to the head/tail of the list. */ @@ -133,7 +133,7 @@ public: void remove(T* t) { this->validate(); Node* node = reinterpret_cast(t); - SkASSERT(reinterpret_cast(node->fObj) == t); + SkASSERT(reinterpret_cast(node->fObj.get()) == t); this->removeNode(node); this->validate(); } @@ -210,7 +210,7 @@ public: T* nodeToObj(Node* node) { if (node) { - return reinterpret_cast(node->fObj); + return reinterpret_cast(node->fObj.get()); } else { return nullptr; } @@ -264,7 +264,7 @@ private: void removeNode(Node* node) { SkASSERT(node); fList.remove(node); - SkTCast(node->fObj)->~T(); + reinterpret_cast(node->fObj.get())->~T(); Block* block = node->fBlock; // Don't ever elease the first block, just add its nodes to the free list if (0 == --block->fNodesInUse && block != &fFirstBlock) { diff --git a/src/gpu/GrProgramDesc.cpp b/src/gpu/GrProgramDesc.cpp index c552789d9f..e5da3be6ee 100644 --- a/src/gpu/GrProgramDesc.cpp +++ b/src/gpu/GrProgramDesc.cpp @@ -65,7 +65,7 @@ static void add_sampler_and_image_keys(GrProcessorKeyBuilder* b, const GrResourc if (0 == word32Count) { return; } - uint16_t* k16 = SkTCast(b->add32n(word32Count)); + uint16_t* k16 = reinterpret_cast(b->add32n(word32Count)); int j = 0; for (int i = 0; i < numTextureSamplers; ++i, ++j) { const GrResourceIOProcessor::TextureSampler& sampler = proc.textureSampler(i); diff --git a/src/utils/SkCamera.cpp b/src/utils/SkCamera.cpp index cb364a504e..3475bb53d5 100644 --- a/src/utils/SkCamera.cpp +++ b/src/utils/SkCamera.cpp @@ -218,13 +218,13 @@ void SkCamera3D::doUpdate() const { fAxis.normalize(&axis); { - SkScalar dot = SkUnit3D::Dot(*SkTCast(&fZenith), axis); + SkScalar dot = SkUnit3D::Dot(SkUnit3D{fZenith.fX, fZenith.fY, fZenith.fZ}, axis); zenith.fX = fZenith.fX - dot * axis.fX; zenith.fY = fZenith.fY - dot * axis.fY; zenith.fZ = fZenith.fZ - dot * axis.fZ; - SkTCast(&zenith)->normalize(&zenith); + SkPoint3D{zenith.fX, zenith.fY, zenith.fZ}.normalize(&zenith); } SkUnit3D::Cross(axis, zenith, &cross); @@ -276,8 +276,8 @@ void SkCamera3D::patchToMatrix(const SkPatch3D& quilt, SkMatrix* matrix) const { diff.fY = quilt.fOrigin.fY - fLocation.fY; diff.fZ = quilt.fOrigin.fZ - fLocation.fZ; - dot = SkUnit3D::Dot(*SkTCast(&diff), - *SkTCast(SkTCast(&fOrientation) + 6)); + dot = SkUnit3D::Dot(SkUnit3D{diff.fX, diff.fY, diff.fZ}, + SkUnit3D{mapPtr[6], mapPtr[7], mapPtr[8]}); // This multiplies fOrientation by the matrix [quilt.fU quilt.fV diff] -- U, V, and diff are // column vectors in the matrix -- then divides by the length of the projection of diff onto diff --git a/tests/MatrixTest.cpp b/tests/MatrixTest.cpp index 5dfe2ca936..03661dc437 100644 --- a/tests/MatrixTest.cpp +++ b/tests/MatrixTest.cpp @@ -27,6 +27,12 @@ static bool nearly_equal(const SkMatrix& a, const SkMatrix& b) { return true; } +static int float_bits(float f) { + int result; + memcpy(&result, &f, 4); + return result; +} + static bool are_equal(skiatest::Reporter* reporter, const SkMatrix& a, const SkMatrix& b) { @@ -38,8 +44,8 @@ static bool are_equal(skiatest::Reporter* reporter, for (int i = 0; i < 9; ++i) { float aVal = a.get(i); float bVal = b.get(i); - int aValI = *SkTCast(&aVal); - int bValI = *SkTCast(&bVal); + int aValI = float_bits(aVal); + int bValI = float_bits(bVal); if (0 == aVal && 0 == bVal && aValI != bValI) { foundZeroSignDiff = true; } else { @@ -52,8 +58,8 @@ static bool are_equal(skiatest::Reporter* reporter, for (int i = 0; i < 9; ++i) { float aVal = a.get(i); float bVal = b.get(i); - int aValI = *SkTCast(&aVal); - int bValI = *SkTCast(&bVal); + int aValI = float_bits(aVal); + int bValI = float_bits(bVal); if (sk_float_isnan(aVal) && aValI == bValI) { foundNaN = true; } else { diff --git a/tests/PointTest.cpp b/tests/PointTest.cpp index 76ac3a1a79..4981e89dde 100644 --- a/tests/PointTest.cpp +++ b/tests/PointTest.cpp @@ -14,8 +14,8 @@ static void test_casts(skiatest::Reporter* reporter) { SkPoint p = { 0, 0 }; SkRect r = { 0, 0, 0, 0 }; - const SkScalar* pPtr = SkTCast(&p); - const SkScalar* rPtr = SkTCast(&r); + const SkScalar* pPtr = reinterpret_cast(&p); + const SkScalar* rPtr = reinterpret_cast(&r); REPORTER_ASSERT(reporter, SkPointPriv::AsScalars(p) == pPtr); REPORTER_ASSERT(reporter, r.asScalars() == rPtr); diff --git a/tests/RectangleTextureTest.cpp b/tests/RectangleTextureTest.cpp index 90b723a3eb..e473ed86f3 100644 --- a/tests/RectangleTextureTest.cpp +++ b/tests/RectangleTextureTest.cpp @@ -55,7 +55,7 @@ static void test_clear(skiatest::Reporter* reporter, GrSurfaceContext* rectConte // The clear color is a GrColor, our readback is to kRGBA_8888, which may be different. uint32_t expectedColor0 = 0; - uint8_t* expectedBytes0 = SkTCast(&expectedColor0); + uint8_t* expectedBytes0 = reinterpret_cast(&expectedColor0); expectedBytes0[0] = GrColorUnpackR(color0); expectedBytes0[1] = GrColorUnpackG(color0); expectedBytes0[2] = GrColorUnpackB(color0); @@ -70,7 +70,7 @@ static void test_clear(skiatest::Reporter* reporter, GrSurfaceContext* rectConte rtc->clear(&rect, color1, GrRenderTargetContext::CanClearFullscreen::kNo); uint32_t expectedColor1 = 0; - uint8_t* expectedBytes1 = SkTCast(&expectedColor1); + uint8_t* expectedBytes1 = reinterpret_cast(&expectedColor1); expectedBytes1[0] = GrColorUnpackR(color1); expectedBytes1[1] = GrColorUnpackG(color1); expectedBytes1[2] = GrColorUnpackB(color1); -- cgit v1.2.3