Re: PR libstdc++/83860 avoid dangling references in valarray closure types
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 f();
auto x = f() * 2;
std::valarray 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 <jwak...@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&);
+#if __cplusplus < 201103L
// not implemented
gslic
Re: PR libstdc++/83860 avoid dangling references in valarray closure types
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 f(); >> auto x = f() * 2; >> std::valarray 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.
Re: PR libstdc++/83860 avoid dangling references in valarray closure types
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 f(); auto x = f() * 2; std::valarray 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. -- Marc Glisse
PR libstdc++/83860 avoid dangling references in valarray closure types
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 f();
auto x = f() * 2;
std::valarray 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 <jwak...@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.
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&);
+#if __cplusplus < 201103L
// not implemented
gslice_array();
+#else
+public:
+ gslice_array() = delete;
+#endif
};
template
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 _M_mask;
const _Array<_Tp> _M_array;
+#if __cplusplus < 201103L
// not implemented
mask_array();
+#else
+public:
+ mask_array() = delete;
+#endif
};
template
diff --git a/libstdc++-v3/include/bits/slice_array.h
b/libstdc++-v3/include/bits/slice_array.h
index 05b096bb5a9.
