[NFC] Add checks for self-assignment.

[schittir]

Please consider these changes for review.

[tahonermann]

Hmm, it seems exceptionally unlikely for a move assignment operator to be invoked on an object with itself as the source argument. That would require an explicit use of std::move() as in:

Value x = ...;
x = std::move(x);

Rather than protecting against that and making it a no-opt, I think we would want to be informed of code that manages to do so because it is probably not intended. We could instead assert(this !=RHS), but I'm not sure that is warranted either.

I'm assuming this patch was made in response to a static analysis report. If so, I suggest we follow up with the static analysis vendor.

[tahonermann]

On second thought, and after having read https://stackoverflow.com/a/9322542/11634221, I think this change is fine. There are legitimate scenarios for which self move assignment should work in the sense that the object value is preserved or is left in a valid but unspecified state; UB should be avoided.

[schittir]

@tahonermann Thank you for considering this thoroughly. This patch is now landed.

[jplehr]

Hi,
it seems that this broke the AMDGPU OpenMP buildbot https://lab.llvm.org/buildbot/#/builders/193/builds/35271
Happy to help if you need more info etc.

[MitalAshok]

This doesn't compile, could you fix this or revert 8ac137acefc01caf636db5f95eb0977c97def1ba ?

clang/lib/AST/APValue.cpp

393–399

Ditto for the other changes

[michaelplatings]

This comment was removed by michaelplatings.

[schittir]

Fix errors.

[schittir]

@jplehr @michaelplatings @MitalAshok @tahonermann - sorry this one slipped through the cracks. I fixed the errors now. Thank you for your review.

[aaron.ballman]

LGTM

[tahonermann]

Thank you for following up, Sindhu! Looks good!

[schittir]

Thank you for the review, @aaron.ballman and @tahonermann
Investigating the build failure...
Test failure seems unrelated to this patch's changes.

clang/lib/AST/APValue.cpp

 
 APValue &APValue::operator=(const APValue &RHS) {
   if (this != &RHS)
     *this = APValue(RHS);
   return *this;
 }
 
 APValue &APValue::operator=(APValue &&RHS) {
+  if (this != &RHS) {
     if (Kind != None && Kind != Indeterminate)
       DestroyDataAndMakeUninit();
     Kind = RHS.Kind;
     Data = RHS.Data;
     RHS.Kind = None;
+  }
   return *this;
 }
 
 void APValue::DestroyDataAndMakeUninit() {
   if (Kind == Int)
     ((APSInt *)(char *)&Data)->~APSInt();
   else if (Kind == Float)
     ((APFloat *)(char *)&Data)->~APFloat();

clang/lib/CodeGen/CGDebugInfo.h

public:
   ApplyDebugLocation(CodeGenFunction &CGF, const Expr *E);
   ApplyDebugLocation(CodeGenFunction &CGF, llvm::DebugLoc Loc);
   ApplyDebugLocation(ApplyDebugLocation &&Other) : CGF(Other.CGF) {
     Other.CGF = nullptr;
   }
 
   // Define copy assignment operator.
   ApplyDebugLocation &operator=(ApplyDebugLocation &&Other) {
+    if (this != &Other) {
       CGF = Other.CGF;
       Other.CGF = nullptr;
+    }
     return *this;
   }
 
   ~ApplyDebugLocation();
 
   /// Apply TemporaryLocation if it is valid. Otherwise switch
   /// to an artificial debug location that has a valid scope, but no
   /// line information.

clang/lib/Interpreter/Value.cpp

Value &Value::operator=(const Value &RHS) {
 
   if (IsManuallyAlloc)
     ValueStorage::getFromPayload(getPtr())->Retain();
 
   return *this;
 }
 
 Value &Value::operator=(Value &&RHS) noexcept {
+  if (this != &RHS) {
     if (IsManuallyAlloc)
       ValueStorage::getFromPayload(getPtr())->Release();
 
     Interp = std::exchange(RHS.Interp, nullptr);
     OpaqueType = std::exchange(RHS.OpaqueType, nullptr);
     ValueKind = std::exchange(RHS.ValueKind, K_Unspecified);
     IsManuallyAlloc = std::exchange(RHS.IsManuallyAlloc, false);
 
     Data = RHS.Data;
-
+  }
   return *this;
 }
 
 void Value::clear() {
   if (IsManuallyAlloc)
     ValueStorage::getFromPayload(getPtr())->Release();
   ValueKind = K_Unspecified;
   OpaqueType = nullptr;