From aa999cb952753d373c03befa7fce8c4700ef9308 Mon Sep 17 00:00:00 2001 From: mtklein Date: Fri, 22 May 2015 17:18:21 -0700 Subject: [PATCH] Everyone gets a namespace {}. If we include Sk4px.h, SkPMFloat.h, or SkNx.h into files with different SIMD flags, that could cause different definitions of the same method. Normally that's moot, because all the code inlines, but in Debug it tends not to. So in Debug, the linker picks one definition for us. That breaks _someone_. Wrapping everything in a namespace {} keeps the definitions separate. Tested locally, it fixes this bug. BUG=skia:3861 This code is not yet enabled in Chrome, so shouldn't affect the roll. NOTREECHECKS=true Review URL: https://codereview.chromium.org/1154523004 --- src/core/Sk4px.h | 9 +++++++++ src/core/SkNx.h | 8 ++++++++ src/core/SkPMFloat.h | 9 +++++++++ src/opts/Sk4px_NEON.h | 4 ++++ src/opts/Sk4px_SSE2.h | 4 ++++ src/opts/Sk4px_none.h | 3 +++ src/opts/SkNx_neon.h | 4 +++- src/opts/SkNx_sse.h | 5 ++++- src/opts/SkPMFloat_SSE2.h | 4 ++++ src/opts/SkPMFloat_SSSE3.h | 4 ++++ src/opts/SkPMFloat_neon.h | 4 ++++ src/opts/SkPMFloat_none.h | 4 ++++ 12 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src/core/Sk4px.h b/src/core/Sk4px.h index 48e09e1..8fb546a 100644 --- a/src/core/Sk4px.h +++ b/src/core/Sk4px.h @@ -11,6 +11,13 @@ #include "SkNx.h" #include "SkColor.h" +// This file may be included multiple times by .cpp files with different flags, leading +// to different definitions. Usually that doesn't matter because it's all inlined, but +// in Debug modes the compilers may not inline everything. So wrap everything in an +// anonymous namespace to give each includer their own silo of this code (or the linker +// will probably pick one randomly for us, which is rarely correct). +namespace { + // 1, 2 or 4 SkPMColors, generally vectorized. class Sk4px : public Sk16b { public: @@ -171,6 +178,8 @@ private: typedef Sk16b INHERITED; }; +} // namespace + #ifdef SKNX_NO_SIMD #include "../opts/Sk4px_none.h" #else diff --git a/src/core/SkNx.h b/src/core/SkNx.h index af71948..0f5aa82 100644 --- a/src/core/SkNx.h +++ b/src/core/SkNx.h @@ -17,6 +17,13 @@ #include #define REQUIRE(x) static_assert(x, #x) +// This file may be included multiple times by .cpp files with different flags, leading +// to different definitions. Usually that doesn't matter because it's all inlined, but +// in Debug modes the compilers may not inline everything. So wrap everything in an +// anonymous namespace to give each includer their own silo of this code (or the linker +// will probably pick one randomly for us, which is rarely correct). +namespace { + // The default implementations just fall back on a pair of size N/2. // SkNb is a _very_ minimal class representing a vector of bools returned by comparison operators. @@ -251,6 +258,7 @@ protected: T fVal; }; +} // namespace // Generic syntax sugar that should work equally well for all implementations. template T operator - (const T& l) { return T(0) - l; } diff --git a/src/core/SkPMFloat.h b/src/core/SkPMFloat.h index eb575f2..ce7b75e 100644 --- a/src/core/SkPMFloat.h +++ b/src/core/SkPMFloat.h @@ -13,6 +13,13 @@ #include "SkColorPriv.h" #include "SkNx.h" +// This file may be included multiple times by .cpp files with different flags, leading +// to different definitions. Usually that doesn't matter because it's all inlined, but +// in Debug modes the compilers may not inline everything. So wrap everything in an +// anonymous namespace to give each includer their own silo of this code (or the linker +// will probably pick one randomly for us, which is rarely correct). +namespace { + // A pre-multiplied color storing each component in the same order as SkPMColor, // but as a float in the range [0, 255]. class SkPMFloat : public Sk4f { @@ -67,6 +74,8 @@ private: typedef Sk4f INHERITED; }; +} // namespace + #ifdef SKNX_NO_SIMD // Platform implementations of SkPMFloat assume Sk4f uses SSE or NEON. _none is generic. #include "../opts/SkPMFloat_none.h" diff --git a/src/opts/Sk4px_NEON.h b/src/opts/Sk4px_NEON.h index f0d9b56..494052c 100644 --- a/src/opts/Sk4px_NEON.h +++ b/src/opts/Sk4px_NEON.h @@ -5,6 +5,8 @@ * found in the LICENSE file. */ +namespace { // See Sk4px.h + inline Sk4px::Sk4px(SkPMColor px) : INHERITED((uint8x16_t)vdupq_n_u32(px)) {} inline Sk4px Sk4px::Load4(const SkPMColor px[4]) { @@ -88,3 +90,5 @@ inline Sk4px Sk4px::zeroAlphas() const { return Sk16b(vbicq_u8(this->fVec, (uint8x16_t)vdupq_n_u32(0xFF << SK_A32_SHIFT))); } +} // namespace + diff --git a/src/opts/Sk4px_SSE2.h b/src/opts/Sk4px_SSE2.h index 9ba5103..ee58c38 100644 --- a/src/opts/Sk4px_SSE2.h +++ b/src/opts/Sk4px_SSE2.h @@ -5,6 +5,8 @@ * found in the LICENSE file. */ +namespace { // See Sk4px.h + inline Sk4px::Sk4px(SkPMColor px) : INHERITED(_mm_set1_epi32(px)) {} inline Sk4px Sk4px::Load4(const SkPMColor px[4]) { @@ -85,3 +87,5 @@ inline Sk4px Sk4px::zeroAlphas() const { // andnot(a,b) == ~a & b return Sk16b(_mm_andnot_si128(_mm_set1_epi32(0xFF << SK_A32_SHIFT), this->fVec)); } + +} // namespace diff --git a/src/opts/Sk4px_none.h b/src/opts/Sk4px_none.h index 541443d..d3ead31 100644 --- a/src/opts/Sk4px_none.h +++ b/src/opts/Sk4px_none.h @@ -7,6 +7,8 @@ #include "SkUtils.h" +namespace { // See Sk4px.h + static_assert(sizeof(Sk4px) == 16, "This file uses memcpy / sk_memset32, so exact size matters."); inline Sk4px::Sk4px(SkPMColor px) { @@ -94,3 +96,4 @@ inline Sk4px Sk4px::zeroColors() const { 0,0,0, this->kth<15>()); } +} // namespace diff --git a/src/opts/SkNx_neon.h b/src/opts/SkNx_neon.h index 08691fe..2087b88 100644 --- a/src/opts/SkNx_neon.h +++ b/src/opts/SkNx_neon.h @@ -8,7 +8,7 @@ #ifndef SkNx_neon_DEFINED #define SkNx_neon_DEFINED -#include +namespace { // See SkNx.h // Well, this is absurd. The shifts require compile-time constant arguments. @@ -380,4 +380,6 @@ public: #undef SHIFT16 #undef SHIFT8 +} // namespace + #endif//SkNx_neon_DEFINED diff --git a/src/opts/SkNx_sse.h b/src/opts/SkNx_sse.h index 0e9494c..ed78892 100644 --- a/src/opts/SkNx_sse.h +++ b/src/opts/SkNx_sse.h @@ -9,7 +9,8 @@ #define SkNx_sse_DEFINED // This file may assume <= SSE2, but must check SK_CPU_SSE_LEVEL for anything more recent. -#include + +namespace { // See SkNx.h template <> class SkNb<2, 4> { @@ -327,4 +328,6 @@ public: __m128i fVec; }; +} // namespace + #endif//SkNx_sse_DEFINED diff --git a/src/opts/SkPMFloat_SSE2.h b/src/opts/SkPMFloat_SSE2.h index 88a3803..c7e791f 100644 --- a/src/opts/SkPMFloat_SSE2.h +++ b/src/opts/SkPMFloat_SSE2.h @@ -5,6 +5,8 @@ * found in the LICENSE file. */ +namespace { // See SkPMFloat.h + // For SkPMFloat(SkPMColor), we widen our 8 bit components (fix8) to 8-bit components in 16 bits // (fix8_16), then widen those to 8-bit-in-32-bits (fix8_32), and finally convert those to floats. @@ -78,3 +80,5 @@ inline void SkPMFloat::RoundClampTo4PMColors( SkPMColorAssert(colors[2]); SkPMColorAssert(colors[3]); } + +} // namespace diff --git a/src/opts/SkPMFloat_SSSE3.h b/src/opts/SkPMFloat_SSSE3.h index 9ff7356..67116ec 100644 --- a/src/opts/SkPMFloat_SSSE3.h +++ b/src/opts/SkPMFloat_SSSE3.h @@ -5,6 +5,8 @@ * found in the LICENSE file. */ +namespace { // See SkPMFloat.h + // For SkPMFloat(SkPMColor), we widen our 8 bit components (fix8) to 8-bit components in 32 bits // (fix8_32), then convert those to floats. @@ -81,3 +83,5 @@ inline void SkPMFloat::RoundClampTo4PMColors( SkPMColorAssert(colors[2]); SkPMColorAssert(colors[3]); } + +} // namespace diff --git a/src/opts/SkPMFloat_neon.h b/src/opts/SkPMFloat_neon.h index 1c01bc4..cabb29a 100644 --- a/src/opts/SkPMFloat_neon.h +++ b/src/opts/SkPMFloat_neon.h @@ -5,6 +5,8 @@ * found in the LICENSE file. */ +namespace { // See SkPMFloat.h + // For SkPMFloat(SkPMFColor), we widen our 8 bit components (fix8) to 8-bit components in 16 bits // (fix8_16), then widen those to 8-bit-in-32-bits (fix8_32), and finally convert those to floats. @@ -70,3 +72,5 @@ inline void SkPMFloat::RoundClampTo4PMColors( colors[2] = c.roundClamp(); colors[3] = d.roundClamp(); } + +} // namespace diff --git a/src/opts/SkPMFloat_none.h b/src/opts/SkPMFloat_none.h index 50f2427..9bb584e 100644 --- a/src/opts/SkPMFloat_none.h +++ b/src/opts/SkPMFloat_none.h @@ -5,6 +5,8 @@ * found in the LICENSE file. */ +namespace { // See SkPMFloat.h + inline SkPMFloat::SkPMFloat(SkPMColor c) { *this = SkPMFloat::FromARGB(SkGetPackedA32(c), SkGetPackedR32(c), @@ -62,3 +64,5 @@ inline void SkPMFloat::RoundClampTo4PMColors( colors[2] = c.roundClamp(); colors[3] = d.roundClamp(); } + +} // namespace -- 2.7.4