Bug 181469 – Add nullptr_t specialization of poison

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Don Olmstead]

After wtf/Poisoned.h was added there are a number of warnings from Clang when building.

Source\WTF\wtf\Poisoned.h(204,62): warning : implicit conversion of nullptr constant to 'bool' [-Wnull-conversion]

This should be globally defined since it follows with the style guide.

[Don Olmstead]

Created attachment 330875 [details]
Patch

[Don Olmstead]

Created attachment 330876 [details]
Patch

[Michael Catanzaro]

Hm, can you show an example of code that triggers this warning? It sounds like it warns when a nullptr constant should be changed to "false" but I presume you would not be reporting this as a bug unless there were more to it....

[EWS Watchlist]

Comment on attachment 330876 [details]
Patch

Attachment 330876 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6011905

New failing tests:
http/tests/workers/service/postmessage-after-sw-process-crash.https.html

[EWS Watchlist]

Created attachment 330881 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Konstantin Tokarev]

(In reply to Michael Catanzaro from comment #3)
> Hm, can you show an example of code that triggers this warning? It sounds
> like it warns when a nullptr constant should be changed to "false" but I
> presume you would not be reporting this as a bug unless there were more to
> it....

Don mentioned https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/Poisoned.h#L204 in IRC. This code looks fine, however I'm not sure if this warning should be really ignored everywhere

[Darin Adler]

I think this warning is possibly slightly valuable and just turning it off is the wrong thing to do unless it’s really hard to silence it.

I don’t agree that our coding style requires disabling this warning; it’s true that our coding style contributes slightly to the problem because we are supposed to do things like "ptr ? x : y" rather than "ptr != nullptr ? x : y", but I’d like to help fix the warnings rather than disabling them. Unless the implementation of the warning is particularly poor.

I can think of two ways to fix the warning in the particular case of Poisoned::poison. The trick is to overload for nullptr_t. Adding line of code like this almost certainly work:

    constexpr static PoisonedBits poison(std::nullptr_t) { return 0; }

An alternate solution, a little less elegant, assuming that it’s specifically the exchange function that’s causing the problem, is to overload exchange instead:

    T exchange(std::nullptr_t)
    {
        T oldValue = unpoisoned();
        m_poisonedBits = 0;
        return oldValue;
    }

Are there lots of other cases or is this the one place this warning is triggering?

[Michael Catanzaro]

Note also that this warning is already disabled in WebCore. Don's change extends it to be disabled throughout all WebKit subprojects.

[Darin Adler]

(In reply to Michael Catanzaro from comment #8)
> Note also that this warning is already disabled in WebCore.

Sure, my comment applies to that original decision, not this patch.

[Darin Adler]

It’s fine with me to consistently turn it off everywhere for now, but I would like to separately consider if we can make a couple simple changes and then turn it back on.

From a “port” point of view, lets turn it off since it’s inconvenient to have a warning that other ports aren’t seeing. From an overall health of the project point of view, lets consider turning it back on especially if one of the EWS bots can have a new enough compiler that people will see this warning before landing.

[Konstantin Tokarev]

Darin, I've tested your fix and there are no more Wnull-conversion warnings

diff --git a/Source/WTF/wtf/Poisoned.h b/Source/WTF/wtf/Poisoned.h
index c7371357642..f10dfeb6c18 100644
--- a/Source/WTF/wtf/Poisoned.h
+++ b/Source/WTF/wtf/Poisoned.h
@@ -200,6 +200,7 @@ public:
 
 private:
 #if ENABLE(POISON)
+    constexpr static PoisonedBits poison(std::nullptr_t) { return 0; }
     template<typename U>
     ALWAYS_INLINE static PoisonedBits poison(U ptr) { return ptr ? bitwise_cast<PoisonedBits>(ptr) ^ key : 0; }
     template<typename U = T>

[Konstantin Tokarev]

(In reply to Michael Catanzaro from comment #8)
> Note also that this warning is already disabled in WebCore. Don's change
> extends it to be disabled throughout all WebKit subprojects.

I don't think that expanding warning suppressions is the right direction, unless we want to have these null conversions spread over whole code base.

[EWS Watchlist]

Comment on attachment 330876 [details]
Patch

Attachment 330876 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6023592

New failing tests:
webgl/1.0.2/conformance/uniforms/uniform-default-values.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-redirect-preflight.any.html
imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-framesize.html

[EWS Watchlist]

Created attachment 330944 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Michael Catanzaro]

So Don, we should be good to go with Darin's suggestion, right? We could even reenable the warning in WebCore instead of disabling it.

(In reply to Darin Adler from comment #10)
> From a “port” point of view, lets turn it off since it’s inconvenient to
> have a warning that other ports aren’t seeing.

AFAIK there has been zero effort to keep CMake and XCode compiler warnings in sync, but all CMake ports should have consistent warning flags.

[Don Olmstead]

(In reply to Michael Catanzaro from comment #15)
> So Don, we should be good to go with Darin's suggestion, right? We could
> even reenable the warning in WebCore instead of disabling it.
> 
> (In reply to Darin Adler from comment #10)
> > From a “port” point of view, lets turn it off since it’s inconvenient to
> > have a warning that other ports aren’t seeing.
> 
> AFAIK there has been zero effort to keep CMake and XCode compiler warnings
> in sync, but all CMake ports should have consistent warning flags.

I think I jumped the gun a bit in terms of making the warning global since I saw it enabled in WebCore and it seemed to follow the style guide.

I don't think GTK/WPE saw it because GCC is used and this appears to be a Clang only warning. Not sure why Apple didn't see the warning since they also use Clang.

I'm doing a build with the constexpr specialization that Darin suggested and I don't see the warning so that is probably the proper fix in this case. I can make a patch once my build is complete.

[Don Olmstead]

Created attachment 330950 [details]
Patch

Implementing Darin's suggestion

[JF Bastien]

Comment on attachment 330950 [details]
Patch

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

> Source/WTF/wtf/Poisoned.h:203
> +    constexpr static PoisonedBits poison(std::nullptr_t) { return 0; }

You also need this specialization for the #else clause because bitwise_cast of nullptr_t isn't guaranteed to give 0 in C++.

[Don Olmstead]

Created attachment 330953 [details]
Patch

nullptr_t specialization for all

[JF Bastien]

(In reply to Don Olmstead from comment #19)
> Created attachment 330953 [details]
> Patch
> 
> nullptr_t specialization for all

I think you uploaded the same patch?

Also, I'm sorry C++ is (sometimes) terrible :-)

[Don Olmstead]

Comment on attachment 330953 [details]
Patch

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

> Source/WTF/wtf/Poisoned.h:202
> +    constexpr static PoisonedBits poison(std::nullptr_t) { return 0; }

Its outside the #if ENABLE(POISON) so its in the common implementation

[JF Bastien]

Comment on attachment 330953 [details]
Patch

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

r=me

>> Source/WTF/wtf/Poisoned.h:202
>> +    constexpr static PoisonedBits poison(std::nullptr_t) { return 0; }
> 
> Its outside the #if ENABLE(POISON) so its in the common implementation

Ha! I totally missed this, sorry.

[Don Olmstead]

(In reply to JF Bastien from comment #22)
> Comment on attachment 330953 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330953&action=review
> 
> r=me
> 
> >> Source/WTF/wtf/Poisoned.h:202
> >> +    constexpr static PoisonedBits poison(std::nullptr_t) { return 0; }
> > 
> > Its outside the #if ENABLE(POISON) so its in the common implementation
> 
> Ha! I totally missed this, sorry.

Thanks for the review! Will commit after EWS comes back.

Also thanks for getting Bell Biv Davoe stuck in my head since I encountered this warning :)

[EWS Watchlist]

Comment on attachment 330953 [details]
Patch

Attachment 330953 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6026997

New failing tests:
webgl/1.0.2/conformance/uniforms/uniform-default-values.html

[EWS Watchlist]

Created attachment 330995 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Konstantin Tokarev]

Comment on attachment 330953 [details]
Patch

Clearing flags on attachment: 330953

Committed r226747: <https://trac.webkit.org/changeset/226747>

[Konstantin Tokarev]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36423271>