Bug 161244 - bitwise_cast uses inactive member of union
Summary: bitwise_cast uses inactive member of union
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-26 08:04 PDT by JF Bastien
Modified: 2016-08-26 19:41 PDT (History)
9 users (show)

See Also:


Attachments
patch (2.17 KB, patch)
2016-08-26 08:07 PDT, JF Bastien
no flags Details | Formatted Diff | Diff
patch (5.23 KB, patch)
2016-08-26 09:06 PDT, JF Bastien
saam: review+
Details | Formatted Diff | Diff
patch (5.23 KB, patch)
2016-08-26 10:48 PDT, JF Bastien
saam: review+
saam: commit-queue-
Details | Formatted Diff | Diff
patch (7.51 KB, patch)
2016-08-26 13:23 PDT, JF Bastien
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (904.92 KB, application/zip)
2016-08-26 14:31 PDT, Build Bot
no flags Details
patch (7.62 KB, patch)
2016-08-26 18:29 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 JF Bastien 2016-08-26 08:04:53 PDT
bitwise_cast stores into a union with one type and reads with another, which is technically C++ undefined behavior because it's accessing the wrong active member of the union. The better way to do this is through memcpy, which compilers optimize as well because it's known-size in known-not-to-escape storage.

While we're at it, checking that sizeof(To) == sizeof(From) is good, but we should also check that both types are trivially copyable (can have a ctor, no dtor, and copy is defaulted as if by memcpy for type and all subtypes). Unfortunately that trait isn't implemented consistently in all recent compiler+stdlib implementations, but recent GCC+clang have an equivalent builtin (other compilers simply won't do the check, and will break on bots with the right compilers which is better than the current silent breakage). This builtin hack also avoids #include <type_traits> which really doesn't save much.
Comment 1 JF Bastien 2016-08-26 08:07:13 PDT
Created attachment 287104 [details]
patch
Comment 2 JF Bastien 2016-08-26 09:06:04 PDT
Created attachment 287109 [details]
patch

Updated patch with COMPILER_HAS_CLANG_FEATURE.
Comment 3 WebKit Commit Bot 2016-08-26 09:09:21 PDT
Attachment 287109 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Compiler.h:46:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WTF/wtf/Compiler.h:46:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Saam Barati 2016-08-26 10:43:09 PDT
Comment on attachment 287109 [details]
patch

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

> Source/WTF/wtf/StdLibExtras.h:149
> +#   if COMPILER_HAS_CLANG_FEATURE(is_trivially_copyable)

Style: don't indent #if

> Source/WTF/wtf/StdLibExtras.h:153
> +#   endif

ditto
Comment 5 JF Bastien 2016-08-26 10:48:51 PDT
Created attachment 287121 [details]
patch

Fix style.
Comment 6 WebKit Commit Bot 2016-08-26 10:52:37 PDT
Attachment 287121 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Compiler.h:46:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WTF/wtf/Compiler.h:46:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Saam Barati 2016-08-26 12:48:38 PDT
Comment on attachment 287121 [details]
patch

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

> Source/WTF/ChangeLog:7
> +

I think it's worth putting the paragraph of explanation here that you have in bugzilla. That text is informative, and it's the exact kind of thing that we want in a changelog.
Comment 8 Daniel Bates 2016-08-26 12:55:55 PDT
Comment on attachment 287121 [details]
patch

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

> Source/WTF/wtf/StdLibExtras.h:149
> +#if COMPILER_HAS_CLANG_FEATURE(is_trivially_copyable)

How did you come to the decision to add his new macro function as opposed to using COMPLILER_SUPPORTS()? My understanding of the latter macro function is that it allows checking for a compiler feature in a way that is compiler agnostic. At the time of writing, COMPILER_SUPPORTS() is only
Implemented to return a non-undefined value for Clang. I do not see why we cannot extend this macro function to work with other toolchains.
Comment 9 JF Bastien 2016-08-26 13:22:19 PDT
(In reply to comment #8)
> Comment on attachment 287121 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287121&action=review
> 
> > Source/WTF/wtf/StdLibExtras.h:149
> > +#if COMPILER_HAS_CLANG_FEATURE(is_trivially_copyable)
> 
> How did you come to the decision to add his new macro function as opposed to
> using COMPLILER_SUPPORTS()? My understanding of the latter macro function is
> that it allows checking for a compiler feature in a way that is compiler
> agnostic. At the time of writing, COMPILER_SUPPORTS() is only
> Implemented to return a non-undefined value for Clang. I do not see why we
> cannot extend this macro function to work with other toolchains.

__has_feature is clang's favorite way of declaring what it supports. It has a bunch of options, so I think it's inconvenient to define all the COMPILER_SUPPORTS macros in Compiler.h and then use them were needed (Ideally I'd just modify StdLibExtras.h in this patch).

It's also much nicer to do:
  #if I_CAN_HAZ(...)
    // Kittens!
  #endif
Rather than:
  #if defined(I_CAN_HAZ) && I_CAN_HAZ(...)
    // Kittens!
  #endif
The former is the COMPILER_HAS_CLANG_FEATURE usage, whereas the latter is the COMPILER_SUPPORTS usage.

The C++ standard also has recommended "feature testing" support which GCC and clang have started to support, which is very similar to __has_feature. It's unfortunately not uniformly supported by standard library implementations.

I think What I've done is therefore pretty idiomatic C++ and will become more so in the future. I'd move *away* from COMPILER_SUPPORTS and instead use COMPILER_HAS_CLANG_FEATURE and feature testing :-)

But hey I'm new here, just a C++ dork doing what seems obvious, LMK if it's odd (I mean, besides C++ being odd in general).
Comment 10 JF Bastien 2016-08-26 13:23:16 PDT
Created attachment 287137 [details]
patch

Move bugzilla comment to ChangeLog, and enhance it with references to the C++ Standard which back up my claims.
Comment 11 WebKit Commit Bot 2016-08-26 13:26:06 PDT
Attachment 287137 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Compiler.h:46:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WTF/wtf/Compiler.h:46:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Daniel Bates 2016-08-26 14:07:35 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 287121 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=287121&action=review
> > 
> > > Source/WTF/wtf/StdLibExtras.h:149
> > > +#if COMPILER_HAS_CLANG_FEATURE(is_trivially_copyable)
> > 
> > How did you come to the decision to add his new macro function as opposed to
> > using COMPLILER_SUPPORTS()? My understanding of the latter macro function is
> > that it allows checking for a compiler feature in a way that is compiler
> > agnostic. At the time of writing, COMPILER_SUPPORTS() is only
> > Implemented to return a non-undefined value for Clang. I do not see why we
> > cannot extend this macro function to work with other toolchains.
> 
> __has_feature is clang's favorite way of declaring what it supports. It has
> a bunch of options, so I think it's inconvenient to define all the
> COMPILER_SUPPORTS macros in Compiler.h and then use them were needed
> (Ideally I'd just modify StdLibExtras.h in this patch).
> 

With the exception of WTF_COMPILER_SUPPORTS_FALLTHROUGH_WARNINGS we seem to names these macro defines following the same naming as the Clang feature. I do not see why we should adopt such a convention. The COMPILER_SUPPORTS() macro gives us an opportunity to come up with names that are not Clang-specific and may even better describe the feature they are tested, much like how FALLTHROUGH_WARNINGS is more descriptive than its implementation.

> It's also much nicer to do:
>   #if I_CAN_HAZ(...)
>     // Kittens!
>   #endif
> Rather than:
>   #if defined(I_CAN_HAZ) && I_CAN_HAZ(...)
>     // Kittens!
>   #endif
> The former is the COMPILER_HAS_CLANG_FEATURE usage, whereas the latter is
> the COMPILER_SUPPORTS usage.
> 

Notice that COMPILER_SUPPORTS(X) is a macro function that takes care of this expansion, <https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Compiler.h#L33>.2

> The C++ standard also has recommended "feature testing" support which GCC
> and clang have started to support, which is very similar to __has_feature.
> It's unfortunately not uniformly supported by standard library
> implementations.
> 

Then would it not make sense to use COMPILER_SUPPORTS() so that one day we can implement the WTF_COMPILER_SUPPORTS_X macros in terms of the C++ standard machinery?This has the benefit that all compliant compilers will make use of such macro-guarded code without the need to change each call site.

> I think What I've done is therefore pretty idiomatic C++ and will become
> more so in the future. I'd move *away* from COMPILER_SUPPORTS and instead
> use COMPILER_HAS_CLANG_FEATURE and feature testing :-)
> 
> But hey I'm new here, just a C++ dork doing what seems obvious, LMK if it's
> odd (I mean, besides C++ being odd in general).

(In reply to comment #8)
> Comment on attachment 287121 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287121&action=review
> 
> > Source/WTF/wtf/StdLibExtras.h:149
> > +#if COMPILER_HAS_CLANG_FEATURE(is_trivially_copyable)
> 
> How did you come to the decision to add his new macro function as opposed to
> using COMPLILER_SUPPORTS()? My understanding of the latter macro function is
> that it allows checking for a compiler feature in a way that is compiler
> agnostic. At the time of writing, COMPILER_SUPPORTS() is only
> Implemented to return a non-undefined value for Clang. I do not see why we
> cannot extend this macro function to work with other toolchains.
Comment 13 Build Bot 2016-08-26 14:31:51 PDT
Comment on attachment 287137 [details]
patch

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

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
Comment 14 Build Bot 2016-08-26 14:31:55 PDT
Created attachment 287148 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 15 Alexey Proskuryakov 2016-08-26 17:09:14 PDT
The imported/w3c/web-platform-tests/html/dom/interfaces.html failure is tracked in bug 161142.

I always wondered where the recommendation to use unions for type casting came from. I didn't get it first hand, but it seemed like the recommended way to work around strict aliasing issues.
Comment 16 JF Bastien 2016-08-26 18:28:45 PDT
> With the exception of WTF_COMPILER_SUPPORTS_FALLTHROUGH_WARNINGS we seem to
> names these macro defines following the same naming as the Clang feature. I
> do not see why we should adopt such a convention. The COMPILER_SUPPORTS()
> macro gives us an opportunity to come up with names that are not
> Clang-specific and may even better describe the feature they are tested,
> much like how FALLTHROUGH_WARNINGS is more descriptive than its
> implementation.

Ah you're right, I'd misunderstood. I've updated the patch to use COMPILER_SUPPORTS.

> Then would it not make sense to use COMPILER_SUPPORTS() so that one day we
> can implement the WTF_COMPILER_SUPPORTS_X macros in terms of the C++
> standard machinery?This has the benefit that all compliant compilers will
> make use of such macro-guarded code without the need to change each call
> site.

Indeed.
Comment 17 JF Bastien 2016-08-26 18:29:20 PDT
Created attachment 287185 [details]
patch

Update macros.
Comment 18 WebKit Commit Bot 2016-08-26 18:30:34 PDT
Attachment 287185 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Compiler.h:46:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WTF/wtf/Compiler.h:46:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Benjamin Poulain 2016-08-26 19:20:26 PDT
Comment on attachment 287185 [details]
patch

LGTM.
Looks like the WK2 bot is just broken at the moment.
Comment 20 WebKit Commit Bot 2016-08-26 19:41:27 PDT
Comment on attachment 287185 [details]
patch

Clearing flags on attachment: 287185

Committed r205066: <http://trac.webkit.org/changeset/205066>
Comment 21 WebKit Commit Bot 2016-08-26 19:41:32 PDT
All reviewed patches have been landed.  Closing bug.