[Bug libstdc++/39480] New: generated memcpy causes trouble in assignment

[Bug libstdc++/39480] New: generated memcpy causes trouble in assignment

[fpbeekhof at gmail dot com]

The following code is unsafe, and I think it's not my fault.

$ g++ -Wall -g testIterSwap.cc ; valgrind -q ./a.out 
==1674== Source and destination overlap in memcpy(0x7FF000500, 0x7FF000500, 20)
==1674==    at 0x4C2508B: memcpy (mc_replace_strmem.c:402)
==1674==    by 0x4008D2: void swap_from_glibcxx<HarrisPoint<float, int>
>(HarrisPoint<float, int>&, HarrisPoint<float, int>&) (testIterSwap.cc:22)
==1674==    by 0x400793: main (testIterSwap.cc:68)

It would appear that the auto-generated operator=() uses a memcpy() which is
not safe. The operator=() that is provided below solves the problem.

-------- cut here ------------

#include <tr1/array>
// #include <algorithm>


 /**
   *  @brief Swaps two values.
   *  @param  a  A thing of arbitrary type.
   *  @param  b  Another thing of arbitrary type.
   *  @return   Nothing.
   *
   *  This is the simple classic generic implementation.  It will work on
   *  any type which has a copy constructor and an assignment operator.
  */
  template<typename _Tp>
    inline void
    swap_from_glibcxx(_Tp& __a, _Tp& __b)
    {
      // concept requirements
//      __glibcxx_function_requires(_SGIAssignableConcept<_Tp>)

      _Tp __tmp = __a;
      __a = __b;
      __b = __tmp;
    }



template <typename T>
struct Point2D :  public std::tr1::array<T, 2>
{
        template <typename U, typename V>
        Point2D(const U x, const V y)
        {
                (*this)[0] = x;
                (*this)[1] = y;
        }
};

template <typename T, typename U>
class FeaturePoint : public Point2D<U>
{
        public:
                typedef T data_type;

                FeaturePoint(const U h, const U w) : Point2D<U>(h,w) {}
};

template <typename T, typename U>
class HarrisPoint : public FeaturePoint<T, U>
{
        public:
                typedef T data_type;

                HarrisPoint(const U h, const U w, const T value) :
                        FeaturePoint<T, U>(h, w), value_(value) { }
/*
                // THIS FIXES IT.
                HarrisPoint &operator=(const HarrisPoint &rhs)
                {
                        std::memmove(this, &rhs, sizeof(HarrisPoint));
                        return *this;
                }
*/
        private:
                T value_;
};

int main()
{
        HarrisPoint<float, int> a(1, 2, 3.0f);
//      std::tr1::array<int, 2> a; a[0] = 3; a[1] = 5; // fine

        a = a; // fine
//      std::swap(a, a);
        swap_from_glibcxx(a, a);

        return 0;
}


-- 
           Summary: generated memcpy causes trouble in assignment
           Product: gcc
           Version: 4.2.4
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: fpbeekhof at gmail dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[rguenth at gcc dot gnu dot org]

------- Comment #1 from rguenth at gcc dot gnu dot org  2009-03-17 10:59 -------
calling memcpy with exactly overlapping operands is safe (obviously).  Thus
this is a bug in valgrind.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[paolo dot carlini at oracle dot com]

------- Comment #2 from paolo dot carlini at oracle dot com  2009-03-17 11:04 -------
by the way, I cannot reproduce it with valgrind 3.3.1 and either mainline or
current 4_3-branch or 4_2-branch.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[fpbeekhof at gmail dot com]

------- Comment #3 from fpbeekhof at gmail dot com  2009-03-17 11:13 -------
Subject: Re:  generated memcpy causes trouble in assignment



rguenth at gcc dot gnu dot org wrote:
> ------- Comment #1 from rguenth at gcc dot gnu dot org  2009-03-17 10:59 -------
> calling memcpy with exactly overlapping operands is safe (obviously).  Thus
> this is a bug in valgrind.
> 
> 

It's not so obvious actually, I find the following text in different
places: "If the source and destination overlap, the behavior of memcpy
is undefined."

I guess this is a line from the standard, so when calling memcpy with
exactly overlapping operands, it is not officially safe. Thus, if it is
safe in libstd++ then that's nice, but it's not a bug in valgrind.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[fpbeekhof at gmail dot com]

------- Comment #4 from fpbeekhof at gmail dot com  2009-03-17 11:15 -------
Subject: Re:  generated memcpy causes trouble in assignment



paolo dot carlini at oracle dot com wrote:
> ------- Comment #2 from paolo dot carlini at oracle dot com  2009-03-17 11:04 -------
> by the way, I cannot reproduce it with valgrind 3.3.1 and either mainline or
> current 4_3-branch or 4_2-branch.
> 
> 

If it's any help, this is Ubuntu 8.04

