[libc++] Avoid creating temporaries in unary expressions involving valarray

[ldionne]

Summary

Currently, unary expressions involving valarray will create a temporary.
This leads to dangling references in expressions like -a * b, because
-a is a temporary and the resulting expression will refer to it. This
patch fixes the problem by creating a lazy expression to perform the unary
operation instead of eagerly creating a temporary valarray. This is
permitted by the Standard.

rdar://90152242

[ldionne]

Fix typo in test.

[philnik]

Isn't this ABI breaking? Or is it in this case OK for some reason?

libcxx/include/valarray

926–930

Add _LIBCPP_HIDE_FROM_ABI?

3306–3307

[Mordante]

libcxx/test/std/numerics/numarray/template.valarray/valarray.unary/bit_not.pass.cpp

59

C++03 fails due to these initializer lists.

62

Can you also test the contents of the resulting array? Same for the other tests.

[ldionne]

In D125019#3494305, @philnik wrote:

Isn't this ABI breaking? Or is it in this case OK for some reason?

It is technically an ABI break. However, the only way I can think of being broken by this is to depend on the exact type of an expression involving valarray (such as decltype(!a && b)). Furthermore, if that were the case, it would almost surely show up as a link-time error, not a runtime error (it would be a runtime error if a function's return type suddenly changed as a result of this change). Given that the current behavior is extremely broken and that valarray doesn't see all that much use in the wild, I believe that while the risk of breakage is non-zero, the risk-to-reward ratio makes it a no brainer to make the change. I'll add a release note, though.

Note that in particular, I am erring towards not introducing an ABI macro for this because it fixes a bug and anyone having used these methods non-trivially is probably broken anyways. Please let me know if you think that's reasonable. Also, pinging libc++ vendors in case they have something to add.

libcxx/include/valarray

926–930

I'll use _LIBCPP_INLINE_VISIBILITY for consistency with the rest of the file.

[ldionne]

Address comments.

[ldionne]

In D125019#3546339, @ldionne wrote:

In D125019#3494305, @philnik wrote:

Isn't this ABI breaking? Or is it in this case OK for some reason?

It is technically an ABI break. However, the only way I can think of being broken by this is to depend on the exact type of an expression involving valarray (such as decltype(!a && b)). [...]

It's also worth noting that the Standard does not mandate that the result of !a & friends be std::valarray itself. Hence, if someone has been relying on that at their ABI boundary, they've been really playing with fire (most likely unknowingly). However, like I said, I think this is unlikely to bite anyone -- one would need an unfortunate combination of auto-deduced return types and unary expressions on valarray at an ABI boundary to get bitten.

[hubert.reinterpretcast]

In D125019#3546352, @ldionne wrote:

In D125019#3546339, @ldionne wrote:

In D125019#3494305, @philnik wrote:

Isn't this ABI breaking? Or is it in this case OK for some reason?

It is technically an ABI break. However, the only way I can think of being broken by this is to depend on the exact type of an expression involving valarray (such as decltype(!a && b)). [...]

It's also worth noting that the Standard does not mandate that the result of !a & friends be std::valarray itself. Hence, if someone has been relying on that at their ABI boundary, they've been really playing with fire (most likely unknowingly). However, like I said, I think this is unlikely to bite anyone -- one would need an unfortunate combination of auto-deduced return types and unary expressions on valarray at an ABI boundary to get bitten.

Wouldn't one be also bitten if they explicitly instantiated an instance of valarray with a program-defined type and made the instantiated member functions available as part of their ABI interface?

[ldionne]

In D125019#3546592, @hubert.reinterpretcast wrote:

Wouldn't one be also bitten if they explicitly instantiated an instance of valarray with a program-defined type and made the instantiated member functions available as part of their ABI interface?

Yes, although that comment can be made whenever we add _LIBCPP_HIDE_FROM_ABI (aka _LIBCPP_INLINE_VISIBILITY) to a method, since that causes the method to be excluded from any explicit instantiation. Concretely, explicitly instantiating standard library types is very brittle (some other gotchas are described here), and fortunately very few people do it. My intuition tells me that the intersection of people using valarray with a user-defined type *and* explicitly instantiating it as part of their ABI is probably empty, but I could be wrong.

IMO, it's a better tradeoff to fix this bug (which will bite unsuspecting users doing normal stuff) and potentially break users who use valarray in technically-valid-yet-niche ways, than to leave the bug here to preserve other more niche use cases. Especially since the breakage should be at link time for most of these valid-but-niche use cases. If someone disagrees, let's have a discussion, I'm entirely open to reconsidering this.

If there's no additional discussion by EOW, I'll assume that folks are OK with this direction.

libcxx/include/valarray

 #ifndef _LIBCPP_CXX03_LANG
     _LIBCPP_INLINE_VISIBILITY
     __val_expr<__indirect_expr<const valarray&> > operator[](valarray<size_t>&& __vs) const;
     _LIBCPP_INLINE_VISIBILITY
     indirect_array<value_type>                    operator[](valarray<size_t>&& __vs);
 #endif // _LIBCPP_CXX03_LANG
 
     // unary operators:
-    valarray       operator+() const;
-    valarray       operator-() const;
-    valarray       operator~() const;
-    valarray<bool> operator!() const;
+    __val_expr<_UnaryOp<__unary_plus<_Tp>, const valarray&> >   operator+() const;
+    __val_expr<_UnaryOp<negate<_Tp>, const valarray&> >         operator-() const;
+    __val_expr<_UnaryOp<__bit_not<_Tp>, const valarray&> >      operator~() const;
+    __val_expr<_UnaryOp<logical_not<_Tp>, const valarray&> >    operator!() const;
 
     // computed assignment:
     _LIBCPP_INLINE_VISIBILITY
     valarray& operator*= (const value_type& __x);
     _LIBCPP_INLINE_VISIBILITY
     valarray& operator/= (const value_type& __x);
     _LIBCPP_INLINE_VISIBILITY
     valarray& operator%= (const value_type& __x);
     _LIBCPP_INLINE_VISIBILITY

 valarray<_Tp>::operator[](valarray<size_t>&& __vs)
 {
     return indirect_array<value_type>(std::move(__vs), *this);
 }
 
 #endif // _LIBCPP_CXX03_LANG
 
 template <class _Tp>
-valarray<_Tp>
+__val_expr<_UnaryOp<__unary_plus<_Tp>, const valarray<_Tp>&> >
 valarray<_Tp>::operator+() const
 {
-    valarray<value_type> __r;
-    size_t __n = size();
-    if (__n)
-    {
-        __r.__begin_ = __r.__end_ = allocator<value_type>().allocate(__n);
-        for (const value_type* __p = __begin_; __n; ++__r.__end_, ++__p, --__n)
-            ::new ((void*)__r.__end_) value_type(+*__p);
-    }
-    return __r;
+    typedef _UnaryOp<__unary_plus<_Tp>, const valarray<_Tp>&> _Op;
+    return __val_expr<_Op>(_Op(__unary_plus<_Tp>(), *this));
 }
 
 template <class _Tp>
-valarray<_Tp>
+__val_expr<_UnaryOp<negate<_Tp>, const valarray<_Tp>&> >
 valarray<_Tp>::operator-() const
 {
-    valarray<value_type> __r;
-    size_t __n = size();
-    if (__n)
-    {
-        __r.__begin_ = __r.__end_ = allocator<value_type>().allocate(__n);
-        for (const value_type* __p = __begin_; __n; ++__r.__end_, ++__p, --__n)
-            ::new ((void*)__r.__end_) value_type(-*__p);
-    }
-    return __r;
+    typedef _UnaryOp<negate<_Tp>, const valarray<_Tp>&> _Op;
+    return __val_expr<_Op>(_Op(negate<_Tp>(), *this));
 }
 
 template <class _Tp>
-valarray<_Tp>
+__val_expr<_UnaryOp<__bit_not<_Tp>, const valarray<_Tp>&> >
 valarray<_Tp>::operator~() const
 {
-    valarray<value_type> __r;
-    size_t __n = size();
-    if (__n)
-    {
-        __r.__begin_ = __r.__end_ = allocator<value_type>().allocate(__n);
-        for (const value_type* __p = __begin_; __n; ++__r.__end_, ++__p, --__n)
-            ::new ((void*)__r.__end_) value_type(~*__p);
-    }
-    return __r;
+    typedef _UnaryOp<__bit_not<_Tp>, const valarray<_Tp>&> _Op;
+    return __val_expr<_Op>(_Op(__bit_not<_Tp>(), *this));
 }
 
 template <class _Tp>
-valarray<bool>
+__val_expr<_UnaryOp<logical_not<_Tp>, const valarray<_Tp>&> >
 valarray<_Tp>::operator!() const
 {
-    valarray<bool> __r;
-    size_t __n = size();
-    if (__n)
-    {
-        __r.__begin_ = __r.__end_ = allocator<bool>().allocate(__n);
-        for (const value_type* __p = __begin_; __n; ++__r.__end_, ++__p, --__n)
-            ::new ((void*)__r.__end_) bool(!*__p);
-    }
-    return __r;
+    typedef _UnaryOp<logical_not<_Tp>, const valarray<_Tp>&> _Op;
+    return __val_expr<_Op>(_Op(logical_not<_Tp>(), *this));
 }
 
 template <class _Tp>
 inline
 valarray<_Tp>&
 valarray<_Tp>::operator*=(const value_type& __x)
 {
     for (value_type* __p = __begin_; __p != __end_; ++__p)

libcxx/test/std/numerics/numarray/template.valarray/valarray.unary/bit_not.pass.cpp

int main(int, char**)
         T a[] = {1, 2, 3, 4, 5};
         const unsigned N = sizeof(a)/sizeof(a[0]);
         std::valarray<T> v(a, N);
         std::valarray<T> v2 = ~(v + v);
         assert(v2.size() == v.size());
         for (std::size_t i = 0; i < v2.size(); ++i)
             assert(v2[i] == ~(2*v[i]));
     }
+    {
+        // Make sure we don't have dangling reference problems with unary expressions
+        std::valarray<int> a = {1, 2, 3};
+        std::valarray<int> b = {1, 2, 3};
+        auto c = ~a * b;
+        assert(c.size() == 3);
+    }
 
     return 0;
 }

