Closed Bug 1108085 Opened 5 years ago Closed 4 years ago

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]

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

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.
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).
I assume this SkRect.h warning is no longer a problem because we build with warnings-as-errors by default now.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
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!
I believe we turned off warning-as-error for skia explicitly.
(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.)
You need to log in before you can comment on or make changes to this bug.