| Message ID | CAH6eHdTj1T1HYvTUWs+5UdznK+Czn-6qf8qLZ6-y_mJD_pmkbg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PR libstdc++/83860 avoid dangling references in valarray closure types | expand |
On Fri, 6 Apr 2018, Jonathan Wakely wrote: > This attempts to solve some of the problems when mixing std::valarray > operations and 'auto', by storing nested closure objects as values > instead of references. This means we don't end up with dangling > references to temporary closures that have already been destroyed. > > This makes the closure objects introduced by the library less > error-prone, but it's still possible to write incorrect code by using > temporary valarrays, e.g. > > std::valarray<int> f(); > auto x = f() * 2; > std::valarray<int> y = x; > > Here the closure object 'x' has a dangling reference to the temporary > returned by f(). It might be possible to do something about this by > overloading the operators for valarray rvalues and moving the valarray > into the closure, instead of holding a const-reference. I don't plan > to work on that. > > Performance seems to be unaffected by this patch, although I didn't > test that very thoroughly. > > Strictly speaking this is an ABI change as it changes the size and > layout of the closure types like _BinClos etc. so that their _M_expr > member is at least two words, not just one for a reference. Those > closure types should never appear in API boundaries or as class > members (if anybody was doing that by using 'auto' it wouldn't have > worked correctly anyway) so I think we can just change them, without > renaming the types or moving them into an inline namespace so they > mangle differently. Does anybody disagree? When I was looking into doing something similar for GMP, I had decided to rename the class. Some inline functions have incompatible behavior before and after the change, and if they are not actually inlined and you mix the 2 types of objects (through a library for instance) without a lot of care on symbol visibility, a single version will be used for both. If you think that's too contrived a scenario, then renaming may not be needed. > The PR is marked as a regression because the example in the PR used to > "work" with older releases. That's probably only because they didn't > optimize as aggressively and so the stack space of the expired > temporaries wasn't reused (ASan still shows the bug was there in older > releases even if it doesn't crash). As a regression this should be > backported, but the layout changes make me pause a little when > considering making the change on stable release branches. I wouldn't count it as a regression.
On 6 April 2018 at 09:50, Marc Glisse wrote: > On Fri, 6 Apr 2018, Jonathan Wakely wrote: > >> This attempts to solve some of the problems when mixing std::valarray >> operations and 'auto', by storing nested closure objects as values >> instead of references. This means we don't end up with dangling >> references to temporary closures that have already been destroyed. >> >> This makes the closure objects introduced by the library less >> error-prone, but it's still possible to write incorrect code by using >> temporary valarrays, e.g. >> >> std::valarray<int> f(); >> auto x = f() * 2; >> std::valarray<int> y = x; >> >> Here the closure object 'x' has a dangling reference to the temporary >> returned by f(). It might be possible to do something about this by >> overloading the operators for valarray rvalues and moving the valarray >> into the closure, instead of holding a const-reference. I don't plan >> to work on that. >> >> Performance seems to be unaffected by this patch, although I didn't >> test that very thoroughly. >> >> Strictly speaking this is an ABI change as it changes the size and >> layout of the closure types like _BinClos etc. so that their _M_expr >> member is at least two words, not just one for a reference. Those >> closure types should never appear in API boundaries or as class >> members (if anybody was doing that by using 'auto' it wouldn't have >> worked correctly anyway) so I think we can just change them, without >> renaming the types or moving them into an inline namespace so they >> mangle differently. Does anybody disagree? > > > When I was looking into doing something similar for GMP, I had decided to > rename the class. Some inline functions have incompatible behavior before > and after the change, and if they are not actually inlined and you mix the 2 > types of objects (through a library for instance) without a lot of care on > symbol visibility, a single version will be used for both. > > If you think that's too contrived a scenario, then renaming may not be > needed. Maybe that's safest. We can just enclose them in namespace __detail or __valarray and then add using-declarations to add them back to namespace std. >> The PR is marked as a regression because the example in the PR used to >> "work" with older releases. That's probably only because they didn't >> optimize as aggressively and so the stack space of the expired >> temporaries wasn't reused (ASan still shows the bug was there in older >> releases even if it doesn't crash). As a regression this should be >> backported, but the layout changes make me pause a little when >> considering making the change on stable release branches. > > > I wouldn't count it as a regression. OK thanks for the comments. I think this can probably wait for stage 1.
On 06/04/18 11:23 +0100, Jonathan Wakely wrote: >On 6 April 2018 at 09:50, Marc Glisse wrote: >> On Fri, 6 Apr 2018, Jonathan Wakely wrote: >> >>> This attempts to solve some of the problems when mixing std::valarray >>> operations and 'auto', by storing nested closure objects as values >>> instead of references. This means we don't end up with dangling >>> references to temporary closures that have already been destroyed. >>> >>> This makes the closure objects introduced by the library less >>> error-prone, but it's still possible to write incorrect code by using >>> temporary valarrays, e.g. >>> >>> std::valarray<int> f(); >>> auto x = f() * 2; >>> std::valarray<int> y = x; >>> >>> Here the closure object 'x' has a dangling reference to the temporary >>> returned by f(). It might be possible to do something about this by >>> overloading the operators for valarray rvalues and moving the valarray >>> into the closure, instead of holding a const-reference. I don't plan >>> to work on that. >>> >>> Performance seems to be unaffected by this patch, although I didn't >>> test that very thoroughly. >>> >>> Strictly speaking this is an ABI change as it changes the size and >>> layout of the closure types like _BinClos etc. so that their _M_expr >>> member is at least two words, not just one for a reference. Those >>> closure types should never appear in API boundaries or as class >>> members (if anybody was doing that by using 'auto' it wouldn't have >>> worked correctly anyway) so I think we can just change them, without >>> renaming the types or moving them into an inline namespace so they >>> mangle differently. Does anybody disagree? >> >> >> When I was looking into doing something similar for GMP, I had decided to >> rename the class. Some inline functions have incompatible behavior before >> and after the change, and if they are not actually inlined and you mix the 2 >> types of objects (through a library for instance) without a lot of care on >> symbol visibility, a single version will be used for both. >> >> If you think that's too contrived a scenario, then renaming may not be >> needed. > >Maybe that's safest. We can just enclose them in namespace __detail or >__valarray and then add using-declarations to add them back to >namespace std. > >>> The PR is marked as a regression because the example in the PR used to >>> "work" with older releases. That's probably only because they didn't >>> optimize as aggressively and so the stack space of the expired >>> temporaries wasn't reused (ASan still shows the bug was there in older >>> releases even if it doesn't crash). As a regression this should be >>> backported, but the layout changes make me pause a little when >>> considering making the change on stable release branches. >> >> >> I wouldn't count it as a regression. > >OK thanks for the comments. I think this can probably wait for stage 1. I've committed this to trunk now. commit e41006c3d97121574e9a2f61aea8533c33bd6c40 Author: Jonathan Wakely <jwakely@redhat.com> Date: Fri Apr 6 01:30:19 2018 +0100 PR libstdc++/83860 avoid dangling references in valarray closure types Store nested closures by value not by reference, to prevent holding invalid references to temporaries that have been destroyed. This changes the layout of the closure types, so change their linkage names, but moving them to a different namespace. PR libstdc++/57997 PR libstdc++/83860 * include/bits/gslice_array.h (gslice_array): Define default constructor as deleted, as per C++11 standard. * include/bits/mask_array.h (mask_array): Likewise. * include/bits/slice_array.h (slice_array): Likewise. * include/bits/valarray_after.h (_GBase, _GClos, _IBase, _IClos): Move to namespace __detail. (_GBase::_M_expr, _IBase::_M_expr): Use _ValArrayRef for type of data members. * include/bits/valarray_before.h (_ValArrayRef): New helper for type of data members in closure objects. (_FunBase, _ValFunClos, _RefFunClos, _UnBase, _UnClos, _BinBase) (_BinBase2, _BinBase1, _BinClos, _SBase, _SClos): Move to namespace __detail. (_FunBase::_M_expr, _UnBase::_M_expr, _BinBase::_M_expr1) (_BinBase::_M_expr2, _BinBase2::_M_expr1, _BinBase1::_M_expr2) (_SBase::_M_expr): Use _ValArrayRef for type of data members. * include/std/valarray (_UnClos, _BinClos, _SClos, _GClos, _IClos) (_ValFunClos, _RefFunClos): Move to namespace __detail and add using-declarations to namespace std. * testsuite/26_numerics/valarray/83860.cc: New. diff --git a/libstdc++-v3/include/bits/gslice_array.h b/libstdc++-v3/include/bits/gslice_array.h index 2da7e0442bb..715c53bd616 100644 --- a/libstdc++-v3/include/bits/gslice_array.h +++ b/libstdc++-v3/include/bits/gslice_array.h @@ -128,8 +128,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION gslice_array(_Array<_Tp>, const valarray<size_t>&); +#if __cplusplus < 201103L // not implemented gslice_array(); +#else + public: + gslice_array() = delete; +#endif }; template<typename _Tp> diff --git a/libstdc++-v3/include/bits/mask_array.h b/libstdc++-v3/include/bits/mask_array.h index 84671cb43d6..c11691a243a 100644 --- a/libstdc++-v3/include/bits/mask_array.h +++ b/libstdc++-v3/include/bits/mask_array.h @@ -131,8 +131,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const _Array<bool> _M_mask; const _Array<_Tp> _M_array; +#if __cplusplus < 201103L // not implemented mask_array(); +#else + public: + mask_array() = delete; +#endif }; template<typename _Tp> diff --git a/libstdc++-v3/include/bits/slice_array.h b/libstdc++-v3/include/bits/slice_array.h index 05b096bb5a9..b025373180f 100644 --- a/libstdc++-v3/include/bits/slice_array.h +++ b/libstdc++-v3/include/bits/slice_array.h @@ -192,8 +192,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const size_t _M_stride; const _Array<_Tp> _M_array; +#if __cplusplus < 201103L // not implemented slice_array(); +#else + public: + slice_array() = delete; +#endif }; template<typename _Tp> diff --git a/libstdc++-v3/include/bits/valarray_after.h b/libstdc++-v3/include/bits/valarray_after.h index 7f62b292ed5..bb1a3c9b513 100644 --- a/libstdc++-v3/include/bits/valarray_after.h +++ b/libstdc++-v3/include/bits/valarray_after.h @@ -38,6 +38,8 @@ namespace std _GLIBCXX_VISIBILITY(default) { _GLIBCXX_BEGIN_NAMESPACE_VERSION +namespace __detail +{ // // gslice_array closure. // @@ -59,8 +61,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return _M_index.size(); } private: - const _Dom& _M_expr; - const valarray<size_t>& _M_index; + typename _ValArrayRef<_Dom>::__type _M_expr; + const valarray<size_t>& _M_index; }; template<typename _Tp> @@ -128,8 +130,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return _M_index.size(); } private: - const _Dom& _M_expr; - const valarray<size_t>& _M_index; + typename _ValArrayRef<_Dom>::__type _M_expr; + const valarray<size_t>& _M_index; }; template<class _Dom> @@ -153,6 +155,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _IClos (const valarray<_Tp>& __a, const valarray<size_t>& __i) : _Base (__a, __i) {} }; +} // namespace __detail // // class _Expr diff --git a/libstdc++-v3/include/bits/valarray_before.h b/libstdc++-v3/include/bits/valarray_before.h index a77fdf203b2..fe41787c19f 100644 --- a/libstdc++-v3/include/bits/valarray_before.h +++ b/libstdc++-v3/include/bits/valarray_before.h @@ -406,6 +406,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION typedef bool result_type; }; +namespace __detail +{ + // Closure types already have reference semantics and are often short-lived, + // so store them by value to avoid (some cases of) dangling references to + // out-of-scope temporaries. + template<typename _Tp> + struct _ValArrayRef + { typedef const _Tp __type; }; + + // Use real references for std::valarray objects. + template<typename _Tp> + struct _ValArrayRef< valarray<_Tp> > + { typedef const valarray<_Tp>& __type; }; + // // Apply function taking a value/const reference closure // @@ -425,7 +439,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_t size() const { return _M_expr.size ();} private: - const _Dom& _M_expr; + typename _ValArrayRef<_Dom>::__type _M_expr; value_type (*_M_func)(_Arg); }; @@ -490,7 +504,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_t size() const { return _M_expr.size(); } private: - const _Arg& _M_expr; + typename _ValArrayRef<_Arg>::__type _M_expr; }; template<class _Oper, class _Dom> @@ -536,8 +550,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_t size() const { return _M_expr1.size(); } private: - const _FirstArg& _M_expr1; - const _SecondArg& _M_expr2; + typename _ValArrayRef<_FirstArg>::__type _M_expr1; + typename _ValArrayRef<_SecondArg>::__type _M_expr2; }; @@ -557,8 +571,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_t size() const { return _M_expr1.size(); } private: - const _Clos& _M_expr1; - const _Vt& _M_expr2; + typename _ValArrayRef<_Clos>::__type _M_expr1; + _Vt _M_expr2; }; template<class _Oper, class _Clos> @@ -577,8 +591,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_t size() const { return _M_expr2.size(); } private: - const _Vt& _M_expr1; - const _Clos& _M_expr2; + _Vt _M_expr1; + typename _ValArrayRef<_Clos>::__type _M_expr2; }; template<class _Oper, class _Dom1, class _Dom2> @@ -592,7 +606,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION }; template<class _Oper, typename _Tp> - struct _BinClos<_Oper,_ValArray, _ValArray, _Tp, _Tp> + struct _BinClos<_Oper, _ValArray, _ValArray, _Tp, _Tp> : _BinBase<_Oper, valarray<_Tp>, valarray<_Tp> > { typedef _BinBase<_Oper, valarray<_Tp>, valarray<_Tp> > _Base; @@ -668,10 +682,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _BinClos(const _Tp& __t, const valarray<_Tp>& __v) : _Base(__t, __v) {} }; - // - // slice_array closure. - // - template<typename _Dom> + // + // slice_array closure. + // + template<typename _Dom> class _SBase { public: @@ -689,7 +703,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return _M_slice.size (); } private: - const _Dom& _M_expr; + typename _ValArrayRef<_Dom>::__type _M_expr; const slice& _M_slice; }; @@ -736,6 +750,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _SClos (_Array<_Tp> __a, const slice& __s) : _Base (__a, __s) {} }; +} // namespace __detail _GLIBCXX_END_NAMESPACE_VERSION } // namespace diff --git a/libstdc++-v3/include/std/valarray b/libstdc++-v3/include/std/valarray index 761a2fc98be..03e0badfc53 100644 --- a/libstdc++-v3/include/std/valarray +++ b/libstdc++-v3/include/std/valarray @@ -51,6 +51,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template<typename _Tp1, typename _Tp2> class _ValArray; +namespace __detail +{ template<class _Oper, template<class, class> class _Meta, class _Dom> struct _UnClos; @@ -74,6 +76,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template<template<class, class> class _Meta, class _Dom> class _RefFunClos; +} // namespace __detail + + using __detail::_UnClos; + using __detail::_BinClos; + using __detail::_SClos; + using __detail::_GClos; + using __detail::_IClos; + using __detail::_ValFunClos; + using __detail::_RefFunClos; template<class _Tp> class valarray; // An array of type _Tp class slice; // BLAS-like slice out of an array diff --git a/libstdc++-v3/testsuite/26_numerics/valarray/83860.cc b/libstdc++-v3/testsuite/26_numerics/valarray/83860.cc new file mode 100644 index 00000000000..6d82ef5583f --- /dev/null +++ b/libstdc++-v3/testsuite/26_numerics/valarray/83860.cc @@ -0,0 +1,110 @@ +// Copyright (C) 2018 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-do run { target c++11 } } + +#include <valarray> +#include <testsuite_hooks.h> + +const std::valarray<int> v{ + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 +}; + +bool +all_of(const std::valarray<bool>& vals) +{ + for (bool b : vals) + if (!b) + return false; + return true; +} + +void +test01() +{ + // PR libstdc++/83860 + const std::valarray<int> va(v), vb(v), vc(v); + auto sum = va + vb + vc; + std::valarray<int> vsum = sum; + VERIFY( all_of( vsum == (3 * v) ) ); +} + +void +test02() +{ + auto neg = -(-v); + std::valarray<int> vneg = neg; + VERIFY( all_of( vneg == v ) ); +} + +void +test03() +{ + const std::valarray<int> va(v), vb(v); + auto diff = va + -vb; + std::valarray<int> vdiff = diff; + VERIFY( all_of( vdiff == (va - vb) ) ); +} + +void +test04() +{ + const std::valarray<int> va(v), vb(v); + auto sum = -va + -vb; + std::valarray<int> vsum = sum; + VERIFY( all_of( vsum == (-2 * v) ) ); +} + +void +test05() +{ + const std::valarray<int> va(v), vb(v); + auto sum = -(-va + -vb); + std::valarray<int> vsum = sum; + VERIFY( all_of( vsum == (2 * v) ) ); +} + +void +test06() +{ + auto prod = 3 * +v * 2; + std::valarray<int> vprod = prod; + VERIFY( all_of( vprod == (6 * v) ) ); +} + +void +test07() +{ + const std::valarray<int> va(v), vb(v); + auto valfun = [](int i) { return i; }; + auto reffun = [](const int& i) { return i; }; + auto sum = (va.apply(valfun) + vb.apply(reffun)); + std::valarray<int> vsum = sum; + VERIFY( all_of( vsum == (va + vb) ) ); +} + +int +main() +{ + test01(); + test02(); + test03(); + test04(); + test05(); + test06(); + test07(); +}
diff --git a/libstdc++-v3/include/bits/gslice_array.h b/libstdc++-v3/include/bits/gslice_array.h index 2da7e0442bb..715c53bd616 100644 --- a/libstdc++-v3/include/bits/gslice_array.h +++ b/libstdc++-v3/include/bits/gslice_array.h @@ -128,8 +128,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION gslice_array(_Array<_Tp>, const valarray<size_t>&); +#if __cplusplus < 201103L // not implemented gslice_array(); +#else + public: + gslice_array() = delete; +#endif }; template<typename _Tp> diff --git a/libstdc++-v3/include/bits/mask_array.h b/libstdc++-v3/include/bits/mask_array.h index 84671cb43d6..c11691a243a 100644 --- a/libstdc++-v3/include/bits/mask_array.h +++ b/libstdc++-v3/include/bits/mask_array.h @@ -131,8 +131,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const _Array<bool> _M_mask; const _Array<_Tp> _M_array; +#if __cplusplus < 201103L // not implemented mask_array(); +#else + public: + mask_array() = delete; +#endif }; template<typename _Tp> diff --git a/libstdc++-v3/include/bits/slice_array.h b/libstdc++-v3/include/bits/slice_array.h index 05b096bb5a9..b025373180f 100644 --- a/libstdc++-v3/include/bits/slice_array.h +++ b/libstdc++-v3/include/bits/slice_array.h @@ -192,8 +192,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const size_t _M_stride; const _Array<_Tp> _M_array; +#if __cplusplus < 201103L // not implemented slice_array(); +#else + public: + slice_array() = delete; +#endif }; template<typename _Tp> diff --git a/libstdc++-v3/include/bits/valarray_after.h b/libstdc++-v3/include/bits/valarray_after.h index 7f62b292ed5..3cfd6a510d7 100644 --- a/libstdc++-v3/include/bits/valarray_after.h +++ b/libstdc++-v3/include/bits/valarray_after.h @@ -59,8 +59,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return _M_index.size(); } private: - const _Dom& _M_expr; - const valarray<size_t>& _M_index; + typename _ValArrayRef<_Dom>::__type _M_expr; + const valarray<size_t>& _M_index; }; template<typename _Tp> @@ -128,8 +128,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return _M_index.size(); } private: - const _Dom& _M_expr; - const valarray<size_t>& _M_index; + typename _ValArrayRef<_Dom>::__type _M_expr; + const valarray<size_t>& _M_index; }; template<class _Dom> diff --git a/libstdc++-v3/include/bits/valarray_before.h b/libstdc++-v3/include/bits/valarray_before.h index a77fdf203b2..73f0b4f61e5 100644 --- a/libstdc++-v3/include/bits/valarray_before.h +++ b/libstdc++-v3/include/bits/valarray_before.h @@ -406,6 +406,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION typedef bool result_type; }; + // Closure types already have reference semantics and are often short-lived, + // so store them by value to avoid (some cases of) dangling references to + // out-of-scope temporaries. + template<typename _Tp> + struct _ValArrayRef + { typedef const _Tp __type; }; + + // Use real references for std::valarray objects. + template<typename _Tp> + struct _ValArrayRef< valarray<_Tp> > + { typedef const valarray<_Tp>& __type; }; + // // Apply function taking a value/const reference closure // @@ -425,7 +437,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_t size() const { return _M_expr.size ();} private: - const _Dom& _M_expr; + typename _ValArrayRef<_Dom>::__type _M_expr; value_type (*_M_func)(_Arg); }; @@ -490,7 +502,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_t size() const { return _M_expr.size(); } private: - const _Arg& _M_expr; + typename _ValArrayRef<_Arg>::__type _M_expr; }; template<class _Oper, class _Dom> @@ -536,8 +548,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_t size() const { return _M_expr1.size(); } private: - const _FirstArg& _M_expr1; - const _SecondArg& _M_expr2; + typename _ValArrayRef<_FirstArg>::__type _M_expr1; + typename _ValArrayRef<_SecondArg>::__type _M_expr2; }; @@ -557,8 +569,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_t size() const { return _M_expr1.size(); } private: - const _Clos& _M_expr1; - const _Vt& _M_expr2; + typename _ValArrayRef<_Clos>::__type _M_expr1; + _Vt _M_expr2; }; template<class _Oper, class _Clos> @@ -577,8 +589,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_t size() const { return _M_expr2.size(); } private: - const _Vt& _M_expr1; - const _Clos& _M_expr2; + _Vt _M_expr1; + typename _ValArrayRef<_Clos>::__type _M_expr2; }; template<class _Oper, class _Dom1, class _Dom2> @@ -592,7 +604,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION }; template<class _Oper, typename _Tp> - struct _BinClos<_Oper,_ValArray, _ValArray, _Tp, _Tp> + struct _BinClos<_Oper, _ValArray, _ValArray, _Tp, _Tp> : _BinBase<_Oper, valarray<_Tp>, valarray<_Tp> > { typedef _BinBase<_Oper, valarray<_Tp>, valarray<_Tp> > _Base; @@ -671,7 +683,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // // slice_array closure. // - template<typename _Dom> + template<typename _Dom> class _SBase { public: @@ -689,7 +701,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return _M_slice.size (); } private: - const _Dom& _M_expr; + typename _ValArrayRef<_Dom>::__type _M_expr; const slice& _M_slice; }; diff --git a/libstdc++-v3/testsuite/26_numerics/valarray/83860.cc b/libstdc++-v3/testsuite/26_numerics/valarray/83860.cc new file mode 100644 index 00000000000..a9599798b41 --- /dev/null +++ b/libstdc++-v3/testsuite/26_numerics/valarray/83860.cc @@ -0,0 +1,105 @@ +// Copyright (C) 2018 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-do run { target c++11 } } + +#include <valarray> +#include <testsuite_hooks.h> + +const std::valarray<int> v{ + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 +}; + +bool +all_of(const std::valarray<bool>& vals) +{ + for (bool b : vals) + if (!b) + return false; + return true; +} + +void +test01() +{ + // PR libstdc++/83860 + auto sum = v + v + v; + std::valarray<int> vsum = sum; + VERIFY( all_of( vsum == (v + v + v) ) ); +} + +void +test02() +{ + auto neg = -(-v); + std::valarray<int> vneg = neg; + VERIFY( all_of( vneg == v ) ); +} + +void +test03() +{ + auto diff = v + -v; + std::valarray<int> vdiff = diff; + VERIFY( all_of( vdiff == (v - v) ) ); +} + +void +test04() +{ + auto sum = -v + -v; + std::valarray<int> vsum = sum; + VERIFY( all_of( vsum == (-v + -v) ) ); +} + +void +test05() +{ + auto sum = -(-v + -v); + std::valarray<int> vsum = sum; + VERIFY( all_of( vsum == (v + v) ) ); +} + +void +test06() +{ + auto prod = 3 * +v * 2; + std::valarray<int> vprod = prod; + VERIFY( all_of( vprod == (6 * v) ) ); +} + +void +test07() +{ + auto valfun = [](int i) { return i; }; + auto reffun = [](const int& i) { return i; }; + auto sum = (v.apply(valfun) + v.apply(reffun)); + std::valarray<int> vsum = sum; + VERIFY( all_of( vsum == (v + v) ) ); +} + +int +main() +{ + test01(); + test02(); + test03(); + test04(); + test05(); + test06(); + test07(); +}
This attempts to solve some of the problems when mixing std::valarray operations and 'auto', by storing nested closure objects as values instead of references. This means we don't end up with dangling references to temporary closures that have already been destroyed. This makes the closure objects introduced by the library less error-prone, but it's still possible to write incorrect code by using temporary valarrays, e.g. std::valarray<int> f(); auto x = f() * 2; std::valarray<int> y = x; Here the closure object 'x' has a dangling reference to the temporary returned by f(). It might be possible to do something about this by overloading the operators for valarray rvalues and moving the valarray into the closure, instead of holding a const-reference. I don't plan to work on that. Performance seems to be unaffected by this patch, although I didn't test that very thoroughly. Strictly speaking this is an ABI change as it changes the size and layout of the closure types like _BinClos etc. so that their _M_expr member is at least two words, not just one for a reference. Those closure types should never appear in API boundaries or as class members (if anybody was doing that by using 'auto' it wouldn't have worked correctly anyway) so I think we can just change them, without renaming the types or moving them into an inline namespace so they mangle differently. Does anybody disagree? The PR is marked as a regression because the example in the PR used to "work" with older releases. That's probably only because they didn't optimize as aggressively and so the stack space of the expired temporaries wasn't reused (ASan still shows the bug was there in older releases even if it doesn't crash). As a regression this should be backported, but the layout changes make me pause a little when considering making the change on stable release branches. PR libstdc++/83860 * include/bits/gslice_array.h (gslice_array): Define default constructor as deleted, as per C++11 standard. * include/bits/mask_array.h (mask_array): Likewise. * include/bits/slice_array.h (slice_array): Likewise. * include/bits/valarray_after.h (_GBase::_M_expr, _IBase::_M_expr): Use _ValArrayRef for type of data members. * include/bits/valarray_before.h (_ValArrayRef): New helper for type of data members in closure objects. (_FunBase::_M_expr, _UnBase::_M_expr, _BinBase::_M_expr1) (_BinBase::_M_expr2, _BinBase2::_M_expr1, _BinBase1::_M_expr2) (_SBase::_M_expr): Use _ValArrayRef for type of data members. * testsuite/26_numerics/valarray/83860.cc: New. I'll commit this to trunk only for now, unless anybody sees a problem with the approach, or thinks the layout changes require new mangled names for the closures. commit 1dd9f0f54457eb2c44c6b1125800d94eaac0cb2f Author: Jonathan Wakely <jwakely@redhat.com> Date: Fri Apr 6 01:30:19 2018 +0100 PR libstdc++/83860 avoid dangling references in valarray closure types PR libstdc++/83860 * include/bits/gslice_array.h (gslice_array): Define default constructor as deleted, as per C++11 standard. * include/bits/mask_array.h (mask_array): Likewise. * include/bits/slice_array.h (slice_array): Likewise. * include/bits/valarray_after.h (_GBase::_M_expr, _IBase::_M_expr): Use _ValArrayRef for type of data members. * include/bits/valarray_before.h (_ValArrayRef): New helper for type of data members in closure objects. (_FunBase::_M_expr, _UnBase::_M_expr, _BinBase::_M_expr1) (_BinBase::_M_expr2, _BinBase2::_M_expr1, _BinBase1::_M_expr2) (_SBase::_M_expr): Use _ValArrayRef for type of data members. * testsuite/26_numerics/valarray/83860.cc: New.