1108085 - clang 3.6 build warning (treated as error): SkRect.h:280:19: error: reference cannot be bound to dereferenced null pointer in well-defined C++ code; pointer may be assumed to always convert to true [-Werror,-Wundefined-bool-conversion]

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect)

Description

[Daniel Holbert [:dholbert]]

Build warning, treated as error in --enable-warnings-as-errors debug builds, with clang 3.6:
{
In file included from $OBJDIR/gfx/gl/Unified_cpp_gfx_gl1.cpp:38:
In file included from $SRCDIR/gfx/gl/SkiaGLGlue.cpp:6:
In file included from ../../dist/include/skia/GrContext.h:11:
In file included from ../../dist/include/skia/GrClipData.h:11:
In file included from ../../dist/include/skia/SkClipStack.h:12:
In file included from ../../dist/include/skia/SkPath.h:14:
In file included from ../../dist/include/skia/SkMatrix.h:14:
../../dist/include/skia/SkRect.h:280:19: error: reference cannot be bound to dereferenced null pointer in well-defined C++ code; pointer may be assumed to always convert to true [-Werror,-Wundefined-bool-conversion]
        SkASSERT(&a && &b);
                  ^ ~~
../../dist/include/skia/SkTypes.h:96:56: note: expanded from macro 'SkASSERT'
    #define SkASSERT(cond)              SK_ALWAYSBREAK(cond)
                                                       ^
../../dist/include/skia/SkPostConfig.h:173:19: note: expanded from macro 'SK_ALWAYSBREAK'
              if (cond) break; \
                  ^
}

These asserts are silly, and this file has a bunch of them. Code quote:
> 279     bool intersect(const SkIRect& a, const SkIRect& b) {
> 280         SkASSERT(&a && &b);

The only way the assertion would fail is if someone passed in a dereferenced null pointer for one of the arguments, and they should crash when they do that dereferencing (and never reach the assertion).

We presumably need to fix this upstream, but I'm filing this as a first step.

Comment 1

[Daniel Holbert [:dholbert]]

MXR link to the skia header with two instances of this highlighted:
https://mxr.mozilla.org/mozilla-central/source/gfx/skia/trunk/include/core/SkRect.h?rev=883cd6be06d2&mark=270-271,300-301#270

BTW: we generally don't treat warnings as errors in 3rd-party code -- so e.g. we don't have FAIL_ON_WARNINGS in gfx/skia, as far as I know. But we are getting errors in this case, because we're including this 3rd-party header in Mozilla-authored code (via the "In file" path in comment 0), and *that* code *is* marked as FAIL_ON_WARNINGS (in gfx/gl/moz.build).

Comment 2

[Chris Peterson [:cpeterson]]

I assume this SkRect.h warning is no longer a problem because we build with warnings-as-errors by default now.

Comment 3

[Daniel Holbert [:dholbert]]

It looks like this Skia header still has one assert that that should trigger this:
> 270     bool intersect(const SkIRect& r) {
> 271         SkASSERT(&r);
https://mxr.mozilla.org/mozilla-central/source/gfx/skia/skia/include/core/SkRect.h?rev=e6523d21523f#270
...but I don't see this warning in my ./mach warnings-list output anymore. We must've disabled this warning, or maybe clang adjusted its rules about when to report it. Or maybe we never call that function and it gets optimized away.

Anyway, happy to have this WFM. Thanks!

Comment 4

[George Wright (:gw280) (needinfo me!)]

I believe we turned off warning-as-error for skia explicitly.

Comment 5

[Daniel Holbert [:dholbert]]

(Right, but this bug is about us tripping this warning when compiling gfx/gl/SkiaGLGlue.cpp (our own code, which includes this header) -- not when compiling skia code.)