This reverts commit a6e1080b87db8fbe0e1afadd96af5a3c0bd5e279.
Fix the conditions when the memmove optimization can be applied and refactor them out into a reusable type trait, fix and significantly expand the tests.
Details
Details
- Reviewers
jdoerfert ldionne - Group Reviewers
Restricted Project - Commits
- rG5629d492df38: Reapply "[libc++][ranges]Refactor `copy{,_backward}` and `move{,_backward}`"
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| libcxx/include/__algorithm/copy_move_common.h | ||
|---|---|---|
| 42 | I think the volatile condition is only relevant for memmove and friends -- bit_cast is happy to convert between volatile and non-volatile, and __are_always_bitcastable should probably follow suit. | |
| libcxx/include/__type_traits/are_always_bitcastable.h | ||
| 25 ↗ | (On Diff #480804) | This is my attempt to formalize the trait. Some things that I think are worth calling out:
|
| 33 ↗ | (On Diff #480804) | Do we still need this in new code? |
| 33 ↗ | (On Diff #480804) | Do we need to inherit from integral_constant for internal traits? |
| 45–47 ↗ | (On Diff #480804) | This is the closest I could get to a "formal" definition of what this trait is trying to do. Please let me know what you think. |
| 67 ↗ | (On Diff #480804) | From testing, this currently isn't true -- somehow e.g. int[1] is considered bit-castable to itself. |
| 68 ↗ | (On Diff #480804) | (or so I think) |
| libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp | ||
| 238 | Please let me know if you think this test is overkill (or alternatively, if you'd like to test more types -- I'm operating on the assumption that if it works for one signed/unsigned pair, it works for all of them). | |
| libcxx/test/libcxx/type_traits/are_always_bitcastable.compile.pass.cpp | ||
| 13 ↗ | (On Diff #480804) | Please let me know if you think these tests are overkill. |
| 16 ↗ | (On Diff #480804) | I think it's better to avoid including this file from <type_traits> -- users shouldn't pay the extra compilation penalty for our internal stuff (even if that penalty is very small). |
| 205 ↗ | (On Diff #480804) | This is the behavior that we want, right? |
| 211 ↗ | (On Diff #480804) | I found it surprising, but arrays are currently considered bit-convertible. What do you think? |
| libcxx/include/__algorithm/copy_move_common.h | ||
|---|---|---|
| 52 | Not sure why the LLVM redirect is broken on this, here is the github link I meant: https://github.com/llvm/llvm-project/issues/28901 | |
Comment Actions
I'll have another look but this LGTM w/ comments applied. Thanks a lot for all the work on this patch, the rabbit hole is much deeper than I initially thought.
| libcxx/include/__algorithm/copy_move_common.h | ||
|---|---|---|
| 42 | Do we have a test with an _In and/or an _Out that is volatile? | |
| libcxx/include/__type_traits/are_always_bitcastable.h | ||
| 33 ↗ | (On Diff #480804) | _LIBCPP_TEMPLATE_VIS isn't needed, not because it's new code, but because this gives default visibility to a struct that doesn't require any visibility. There's nothing to give visibility to.
No. |
| 45–47 ↗ | (On Diff #480804) | I think I like this. |
| 1 ↗ | (On Diff #480812) | @jfb Just tagging you here in case you're curious to take a look. Your pair of eyes on this file would certainly be a bonus. |
| 27 ↗ | (On Diff #480812) | I would also add a mention of http://eel.is/c++draft/basic.types.general#3 which blesses what we're doing here with trivially copyable types. |
| 59–61 ↗ | (On Diff #480812) | |
| libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp | ||
| 16–17 | If it's just a warning, you could use // ADDITIONAL_COMPILE_FLAGS(gcc): -Wno-whatever. | |
| libcxx/test/libcxx/type_traits/are_always_bitcastable.compile.pass.cpp | ||
| 62–65 ↗ | (On Diff #480812) | Then you get the same benefit for eliminating repetition, but IMO this is less arbitrary. And for the "same types" case, we're just instantiating the same function twice (well once really because compiler memoization). |
| 67–70 ↗ | (On Diff #480812) | IMO this doesn't pull its weight. |
| 16 ↗ | (On Diff #480804) | I would argue that's a "libc++ policy" decision and it's contrary to what we had agreed on when we started granularizing headers. IIRC, we had decided that a header like <foo> should include everything inside <__foo/*.h>, regardless of whether it's public API or not. That being said, that simply doesn't work -- I've run into cases where including everything inside <__foo/*.h> leads to circular dependencies, so this is something we'll likely have to revisit. The status quo is also that we are not systematically including all of <__foo/*.h>. So I'm fine with what you've done. |
| 205 ↗ | (On Diff #480804) | I think so, because references are not objects. |
| 211 ↗ | (On Diff #480804) | I think that for the purpose of this trait, we should probably also enforce that the two types are std::is_trivially_assignable_v<To, From>, and that would handle the array case. WDYT? Perhaps this means that the name and purpose of the trait must change a tiny bit, maybe something like __copy_assignment_is_bitcast? Basically anything that will hint at the fact that this does a bit more than checking whether the two types are bit-castable, since we're also taking into account other unrelated language rules like t = u being valid. We should make sure to have a test that would catch it if our std::copy suddenly started to accept things that are not assignable to each other just because we are using std::memcpy under the hood. |
Comment Actions
The title of the commit could be changed to "Reapply" instead of "Revert Revert" -- that's always a bit confusing IMO.
| libcxx/include/__type_traits/is_assignment_equivalent_to_bitcast.h | ||
|---|---|---|
| 37–80 ↗ | (On Diff #482744) | // This is the trait for the core type property, separate from syntactic requirements
template <class _From, class _To> // those are cv-unqualified types always
struct __is_always_bitcastable {
static constexpr bool value =
// basic case, only one type, check that it's trivially copyable
(is_same< _From, _To >::value && is_trivially_copyable<_From>::value) ||
(
sizeof(_From) == sizeof(_To) &&
is_integral<_From>::value &&
is_integral<_To>::value &&
!is_same<_To, bool>::value
);
};
// Then inside std::copy, do:
enable_if_t<
is_assignable<_From, _To>::value && __is_always_bitcastable<_From, _To>::value
>
It copy(...) {
// use memcpy, which we know is valid since __is_always_bitcastable is true
}That should also make the tests for this trait less weird, since now it will be natural that array types are always bitcastable, per the definition of trivially-copyable. |
| libcxx/test/libcxx/type_traits/is_assignment_equivalent_to_bitcast.compile.pass.cpp | ||
| 70–75 ↗ | (On Diff #482744) | Let's inline this into check below? It's only used once. |
| libcxx/include/__algorithm/copy_move_common.h | ||
|---|---|---|
| 42 | Yes (the very last test case in copy_move_nontrivial.pass.cpp). | |
| 52 | Yes, it fixes the compilation error when passing pointers to volatile. Thanks for finding the issue! | |
| libcxx/include/__type_traits/are_always_bitcastable.h | ||
| 27 ↗ | (On Diff #480812) | Slightly rephrased as In other words, `From` and `To` have the same value representation and the set of values of `From` is a subset of the set of values of `To`. (I think that having the same object representation can be implied, but the bit about having the set of values is important because e.g. a boolean and its corresponding integer type are said to have the same value representation) I'm not sure about adding that particular section. It explicitly blesses the simple case (copying between objects of the same type), but this trait is primarily for conversions. |
| libcxx/include/__type_traits/is_assignment_equivalent_to_bitcast.h | ||
| 37–80 ↗ | (On Diff #482744) | Done. Please check the new rationale for (rejecting) pointers. Arrays are accepted now like we discussed. |
| libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp | ||
| 16–17 | I'm no longer getting that when running the buildbot locally. Perhaps the warning doesn't cover the case when the ambiguating function is a template. I'll wait for CI to be sure. | |
| libcxx/test/libcxx/type_traits/are_always_bitcastable.compile.pass.cpp | ||
| 16 ↗ | (On Diff #480804) | Thanks for explaining the additional context! |
| 211 ↗ | (On Diff #480804) | I went with __is_assignment_equivalent_to_bitcast which is quite a mouthful -- happy to improve the name. We might split this into two traits (one that doesn't check for is_trivially_assignable but checks everything else, and the other is a conjunction of the first trait and is_trivially_assignable), but I'd wait until we have a use case for that (assuming it's a good idea in the first place). |
| libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp | ||
|---|---|---|
| 16–17 | Hmm, the CI still gets the error, and apparently it's a bona fide error, not just a warning. Unfortunately, I had to mark GCC as unsupported again. | |
| libcxx/include/valarray | ||
|---|---|---|
| 4936 | @ldionne I'm getting $ "diff" "-w" "/llvm/libcxx/test/libcxx/transitive_includes/cxx2b.csv" "/llvm/build/generic-cxx2b/test/libcxx/Output/transitive_includes.sh.cpp.dir/t.tmp/transitive_includes.csv" # command output: *** /llvm/libcxx/test/libcxx/transitive_includes/cxx2b.csv --- /llvm/build/generic-cxx2b/test/libcxx/Output/transitive_includes.sh.cpp.dir/t.tmp/transitive_includes.csv *************** *** 722,728 **** valarraycmath valarraycstddef valarraycstdlib - valarraycstring valarrayinitializer_list valarraylimits valarraynew --- 722,727 ---- error: command failed with exit status: 1 What's the reason this is only defined in C++20 and below? | |
| libcxx/include/valarray | ||
|---|---|---|
| 4936 | We've decided to remove all transitive includes from C++2b, since it's not considered stable. You can just remove the valarray cstring line from Cxx2b.csv. | |
Comment Actions
Heads-up that we're seeing crashes due to the recommit of this change.
msan reports "use-of-uninitialized-value" in multiple places, where input is clearly initialized.
Comment Actions
This is going to be extremely difficult to back out since we've made changes on top of that. Please work with us to understand this issue and fix it forward ASAP. In particular, we will want to make sure it lands in LLVM 16 as early as possible.
Thanks!
Comment Actions
Not a breakage report, but just a comment in case it wasn't expected/intended: it looks like this increases the conformance requirement for custom iterators passed to std::copy and friends. From a couple tests, it seems like gcc/libstdc++ already has stricter requirements than libc++, so the conformance requirement is being matched between libc++ and libstdc++ now. As an example:
struct MyIterator {
using iterator_category = std::forward_iterator_tag;
using value_type = float;
// These two lines are now also needed
// using pointer = float*;
// using reference = const float&;
using difference_type = std::ptrdiff_t;
value_type operator*();
MyIterator& operator++();
friend bool operator==(const MyIterator&, const MyIterator&);
friend bool operator!=(const MyIterator&, const MyIterator&);
};
void func() {
std::vector<float> dest;
std::copy(MyIterator(), MyIterator(), std::back_inserter(dest));
}https://godbolt.org/z/jTdbT4xcP
It seems pretty clear the change here is correct, this is just an FYI of the impact & in case anyone else bumps into this.
Comment Actions
We've found the cause of the problems @asbirlea reported above: violation of aliasing rules. Something similar to this snippet: https://gcc.godbolt.org/z/3Y9GEMW4e
Thus, again, the change is correct, but it's worth mentioning in the release notes that more aggressive optimizations in std::copy and friends may expose a lot more UB around std::vector and not only.