Skip to content

Commit

Permalink
Revert "make enum santizer fatal"
Browse files Browse the repository at this point in the history
This reverts commit 166dbd3.

Reason for revert: illegal is not advanced, docs

Original change's description:
> 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 <brianosman@google.com>

TBR=mtklein@chromium.org,mtklein@google.com,brianosman@google.com

Change-Id: I691c08092340a6273e442c0f098b844f7d0363ba
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/168581
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
  • Loading branch information
Mike Klein authored and Skia Commit-Bot committed Nov 6, 2018
1 parent 4529cb5 commit f2b35e4
Show file tree
Hide file tree
Showing 14 changed files with 19 additions and 49 deletions.
2 changes: 1 addition & 1 deletion gn/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ config("default") {

# A whitelist of checks we can't yet pass.
if (fyi_sanitize == "" && !is_android) {
fyi_sanitizers = "float-divide-by-zero"
fyi_sanitizers = "enum,float-divide-by-zero"
}

if (is_android) {
Expand Down
2 changes: 1 addition & 1 deletion include/core/SkBlurTypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

#include "SkTypes.h"

enum SkBlurStyle : int {
enum SkBlurStyle {
kNormal_SkBlurStyle, //!< fuzzy inside and outside
kSolid_SkBlurStyle, //!< solid inside, fuzzy outside
kOuter_SkBlurStyle, //!< nothing inside, fuzzy outside
Expand Down
2 changes: 1 addition & 1 deletion include/core/SkPath.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class SK_API SkPath {
kCW_Direction travel clockwise; the same added with kCCW_Direction
travel counterclockwise.
*/
enum Direction : int {
enum Direction {
kCW_Direction, //!< contour travels clockwise
kCCW_Direction, //!< contour travels counterclockwise
};
Expand Down
8 changes: 2 additions & 6 deletions include/gpu/GrBlend.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,8 @@ enum GrBlendEquation {
kHSLColor_GrBlendEquation,
kHSLLuminosity_GrBlendEquation,

kIllegal_GrBlendEquation,

kFirstAdvancedGrBlendEquation = kScreen_GrBlendEquation,
kLast_GrBlendEquation = kIllegal_GrBlendEquation,
kLast_GrBlendEquation = kHSLLuminosity_GrBlendEquation
};

static const int kGrBlendEquationCnt = kLast_GrBlendEquation + 1;
Expand Down Expand Up @@ -69,9 +67,7 @@ enum GrBlendCoeff {
kS2A_GrBlendCoeff,
kIS2A_GrBlendCoeff,

kIllegal_GrBlendCoeff,

kLast_GrBlendCoeff = kIllegal_GrBlendCoeff,
kLast_GrBlendCoeff = kIS2A_GrBlendCoeff
};

static const int kGrBlendCoeffCnt = kLast_GrBlendCoeff + 1;
Expand Down
2 changes: 1 addition & 1 deletion include/gpu/GrTypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 : int {
enum GrSurfaceOrigin {
kTopLeft_GrSurfaceOrigin,
kBottomLeft_GrSurfaceOrigin,
};
Expand Down
2 changes: 1 addition & 1 deletion include/private/GrTypesPriv.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 : int {
enum GrSLPrecision {
kLow_GrSLPrecision,
kMedium_GrSLPrecision,
kHigh_GrSLPrecision,
Expand Down
2 changes: 1 addition & 1 deletion src/core/SkPathPriv.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class SkPathPriv {
static const int kPathRefGenIDBitCnt = 32;
#endif

enum FirstDirection : int {
enum FirstDirection {
kCW_FirstDirection, // == SkPath::kCW_Direction
kCCW_FirstDirection, // == SkPath::kCCW_Direction
kUnknown_FirstDirection,
Expand Down
6 changes: 0 additions & 6 deletions src/gpu/GrXferProcessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,6 @@ static const char* equation_string(GrBlendEquation eq) {
return "hsl_color";
case kHSLLuminosity_GrBlendEquation:
return "hsl_luminosity";
case kIllegal_GrBlendEquation:
SkASSERT(false);
return "<illegal>";
};
return "";
}
Expand Down Expand Up @@ -144,9 +141,6 @@ static const char* coeff_string(GrBlendCoeff coeff) {
return "src2_alpha";
case kIS2A_GrBlendCoeff:
return "inv_src2_alpha";
case kIllegal_GrBlendCoeff:
SkASSERT(false);
return "<illegal>";
}
return "";
}
Expand Down
9 changes: 3 additions & 6 deletions src/gpu/effects/GrCustomXfermode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,7 @@ 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);

// 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);

GR_STATIC_ASSERT(kGrBlendEquationCnt == (int)SkBlendMode::kLastMode + 1 + EQ_OFFSET);
return static_cast<GrBlendEquation>((int)mode + EQ_OFFSET);
#undef EQ_OFFSET
}
Expand Down Expand Up @@ -82,15 +79,15 @@ class CustomXP : public GrXferProcessor {
CustomXP(bool hasMixedSamples, SkBlendMode mode, GrProcessorAnalysisCoverage coverage)
: INHERITED(kCustomXP_ClassID, true, hasMixedSamples, coverage)
, fMode(mode)
, fHWBlendEquation(kIllegal_GrBlendEquation) {
, fHWBlendEquation(static_cast<GrBlendEquation>(-1)) {
}

const char* name() const override { return "Custom Xfermode"; }

GrGLSLXferProcessor* createGLSLInstance() const override;

SkBlendMode mode() const { return fMode; }
bool hasHWBlendEquation() const { return kIllegal_GrBlendEquation != fHWBlendEquation; }
bool hasHWBlendEquation() const { return -1 != static_cast<int>(fHWBlendEquation); }

GrBlendEquation hwBlendEquation() const {
SkASSERT(this->hasHWBlendEquation());
Expand Down
11 changes: 1 addition & 10 deletions src/gpu/gl/GrGLGpu.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,7 @@ static const GrGLenum gXfermodeEquation2Blend[] = {
GR_GL_HSL_HUE,
GR_GL_HSL_SATURATION,
GR_GL_HSL_COLOR,
GR_GL_HSL_LUMINOSITY,

// Illegal... needs to map to something.
GR_GL_FUNC_ADD,
GR_GL_HSL_LUMINOSITY
};
GR_STATIC_ASSERT(0 == kAdd_GrBlendEquation);
GR_STATIC_ASSERT(1 == kSubtract_GrBlendEquation);
Expand Down Expand Up @@ -124,9 +121,6 @@ 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) {
Expand All @@ -151,9 +145,6 @@ bool GrGLGpu::BlendCoeffReferencesConstant(GrBlendCoeff coeff) {
false,
false,
false,

// Illegal.
false,
};
return gCoeffReferencesBlendConst[coeff];
GR_STATIC_ASSERT(kGrBlendCoeffCnt == SK_ARRAY_COUNT(gCoeffReferencesBlendConst));
Expand Down
6 changes: 3 additions & 3 deletions src/gpu/gl/GrGLGpu.h
Original file line number Diff line number Diff line change
Expand Up @@ -566,9 +566,9 @@ class GrGLGpu final : public GrGpu, private GrMesh::SendToGpuImpl {
TriState fEnabled;

void invalidate() {
fEquation = kIllegal_GrBlendEquation;
fSrcCoeff = kIllegal_GrBlendCoeff;
fDstCoeff = kIllegal_GrBlendCoeff;
fEquation = static_cast<GrBlendEquation>(-1);
fSrcCoeff = static_cast<GrBlendCoeff>(-1);
fDstCoeff = static_cast<GrBlendCoeff>(-1);
fConstColorValid = false;
fEnabled = kUnknown_TriState;
}
Expand Down
3 changes: 1 addition & 2 deletions src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,8 @@ 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 - 1);
kGrBlendEquationCnt - kFirstAdvancedGrBlendEquation);
}

uint8_t GrGLSLFragmentShaderBuilder::KeyForSurfaceOrigin(GrSurfaceOrigin origin) {
Expand Down
3 changes: 1 addition & 2 deletions src/gpu/mtl/GrMtlPipelineStateBuilder.mm
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,6 @@ 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);
Expand Down Expand Up @@ -264,7 +263,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 < kGrBlendEquationCnt);
SkASSERT((unsigned)equation < kGrBlendCoeffCnt);
return gTable[equation];
}

Expand Down
10 changes: 2 additions & 8 deletions src/gpu/vk/GrVkPipeline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -369,10 +369,7 @@ 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,

// Illegal.
VK_BLEND_OP_ADD,
VK_BLEND_OP_HSL_LUMINOSITY_EXT
};
GR_STATIC_ASSERT(0 == kAdd_GrBlendEquation);
GR_STATIC_ASSERT(1 == kSubtract_GrBlendEquation);
Expand Down Expand Up @@ -420,9 +417,6 @@ static bool blend_coeff_refs_constant(GrBlendCoeff coeff) {
false,
false,
false,

// Illegal
false,
};
return gCoeffReferencesBlendConst[coeff];
GR_STATIC_ASSERT(kGrBlendCoeffCnt == SK_ARRAY_COUNT(gCoeffReferencesBlendConst));
Expand Down

0 comments on commit f2b35e4

Please sign in to comment.