[[patch] valarray (was Re: valarray_copy implementation question)
Gabriel Dos Reis
gdr@integrable-solutions.net
Wed Nov 25 17:25:00 GMT 2009
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;
> }
>
>
More information about the Libstdc++
mailing list