$ valgrind --version
valgrind-3.3.0-Debian
$ gcc  -v
Using built-in specs.
Target: x86_64-linux-gnu
Configured with: ../src/configure -v
--enable-languages=c,c++,fortran,objc,obj-c++,treelang --prefix=/usr
--enable-shared --with-system-zlib --libexecdir=/usr/lib
--without-included-gettext --enable-threads=posix --enable-nls
--with-gxx-include-dir=/usr/include/c++/4.2 --program-suffix=-4.2
--enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc
--enable-mpfr --enable-checking=release --build=x86_64-linux-gnu
--host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.2.4 (Ubuntu 4.2.4-1ubuntu3)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[paolo dot carlini at oracle dot com]

------- Comment #5 from paolo dot carlini at oracle dot com  2009-03-17 11:28 -------
By the way, I don't see what could be possibly wrong in libstdc++: can you
please tell us which specific line of code you are suspecting? Certainly, not
the entire swap implementation, which is definitely the correct one, no other
options.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[falk at debian dot org]

------- Comment #6 from falk at debian dot org  2009-03-17 12:10 -------
(In reply to comment #1)
> calling memcpy with exactly overlapping operands is safe (obviously).

It's not safe at all. A memcpy implementation might use a "prefetch with write
intent" instruction that allocates a cache line to a memory location without
actually fetching it, such as wh64 on Alpha, which is actually being used by
glibc.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[fpbeekhof at gmail dot com]

------- Comment #7 from fpbeekhof at gmail dot com  2009-03-17 12:21 -------
Subject: Re:  generated memcpy causes trouble in assignment



paolo dot carlini at oracle dot com wrote:
> ------- Comment #5 from paolo dot carlini at oracle dot com  2009-03-17 11:28 -------
> By the way, I don't see what could be possibly wrong in libstdc++: can you
> please tell us which specific line of code you are suspecting? Certainly, not
> the entire swap implementation, which is definitely the correct one, no other
> options.
> 
> 

Well, a specific line of code I cannot give, because this bug is in the
operator=() that is generated by the compiler. Given comment #6 by Falk,
 I think we can safely say that there is an actual bug here.

Perhaps libstdc++ is wrong place to submit this bug, but I originally
encountered it whilst using iter_swap() from <algorithm> and assumed
that to be the cause of the problem.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[paolo dot carlini at oracle dot com]

------- Comment #8 from paolo dot carlini at oracle dot com  2009-03-17 12:33 -------
Yes, if there is a bug, certainly it is not in libstdc++. Also, please find
somebody able to reproduce it, I can't.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[fpbeekhof at gmail dot com]

------- Comment #9 from fpbeekhof at gmail dot com  2009-03-17 13:11 -------
Subject: Re:  generated memcpy causes trouble in assignment



paolo dot carlini at oracle dot com wrote:
> ------- Comment #8 from paolo dot carlini at oracle dot com  2009-03-17 12:33 -------
> Yes, if there is a bug, certainly it is not in libstdc++. Also, please find
> somebody able to reproduce it, I can't.

Someone should check whether the part that generates operator=()
generates a memcpy(), in which case there is a bug because that cannot
handle assignment to self, which implies overlapping memory areas, which
in turn, as stated by the standard and confirmed by the existence of
certain pre-fetch instructions, leads to undefined behavior.

This reasoning is valid even if anyone, at any particular time, is able
not to see the symptoms.

If that bug is there, a possible fix would be to replace memcpy by
memmove, as I have suggested.

I don't know in what component gcc generates its operator=(), so if
someone can tell me I'll assign it to that.

Second thing to do is to re-open the bug because it is very real unless
it has been fixed already in a later version.

Third thing is to increase the priority, because this is likely to
affect probably almost all C++ programs, and make all of their behavior
undefined.

I'm not familiar with gcc policies, and maybe someone can check if this
has already been fixed, so I'll wait a few moments before doing these 3
things, to see if there are any reactions.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[paolo dot carlini at oracle dot com]

------- Comment #10 from paolo dot carlini at oracle dot com  2009-03-17 13:15 -------
The component would be c++, because that is a C++ thing.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[fpbeekhof at gmail dot com]

------- Comment #11 from fpbeekhof at gmail dot com  2009-03-17 13:19 -------
Subject: Re:  generated memcpy causes trouble in assignment



paolo dot carlini at oracle dot com wrote:
> ------- Comment #10 from paolo dot carlini at oracle dot com  2009-03-17 13:15 -------
> The component would be c++, because that is a C++ thing.
> 
> 

Ok, that's good to know. Any suggestion what sort of priority I should
give to this ?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[paolo dot carlini at oracle dot com]

------- Comment #12 from paolo dot carlini at oracle dot com  2009-03-17 13:25 -------
Apparently, mainline and 4_3-branch do not call memcpy, expand the operator
inline.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[paolo dot carlini at oracle dot com]

------- Comment #13 from paolo dot carlini at oracle dot com  2009-03-17 13:26 -------
If it affects only 4_2-branch would be zero, because it's no longer maintained
;)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[fpbeekhof at gmail dot com]

