Bug 161365 - bitwise_cast infinite loops if called from the default constructor in ToType
Summary: bitwise_cast infinite loops if called from the default constructor in ToType
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-29 17:22 PDT by Saam Barati
Modified: 2016-09-02 12:41 PDT (History)
6 users (show)

See Also:


Attachments
patch (1.49 KB, patch)
2016-09-01 12:00 PDT, JF Bastien
sbarati: review+
sbarati: commit-queue-
Details | Formatted Diff | Diff
patch (1.48 KB, patch)
2016-09-02 12:09 PDT, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-08-29 17:22:52 PDT
...
Comment 1 JF Bastien 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 ;-)
Comment 2 JF Bastien 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...
Comment 3 JF Bastien 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.
Comment 4 WebKit Commit Bot 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.
Comment 5 Saam Barati 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.
Comment 6 JF Bastien 2016-09-02 12:09:45 PDT
Created attachment 287794 [details]
patch

Fix style.
Comment 7 WebKit Commit Bot 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.
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2016-09-02 12:41:17 PDT
All reviewed patches have been landed.  Closing bug.