| Message ID | 20200225124056.GA2337630@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed] libstdc++: Fix regression in std::move algorithm (PR 93872) | expand |
On 25/02/20 12:40 +0000, Jonathan Wakely wrote: >The std::move and std::move_backward algorithms dispatch to the >std::__memmove helper when appropriate. That function uses a >pointer-to-const for the source values, preventing them from being >moved. The two callers of that function have the same problem. > >Rather than altering __memmove and its callers to work with const or >non-const source pointers, this takes a more conservative approach of >casting away the const at the point where we want to do a move >assignment. This relies on the fact that we only use __memmove when the >type is trivially copyable, so we know the move assignment doesn't alter >the source anyway. > > PR libstdc++/93872 > * include/bits/stl_algobase.h (__memmove): Cast away const before > doing move assignment. > * testsuite/25_algorithms/move/93872.cc: New test. > * testsuite/25_algorithms/move_backward/93872.cc: New test. > Oops, I forgot to 'git add' one of the new tests. Tested x86_64-linux, committed to master.
On 25/02/20 12:40 +0000, Jonathan Wakely wrote: >The std::move and std::move_backward algorithms dispatch to the >std::__memmove helper when appropriate. That function uses a >pointer-to-const for the source values, preventing them from being >moved. The two callers of that function have the same problem. > >Rather than altering __memmove and its callers to work with const or >non-const source pointers, this takes a more conservative approach of >casting away the const at the point where we want to do a move >assignment. This relies on the fact that we only use __memmove when the >type is trivially copyable, so we know the move assignment doesn't alter >the source anyway. > > PR libstdc++/93872 > * include/bits/stl_algobase.h (__memmove): Cast away const before > doing move assignment. > * testsuite/25_algorithms/move/93872.cc: New test. > * testsuite/25_algorithms/move_backward/93872.cc: New test. I think what I'd really like to do is get rid of __memmove entirely. We already have code that does the explicit assignment in a loop, for the cases where we can't use __builtin_memmove because the type is not trivially copyable. We should just use that existing code during constant evaluation, i.e. don't do the __builtin_memmove optimizations during constant evaluation. It seems much cleaner to just not use the optimization rather than wrap it to be usable in constant expressions. We already have to do that for {copy,move}_backward anyway, because __memmove doesn't correctly implement the std::memmove semantics for overlapping ranges. But we do it **wrong** and turn copy_backward into move_backward during constant evaluation. Here's a patch that gets rid of __memmove and fixes that bug (generated with 'git diff -b' so that the changes to the logic aren't obscured by the whitespace changes caused by re-indenting). Maybe I should just go ahead and do this now, since __memmove (and the problems it causes) are new for GCC 10 anyway. That would revert <bits/stl_algobase.h> to something closer to the GCC 9 version.
On Tue, 25 Feb 2020 at 15:36, Jonathan Wakely <jwakely@redhat.com> wrote: > I think what I'd really like to do is get rid of __memmove entirely. > We already have code that does the explicit assignment in a loop, for > the cases where we can't use __builtin_memmove because the type is not > trivially copyable. > > We should just use that existing code during constant evaluation, i.e. > don't do the __builtin_memmove optimizations during constant > evaluation. It seems much cleaner to just not use the optimization > rather than wrap it to be usable in constant expressions. > > We already have to do that for {copy,move}_backward anyway, because > __memmove doesn't correctly implement the std::memmove semantics for > overlapping ranges. But we do it **wrong** and turn copy_backward into > move_backward during constant evaluation. > > Here's a patch that gets rid of __memmove and fixes that bug > (generated with 'git diff -b' so that the changes to the logic aren't > obscured by the whitespace changes caused by re-indenting). > > Maybe I should just go ahead and do this now, since __memmove (and the > problems it causes) are new for GCC 10 anyway. That would revert > <bits/stl_algobase.h> to something closer to the GCC 9 version. Looks good to me.
On 25/02/20 16:01 +0200, Ville Voutilainen wrote: >On Tue, 25 Feb 2020 at 15:36, Jonathan Wakely <jwakely@redhat.com> wrote: >> I think what I'd really like to do is get rid of __memmove entirely. >> We already have code that does the explicit assignment in a loop, for >> the cases where we can't use __builtin_memmove because the type is not >> trivially copyable. >> >> We should just use that existing code during constant evaluation, i.e. >> don't do the __builtin_memmove optimizations during constant >> evaluation. It seems much cleaner to just not use the optimization >> rather than wrap it to be usable in constant expressions. >> >> We already have to do that for {copy,move}_backward anyway, because >> __memmove doesn't correctly implement the std::memmove semantics for >> overlapping ranges. But we do it **wrong** and turn copy_backward into >> move_backward during constant evaluation. >> >> Here's a patch that gets rid of __memmove and fixes that bug >> (generated with 'git diff -b' so that the changes to the logic aren't >> obscured by the whitespace changes caused by re-indenting). >> >> Maybe I should just go ahead and do this now, since __memmove (and the >> problems it causes) are new for GCC 10 anyway. That would revert >> <bits/stl_algobase.h> to something closer to the GCC 9 version. > >Looks good to me. I've committed it now. It's not strictly a regression, because the bug only happens when using std::copy_backwards in a constant expression, which never compiled before GCC 10 anyway. But enabling it to be used in constant expressions but with incorrect results doesn't seem useful.
diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h index efda15f816e..c6b7148b39c 100644 --- a/libstdc++-v3/include/bits/stl_algobase.h +++ b/libstdc++-v3/include/bits/stl_algobase.h @@ -95,7 +95,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION for(; __num > 0; --__num) { if constexpr (_IsMove) - *__dst = std::move(*__src); + // This const_cast looks unsafe, but we only use this function + // for trivially-copyable types, which means this assignment + // is trivial and so doesn't alter the source anyway. + // See PR 93872 for why it's needed. + *__dst = std::move(*const_cast<_Tp*>(__src)); else *__dst = *__src; ++__src; diff --git a/libstdc++-v3/testsuite/25_algorithms/move/93872.cc b/libstdc++-v3/testsuite/25_algorithms/move/93872.cc new file mode 100644 index 00000000000..c4dd43dfb64 --- /dev/null +++ b/libstdc++-v3/testsuite/25_algorithms/move/93872.cc @@ -0,0 +1,39 @@ +// Copyright (C) 2020 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +// { dg-options "-std=gnu++2a" } +// { dg-do compile { target c++2a } } + +#include <algorithm> + +struct X +{ + X() = default; + + X(const X&) = delete; + X& operator=(const X&) = delete; + + X(X&&) = default; + X& operator=(X&&) = default; +}; + +void +test01() +{ + X a[2], b[2]; + std::move(std::begin(a), std::end(a), std::begin(b)); +}
The std::move and std::move_backward algorithms dispatch to the std::__memmove helper when appropriate. That function uses a pointer-to-const for the source values, preventing them from being moved. The two callers of that function have the same problem. Rather than altering __memmove and its callers to work with const or non-const source pointers, this takes a more conservative approach of casting away the const at the point where we want to do a move assignment. This relies on the fact that we only use __memmove when the type is trivially copyable, so we know the move assignment doesn't alter the source anyway. PR libstdc++/93872 * include/bits/stl_algobase.h (__memmove): Cast away const before doing move assignment. * testsuite/25_algorithms/move/93872.cc: New test. * testsuite/25_algorithms/move_backward/93872.cc: New test. Tested powerpc64le-linux, committed to master. commit 5b904f175ff26269615f148459a8604f45880591 Author: Jonathan Wakely <jwakely@redhat.com> Date: Tue Feb 25 12:21:44 2020 +0000 libstdc++: Fix regression in std::move algorithm (PR 93872) The std::move and std::move_backward algorithms dispatch to the std::__memmove helper when appropriate. That function uses a pointer-to-const for the source values, preventing them from being moved. The two callers of that function have the same problem. Rather than altering __memmove and its callers to work with const or non-const source pointers, this takes a more conservative approach of casting away the const at the point where we want to do a move assignment. This relies on the fact that we only use __memmove when the type is trivially copyable, so we know the move assignment doesn't alter the source anyway. PR libstdc++/93872 * include/bits/stl_algobase.h (__memmove): Cast away const before doing move assignment. * testsuite/25_algorithms/move/93872.cc: New test. * testsuite/25_algorithms/move_backward/93872.cc: New test.