[llvm] r285298 - Fix memory issue in AttrBuilder::removeAttribute uses.

[David Blaikie via llvm-commits]

Richard - do you remember what the call was with this? I seem to recall it
coming up before.

Are we just going to rely on/assume the implementation's memcpy is safe for
identical addresses? Is there a flag to opt-out? (perhaps that could be
used when doing a valgrind run)

It doesn't seem useful to change code (in this case the code was actually a
no-op and is better for being changed, admittedly) to account for this, be
it a bug in Clang or Valgrind, or somewhere between the two.

On Mon, Oct 31, 2016 at 12:54 PM Björn Pettersson A <
bjorn.a.pettersson at ericsson.com> wrote:

> I’m no expert on the C++ standard, but I think that you are right.
>
> The default copy/move assignments should behave as-if it was a memmove
> rather than a memcpy,
>
> even though it can be implemented as a conditional memcpy to avoid the
> self-assignment.
>
>
>
> I created a small test case out of curiosity:
>
>
>
> //-----------------------------------------------
>
> #include <iostream>
>
> struct A {
>
>   void operator=(const A& rhs) {
>
>     if(this==&rhs) std::cout << "Self-assigned\n";
>
>   }
>
> };
>
>
>
> struct B {
>
>   A a;
>
>   int c[8000];
>
> };
>
>
>
> int main()
>
> {
>
>   B b;
>
>   b = b;
>
> }
>
> //-----------------------------------------------
>
>
>
> Both “g++ (SUSE Linux) 4.3.4” and clang (compiled from the trunk) will
> copy the array “c” using an unconditional memcpy.
>
> So valgrind complains like this:
>
>
>
> > clang++ -O0 foo.cpp
>
> > valgrind ./a.out
>
> ==10444== Memcheck, a memory error detector
>
> ==10444== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
>
> ==10444== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
>
> ==10444== Command: ./a.out
>
> ==10444==
>
> Self-assigned
>
> ==10444== Source and destination overlap in memcpy(0x7feff72fc,
> 0x7feff72fc, 32000)
>
> ==10444==    at 0x4C2B88E: memcpy (in
> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>
> ==10444==    by 0x40088D: B::operator=(B const&) (in ./a.out)
>
> ==10444==    by 0x40081C: main (in ./a.out)
>
> ==10444==
>
>
>
>
>
> Normally I guess the libstd++ implementation of memcpy is safe to use when
> there is an exact overlap.
>
> But that is not guaranteed by libstd++, so it is not something that clang
> should rely on.
>
>
>
>
>
> *From:* David Blaikie [mailto:dblaikie at gmail.com]
> *Sent:* den 31 oktober 2016 19:06
> *To:* Björn Pettersson A <bjorn.a.pettersson at ericsson.com>;
> llvm-commits at lists.llvm.org
> *Subject:* Re: [llvm] r285298 - Fix memory issue in
> AttrBuilder::removeAttribute uses.
>
>
>
> This seems a bit problematic, though - self assignment should be valid in
> C++, if we're optimizing it to a memcpy without a check for non-equality of
> location, isn't that a bug in the compiler we need to fix?
>
>
>
> On Thu, Oct 27, 2016 at 7:57 AM Bjorn Pettersson via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> Author: bjope
> Date: Thu Oct 27 09:48:09 2016
> New Revision: 285298
>
> URL: http://llvm.org/viewvc/llvm-project?rev=285298&view=rev
> Log:
> Fix memory issue in AttrBuilder::removeAttribute uses.
>
> Summary:
> Found when running Valgrind.
>
> This removes two unnecessary assignments when using
> AttrBuilder::removeAttribute.
>
> AttrBuilder::removeAttribute returns a reference to the object.
> As the LHSes were the same as the callees, the assignments
> resulted in memcpy calls where dst = src.
>
> Commited on behalf-of: dstenb (David Stenberg)
>
> Reviewers: mkuper, rnk
>
> Subscribers: llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D25460
>
> Modified:
>     llvm/trunk/lib/CodeGen/Analysis.cpp
>
> Modified: llvm/trunk/lib/CodeGen/Analysis.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/Analysis.cpp?rev=285298&r1=285297&r2=285298&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/Analysis.cpp (original)
> +++ llvm/trunk/lib/CodeGen/Analysis.cpp Thu Oct 27 09:48:09 2016
> @@ -541,8 +541,8 @@ bool llvm::attributesPermitTailCall(cons
>
>    // Noalias is completely benign as far as calling convention goes, it
>    // shouldn't affect whether the call is a tail call.
> -  CallerAttrs = CallerAttrs.removeAttribute(Attribute::NoAlias);
> -  CalleeAttrs = CalleeAttrs.removeAttribute(Attribute::NoAlias);
> +  CallerAttrs.removeAttribute(Attribute::NoAlias);
> +  CalleeAttrs.removeAttribute(Attribute::NoAlias);
>
>    if (CallerAttrs.contains(Attribute::ZExt)) {
>      if (!CalleeAttrs.contains(Attribute::ZExt))
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161031/6f161850/attachment.html>