Revert "[libc++] Fix std::copy and std::move for ranges with potentially overlapping tail padding"

[philnik]

Summary

This reverts commit c4e98722ca79c827dd57b809e0ef16b3b8da8c72, which breaks
vector<T>::erase(...) for move-only types.

See https://reviews.llvm.org/D151953#4472195

[alexfh]

Fix conflict resolution in module map (hopefully) correctly.

[ldionne]

We'll fix this one forward since we have a good understanding of what to do. Also, the diff was uploaded wrong but it shouldn't matter.

[alexfh]

In D154595#4476928, @ldionne wrote:

We'll fix this one forward since we have a good understanding of what to do. Also, the diff was uploaded wrong but it shouldn't matter.

Thanks! I started with the revert, since there was no further reaction after your comment on https://reviews.llvm.org/D151953#4472195

Given the impact, I'd expect a quicker resolution of the issue, one way or another.

[ldionne]

See D154613. The original issue was reported on the 4th of July, which was a Holiday in the US so @philnik couldn't take a look.

[ldionne]

(I'd abandon this to clear the review queue)

[alexfh]

Trying to upload the full patch. Just in case.

[alexfh]

In D154595#4477290, @ldionne wrote:

See D154613. The original issue was reported on the 4th of July, which was a Holiday in the US so @philnik couldn't take a look.

I understand there was a holiday in the US and many folks may take more days off on the week. But knowing this doesn't help folks in other parts of the world, whose work is blocked by a certain commit. Reverting is often the safest way to get back to good state.

Thanks for preparing the tests in D154613. How soon should we expect a fix?

[ldionne]

In D154595#4477589, @alexfh wrote:

In D154595#4477290, @ldionne wrote:

See D154613. The original issue was reported on the 4th of July, which was a Holiday in the US so @philnik couldn't take a look.

I understand there was a holiday in the US and many folks may take more days off on the week. But knowing this doesn't help folks in other parts of the world, whose work is blocked by a certain commit. Reverting is often the safest way to get back to good state.

Thanks for preparing the tests in D154613. How soon should we expect a fix?

Reverting this patch re-introduces the issue that this patch was fixing. It actually trades a compile-time issue for a runtime issue.

But knowing this doesn't help folks in other parts of the world, whose work is blocked by a certain commit.

We have a problem here, and it's not the first time this happens. The fact that you folks are living on trunk is great because it leads to important early feedback like this. This is a relationship we want to maintain because it's really useful to the project, and I think that is also useful to you because we do act on that feedback. However, there is clearly some tension around how you want us to deal with these reports and how we want to deal with them. In the general case, it's entirely unreasonable to expect that our tree is going to be free of defects at arbitrary points between releases. For example, if you take a look at the current state of hardening, it's not working as intended because it is *in development*. That's the principle of making incremental progress on features, and really all software development works that way. Does that mean we should refrain from making incremental changes or going through tree states that we know are not ready for a release? I don't think so.

The developer policy relates to keeping the CI (buildbot and others) green, and we *do* keep it green. You folks are living outside of that. That's where we draw the line: when a commit breaks the CI, you'll see us reverting the commit without too much discussion. But in this case, it's an entirely different story. It broke some funky code (*) internally in some organization and they just happen to be unwilling to unblock themselves by doing a temporary revert or using some branching scheme to only ship stuff that has been stabilized for a few days. We'll need to figure out a way to collaborate effectively on issues like this in the future, because the current tension is not healthy for anyone. I'll reach out for us to talk offline.

(*) The code is somewhat funky because the types for which this broke is a small set of types. It's only types that are TriviallyCopyable but that have a deleted copy assignment operator, not all move-only types like we initially thought. That's probably something you'd want to fix in your code BTW.

[alexfh]

In D154595#4480304, @ldionne wrote:

In D154595#4477589, @alexfh wrote:

In D154595#4477290, @ldionne wrote:

See D154613. The original issue was reported on the 4th of July, which was a Holiday in the US so @philnik couldn't take a look.

I understand there was a holiday in the US and many folks may take more days off on the week. But knowing this doesn't help folks in other parts of the world, whose work is blocked by a certain commit. Reverting is often the safest way to get back to good state.

Thanks for preparing the tests in D154613. How soon should we expect a fix?

Reverting this patch re-introduces the issue that this patch was fixing. It actually trades a compile-time issue for a runtime issue.

But knowing this doesn't help folks in other parts of the world, whose work is blocked by a certain commit.

We have a problem here, and it's not the first time this happens. The fact that you folks are living on trunk is great because it leads to important early feedback like this. This is a relationship we want to maintain because it's really useful to the project, and I think that is also useful to you because we do act on that feedback. However, there is clearly some tension around how you want us to deal with these reports and how we want to deal with them. In the general case, it's entirely unreasonable to expect that our tree is going to be free of defects at arbitrary points between releases. For example, if you take a look at the current state of hardening, it's not working as intended because it is *in development*. That's the principle of making incremental progress on features, and really all software development works that way. Does that mean we should refrain from making incremental changes or going through tree states that we know are not ready for a release? I don't think so.

I totally appreciate the hard work LLVM community (and libc++ part of it in particular) puts into the project and the readiness to fix issues we and others detect. And I also understand that incremental progress on features means some features would be in a half-baked state for certain periods of time. On the other hand, https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy claims that LLVM tends to keep the tree healthy, but we need to come to an agreement of what "healthy" means. While "CI is green" is clear and unambiguous, this criterion is only as good as the tests are. If a certain important use case is not covered by tests, it only means that tests are insufficient. "Ready for a release" is on the other hand too subjective, expensive and long to verify. Apart from that, we're not expecting LLVM tree (libc++ included) to be in a good state after each commit. Our expectation is that for each problem found at a specific commit there is a fix (revert or fix forward) in a reasonable time after the problem is found. Now, we may need to agree on what "reasonable time" is or whether this expectation is reasonable in general. But for that I'll take your offer of bringing this conversation offline.

The developer policy relates to keeping the CI (buildbot and others) green, and we *do* keep it green. You folks are living outside of that. That's where we draw the line: when a commit breaks the CI, you'll see us reverting the commit without too much discussion. But in this case, it's an entirely different story. It broke some funky code (*) internally in some organization and they just happen to be unwilling to unblock themselves by doing a temporary revert or using some branching scheme to only ship stuff that has been stabilized for a few days. We'll need to figure out a way to collaborate effectively on issues like this in the future, because the current tension is not healthy for anyone. I'll reach out for us to talk offline.

(*) The code is somewhat funky because the types for which this broke is a small set of types. It's only types that are TriviallyCopyable but that have a deleted copy assignment operator, not all move-only types like we initially thought. That's probably something you'd want to fix in your code BTW.

This last point is interesting on its own. Having a large corpus of internal and third-party code, we're in a position where we regularly see some "funky code". And though we put a lot of effort into keeping the code healthy, we can't be ready to fix hundreds, thousands, or even more instances of "funky code" (by some definition) at random points in time. Thus, we expect a certain grace period (and short- or long-term opt-out mechanisms) for breaking changes with high impact.

[philnik]

@alexfh Please abandon this to clear up the review queue.

libcxx/include/module.modulemap.in

module type_traits {
     module common_type                               {
       private header "__type_traits/common_type.h"
       export utility.__utility.declval
     }
     module conditional                               { private header "__type_traits/conditional.h" }
     module conjunction                               { private header "__type_traits/conjunction.h" }
     module copy_cv                                   { private header "__type_traits/copy_cv.h" }
     module copy_cvref                                { private header "__type_traits/copy_cvref.h" }
-    module datasizeof                                { private header "__type_traits/datasizeof.h" }
     module decay                                     {
       private header "__type_traits/decay.h"
       export add_pointer
     }
     module dependent_type                            { private header "__type_traits/dependent_type.h" }
     module disjunction                               { private header "__type_traits/disjunction.h" }
     module enable_if                                 { private header "__type_traits/enable_if.h" }
     module extent                                    { private header "__type_traits/extent.h" }