Bug 161365

Summary: bitwise_cast infinite loops if called from the default constructor in ToType
Product: WebKit Reporter: Saam Barati <saam>
Component: Web Template FrameworkAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
saam: review+, saam: commit-queue-
patch none

Saam Barati
Reported 2016-08-29 17:22:52 PDT
...
Attachments
patch (1.49 KB, patch)
2016-09-01 12:00 PDT, JF Bastien
saam: review+
saam: commit-queue-
patch (1.48 KB, patch)
2016-09-02 12:09 PDT, JF Bastien
no flags
JF Bastien
Comment 1 2016-08-29 17:27:04 PDT
This is a mild regression from the previous code which didn't call either type's ctor. There's now kind-of an assumption that ToType is default-constructible, and that its default ctor behaves (almost as if it's POD). That's not great. I'll think of a clever C++-ism to fix. Funny that this is the first time someone runs into this issue just after my change ;-)
JF Bastien
Comment 2 2016-08-29 17:49:38 PDT
I take back what I said: it wasn't possible before to use bitwise_cast when ToType had a non-trivial ctor (in that case the default ctor of the union is deleted), but could have been made so with a noop ctor in the union: template<typename To, typename From> To bit_cast(From from) { union U { U() {}; To to; From from; } u; u.from = from; return u.to; } struct A { A(int in = 5) { v = in; } operator int() { return v; } int v; }; int main() { return bit_cast<A>(3.14); } This is horrible and should not be done. The standard says: If any non-static data member of a union has a non-trivial default constructor (12.1), copy constructor (12.8), move constructor (12.8), copy assignment operator (12.8), move assignment operator (12.8), or destructor (12.4), the corresponding member function of the union must be user-provided or it will be implicitly deleted (8.4.3) for the union. This is therefore a regression in that the new version of bitwise_cast is *more* permissive because it accepts ToTypes which aren't trivially constructible, but will cause sad recursion when the ToType calls bitwise_cast. Let's think about this some more...
JF Bastien
Comment 3 2016-09-01 12:00:37 PDT
Created attachment 287651 [details] patch This patch fixes the one issue. I'm still not happy about From being copied in (it should be an rvalue ref, or at least a const ref), but the build disagrees with me. I'm working on adding this to the C++ standard library so we'd have this fixed once and for all. I still need to add concepts (instead of static_assert or SFINAE) and make constexpr work (ugh!) but I have some proposed wording and a bunch of tests. I think fixing this minor issue is good enough for now, and C++20 will have the most wonderful fix.
WebKit Commit Bot
Comment 4 2016-09-01 12:02:45 PDT
Attachment 287651 [details] did not pass style-queue: ERROR: Source/WTF/wtf/StdLibExtras.h:154: Missing space before { [whitespace/braces] [5] ERROR: Source/WTF/wtf/StdLibExtras.h:154: Missing space inside { }. [whitespace/braces] [5] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Saam Barati
Comment 5 2016-09-01 16:18:39 PDT
Comment on attachment 287651 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=287651&action=review r=me > Source/WTF/wtf/StdLibExtras.h:154 > + ToType to{}; Nit: I think webkit style is to have "{ }" instead of "{}". It's worth verifying.
JF Bastien
Comment 6 2016-09-02 12:09:45 PDT
Created attachment 287794 [details] patch Fix style.
WebKit Commit Bot
Comment 7 2016-09-02 12:11:39 PDT
Attachment 287794 [details] did not pass style-queue: ERROR: Source/WTF/wtf/StdLibExtras.h:154: Missing space before { [whitespace/braces] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 8 2016-09-02 12:40:56 PDT
Comment on attachment 287794 [details] patch Clearing flags on attachment: 287794 Committed r205362: <http://trac.webkit.org/changeset/205362>
WebKit Commit Bot
Comment 9 2016-09-02 12:41:17 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.