Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1771303002:
WTF: Fix Vector<T> memcpy'ing incorrectly for some T. (Closed)
|
Created:
4 years, 9 months ago by Yuta Kitamura Modified:
4 years, 2 months ago CC:
chromium-reviews, blink-reviews, blink-reviews-wtf_chromium.org, Mikhail Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWTF: Fix Vector<T> memcpy'ing incorrectly for some T.
The core issue here is the implementation of IsTriviallyCopyAssignable
and IsTriviallyMoveAssignable was incorrect. They both use a compiler
builtin __has_trivial_assign(T), but it returns true if T's copy
assignment operator is declared as deleted.
To workaround that, a new type trait IsAssignable<T, From> is
implemented in the same way as std::is_assignable (as it's not available
at this time), and the assignability is used to limit the scope of
IsTrivially{Copy,Move}Assignable.
IsMoveAssignable isn't really right even with my patch for the reason
stated in the comments in the patch. I think it's impossible to
workaround that, though.
A bunch of static_asserts are added to test some invariants related to
this change.
BUG=592767
Committed: https://crrev.com/d7225e41c5557ef72e5ca439fcf8287be37e7c0f
Cr-Commit-Position: refs/heads/master@{#380329}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fix comments. #Patch Set 3 : Blind SFINAE juggling for MSVC errors. #Patch Set 4 : Try 2. #Patch Set 5 : Final MSVC workaround. #
Total comments: 1
Messages
Total messages: 31 (12 generated)
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1771303002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1771303002/1
yutak@chromium.org changed reviewers: + haraken@chromium.org, tzik@chromium.org, yzshen@chromium.org
yutak@chromium.org changed required reviewers: + haraken@chromium.org, tzik@chromium.org
PTAL. In short: C++ is hard.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/TypeTraits.h (right): https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/TypeTraits.h:58: template <typename T2, typename From2, typename = decltype(std::declval<T2>() = std::declval<From2>(), 0)> Does this ", 0" take effect? If it's not, can we remove this? https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/TypeTraits.h:84: // TODO(yutak): This isn't really correct, because __has_trivial_assign appears to look only at copy constructor. s/copy constructor/copy assignment/? https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/TypeTraits.h:95: static const bool value = __has_trivial_constructor(T); Could you fix this too or leave a TODO comment here? https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/TypeTraits.h:99: static const bool value = __has_trivial_destructor(T); ditto
Thanks for working on this! https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/TypeTraits.h (right): https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/TypeTraits.h:84: // TODO(yutak): This isn't really correct, because __has_trivial_assign appears to look only at copy constructor. This comment scares me a little bit. :/ I think optimization is nice but correctness takes priority. :) Does it make sense to remove all these traits entirely for now until we can support them in a clear and correct way? https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/VectorTest.cpp (right): https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/VectorTest.cpp:469: class MojoMoveOnlyType final { Nit: this class itself doesn't have anything to do with Mojo. Maybe just name it MoveOnlyType, or CopyDeletedType?
https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/TypeTraits.h (right): https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/TypeTraits.h:58: template <typename T2, typename From2, typename = decltype(std::declval<T2>() = std::declval<From2>(), 0)> On 2016/03/08 11:52:48, tzik wrote: > Does this ", 0" take effect? If it's not, can we remove this? Yeah, right. Fixed. https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/TypeTraits.h:84: // TODO(yutak): This isn't really correct, because __has_trivial_assign appears to look only at copy constructor. On 2016/03/08 11:52:49, tzik wrote: > s/copy constructor/copy assignment/? Yep you are right. Fixed. https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/TypeTraits.h:84: // TODO(yutak): This isn't really correct, because __has_trivial_assign appears to look only at copy constructor. On 2016/03/08 21:30:38, yzshen1 wrote: > This comment scares me a little bit. :/ I think optimization is nice but > correctness takes priority. :) > > Does it make sense to remove all these traits entirely for now until we can > support them in a clear and correct way? I think that is a big no-no for us, because that could regress the performance in a critical way. I started to feel like this is not as scary as I initially thought; the definition basically says: if T is copy assignable, T is (mostly) move assignable. This misses the case where T is move-only assignable, but we have very little instances of such T, so my definition is a reasonable conservative approximation. I think the third clause isn't necessary, because T is definitely move assignable if T is copy assignable. Will rewrite the comment with the above in mind. https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/TypeTraits.h:95: static const bool value = __has_trivial_constructor(T); On 2016/03/08 11:52:48, tzik wrote: > Could you fix this too or leave a TODO comment here? Done. https://codereview.chromium.org/1771303002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/TypeTraits.h:99: static const bool value = __has_trivial_destructor(T); On 2016/03/08 11:52:48, tzik wrote: > ditto Done.
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1771303002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1771303002/40001
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1771303002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1771303002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1771303002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1771303002/80001
I *think* I managed to workaround MSVC errors; PTAL again?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
owner LGTM (relying on tzik's LG).
On 2016/03/09 11:40:39, haraken wrote: > owner LGTM (relying on tzik's LG). LGTM
The CQ bit was checked by yutak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1771303002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1771303002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
WTF: Fix Vector<T> memcpy'ing incorrectly for some T.
The core issue here is the implementation of IsTriviallyCopyAssignable
and IsTriviallyMoveAssignable was incorrect. They both use a compiler
builtin __has_trivial_assign(T), but it returns true if T's copy
assignment operator is declared as deleted.
To workaround that, a new type trait IsAssignable<T, From> is
implemented in the same way as std::is_assignable (as it's not available
at this time), and the assignability is used to limit the scope of
IsTrivially{Copy,Move}Assignable.
IsMoveAssignable isn't really right even with my patch for the reason
stated in the comments in the patch. I think it's impossible to
workaround that, though.
A bunch of static_asserts are added to test some invariants related to
this change.
BUG=592767
==========
to
==========
WTF: Fix Vector<T> memcpy'ing incorrectly for some T.
The core issue here is the implementation of IsTriviallyCopyAssignable
and IsTriviallyMoveAssignable was incorrect. They both use a compiler
builtin __has_trivial_assign(T), but it returns true if T's copy
assignment operator is declared as deleted.
To workaround that, a new type trait IsAssignable<T, From> is
implemented in the same way as std::is_assignable (as it's not available
at this time), and the assignability is used to limit the scope of
IsTrivially{Copy,Move}Assignable.
IsMoveAssignable isn't really right even with my patch for the reason
stated in the comments in the patch. I think it's impossible to
workaround that, though.
A bunch of static_asserts are added to test some invariants related to
this change.
BUG=592767
Committed: https://crrev.com/d7225e41c5557ef72e5ca439fcf8287be37e7c0f
Cr-Commit-Position: refs/heads/master@{#380329}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d7225e41c5557ef72e5ca439fcf8287be37e7c0f Cr-Commit-Position: refs/heads/master@{#380329}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1771303002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/TypeTraits.h (right): https://codereview.chromium.org/1771303002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/TypeTraits.h:56: // To workaround that, here we have IsAssignable<T, From> class template, but unfortunately, MSVC 2013 cannot compile is this still needed now that we're on 2015? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
