Bug 152408 – Avoid triggering clang's -Wundefined-bool-conversion

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Catanzaro]

TreeScope.cpp:287 triggers clang's -Wundefined-bool-conversion warning:

[2278/5753] Building CXX object Source...eFiles/WebCore.dir/dom/TreeScope.cpp.o
../../Source/WebCore/dom/TreeScope.cpp:287:8: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion]
    (!(this) ? (WTFReportAssertionFailure("../../Source/WebCore/dom/TreeScope.cpp", 287, __PRETTY_FUNCTION__, "this"), WTFCrash()) : (void)0);
     ~ ^~~~
1 warning generated.

[Michael Catanzaro]

Also:

[2754/5753] Building CXX object Source...MakeFiles/WebCore.dir/page/Frame.cpp.o
../../Source/WebCore/page/Frame.cpp:873:8: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion]
    (!(this) ? (WTFReportAssertionFailure("../../Source/WebCore/page/Frame.cpp", 873, __PRETTY_FUNCTION__, "this"), WTFCrash()) : (void)0);
     ~ ^~~~
1 warning generated.

[Michael Catanzaro]

More complicated one:

[5126/5753] Building CXX object Source...cess/Plugins/Netscape/JSNPObject.cpp.o
../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp:110:18: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion]
    do { do { (!(this) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 110, __PRETTY_FUNCTION__, "this"), WTFCrash()) : (void)0); (!(this->structure()->structure() == this->structure()->structure()->structure()) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 110, __PRETTY_FUNCTION__, "this->structure()->structure() == this->structure()->structure()->structure()"), WTFCrash()) : (void)0); } while (0); (!(this->inherits(info())) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 110, __PRETTY_FUNCTION__, "this->inherits(info())"), WTFCrash()) : (void)0); } while (0);
               ~ ^~~~
../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp:154:18: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion]
    do { do { (!(this) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 154, __PRETTY_FUNCTION__, "this"), WTFCrash()) : (void)0); (!(this->structure()->structure() == this->structure()->structure()->structure()) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 154, __PRETTY_FUNCTION__, "this->structure()->structure() == this->structure()->structure()->structure()"), WTFCrash()) : (void)0); } while (0); (!(this->inherits(info())) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 154, __PRETTY_FUNCTION__, "this->inherits(info())"), WTFCrash()) : (void)0); } while (0);
               ~ ^~~~
../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp:194:18: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion]
    do { do { (!(this) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 194, __PRETTY_FUNCTION__, "this"), WTFCrash()) : (void)0); (!(this->structure()->structure() == this->structure()->structure()->structure()) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 194, __PRETTY_FUNCTION__, "this->structure()->structure() == this->structure()->structure()->structure()"), WTFCrash()) : (void)0); } while (0); (!(this->inherits(info())) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 194, __PRETTY_FUNCTION__, "this->inherits(info())"), WTFCrash()) : (void)0); } while (0);
               ~ ^~~~
../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp:354:18: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion]
    do { do { (!(this) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 354, __PRETTY_FUNCTION__, "this"), WTFCrash()) : (void)0); (!(this->structure()->structure() == this->structure()->structure()->structure()) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 354, __PRETTY_FUNCTION__, "this->structure()->structure() == this->structure()->structure()->structure()"), WTFCrash()) : (void)0); } while (0); (!(this->inherits(info())) ? (WTFReportAssertionFailure("../../Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp", 354, __PRETTY_FUNCTION__, "this->inherits(info())"), WTFCrash()) : (void)0); } while (0);
               ~ ^~~~
4 warnings generated.

[Michael Catanzaro]

Created attachment 267603 [details]
Patch

[Mark Lam]

Comment on attachment 267603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267603&action=review

> Source/WebCore/dom/TreeScope.cpp:-287
> -    ASSERT(this);

I know style-wise, we don't like "ASSERT(!!this);", but with this change, we're losing the detection that we may be calling this method on a nullptr.  Granted, it will just crash below when it derefs this to access any fields.

Out of curiousity, would clang complain about "ASSERT(!!this)"?

[Michael Catanzaro]