libcxx/test/std/numerics/numarray/template.valarray/valarray.unary/negate.pass.cpp

int main(int, char**)
         T a[] = {1, 2.5, 3, 4.25, 5};
         const unsigned N = sizeof(a)/sizeof(a[0]);
         std::valarray<T> v(a, N);
         std::valarray<T> v2 = -(v + v);
         assert(v2.size() == v.size());
         for (std::size_t i = 0; i < v2.size(); ++i)
             assert(v2[i] == -2*v[i]);
     }
+    {
+        // Make sure we don't have dangling reference problems with unary expressions
+        std::valarray<int> a = {1, 2, 3};
+        std::valarray<int> b = {1, 2, 3};
+        auto c = -a * b;
+        assert(c.size() == 3);
+    }
 
     return 0;
 }

libcxx/test/std/numerics/numarray/template.valarray/valarray.unary/not.pass.cpp

int main(int, char**)
         T a[] = {1, 2.5, 3, 4.25, 5};
         const unsigned N = sizeof(a)/sizeof(a[0]);
         std::valarray<T> v(a, N);
         std::valarray<bool> v2 = !(v + v);
         assert(v2.size() == v.size());
         for (std::size_t i = 0; i < v2.size(); ++i)
             assert(v2[i] == !(v[i] + v[i]));
     }
+    {
+        // Make sure we don't have dangling reference problems with unary expressions
+        std::valarray<bool> a = {true, false, true};
+        std::valarray<bool> b = {true, false, true};
+        auto c = !a && b;
+        assert(c.size() == 3);
+    }
 
     return 0;
 }

libcxx/test/std/numerics/numarray/template.valarray/valarray.unary/plus.pass.cpp

int main(int, char**)
         T a[] = {1, 2.5, 3, 4.25, 5};
         const unsigned N = sizeof(a)/sizeof(a[0]);
         std::valarray<T> v(a, N);
         std::valarray<T> v2 = +(v + v);
         assert(v2.size() == v.size());
         for (std::size_t i = 0; i < v2.size(); ++i)
             assert(v2[i] == +2*v[i]);
     }
+    {
+        // Make sure we don't have dangling reference problems with unary expressions
+        std::valarray<int> a = {1, 2, 3};
+        std::valarray<int> b = {1, 2, 3};
+        auto c = +a * b;
+        assert(c.size() == 3);
+    }
 
     return 0;
 }