This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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<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.

Attachment: patch.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]