(In reply to comment #4)
> I know style-wise, we don't like "ASSERT(!!this);", but with this change,
> we're losing the detection that we may be calling this method on a nullptr. 
> Granted, it will just crash below when it derefs this to access any fields.
> 
> Out of curiousity, would clang complain about "ASSERT(!!this)"?

Yeah, Clang does also warn about ASSERT(!!this). The problem is that modern compilers (at least GCC and Clang) assume "this" is always valid, because the standard says they can, so converting "this" to a bool will always return true, and !!this is also always true. (At least in optimized builds. I am not sure whether or not that holds true at -O0; if not, then we are indeed losing a useful assert.)

Anyway, I favor removing the check regardless, because I think the value of it is less than the value of not creating a mess trying to suppress the warnings. The best way I could come up with to keep the assert short of disabling -Wundefined-bool-conversion globally was this:

#if COMPILER(CLANG)
#if __has_warning("-Wundefined-bool-conversion")
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wundefined-bool-conversion"
#endif
#endif
ASSERT(this);
#if COMPILER(CLANG)
#if __has_warning("-Wundefined-bool-conversion")
#pragma clang diagnostic pop
#endif
#endif

We definitely have to guard the #pragma because MSVCC will choke on it otherwise, I think we have to guard use of __has_warning because some compiler (older GCC?) will choke on THAT otherwise, and the __has_warning check is probably needed because GCC warns if you try to ignore a warning that doesn't exist, so I assume Clang does as well (but I did not check; if it's not needed, at least that would be a bit simpler). So that is quite a mess to have for one assert.

In the future, I expect GCC will add the same warning. Then we would have this:

#if COMPILER(CLANG)
#if __has_warning("-Wundefined-bool-conversion")
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wundefined-bool-conversion"
#endif
#endif
#if COMPILER(GCC)
#if GCC_VERSION_AT_LEAST(n, m, o)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wundefined-bool-conversion"
#endif
#endif
ASSERT(this); // <------ assert goes there :)
#if COMPILER(GCC)
#if GCC_VERSION_AT_LEAST(n, m, o)
#pragma GCC diagnostic pop
#endif
#endif
#if COMPILER(CLANG)
#if __has_warning("-Wundefined-bool-conversion")
#pragma clang diagnostic pop
#endif
#endif

What really sucks about this is that Clang understands GCC pragmas, so the second set of pragmas would be unnecessary if only GCC would learn to handle __has_warning.

So, I would either disable -Wundefined-bool-conversion project-wide (probably undesirable), or just remove the assert (seems less undesirable), unless you have any better ideas.

[Michael Catanzaro]

(In reply to comment #5)
> I think we have to guard use of __has_warning because some
> compiler (older GCC?) will choke on THAT otherwise

I should clarify: I believe we support some compiler (GCC?) that cannot handle preprocessor statements like:

#if 0 && UNDEFINED_IF_PREVIOUS_VALUE_IS_0
#endif

Hence we have to do:

#if 0
#if UNDEFINED_IF_VALUE_ABOVE_IS_0
#endif
#endif

[Mark Lam]

Comment on attachment 267603 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 267603 [details]
Patch

Clearing flags on attachment: 267603

Committed r194303: <http://trac.webkit.org/changeset/194303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 267603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267603&action=review

> Source/WebCore/ChangeLog:8
> +        Remove use of ASSERT(this).

Saying the same thing Mark is below, I think: It’s too bad that clang is causing us to do this. There’s no runtime check to detect accidentally calling a member function with a null pointer, so there might have been value in ASSERT(this), we just needed a way to write it that let clang know why we were doing it. On the other hand, null dereferences just one line later probably mean these aren’t all that valuable so I think I’m going to just let go and not discuss it further.

[Michael Catanzaro]

I asked a GCC developer about this:

"ASSERT(this) is pointless, it is testing if undefined behavior didn't happen after the fact, that is just too late. As I said, use -fsanitize=undefined to make sure you don't call methods on nullptr."

https://lists.fedoraproject.org/archives/list/[e-mail]/message/WX26YZOE5WRYPOEE7HJOZ56QPO7XQ5RS/