[Bug c++/99117] New: cannot accumulate std::valarray

[Bug c++/99117] New: cannot accumulate std::valarray

[yasui at icepp dot s.u-tokyo.ac.jp]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

            Bug ID: 99117
           Summary: cannot accumulate std::valarray
           Product: gcc
           Version: 10.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: yasui at icepp dot s.u-tokyo.ac.jp
  Target Milestone: ---

Created attachment 50193
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50193&action=edit
source file

The result of the summation of the std::valarrays by std::accumulate is
improper when compiling with -O2.
It works properly with -O1.

option:
g++ -std=c++2a -Wall -Wextra -O2 source.cpp

In the source file, it calculates (1,1) + (2,2) = (3,3).
The expected result is outputting nothing.
If compiling with -O2, following message is shown:
a.out: source.cpp:10: int main(): Assertion `sum[0]==3' failed.

[Bug middle-end/99117] [9/10/11 Regression] cannot accumulate std::valarray

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
          Component|c++                         |middle-end
   Last reconfirmed|                            |2021-02-16
   Target Milestone|---                         |9.4
     Ever confirmed|0                           |1
      Known to fail|                            |10.2.1, 11.0
             Status|UNCONFIRMED                 |NEW
      Known to work|                            |7.5.0, 8.4.0
           Keywords|                            |needs-reduction, wrong-code
            Summary|cannot accumulate           |[9/10/11 Regression] cannot
                   |std::valarray               |accumulate std::valarray

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.  If I disable SRA it works but I haven't looked closer yet.

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|middle-end                  |libstdc++

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
FRE5 does

   <bb 23> [local count: 2736164397]:
   # ivtmp.143_71 = PHI <ivtmp.143_186(23), ivtmp.143_187(22)>
   _221 = (void *) ivtmp.143_71;
   _124 = MEM[(int * *)_221 + 8B];
-  _115 = MEM[(const int &)_101];
   _162 = MEM[(const int &)_124];
-  _140 = _115 + _162;
-  *_101 = _140;
+  *_101 = _162;
   __p_61 = _101 + 4;
   _127 = _124 + 4;
-  _131 = MEM[(const int &)__p_61];
   _132 = *_127;
-  _133 = _131 + _132;
-  *__p_61 = _133;
+  *__p_61 = _132;
   __p_136 = __p_61 + 4;
   D.69731 ={v} {CLOBBER};
   ivtmp.143_186 = ivtmp.143_71 + 16;
-  if (ivtmp.143_186 != _60)
+  if (_158 != ivtmp.143_186)
     goto <bb 23>; [89.00%]

which looks wrong at a first glance.  Ah, it's OK because we have

  <bb 23> [local count: 2736164397]:
  # ivtmp.143_71 = PHI <ivtmp.143_186(23), ivtmp.143_187(22)>
  # PT = null { D.69770 } (escaped, escaped heap)
  _221 = (void *) ivtmp.143_71;
  # PT = nonlocal escaped null { D.69771 } (escaped, escaped heap)
  _124 = MEM[(int * *)_221 + 8B clique 11 base 0];
  _115 = MEM[(const int &)_101 clique 11 base 0];
  _162 = MEM[(const int &)_124 clique 11 base 0];
  _140 = _115 + _162;
  MEM[(int *)_101 clique 11 base 1] = _140;
  # PT = { D.69772 } (escaped, escaped heap)
  __p_61 = _101 + 4;
  # PT = nonlocal escaped null { D.69771 } (escaped, escaped heap)
  _127 = _124 + 4;
  _131 = MEM[(const int &)__p_61 clique 11 base 0];
  _132 = MEM[(const int &)_127 clique 11 base 0];
  _133 = _131 + _132;
  MEM[(int *)__p_61 clique 11 base 1] = _133;
  # PT = { D.69772 } (escaped, escaped heap)
  __p_136 = __p_61 + 4;
  D.69731 ={v} {CLOBBER};
  ivtmp.143_186 = ivtmp.143_71 + 16;
  if (ivtmp.143_186 != _60)
    goto <bb 23>; [89.00%]
  else
    goto <bb 21>; [11.00%]

so the load is from clique 11, base 0 while the store is to clique 11, base 1
so they do not alias because of some __restrict marking.  So the issue is
likely 
an invalid testcase or bogus annotation in libstdc++

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Regression started with r260318

  tree-ssa-sccvn.c (vn_reference_lookup_3): Improve memset handling.

  2018-05-17  Richard Biener  <rguenther@suse.de>

          * tree-ssa-sccvn.c (vn_reference_lookup_3): Improve memset handling.

          * gcc.dg/tree-ssa/ssa-fre-63.c: New testcase.

    From-SVN: r260318



The testcase can be reduced to:

#include <valarray>
#include <vector>
#include <cassert>

int main()
{
    std::vector<std::valarray<int>> v = {{1,1}, {2,2}};
    std::valarray<int> sum(2);
    for (const auto& e : v)
      sum = sum + e;
    assert(sum[0]==3);
    assert(sum[1]==3);
}

This is what std::accumulate does, but it works as expected if you do:

    for (const auto& e : v)
      sum += e;

I'm not sure if sum = sum + e is valid, due to the aliasing rules of valarray.

(the test passes if you use an array of valarray instead of std::vector, or
pointers to valarrays instead of vector iterators, I'm not sure why)

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The problem is that sum + e returns an expression template that holds
references to its operands, so that the sum is done lazily.

When that expression template result is assigned back to sum it evaluates the
sum of each element, which is basically:

struct valarray {
  int* __restrict__ _M_data;
  size_t _M_size;
};
valarray sum{ new int[2]{1,1}, 2 };
valarray rhs{ new int[2]{2,2}, 2 };
int* p = sum._M_data;
int* e1 = sum._M_data;
int* e2 = rhs._M_data;
for (size_t i = 0; i < sum._M_size; ++i, ++p)
  *p = e1[i] + e2[i];

Where p and e1 alias, but are marked with __restrict__.

Maybe we need to check for such aliasing in __valarray_copy.

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|7.5.0, 8.4.0                |
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
int* p = sum._M_data;
int* e1 = sum._M_data;

If p and e1 aren't __restrict__ too, shouldn't that be fine?  Reading the same
value multiple times shouldn't create new clique each time it is read.
Though, isn't it before optimization a different argument instead?
I'd think even that should be ok, because using __restrict__ from FIELD_DECLs
is only ok if we can prove it is different structs (say pointers to them also
being restrict), but the current aliasing code probably doesn't do that.

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #6 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 16 Feb 2021, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>       Known to work|7.5.0, 8.4.0                |
>                  CC|                            |jakub at gcc dot gnu.org
> 
> --- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> int* p = sum._M_data;
> int* e1 = sum._M_data;
> 
> If p and e1 aren't __restrict__ too, shouldn't that be fine?  Reading the same

But I think it's two different 'sum' in this case.  The actual flow
is quite hard to follow since as usual expression templates rely on
a lot of inlining and most restrict qualifiers only take effect when
on function parameters (there I see 'this' being restrict qualified
quite often).

> value multiple times shouldn't create new clique each time it is read.

It doesn't.  It uses points-to sets to assign cliques so unless the
PTA solution is wrong the clique assignment shouldn't.

> Though, isn't it before optimization a different argument instead?
> I'd think even that should be ok, because using __restrict__ from FIELD_DECLs
> is only ok if we can prove it is different structs (say pointers to them also
> being restrict), but the current aliasing code probably doesn't do that.

See above.

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #5)
> int* p = sum._M_data;
> int* e1 = sum._M_data;
> 
> If p and e1 aren't __restrict__ too, shouldn't that be fine?

p (called __p below) doesn't use __restrict__:

  template<typename _Tp, class _Dom>
    void
    __valarray_copy(const _Expr<_Dom, _Tp>& __e, size_t __n, _Array<_Tp> __a)
    {
      _Tp* __p (__a._M_data);
      for (size_t __i = 0; __i < __n; ++__i, ++__p)
        *__p = __e[__i];
    }

Here __e is the expression template which has two const valarray<int>& members,
so maybe more accurately it's:

struct valarray {
  int* __restrict__ _M_data;
  size_t _M_size;
};
valarray sum{ new int[2]{1,1}, 2 };
valarray rhs{ new int[2]{2,2}, 2 };
int* p = sum._M_data;
const valarray& e1 = sum;
const valarray& e2 = rhs;
for (size_t i = 0; i < sum._M_size; ++i, ++p)
  *p = e1._M_data[i] + e2._M_data[i];

So e1._M_data is marked with __restrict__.

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #8 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 16 Feb 2021, redi at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117
> 
> --- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> (In reply to Jakub Jelinek from comment #5)
> > int* p = sum._M_data;
> > int* e1 = sum._M_data;
> > 
> > If p and e1 aren't __restrict__ too, shouldn't that be fine?
> 
> p (called __p below) doesn't use __restrict__:
> 
>   template<typename _Tp, class _Dom>
>     void
>     __valarray_copy(const _Expr<_Dom, _Tp>& __e, size_t __n, _Array<_Tp> __a)
>     {
>       _Tp* __p (__a._M_data);
>       for (size_t __i = 0; __i < __n; ++__i, ++__p)
>         *__p = __e[__i];
>     }
> 
> Here __e is the expression template which has two const valarray<int>& members,
> so maybe more accurately it's:
> 
> struct valarray {
>   int* __restrict__ _M_data;
>   size_t _M_size;
> };
> valarray sum{ new int[2]{1,1}, 2 };
> valarray rhs{ new int[2]{2,2}, 2 };
> int* p = sum._M_data;
> const valarray& e1 = sum;
> const valarray& e2 = rhs;
> for (size_t i = 0; i < sum._M_size; ++i, ++p)
>   *p = e1._M_data[i] + e2._M_data[i];
> 
> So e1._M_data is marked with __restrict__.

Note the dump shown,

  _124 = MEM[(int * *)_221 + 8B clique 11 base 0];
  _115 = MEM[(const int &)_101 clique 11 base 0];
  _162 = MEM[(const int &)_124 clique 11 base 0];
  _140 = _115 + _162;
  MEM[(int *)_101 clique 11 base 1] = _140;

proves that the destination pointer was __restrict (it got base 1)
and the sources were not (well, PTA conservatively didn't honor
restrict even if it were present).  But they are still disambiguated
against the store since an access through a __restrict qualified
pointer is disambiguated against everything else.

One could say we maybe should honor the appearant must-alias here
(*_101 and *_101 definitely alias), similar to how we'd done that in
case the accesses are to the same decl through TBAA incompatible
types.  As QOI measure.  But that still means the testcase or the
valarray code has a bug somewhere (and will trigger in case the
must-alias isn't as obvious as in the above case).

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Something like this prevents the miscompilation, at the cost of an extra copy:

--- a/libstdc++-v3/include/std/valarray
+++ b/libstdc++-v3/include/std/valarray
@@ -838,7 +838,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
       // 630. arrays of valarray.
       if (_M_size == __e.size())
-       std::__valarray_copy(__e, _M_size, _Array<_Tp>(_M_data));
+       {
+         if (__e()._M_expr1._M_data == _M_data
+             || __e()._M_expr2._M_data == _M_data)
+           *this = static_cast<valarray>(__e);
+         else
+           std::__valarray_copy(__e, _M_size, _Array<_Tp>(_M_data));
+       }
       else
        {
          if (_M_data)

(This isn't usable because it accesses private members so only compiles with
-fno-access-control, but I wanted to see if it helped).

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|needs-reduction             |

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
So the issue is definitely the __restrict__ qualification of the
valarray::__M_data member.  As you noted __valarray_copy is where
points-to analysis sees the restrict qualification when accessing
_Array<_Tp>::_M_data and that causes it to disambiguate against
the accesses via __e.

So I see nothing wrong with points-to analysis handling the
__restrict qualification.  Whether there's a bug in std::valarray
(or in std::accumulate) or whether the testcase is invalid remains
your choice.  I'm keeping it wrong-code classified for now.

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
So is Jakub wrong in comment 5 when he suggests that this should "lose" the
__restrict__ qualification?

      _Tp* __p (__a._M_data);

Is there a better way to drop that qual so that we can assign something to the
elements of __a._M_data that aliases itself?

Presumably this would be guaranteed to work?

      _Tp* __p (__a._M_data);
      for (size_t __i = 0; __i < __n; ++__i, ++__p)
      {
        _Tp __tmp = __e[__i];
        *__p = __tmp;
      }

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #12 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 23 Feb 2021, redi at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117
> 
> --- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> So is Jakub wrong in comment 5 when he suggests that this should "lose" the
> __restrict__ qualification?
> 
>       _Tp* __p (__a._M_data);

Yes.  __p is clearly based on __a._M_data and thus it can be used
to access data pointed to by the restrict qualified __a._M_data
which in turn means we can, if we prove value equivalence, place
the same restrictions on accesses via __p as we can on accesses
via __a._M_data.  It would be way more awkward implementation-wise
if we couldn't do that.

> Is there a better way to drop that qual so that we can assign something to the
> elements of __a._M_data that aliases itself?
> 
> Presumably this would be guaranteed to work?
> 
>       _Tp* __p (__a._M_data);
>       for (size_t __i = 0; __i < __n; ++__i, ++__p)
>       {
>         _Tp __tmp = __e[__i];
>         *__p = __tmp;
>       }

That's no different.  There's no way to lose restrict qualification
but to obfuscate the code in a way to make the compiler not see
the "based-on" relation of __a._M_data and the pointer used to 
access the data.  Passing __a by reference should also run into
limitations of points-to analysis but I guess it would be an ABI
change.

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
So the valarray behavior boils down to

struct _Array { int * __restrict m_data; };

void foo (struct _Array dest, int *src, int n)
{
  for (int i = 0; i < n; ++i)
    dest.m_data[i] = src[i];
}

which we treat similarly:

  _8 = MEM[(int *)_3 clique 1 base 0];
  MEM[(int *)_7 clique 1 base 1] = _8;

and thus we'd vectorize "bogously" for example if src == dest.m_data + 1

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jsm28 at gcc dot gnu.org

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #13)
> So the valarray behavior boils down to
> 
> struct _Array { int * __restrict m_data; };
> 
> void foo (struct _Array dest, int *src, int n)
> {
>   for (int i = 0; i < n; ++i)
>     dest.m_data[i] = src[i];
> }
> 
> which we treat similarly:
> 
>   _8 = MEM[(int *)_3 clique 1 base 0];
>   MEM[(int *)_7 clique 1 base 1] = _8;
> 
> and thus we'd vectorize "bogously" for example if src == dest.m_data + 1

I'd argue that passing such src to the function is invalid (for C, sure, C++
doesn't have restrict).
Because src is not based on dest.m_data in that case.
So, the big question is what passes that pointer that aliases it.

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, based on IRC discussion, the problem is that in:
  template<typename _Tp> template<class _Dom>
    inline valarray<_Tp>&
    valarray<_Tp>::operator=(const _Expr<_Dom, _Tp>& __e)
    {
      // _GLIBCXX_RESOLVE_LIB_DEFECTS
      // 630. arrays of valarray.
      if (_M_size == __e.size())
        std::__valarray_copy(__e, _M_size, _Array<_Tp>(_M_data));

the temporary _Array<_Tp>(_M_data) object's _M_data passed to __valarray_copy
doesn't fully own the object and that __e's operator[] aliases with that.
Can't libstdc++ for cases where such aliasing is possible use an _Array-like
class without the __restrict (i.e. something that wouldn't lie to the compiler
that it fully owns it)?

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So perhaps (totally untested):
--- libstdc++-v3/include/std/valarray.jj        2021-01-04 10:26:02.366967342
+0100
+++ libstdc++-v3/include/std/valarray   2021-02-23 15:16:22.402688841 +0100
@@ -838,7 +838,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
       // 630. arrays of valarray.
       if (_M_size == __e.size())
-       std::__valarray_copy(__e, _M_size, _Array<_Tp>(_M_data));
+       std::__valarray_copy(__e, _M_size, _M_data);
       else
        {
          if (_M_data)
--- libstdc++-v3/include/bits/valarray_array.tcc.jj     2021-01-04
10:26:03.768951467 +0100
+++ libstdc++-v3/include/bits/valarray_array.tcc        2021-02-23
15:15:29.273282041 +0100
@@ -146,6 +146,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        *__p = __e[__i];
     }

+  // Copy n consecutive elements of e into consecutive elements of p.
+  // I.e. p[i] = e[i].  p can alias what e uses.
+  template<typename _Tp, class _Dom>
+    void
+    __valarray_copy(const _Expr<_Dom, _Tp>& __e, size_t __n, _Tp* __p)
+    {
+      for (size_t __i = 0; __i < __n; ++__i, ++__p)
+       *__p = __e[__i];
+    }
+
   // Copy n consecutive elements of e into elements of a using stride
   // s.  I.e., a[0] = e[0], a[s] = e[1], a[2*s] = e[2].
   template<typename _Tp, class _Dom>

[Bug libstdc++/99117] [9/10/11 Regression] cannot accumulate std::valarray

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Pedantically speaking if __e in the end reads _M_data of some std::valarray
that happens to alias __p it will be still violating aliasing, but at least
current GCC won't do anything with that.

[Bug libstdc++/99117] [9/10/11/12 Regression] cannot accumulate std::valarray

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.4                         |9.5

--- Comment #18 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9.4 is being released, retargeting bugs to GCC 9.5.

[Bug libstdc++/99117] [10/11/12/13 Regression] cannot accumulate std::valarray

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.5                         |10.4

--- Comment #19 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9 branch is being closed

[Bug libstdc++/99117] [10/11/12/13 Regression] cannot accumulate std::valarray

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #20 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug libstdc++/99117] [11/12/13/14 Regression] cannot accumulate std::valarray

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #21 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.

[Bug libstdc++/99117] [11/12/13/14 Regression] cannot accumulate std::valarray

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #22 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Instead of adding yet another __valarray_copy overload, we can just not use it:

--- a/libstdc++-v3/include/std/valarray
+++ b/libstdc++-v3/include/std/valarray
@@ -840,7 +840,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
       // 630. arrays of valarray.
       if (_M_size == __e.size())
-       std::__valarray_copy(__e, _M_size, _Array<_Tp>(_M_data));
+       {
+         // Copy manually instead of using __valarray_copy, because __e might
+         // alias _M_data and the _Array param type of __valarray_copy uses
+         // restrict which doesn't allow aliasing.
+         for (size_t __i = 0; __i < _M_size; ++__i)
+           _M_data[__i] = __e[__i];
+       }
       else
        {
          if (_M_data)


That would actually allow us to remove that __valarray_copy overload, because
this is the only caller.

[Bug libstdc++/99117] [11/12/13/14 Regression] cannot accumulate std::valarray

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #23 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:b58f0e5216a3053486e7f1aa96c3f2443b14d630

commit r14-9000-gb58f0e5216a3053486e7f1aa96c3f2443b14d630
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Feb 8 13:59:42 2024 +0000

    libstdc++: Avoid aliasing violation in std::valarray [PR99117]

    The call to __valarray_copy constructs an _Array object to refer to
    this->_M_data but that means that accesses to this->_M_data are through
    a restrict-qualified pointer. This leads to undefined behaviour when
    copying from an _Expr object that actually aliases this->_M_data.

    Replace the call to __valarray_copy with a plain loop. I think this
    removes the only use of that overload of __valarray_copy, so it could
    probably be removed. I haven't done that here.

    libstdc++-v3/ChangeLog:

            PR libstdc++/99117
            * include/std/valarray (valarray::operator=(const _Expr&)):
            Use loop to copy instead of __valarray_copy with _Array.
            * testsuite/26_numerics/valarray/99117.cc: New test.

[Bug libstdc++/99117] [11/12/13 Regression] cannot accumulate std::valarray

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99117

--- Comment #24 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:3a72c717b311ce8093042d927a1f2f2b940a969c

commit r13-8334-g3a72c717b311ce8093042d927a1f2f2b940a969c
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Feb 8 13:59:42 2024 +0000

    libstdc++: Avoid aliasing violation in std::valarray [PR99117]

    The call to __valarray_copy constructs an _Array object to refer to
    this->_M_data but that means that accesses to this->_M_data are through
    a restrict-qualified pointer. This leads to undefined behaviour when
    copying from an _Expr object that actually aliases this->_M_data.

    Replace the call to __valarray_copy with a plain loop. I think this
    removes the only use of that overload of __valarray_copy, so it could
    probably be removed. I haven't done that here.

    libstdc++-v3/ChangeLog:

            PR libstdc++/99117
            * include/std/valarray (valarray::operator=(const _Expr&)):
            Use loop to copy instead of __valarray_copy with _Array.
            * testsuite/26_numerics/valarray/99117.cc: New test.

    (cherry picked from commit b58f0e5216a3053486e7f1aa96c3f2443b14d630)