------- Comment #14 from fpbeekhof at gmail dot com  2009-03-17 13:28 -------
Subject: Re:  generated memcpy causes trouble in assignment



paolo dot carlini at oracle dot com wrote:
> ------- Comment #13 from paolo dot carlini at oracle dot com  2009-03-17 13:26 -------
> If it affects only 4_2-branch would be zero, because it's no longer maintained
> ;)

Well, then I'd better contact the ubuntu guys, because 4.2.4 is still
the main compiler on their latest LTS release.

Thanks for the help.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[paolo dot carlini at oracle dot com]

------- Comment #15 from paolo dot carlini at oracle dot com  2009-03-17 13:32 -------
To be clear, I'm not sure this is invalid, I agree with you that we should
understand the issue in better detail. I'll make sure we do the additional
investigation.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug libstdc++/39480] generated memcpy causes trouble in assignment

[paolo dot carlini at oracle dot com]

------- Comment #16 from paolo dot carlini at oracle dot com  2009-03-17 13:34 -------
Correction: technically 4_2-branch is still open, but no further releases are
expected and will be definitely closed as soon as the 4_4-branch is created
(very soon)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[paolo dot carlini at oracle dot com]

------- Comment #17 from paolo dot carlini at oracle dot com  2009-03-17 13:35 -------
Jason, could you maybe double check this one too?


-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu dot org
          Component|libstdc++                   |c++


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[jason at gcc dot gnu dot org]

------- Comment #18 from jason at gcc dot gnu dot org  2009-03-18 03:29 -------
I can't imagine an implementation of memcpy that would break when the two
addresses are the same, it just doesn't work if one is offset but overlapping. 
So the valgrind warning is pedantically correct, but I think it doesn't
indicate a real safety issue.

I can reproduce the generated memcpy with trunk if I use the preprocessor
output from 4.2 adjusted for the changes in __is_pod/__is_empty.  It doesn't
appear when just compiling the testcase because the contents of HarrisPoint are
completely different (was there library ABI breakage between 4.2 and 4.3?)

The call comes from the front end in build_over_call, and that code is still
present.

However, it seems that emit_block_move_hints will also use memcpy if
move_by_pieces and movmem aren't suitable, so this is more than just a C++
issue.  For instance, if I comment out the emit_block_move_via_movemem case,
this C testcase uses memcpy, and it has no way to know whether or not the two
pointers are the same.

struct A
{
  int i[200];
};

struct A *p1, *p2;
void f()
{
  *p1 = *p2;
}


-- 

jason at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[falk at debian dot org]

------- Comment #19 from falk at debian dot org  2009-03-18 09:06 -------
(In reply to comment #18)
> I can't imagine an implementation of memcpy that would break when the two
> addresses are the same, it just doesn't work if one is offset but overlapping. 
> So the valgrind warning is pedantically correct, but I think it doesn't
> indicate a real safety issue.

As I mentioned in comment #6, it could go wrong if memcpy uses "cache prefetch
with write intent" instructions.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[jakub at gcc dot gnu dot org]

------- Comment #20 from jakub at gcc dot gnu dot org  2009-03-18 09:10 -------
So let's use memmove on alpha and let targets with sane memcpy not suffer just
because of one dead architecture.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[rguenth at gcc dot gnu dot org]

------- Comment #21 from rguenth at gcc dot gnu dot org  2009-03-18 09:47 -------
For

struct A
{
  int i[200];
};

struct A *p1, *p2;
void f()
{
  *p1 = *p2;
}

we are sure that either p1 == p2 or *p1 and *p2 do not overlap because
otherwise there would be an alias violation.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[fpbeekhof at gmail dot com]

------- Comment #22 from fpbeekhof at gmail dot com  2009-03-18 09:56 -------
Subject: Re:  generated memcpy causes trouble in assignment



jakub at gcc dot gnu dot org wrote:
> ------- Comment #20 from jakub at gcc dot gnu dot org  2009-03-18 09:10 -------
> So let's use memmove on alpha and let targets with sane memcpy not suffer just
> because of one dead architecture.

1) sane memcpy, by the standard, is unsafe.

2) alpha was just an example, I vaguely remember that similar problems
were found on x86 with certain mmx instructions, but don't remember the
details.

A better approach is to use memmove by default, and memcpy when you can
prove that it's safe; rather than assuming that a "non-dead"
architecture extends the standard.

Alternatively, the following should be safe too, in accordance with
comment #21:
Foo &operator=(const Foo &rhs)
{
        if (this != &rhs)
                memcpy(this, &rhs, sizeof(Foo));
}
But that introduces a branch instruction.

