Bug 187971 - Move-constructing NeverDestroyed should move construct underlying object instead of copy constructing it
Summary: Move-constructing NeverDestroyed should move construct underlying object inst...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-24 13:43 PDT by Daniel Bates
Modified: 2018-08-01 17:46 PDT (History)
9 users (show)

See Also:

Attachments
Patch and tests (16.57 KB, patch)
2018-07-24 13:51 PDT, Daniel Bates 		no flags Details | Formatted Diff | Diff
Patch and tests (18.90 KB, patch)
2018-07-24 13:56 PDT, Daniel Bates 		no flags Details | Formatted Diff | Diff
Patch and tests (17.24 KB, patch)
2018-07-24 14:03 PDT, Daniel Bates 		no flags Details | Formatted Diff | Diff
Patch and tests (17.51 KB, patch)
2018-07-24 14:47 PDT, Daniel Bates 		no flags Details | Formatted Diff | Diff
Patch and tests (17.51 KB, patch)
2018-07-24 14:52 PDT, Daniel Bates 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-07-24 13:43:55 PDT 
Consider the following code taken from attachment #336064 [details] (bug #175784):

static const RenderStyle& hackedDefaultStyle()
{
    static auto defaultStyle = makeNeverDestroyed([] {
        auto style = RenderStyle::create();
        style.setColor(Color { });
        style.setTextFillColor(Color { });
        style.setTextStrokeColor(Color { });
        style.setBackgroundColor(Color { });
        style.setTextEmphasisColor(Color { });
        return style;
    }());
    return defaultStyle;
}

This fails to compile on Mac, GTK, and WPE with an error of the form:

