Re: [[patch] valarray (was Re: valarray_copy implementation question)

[Gabriel Dos Reis <gdr at integrable-solutions dot net>]

On Wed, Nov 25, 2009 at 8:42 AM, Christian BRUEL <christian.bruel@st.com> wrote:
> Hello Gabriel, Paolo,
>
> I was able to get a segfault on x86 using the mask_array<_Tp> operator, so
> it's more than just _valarray_copy (case attached).
>
> Looking at the problem more precisely, the interface is expecting to loop on
> the number of True elements, not all the mask array. So It's not a matter of
> adding an additional check.
>
> Fixing the interface to use the correct size solves all the invalid memory
> accesses on mask_array uses for valarrays. Tested for regressions on trunk.
> All testcases discussed in this thread pass.
>
> OK for commit per your review, comments ?

See my comments below.  I need to understand better what you
are doing.

>
> Many thanks
>
> 2009-11-23 ?Christian Bruel ?<christian.bruel@st.com>
>
> ? ? ? ?* include/bits/mask_array.h (mask_array<_Tp>::operator=): Use _M_sz.
> ? ? ? ?(mask_array<_Tp>::operator _Op##): Likewise.
> ? ? ? ?* testsuite/26_numerics/valarray/mask_array.cc: New test.
>
>
> Gabriel Dos Reis wrote:
>>
>> On Tue, Nov 17, 2009 at 8:01 AM, Paolo Carlini <paolo.carlini@oracle.com>
>> wrote:
>>>
>>> Paolo Carlini wrote:
>>>>
>>>> Thanks Gaby, because in fact I'm finding the issue quite substantive,
>>>> isn't just about some strange corner cases: it's really about copying
>>>> all the source elements or just picking some depending on the
>>>> destination mask; it's about checking for overruns of the destination or
>>>> not. A big difference.
>>>>
>>> To better explain what I mean, the below snippet, perfectly legal as far
>>> as I can see and not triggering any memory error, would fail the third
>>> assert at runtime if ?we do implement the simple change suggested by
>>> Christian. Note, the alternate implementation passes the testcase, as
>>> current libstdc++ does.
>>>
>>> I'm wondering if shouldn't just keep the current implementation
>>> unchanged, besides adding to it a check that the destination is not
>>> overrun. But that is also debatable, given the usual
>>> performance-dictated design choices for valarray...
>>>
>>> Paolo.
>>>
>>> ///////////////////
>>>
>>> #include <valarray>
>>> #include <cassert>
>>>
>>> int main()
>>> {
>>> ?int n = 6;
>>>
>>> ?std::valarray<int> ?dest (n);
>>> ?std::valarray<bool> b (false, n);
>>> ?b[2] = b[4] = b[5] = true;
>>>
>>> ?std::valarray<int> ?src (3);
>>> ?src[0] = -1;
>>> ?src[1] = -2;
>>> ?src[2] = -3;
>>>
>>> ?dest[b] = src; // 0
>>>
>>> ?assert ( dest[0] == 0 );
>>> ?assert ( dest[1] == 0 );
>>> ?assert ( dest[2] == -1 );
>>> ?assert ( dest[3] == 0 );
>>> ?assert ( dest[4] == -2 );
>>> ?assert ( dest[5] == -3 );
>>> }
>>
>> This is a good test -- and capture pretty well the issue.
>>
>>>
>>
>
> Index: libstdc++-v3/include/bits/mask_array.h
> ===================================================================
> --- libstdc++-v3/include/bits/mask_array.h ? ? ?(revision 154332)
> +++ libstdc++-v3/include/bits/mask_array.h ? ? ?(working copy)
> @@ -147,8 +147,7 @@
> ? ? inline mask_array<_Tp>&
> ? ? mask_array<_Tp>::operator=(const mask_array<_Tp>& __a)
> ? ? {
> - ? ? ?std::__valarray_copy(__a._M_array, __a._M_mask,
> - ? ? ? ? ? ? ? ? ? ? ? ? ?_M_sz, _M_array, _M_mask);
> + ? ? ?std::__valarray_copy(__a._M_array, __a._M_mask, _M_sz, _M_array,
> _M_mask);


Why is this change necessary?


> ? ? ? return *this;
> ? ? }
>
> @@ -160,30 +159,29 @@
> ? template<typename _Tp>
> ? ? inline void
> ? ? mask_array<_Tp>::operator=(const valarray<_Tp>& __v) const
> - ? ?{ std::__valarray_copy(_Array<_Tp>(__v), __v.size(), _M_array,
> _M_mask); }
> + ? ?{ std::__valarray_copy(_Array<_Tp>(__v), _M_sz, _M_array, _M_mask); }

I do not understand this change.  Why change from __v.size() to _M_sz?

>
> ? template<typename _Tp>
> ? ? template<class _Ex>
> ? ? ? inline void
> ? ? ? mask_array<_Tp>::operator=(const _Expr<_Ex, _Tp>& __e) const
> - ? ? ?{ std::__valarray_copy(__e, __e.size(), _M_array, _M_mask); }
> + ? ? ?{ std::__valarray_copy(__e, _M_sz, _M_array, _M_mask); }

Idem.

>
> ?#undef _DEFINE_VALARRAY_OPERATOR
> -#define _DEFINE_VALARRAY_OPERATOR(_Op, _Name) ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ?template<typename _Tp> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ?inline void
> ? ?\
> - ? ?mask_array<_Tp>::operator _Op##=(const valarray<_Tp>& __v) const ? \
> - ? ?{ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ?_Array_augmented_##_Name(_M_array, _M_mask, ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_Array<_Tp>(__v), __v.size()); ? ? ? ? ? \
> - ? ?} ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ?template<typename _Tp> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ?template<class _Dom> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ? ?inline void ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ?mask_array<_Tp>::operator _Op##=(const _Expr<_Dom, _Tp>& __e) const\
> - ? ? ?{
> ? ?\
> - ? ? ? _Array_augmented_##_Name(_M_array, _M_mask, __e, __e.size()); ? \
> +#define _DEFINE_VALARRAY_OPERATOR(_Op, _Name)
> ?\
> + ?template<typename _Tp>
> \
> + ? ?inline void
> ? ? ? ?\
> + ? ?mask_array<_Tp>::operator _Op##=(const valarray<_Tp>& __v) const
> \
> + ? ?{
> ?\
> + ? ? ?_Array_augmented_##_Name(_M_array, _M_mask, _Array<_Tp>(__v), _M_sz);
> \
> + ? ?}
> ?\
> +
> \
> + ?template<typename _Tp>
> ?\
> + ? ?template<class _Dom>
> \
> + ? ? ?inline void
> ?\
> + ? ? ?mask_array<_Tp>::operator _Op##=(const _Expr<_Dom, _Tp>& __e) const
> \
> + ? ? ?{
> ? ? ? ?\
> + ? ? ? _Array_augmented_##_Name(_M_array, _M_mask, __e, _M_sz);
> ?\
> ? ? ? }
>
> ?_DEFINE_VALARRAY_OPERATOR(*, __multiplies)
>
> // Copyright (C) 2006, 2009 Free Software Foundation, Inc.
> //
> // 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 3, 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 COPYING3. ?If not see
> // <http://www.gnu.org/licenses/>.
>
> // segfaults on x86 and infinite loop on SH.
>
> #include <valarray>
> #include <testsuite_hooks.h>
>
> using namespace std;
>
> int main ()
> {
> ?int n = 10;
>
> ?valarray<int> myarray (1, n);
>
> ?valarray<bool> mymask (n);
> ?myarray[mymask] += valarray<int>(3,5);

I'm not sure I understand this line.
The way I read it, the left hand side is not of length 5.
Is that correct?

>
> ?for (int i = 0; i < n; i++)
> ? ?VERIFY (myarray[i] == 1);
>
> ?mymask[0] = 1;
> ?mymask[9] = 1;
>
> ?myarray[mymask] += valarray<int>(3,5);
>
> ?VERIFY (myarray[0] == 4);
> ?VERIFY (myarray[9] == 4);
> ?VERIFY (myarray[1] == 1);
> ?VERIFY (myarray[2] == 1);
> ?VERIFY (myarray[3] == 1);
> ?VERIFY (myarray[4] == 1);
> ?VERIFY (myarray[5] == 1);
> ?VERIFY (myarray[6] == 1);
> ?VERIFY (myarray[7] == 1);
> ?VERIFY (myarray[8] == 1);
>
> ?return 0;
> }
>
>