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

[Björn Pettersson A via llvm-commits]

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<mailto: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<mailto: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/7ec02b0f/attachment.html>