PR libstdc++/83860 avoid dangling references in valarray closure types

[Jonathan Wakely]

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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 11682 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20180502/4ab650f9/attachment.bin>