Bug 152408 - Avoid triggering clang's -Wundefined-bool-conversion
Summary: Avoid triggering clang's -Wundefined-bool-conversion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Trivial
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks: 145121
  Show dependency treegraph
 
Reported: 2015-12-17 17:02 PST by Michael Catanzaro
Modified: 2016-02-17 10:12 PST (History)
10 users (show)

See Also:


Attachments
Patch (6.53 KB, patch)
2015-12-17 19:17 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-12-17 17:02:32 PST
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.
Comment 1 Michael Catanzaro 2015-12-17 17:07:28 PST
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.
Comment 2 Michael Catanzaro 2015-12-17 17:30:53 PST
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.
Comment 3 Michael Catanzaro 2015-12-17 19:17:44 PST
Created attachment 267603 [details]
Patch
Comment 4 Mark Lam 2015-12-18 07:23:05 PST
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)"?
Comment 5 Michael Catanzaro 2015-12-18 10:28:40 PST
(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.
Comment 6 Michael Catanzaro 2015-12-18 10:31:49 PST
(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
Comment 7 Mark Lam 2015-12-18 11:32:59 PST
Comment on attachment 267603 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2015-12-18 16:16:41 PST
Comment on attachment 267603 [details]
Patch

Clearing flags on attachment: 267603

Committed r194303: <http://trac.webkit.org/changeset/194303>
Comment 9 WebKit Commit Bot 2015-12-18 16:16:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Darin Adler 2016-01-13 08:23:08 PST
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.
Comment 11 Michael Catanzaro 2016-02-17 10:12:00 PST
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/devel@lists.fedoraproject.org/message/WX26YZOE5WRYPOEE7HJOZ56QPO7XQ5RS/