Re: [BUG] valarray_copy, with mask_array, libstdc++-v3

[Gabriel Dos Reis <gdr at codesourcery dot com>]

Luke Kenneth Casson Leighton <lkcl@samba-tng.org> writes:

[...]

Given this declaration:

| > | valarray<int> j;
| > 
| > "j" is empty.
|  
|  or, j is not actually declared here: see below for better
|  example, or also see other earlier post.
| 
| 
| > | i = 9;
| > 
| > All elements are set to 9.
| > 
| > | j = i[b];
| > 
| > What are you expecting from this one?
| 
|  j[0] = 0
|  j[1] = 9
|  j[2] = 9
|  j[3] = 0

This cannot happen.

| > | should create an array (j) of two items.
| 
|  ??? did i write this??  :)  i don't remember writing that! :)
| 
| 
| > No.  There is no such requirement.  Actually that is undefined behaviour.
| 
|  
|   .... ah... um... well, it's not, but it works, and
|   declaring j() has confused the issue, and the "undefined"
|   behaviour _happens_ to be completely irrelevant, as
|   people would find out if they actually bothered to try
|   the example - even the badly written one i sent in -

Please, stop accusing those who take you report in consideration and
study it for not "bothering to try the example".  If you were to
continue on that tone, this will be my last message on this issue.

|   because you still get the buffer over-run even before
|   the assignment of j (undefined) to i[b] (which _is_ defined).

I'm having trouble to see exactly what is your point.  The snippet you
provided featured an undefined behaviour.  As such anything can happen
in any order.

[...]

| > The example you gave features an undefined
| > behaviour. 
| 
|  thank you - i am pleased to be asked a useful question, instead
|  of fobbed off with stupid comments, and am therefore more than
|  happy to answer.
| 
|  i clarified in a later post, i'll check your other message
|  before replying....
| 
|  okay.  let's re-do this example:
| 
|  valarray<bool> b(4);
| 
|  b[1] = true;
|  b[2] = true;
| 
|  valarray<int>  i(4);
| 
|  i = 9;
| 
|  valarray<int> j(i[b]);
| 
|  expected behaviour: 
| 
|  j[0] = 0
|  j[1] = 9
|  j[2] = 9
|  j[3] = 0

That is not correct.  The expected behaviour should be:

   j.size() == 2;
   j[0] == 9;
   j[1] == 9;

|  ACTUAL behaviour:
| 
|  buffer over-run beyond end of j, looking in b[5], b[6], b[7].... for
|  (j.size() - 2) "true" values, over-writing into j[7,8,9,....]
|  etc with i[7,8,9....].

Which version of GCC are you using.  The behaviour you're describing
corresponds to none one can have with current source.  I run the
following program based on the testcase you provided:

    #include <valarray>
    #include <iostream>
    #include <algorithm>

    int main()
    {
	using namespace std;
	valarray<bool> b(4);
	b[1] = true;
	b[2] = true;

	valarray<int> i(4);
	i = 9;

	valarray<int> j(i[b]);

	cout << boolalpha;
	copy(&i[0], &i[0] + 4, ostream_iterator<int>(cout, "     "));
	cout << '\n';
	copy(&b[0], &b[0] + 4, ostream_iterator<bool>(cout, " "));
	cout << '\n';
	copy(&j[0], &j[0] + j.size(), ostream_iterator<int>(cout, "     "));
	cout << endl;

    }

The output is:

     9     9     9     9     
     false true true false 
     9     9     

which matches the expected behaviour.

|  with the patch i submitted, the expected - and most importantly
|  DEFINITELY DEFINED behaviour - occurs.

I really doubt the patch you sent does what is mandated.  Here is a
detailed transcription of what is happening.

1)
   valarray<bool> b(4);

creates a valarray<bool> of length 4, all elements of which are
initialized to false.

2)
  b[1] = true;
  b[2] = true;

set the second and third elements to true.

3)
  valarray<int>  i(4);

creates a valarray<int> of length 4, all elements of which are
initialized to 0.

4)
  i = 9;

sets all elements in "i" to 9, as instructed by:

      template<typename _Tp>
      inline valarray<_Tp>&
      valarray<_Tp>::operator= (const _Tp& __t)
      {
	  __valarray_fill (_M_data, _M_size, __t);
	  return *this;
      }

5)
  valarray<int> j(i[b]);

does two things:

  a) constructs a temporary "tmp" mask_array<int> of length 2 as
     instructed by

  template<typename _Tp>
  inline mask_array<_Tp>
  valarray<_Tp>::operator[] (const valarray<bool>& __m)
  {
      size_t __s (0);
      size_t __e (__m.size ());
      for (size_t __i=0; __i<__e; ++__i)
          if (__m[__i]) ++__s;
      return mask_array<_Tp> (_Array<_Tp>(_M_data), __s, _Array<bool> (__m));
  }

  b) Next, "j" is constructed by

  template<typename _Tp>
  inline valarray<_Tp>::valarray (const mask_array<_Tp>& __ma)
    : _M_size(__ma._M_sz), _M_data(__valarray_get_storage<_Tp>(__ma._M_sz))
  {
    __valarray_copy
      (__ma._M_array, __ma._M_mask, _Array<_Tp>(_M_data), _M_size);
  }

  Here, the __valarray_copy() used is:

   export template<typename _Tp>
   void
   __valarray_copy (_Array<_Tp> __a, _Array<bool> __m, _Array<_Tp> __b, size_t __n)
   {
       _Tp* __p (__a._M_data);
       bool* __ok (__m._M_data);
       for (_Tp* __q=__b._M_data; __q<__b._M_data+__n; ++__q, ++__ok, ++__p) {
	   while (! *__ok) {
	       ++__ok;
	       ++__p;
	   }
	   *__q = *__p;
       }
   }


  which does the right thing.  In all those chains, I can't see where
  the odd behaviour your describing happens.


Thanks,

-- Gaby