Bug 161244 – bitwise_cast uses inactive member of union

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[JF Bastien]

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.

[JF Bastien]

Created attachment 287104 [details]
patch

[JF Bastien]

Created attachment 287109 [details]
patch

Updated patch with COMPILER_HAS_CLANG_FEATURE.

[WebKit Commit Bot]

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.

[Saam Barati]

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

[JF Bastien]

Created attachment 287121 [details]
patch

Fix style.

[WebKit Commit Bot]

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.

[Saam Barati]

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.

[Daniel Bates]

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.

[JF Bastien]

(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).

[JF Bastien]

Created attachment 287137 [details]
patch

Move bugzilla comment to ChangeLog, and enhance it with references to the C++ Standard which back up my claims.

[WebKit Commit Bot]

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.

[Daniel Bates]

(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.

[Build Bot]

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

[Build Bot]

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

[Alexey Proskuryakov]

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.

[JF Bastien]

> 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.

[JF Bastien]

Created attachment 287185 [details]
patch

Update macros.

[WebKit Commit Bot]

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.

[Benjamin Poulain]

Comment on attachment 287185 [details]
patch

LGTM.
Looks like the WK2 bot is just broken at the moment.

[WebKit Commit Bot]

Comment on attachment 287185 [details]
patch

Clearing flags on attachment: 287185

Committed r205066: <http://trac.webkit.org/changeset/205066>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.