[[
/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/NeverDestroyed.h:55:46: error: call to implicitly-deleted copy constructor of 'WebCore::RenderStyle'
        MaybeRelax<T>(new (storagePointer()) T(WTFMove(other)));
                                             ^ ~~~~~~~~~~~~~~
In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource411.cpp:7:
./rendering/InlineTextBox.cpp:773:32: note: in instantiation of member function 'WTF::NeverDestroyed<WebCore::RenderStyle>::NeverDestroyed' requested here
    static auto defaultStyle = makeNeverDestroyed([] {
                               ^
In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource411.cpp:2:
In file included from ./rendering/ImageQualityController.cpp:32:
In file included from ./rendering/RenderBoxModelObject.h:28:
In file included from ./rendering/RenderLayerModelObject.h:25:
In file included from ./rendering/RenderElement.h:26:
In file included from ./rendering/RenderObject.h:35:
/Volumes/Data/EWS/WebKit/Source/WebCore/rendering/style/RenderStyle.h:134:5: note: copy constructor is implicitly deleted because 'RenderStyle' has a user-declared move constructor
    RenderStyle(RenderStyle&&);
...
]]
<https://webkit-queues.webkit.org/results/7027403>
Comment 1 Daniel Bates 2018-07-24 13:45:51 PDT 
The issue is that the NeverDestroyed move constructor is moving the passed NeverDestroyed object instead of the underlying storage when placement-newing T. As a result, the compiler implicitly converts the to-be-moved-from NeverDestroyed to const T& and invokes T's copy constructor.
Comment 2 Daniel Bates 2018-07-24 13:48:42 PDT 
I suspect the reason this does not effect Windows is because MS Visual Studio has a more aggressive optimizer that elides the call to the NeverDestroyed move constructor. Such aggressive eliding is allowed to be performed even if the move constructor has side effects in C++14, but not required. As of C++17 such an optimization is required by all compiler writers and is consistent with my observation that I do not get a compile-time error when compiling the code in comment #0 with clang using C++17.
Comment 3 Daniel Bates 2018-07-24 13:51:30 PDT 
Created attachment 345709 [details]
Patch and tests
Comment 4 EWS Watchlist 2018-07-24 13:54:07 PDT Comment hidden (obsolete)
Comment 5 Daniel Bates 2018-07-24 13:56:59 PDT 
Created attachment 345710 [details]
Patch and tests
Comment 6 Daniel Bates 2018-07-24 14:03:33 PDT 
Created attachment 345712 [details]
Patch and tests
Comment 7 EWS Watchlist 2018-07-24 14:06:27 PDT Comment hidden (obsolete)
Comment 8 Daniel Bates 2018-07-24 14:47:29 PDT 
Created attachment 345715 [details]
Patch and tests

Make move constructing NeverDestroyed<const T> work and add a test.
Comment 9 EWS Watchlist 2018-07-24 14:49:47 PDT Comment hidden (obsolete)
Comment 10 Saam Barati 2018-07-24 14:51:30 PDT 
Comment on attachment 345715 [details]
Patch and tests

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

r=me

> Tools/ChangeLog:9
> +        Fixes an issue where move constructing a NeverDestroyed<T> would always copy construct T into
> +        the destination storage buffer regardless of whether T is move constructable. For example:

Do we really need this changelog copied here too?
Comment 11 Daniel Bates 2018-07-24 14:52:47 PDT 
Created attachment 345716 [details]
Patch and tests
Comment 12 EWS Watchlist 2018-07-24 14:55:05 PDT Comment hidden (obsolete)
Comment 13 Daniel Bates 2018-07-24 15:01:29 PDT 
(In reply to Saam Barati from comment #10)
> > Tools/ChangeLog:9
> > +        Fixes an issue where move constructing a NeverDestroyed<T> would always copy construct T into
> > +        the destination storage buffer regardless of whether T is move constructable. For example:
> 
> Do we really need this changelog copied here too?

Will consider writing a more specific description for Tools/ChangeLog. I do not see an issue with duplicating the ChangeLog description in multiple ChangeLog files so long as it is descriptive. I use script commit-log-editor as my Git editor (this is part of the standard WebKit OpenSource Project setup). So, the commit message when I land the change will not contain duplication as commit-log-editor automatically collapses duplicate ChangeLog descriptions to create the commit message.
Comment 14 Daniel Bates 2018-07-24 16:30:08 PDT 
Committed r234179: <https://trac.webkit.org/changeset/234179>
Comment 15 Radar WebKit Bug Importer 2018-07-24 16:31:25 PDT 
<rdar://problem/42561513>
Comment 16 Ross Kirsling 2018-08-01 15:27:26 PDT 
It turns out that the two tests checking for...
> "construct(name) set-name(x) move-construct(x) destruct(<default>) "

...are failed by MSVC in Debug mode, where "move-construct(x) destruct(<default>) " appears an extra time.
(https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Debug%20%28Tests%29/builds/703/steps/run-api-tests/logs/stdio)

This seems like a valid thing for a compiler to do when optimizations are off though, given that there's an immediately-invoked lambda followed by a makeNeverDestroyed call.

What do you think we should do about this?
Comment 17 Daniel Bates 2018-08-01 17:26:50 PDT 
(In reply to Ross Kirsling from comment #16)
> It turns out that the two tests checking for...
> > "construct(name) set-name(x) move-construct(x) destruct(<default>) "
> 
> ...are failed by MSVC in Debug mode, where "move-construct(x)
> destruct(<default>) " appears an extra time.
> (https://build.webkit.org/builders/WinCairo%2064-
> bit%20WKL%20Debug%20%28Tests%29/builds/703/steps/run-api-tests/logs/stdio)
> 
> This seems like a valid thing for a compiler to do when optimizations are
> off though, given that there's an immediately-invoked lambda followed by a
> makeNeverDestroyed call.
> 
> What do you think we should do about this?

Can we add a compiler-specific result for this test?
Comment 18 Ross Kirsling 2018-08-01 17:46:10 PDT 
(In reply to Daniel Bates from comment #17)
> (In reply to Ross Kirsling from comment #16)
> > It turns out that the two tests checking for...
> > > "construct(name) set-name(x) move-construct(x) destruct(<default>) "
> > 
> > ...are failed by MSVC in Debug mode, where "move-construct(x)
> > destruct(<default>) " appears an extra time.
> > (https://build.webkit.org/builders/WinCairo%2064-
> > bit%20WKL%20Debug%20%28Tests%29/builds/703/steps/run-api-tests/logs/stdio)
> > 
> > This seems like a valid thing for a compiler to do when optimizations are
> > off though, given that there's an immediately-invoked lambda followed by a
> > makeNeverDestroyed call.
> > 
> > What do you think we should do about this?
> 
> Can we add a compiler-specific result for this test?

Works for me! Patch submitted: https://bugs.webkit.org/show_bug.cgi?id=188244