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

[Marc Glisse]

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.

-- 
Marc Glisse