From 166dbd3135e02d0e874591db02f9b1ad7fc961f5 Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Tue, 6 Nov 2018 08:23:30 -0500 Subject: [PATCH] make enum santizer fatal This enum sanitizer checks that all the values of the enum we use fall within the range of the enumerated values. The main thing this helps point out is that the size of enum types in C++ need only be large enough to hold the largest declared value; larger values are undefined. In practice, most enums are implemented as ints for compatibility with C, so while this hasn't pointed out anything egregiously broken, the sanitizer has found a couple possibly dangerous situations in our codebase. For most types using values outside the enum range, we can just explicitly size them to int. This makes their de facto size de jure. But we need to actually make GrBlendEquation and GrBlendCoeff not store values outside their enumerated range. They're packed into bitfields that really can't represent those (negative) values. So for these I've added new kIllegal values to the enums, forcing us to deal with our once-silent illegal values a bit more explicitly. Change-Id: Ib617694cf1aaa83ae99289e9e760f49cb6393a2f Reviewed-on: https://skia-review.googlesource.com/c/168484 Reviewed-by: Brian Osman --- gn/BUILD.gn | 2 +- include/core/SkBlurTypes.h | 2 +- include/core/SkPath.h | 2 +- include/gpu/GrBlend.h | 8 ++++++-- include/gpu/GrTypes.h | 2 +- include/private/GrTypesPriv.h | 2 +- src/core/SkPathPriv.h | 2 +- src/gpu/GrXferProcessor.cpp | 6 ++++++ src/gpu/effects/GrCustomXfermode.cpp | 9 ++++++--- src/gpu/gl/GrGLGpu.cpp | 11 ++++++++++- src/gpu/gl/GrGLGpu.h | 6 +++--- src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp | 3 ++- src/gpu/mtl/GrMtlPipelineStateBuilder.mm | 3 ++- src/gpu/vk/GrVkPipeline.cpp | 10 ++++++++-- 14 files changed, 49 insertions(+), 19 deletions(-) diff --git a/gn/BUILD.gn b/gn/BUILD.gn index 3d4c1104f4..e93bb76dc1 100644 --- a/gn/BUILD.gn +++ b/gn/BUILD.gn @@ -237,7 +237,7 @@ config("default") { # A whitelist of checks we can't yet pass. if (fyi_sanitize == "" && !is_android) { - fyi_sanitizers = "enum,float-divide-by-zero" + fyi_sanitizers = "float-divide-by-zero" } if (is_android) { diff --git a/include/core/SkBlurTypes.h b/include/core/SkBlurTypes.h index 490beafc0c..257e13200f 100644 --- a/include/core/SkBlurTypes.h +++ b/include/core/SkBlurTypes.h @@ -10,7 +10,7 @@ #include "SkTypes.h" -enum SkBlurStyle { +enum SkBlurStyle : int { kNormal_SkBlurStyle, //!< fuzzy inside and outside kSolid_SkBlurStyle, //!< solid inside, fuzzy outside kOuter_SkBlurStyle, //!< nothing inside, fuzzy outside diff --git a/include/core/SkPath.h b/include/core/SkPath.h index e58e7b5300..44c666df5c 100644 --- a/include/core/SkPath.h +++ b/include/core/SkPath.h @@ -63,7 +63,7 @@ public: kCW_Direction travel clockwise; the same added with kCCW_Direction travel counterclockwise. */ - enum Direction { + enum Direction : int { kCW_Direction, //!< contour travels clockwise kCCW_Direction, //!< contour travels counterclockwise }; diff --git a/include/gpu/GrBlend.h b/include/gpu/GrBlend.h index ab75ee215f..6cdf3f6263 100644 --- a/include/gpu/GrBlend.h +++ b/include/gpu/GrBlend.h @@ -37,8 +37,10 @@ enum GrBlendEquation { kHSLColor_GrBlendEquation, kHSLLuminosity_GrBlendEquation, + kIllegal_GrBlendEquation, + kFirstAdvancedGrBlendEquation = kScreen_GrBlendEquation, - kLast_GrBlendEquation = kHSLLuminosity_GrBlendEquation + kLast_GrBlendEquation = kIllegal_GrBlendEquation, }; static const int kGrBlendEquationCnt = kLast_GrBlendEquation + 1; @@ -67,7 +69,9 @@ enum GrBlendCoeff { kS2A_GrBlendCoeff, kIS2A_GrBlendCoeff, - kLast_GrBlendCoeff = kIS2A_GrBlendCoeff + kIllegal_GrBlendCoeff, + + kLast_GrBlendCoeff = kIllegal_GrBlendCoeff, }; static const int kGrBlendCoeffCnt = kLast_GrBlendCoeff + 1; diff --git a/include/gpu/GrTypes.h b/include/gpu/GrTypes.h index 1b841334ad..6082d97127 100644 --- a/include/gpu/GrTypes.h +++ b/include/gpu/GrTypes.h @@ -227,7 +227,7 @@ enum class GrMipMapped : bool { * GPU SkImage and SkSurfaces can be stored such that (0, 0) in texture space may correspond to * either the top-left or bottom-left content pixel. */ -enum GrSurfaceOrigin { +enum GrSurfaceOrigin : int { kTopLeft_GrSurfaceOrigin, kBottomLeft_GrSurfaceOrigin, }; diff --git a/include/private/GrTypesPriv.h b/include/private/GrTypesPriv.h index 4d8e3c0b89..d20b4e35ac 100644 --- a/include/private/GrTypesPriv.h +++ b/include/private/GrTypesPriv.h @@ -384,7 +384,7 @@ GR_MAKE_BITFIELD_OPS(GrShaderFlags); * vary the internal precision based on the qualifiers. These currently only apply to float types ( * including float vectors and matrices). */ -enum GrSLPrecision { +enum GrSLPrecision : int { kLow_GrSLPrecision, kMedium_GrSLPrecision, kHigh_GrSLPrecision, diff --git a/src/core/SkPathPriv.h b/src/core/SkPathPriv.h index bcf85a138d..b6f6ff6fe5 100644 --- a/src/core/SkPathPriv.h +++ b/src/core/SkPathPriv.h @@ -18,7 +18,7 @@ public: static const int kPathRefGenIDBitCnt = 32; #endif - enum FirstDirection { + enum FirstDirection : int { kCW_FirstDirection, // == SkPath::kCW_Direction kCCW_FirstDirection, // == SkPath::kCCW_Direction kUnknown_FirstDirection, diff --git a/src/gpu/GrXferProcessor.cpp b/src/gpu/GrXferProcessor.cpp index b32f25f67d..e40dfd52f9 100644 --- a/src/gpu/GrXferProcessor.cpp +++ b/src/gpu/GrXferProcessor.cpp @@ -99,6 +99,9 @@ static const char* equation_string(GrBlendEquation eq) { return "hsl_color"; case kHSLLuminosity_GrBlendEquation: return "hsl_luminosity"; + case kIllegal_GrBlendEquation: + SkASSERT(false); + return ""; }; return ""; } @@ -141,6 +144,9 @@ static const char* coeff_string(GrBlendCoeff coeff) { return "src2_alpha"; case kIS2A_GrBlendCoeff: return "inv_src2_alpha"; + case kIllegal_GrBlendCoeff: + SkASSERT(false); + return ""; } return ""; } diff --git a/src/gpu/effects/GrCustomXfermode.cpp b/src/gpu/effects/GrCustomXfermode.cpp index 5c352604c6..3d39c54015 100644 --- a/src/gpu/effects/GrCustomXfermode.cpp +++ b/src/gpu/effects/GrCustomXfermode.cpp @@ -46,7 +46,10 @@ static constexpr GrBlendEquation hw_blend_equation(SkBlendMode mode) { GR_STATIC_ASSERT(kHSLSaturation_GrBlendEquation == (int)SkBlendMode::kSaturation + EQ_OFFSET); GR_STATIC_ASSERT(kHSLColor_GrBlendEquation == (int)SkBlendMode::kColor + EQ_OFFSET); GR_STATIC_ASSERT(kHSLLuminosity_GrBlendEquation == (int)SkBlendMode::kLuminosity + EQ_OFFSET); - GR_STATIC_ASSERT(kGrBlendEquationCnt == (int)SkBlendMode::kLastMode + 1 + EQ_OFFSET); + + // There's an illegal GrBlendEquation that corresponds to no SkBlendMode, hence the extra +1. + GR_STATIC_ASSERT(kGrBlendEquationCnt == (int)SkBlendMode::kLastMode + 1 + 1 + EQ_OFFSET); + return static_cast((int)mode + EQ_OFFSET); #undef EQ_OFFSET } @@ -79,7 +82,7 @@ public: CustomXP(bool hasMixedSamples, SkBlendMode mode, GrProcessorAnalysisCoverage coverage) : INHERITED(kCustomXP_ClassID, true, hasMixedSamples, coverage) , fMode(mode) - , fHWBlendEquation(static_cast(-1)) { + , fHWBlendEquation(kIllegal_GrBlendEquation) { } const char* name() const override { return "Custom Xfermode"; } @@ -87,7 +90,7 @@ public: GrGLSLXferProcessor* createGLSLInstance() const override; SkBlendMode mode() const { return fMode; } - bool hasHWBlendEquation() const { return -1 != static_cast(fHWBlendEquation); } + bool hasHWBlendEquation() const { return kIllegal_GrBlendEquation != fHWBlendEquation; } GrBlendEquation hwBlendEquation() const { SkASSERT(this->hasHWBlendEquation()); diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp index f9b621743d..316d46637d 100644 --- a/src/gpu/gl/GrGLGpu.cpp +++ b/src/gpu/gl/GrGLGpu.cpp @@ -78,7 +78,10 @@ static const GrGLenum gXfermodeEquation2Blend[] = { GR_GL_HSL_HUE, GR_GL_HSL_SATURATION, GR_GL_HSL_COLOR, - GR_GL_HSL_LUMINOSITY + GR_GL_HSL_LUMINOSITY, + + // Illegal... needs to map to something. + GR_GL_FUNC_ADD, }; GR_STATIC_ASSERT(0 == kAdd_GrBlendEquation); GR_STATIC_ASSERT(1 == kSubtract_GrBlendEquation); @@ -121,6 +124,9 @@ static const GrGLenum gXfermodeCoeff2Blend[] = { GR_GL_ONE_MINUS_SRC1_COLOR, GR_GL_SRC1_ALPHA, GR_GL_ONE_MINUS_SRC1_ALPHA, + + // Illegal... needs to map to something. + GR_GL_ZERO, }; bool GrGLGpu::BlendCoeffReferencesConstant(GrBlendCoeff coeff) { @@ -145,6 +151,9 @@ bool GrGLGpu::BlendCoeffReferencesConstant(GrBlendCoeff coeff) { false, false, false, + + // Illegal. + false, }; return gCoeffReferencesBlendConst[coeff]; GR_STATIC_ASSERT(kGrBlendCoeffCnt == SK_ARRAY_COUNT(gCoeffReferencesBlendConst)); diff --git a/src/gpu/gl/GrGLGpu.h b/src/gpu/gl/GrGLGpu.h index 270e28a7c7..ded1ffb379 100644 --- a/src/gpu/gl/GrGLGpu.h +++ b/src/gpu/gl/GrGLGpu.h @@ -566,9 +566,9 @@ private: TriState fEnabled; void invalidate() { - fEquation = static_cast(-1); - fSrcCoeff = static_cast(-1); - fDstCoeff = static_cast(-1); + fEquation = kIllegal_GrBlendEquation; + fSrcCoeff = kIllegal_GrBlendCoeff; + fDstCoeff = kIllegal_GrBlendCoeff; fConstColorValid = false; fEnabled = kUnknown_TriState; } diff --git a/src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp b/src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp index 788fd702fe..35f0fba66d 100644 --- a/src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp +++ b/src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp @@ -53,8 +53,9 @@ static const char* specific_layout_qualifier_name(GrBlendEquation equation) { GR_STATIC_ASSERT(12 == kHSLSaturation_GrBlendEquation - kFirstAdvancedGrBlendEquation); GR_STATIC_ASSERT(13 == kHSLColor_GrBlendEquation - kFirstAdvancedGrBlendEquation); GR_STATIC_ASSERT(14 == kHSLLuminosity_GrBlendEquation - kFirstAdvancedGrBlendEquation); + // There's an illegal GrBlendEquation at the end there, hence the -1. GR_STATIC_ASSERT(SK_ARRAY_COUNT(kLayoutQualifierNames) == - kGrBlendEquationCnt - kFirstAdvancedGrBlendEquation); + kGrBlendEquationCnt - kFirstAdvancedGrBlendEquation - 1); } uint8_t GrGLSLFragmentShaderBuilder::KeyForSurfaceOrigin(GrSurfaceOrigin origin) { diff --git a/src/gpu/mtl/GrMtlPipelineStateBuilder.mm b/src/gpu/mtl/GrMtlPipelineStateBuilder.mm index eb65858c17..8e08073b16 100644 --- a/src/gpu/mtl/GrMtlPipelineStateBuilder.mm +++ b/src/gpu/mtl/GrMtlPipelineStateBuilder.mm @@ -227,6 +227,7 @@ static MTLBlendFactor blend_coeff_to_mtl_blend(GrBlendCoeff coeff) { MTLBlendFactorOneMinusSource1Color, // kIS2C_GrBlendCoeff MTLBlendFactorSource1Alpha, // kS2A_GrBlendCoeff MTLBlendFactorOneMinusSource1Alpha, // kIS2A_GrBlendCoeff + MTLBlendFactorZero, // kIllegal_GrBlendCoeff }; GR_STATIC_ASSERT(SK_ARRAY_COUNT(gTable) == kGrBlendCoeffCnt); GR_STATIC_ASSERT(0 == kZero_GrBlendCoeff); @@ -263,7 +264,7 @@ static MTLBlendOperation blend_equation_to_mtl_blend_op(GrBlendEquation equation GR_STATIC_ASSERT(1 == kSubtract_GrBlendEquation); GR_STATIC_ASSERT(2 == kReverseSubtract_GrBlendEquation); - SkASSERT((unsigned)equation < kGrBlendCoeffCnt); + SkASSERT((unsigned)equation < kGrBlendEquationCnt); return gTable[equation]; } diff --git a/src/gpu/vk/GrVkPipeline.cpp b/src/gpu/vk/GrVkPipeline.cpp index e0ede4c043..2675e7adb7 100644 --- a/src/gpu/vk/GrVkPipeline.cpp +++ b/src/gpu/vk/GrVkPipeline.cpp @@ -320,7 +320,7 @@ static VkBlendFactor blend_coeff_to_vk_blend(GrBlendCoeff coeff) { VK_BLEND_FACTOR_ONE_MINUS_SRC1_COLOR, // kIS2C_GrBlendCoeff VK_BLEND_FACTOR_SRC1_ALPHA, // kS2A_GrBlendCoeff VK_BLEND_FACTOR_ONE_MINUS_SRC1_ALPHA, // kIS2A_GrBlendCoeff - + VK_BLEND_FACTOR_ZERO, // kIllegal_GrBlendCoeff }; GR_STATIC_ASSERT(SK_ARRAY_COUNT(gTable) == kGrBlendCoeffCnt); GR_STATIC_ASSERT(0 == kZero_GrBlendCoeff); @@ -369,7 +369,10 @@ static VkBlendOp blend_equation_to_vk_blend_op(GrBlendEquation equation) { VK_BLEND_OP_HSL_HUE_EXT, VK_BLEND_OP_HSL_SATURATION_EXT, VK_BLEND_OP_HSL_COLOR_EXT, - VK_BLEND_OP_HSL_LUMINOSITY_EXT + VK_BLEND_OP_HSL_LUMINOSITY_EXT, + + // Illegal. + VK_BLEND_OP_ADD, }; GR_STATIC_ASSERT(0 == kAdd_GrBlendEquation); GR_STATIC_ASSERT(1 == kSubtract_GrBlendEquation); @@ -417,6 +420,9 @@ static bool blend_coeff_refs_constant(GrBlendCoeff coeff) { false, false, false, + + // Illegal + false, }; return gCoeffReferencesBlendConst[coeff]; GR_STATIC_ASSERT(kGrBlendCoeffCnt == SK_ARRAY_COUNT(gCoeffReferencesBlendConst));