[Patch] libstdc++/28277 valarray bits

[Paolo Carlini <pcarlini at suse dot de>]

Hi all, hi Gaby,

I'm trying to fix the valarray bits of this issue, which seem 
selfcontained. As you can see, currently valarray::shift and 
valarray::cshift go through an unbounded __builtin_alloca in order to 
construct in it the shifted valarray and then construct the result from 
that memory. A lot of work and memory, instead of constructing the 
result in place. Apparently, that is done in order to avoid leaking the 
raw memory allocated by __valarray_get_storage in case either 
__valarray_default_construct or __valarray_copy_construct throw.

However, it's known (and reminded many times in bits/valarray_array.h) 
that valarray is not required to be exception safe (for performance 
reasons, obviously) and, for example, *all* our constructors leak the 
raw memory in case one of the above helpers throws (+ the individual 
elements are not destructed, in other terms we don't use something like 
uninitialized_copy and uninitialized_fill, again see 
bits/valarray_array.h). Therefore, taking into account the general 
design of our implementation I think it's correct to have shift and 
cshift completely similar to the constructors, that is, efficient, using 
well the memory, but not trying to be partially exception safe (at best 
only partially, of course, because the issue with individual elements 
not destructed currently affects shift and csfift too, exactly like the 
constructors).

Gaby, makes sense to you?

Tested x86-linux.

Paolo.

///////////////

2006-07-16  Paolo Carlini  <pcarlini@suse.de>

	PR libstdc++/28277 (partial: valarray bits)
	* include/std/std_valarray.h (valarray<>::shift(int),
	valarray<>::cshift(int)): Avoid __builtin_alloca with no limit.
	* testsuite/26_numerics/valarray/28277.cc: New.

Index: include/std/std_valarray.h
===================================================================
--- include/std/std_valarray.h	(revision 115484)
+++ include/std/std_valarray.h	(working copy)
@@ -778,63 +778,78 @@
       return std::__valarray_sum(_M_data, _M_data + _M_size);
     }
 
-  template <class _Tp>
+  template<class _Tp>
      inline valarray<_Tp>
      valarray<_Tp>::shift(int __n) const
      {
-       _Tp* const __a = static_cast<_Tp*>
-         (__builtin_alloca(sizeof(_Tp) * _M_size));
+       valarray<_Tp> __ret;
+       _Tp* __restrict__ __tmp_M_data =
+	 std::__valarray_get_storage<_Tp>(_M_size);
+
        if (__n == 0)                          // no shift
-         std::__valarray_copy_construct(_M_data, _M_data + _M_size, __a);
+         std::__valarray_copy_construct(_M_data, _M_data + _M_size,
+					__tmp_M_data);
        else if (__n > 0)         // __n > 0: shift left
          {                 
            if (size_t(__n) > _M_size)
-             std::__valarray_default_construct(__a, __a + __n);
+             std::__valarray_default_construct(__tmp_M_data,
+					       __tmp_M_data + __n);
            else
              {
                std::__valarray_copy_construct(_M_data + __n,
-					      _M_data + _M_size, __a);
-               std::__valarray_default_construct(__a + _M_size -__n,
-						 __a + _M_size);
+					      _M_data + _M_size,
+					      __tmp_M_data);
+               std::__valarray_default_construct(__tmp_M_data + _M_size - __n,
+						 __tmp_M_data + _M_size);
              }
          }
        else                        // __n < 0: shift right
          {                          
-           std::__valarray_copy_construct (_M_data, _M_data + _M_size + __n,
-					   __a - __n);
-           std::__valarray_default_construct(__a, __a - __n);
+           std::__valarray_copy_construct(_M_data, _M_data + _M_size + __n,
+					  __tmp_M_data - __n);
+           std::__valarray_default_construct(__tmp_M_data,
+					     __tmp_M_data - __n);
          }
-       return valarray<_Tp>(__a, _M_size);
+
+       __ret._M_size = _M_size;
+       __ret._M_data = __tmp_M_data;
+       return __ret;
      }
 
-  template <class _Tp>
+  template<class _Tp>
      inline valarray<_Tp>
-     valarray<_Tp>::cshift (int __n) const
+     valarray<_Tp>::cshift(int __n) const
      {
-       _Tp* const __a = static_cast<_Tp*>
-         (__builtin_alloca (sizeof(_Tp) * _M_size));
+       valarray<_Tp> __ret;
+       _Tp* __restrict__ __tmp_M_data =
+	 std::__valarray_get_storage<_Tp>(_M_size);
+
        if (__n == 0)               // no cshift
-         std::__valarray_copy_construct(_M_data, _M_data + _M_size, __a);
+         std::__valarray_copy_construct(_M_data, _M_data + _M_size,
+					__tmp_M_data);
        else if (__n > 0)           // cshift left
          {               
            std::__valarray_copy_construct(_M_data, _M_data + __n,
-					  __a + _M_size - __n);
+					  __tmp_M_data + _M_size - __n);
            std::__valarray_copy_construct(_M_data + __n, _M_data + _M_size,
-					  __a);
+					  __tmp_M_data);
          }
        else                        // cshift right
          {                       
            std::__valarray_copy_construct
-             (_M_data + _M_size + __n, _M_data + _M_size, __a);
+             (_M_data + _M_size + __n, _M_data + _M_size, __tmp_M_data);
            std::__valarray_copy_construct
-             (_M_data, _M_data + _M_size+__n, __a - __n);
+             (_M_data, _M_data + _M_size + __n, __tmp_M_data - __n);
          }
-       return valarray<_Tp>(__a, _M_size);
+
+       __ret._M_size = _M_size;
+       __ret._M_data = __tmp_M_data;
+       return __ret;
      }
 
-  template <class _Tp>
+  template<class _Tp>
     inline void
-    valarray<_Tp>::resize (size_t __n, _Tp __c)
+    valarray<_Tp>::resize(size_t __n, _Tp __c)
     {
       // This complication is so to make valarray<valarray<T> > work
       // even though it is not required by the standard.  Nobody should
Index: testsuite/26_numerics/valarray/28277.cc
===================================================================
--- testsuite/26_numerics/valarray/28277.cc	(revision 0)
+++ testsuite/26_numerics/valarray/28277.cc	(revision 0)
@@ -0,0 +1,42 @@
+// 2006-07-15  Paolo Carlini  <pcarlini@suse.de>
+
+// Copyright (C) 2006 Free Software Foundation
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 2, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING.  If not, write to the Free
+// Software Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301,
+// USA.
+
+#include <valarray>
+#include <testsuite_hooks.h>
+
+// libstdc++/28277
+void test01()
+{
+  bool test __attribute__((unused)) = true;
+
+  std::valarray<int> v1(5000000);
+
+  v1.shift(1);
+  VERIFY( v1[0] == 0 );
+
+  v1.cshift(1);
+  VERIFY( v1[v1.size() - 1] == 0 );
+}
+
+int main()
+{
+  test01();
+  return 0;
+}