Another safe alternative is to do what is described in comment #12.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[fpbeekhof at gmail dot com]

------- Comment #23 from fpbeekhof at gmail dot com  2009-03-18 10:04 -------
My last comment (#22) of course does not address issues pointed out by jason in
#18.

Even if "we know that we are sure that either p1 == p2 or *p1 and *p2 do not
overlap because otherwise there would be an alias violation." (comment #21)
there is a problem because if p1 == p2 then memcpy is unsafe.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[jakub at gcc dot gnu dot org]

------- Comment #24 from jakub at gcc dot gnu dot org  2009-03-18 10:11 -------
The problem with that is that in most cases you won't be able to prove it and
memmove is certainly more expensive than memcpy and usually isn't even expanded
inline (unless it is optimized into memcpy by proving non-overlap).
fold_builtin_memory_op currently optimizes memmove into memcpy only when
source is read-only or length is 1 byte, could be improved by analysis of
non-overlapping, but in many cases you really don't know if the two pointers
can or can't overlap.
Having an internal builtin for the case that either source and dest don't
overlap, or are equal could improve things, then when expanding that inline if
we knew the expanded code is ok for src == dst, we could expand it as inline
memcpy and if we didn't know that, we could add (predicted unlikely) jump
around the memcpy call (or inline expansion that is not safe for src == dst).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[jason at gcc dot gnu dot org]

------- Comment #25 from jason at gcc dot gnu dot org  2009-04-09 16:05 -------
The C++ front end bit of this needs 22488 to be fixed before we can remove the
explicit call to the builtin and go back to using a MODIFY_EXPR.  But in the
meantime we can test for pointer equality before the call.


-- 

jason at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at redhat dot com
  BugsThisDependsOn|                            |22488
         AssignedTo|unassigned at gcc dot gnu   |jason at gcc dot gnu dot org
                   |dot org                     |
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2009-04-09 16:05:17
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[jason at gcc dot gnu dot org]

------- Comment #26 from jason at gcc dot gnu dot org  2009-04-13 20:08 -------
I think this actually only comes up at -O0; without optimization we don't try
to expand a call to __builtin_memcpy as a block move because we need TER to
tell us what the real alignment of the operands is.  With optimization we use
emit_block_move_hints, which should be safe.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[jason at gcc dot gnu dot org]

------- Comment #27 from jason at gcc dot gnu dot org  2009-04-13 20:53 -------
Subject: Bug 39480

Author: jason
Date: Mon Apr 13 20:53:34 2009
New Revision: 146011

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=146011
Log:
        PR c++/39480
        * call.c (build_over_call): Don't call memcpy if the target is
        the same as the source.

Added:
    trunk/gcc/testsuite/g++.dg/init/copy7.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/call.c
    trunk/gcc/testsuite/ChangeLog


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[jason at gcc dot gnu dot org]

------- Comment #28 from jason at gcc dot gnu dot org  2009-04-13 20:56 -------
Subject: Bug 39480

Author: jason
Date: Mon Apr 13 20:56:45 2009
New Revision: 146013

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=146013
Log:
        PR c++/39480
        * call.c (build_over_call): Don't call memcpy if the target is
        the same as the source.

Added:
    branches/gcc-4_4-branch/gcc/testsuite/g++.dg/init/copy7.C
      - copied unchanged from r146011, trunk/gcc/testsuite/g++.dg/init/copy7.C
Modified:
    branches/gcc-4_4-branch/gcc/cp/ChangeLog
    branches/gcc-4_4-branch/gcc/cp/call.c
    branches/gcc-4_4-branch/gcc/testsuite/ChangeLog


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[jason at gcc dot gnu dot org]

------- Comment #29 from jason at gcc dot gnu dot org  2009-04-14 03:30 -------
Subject: Bug 39480

Author: jason
Date: Tue Apr 14 03:30:12 2009
New Revision: 146020

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=146020
Log:
        PR c++/39480
        * call.c (build_over_call): Don't call memcpy if the target is
        the same as the source.

Added:
    branches/gcc-4_3-branch/gcc/testsuite/g++.dg/init/copy7.C
      - copied unchanged from r146011, trunk/gcc/testsuite/g++.dg/init/copy7.C
Modified:
    branches/gcc-4_3-branch/gcc/cp/ChangeLog
    branches/gcc-4_3-branch/gcc/cp/call.c
    branches/gcc-4_3-branch/gcc/testsuite/ChangeLog


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480

[Bug c++/39480] generated memcpy causes trouble in assignment

[jason at gcc dot gnu dot org]

------- Comment #30 from jason at gcc dot gnu dot org  2009-04-14 03:31 -------
Fixed for 4.3, 4.4 and 4.5.


-- 

jason at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39480