[llvm] a73e0c3 - [AAPointerInfo] fix assertion at the pass-through use of a pointer

[Sameer Sahasrabuddhe via llvm-commits]

Author: Sameer Sahasrabuddhe
Date: 2023-01-04T16:53:55+05:30
New Revision: a73e0c306cdd653e0aa035fc89e91acf771bd318

URL: https://github.com/llvm/llvm-project/commit/a73e0c306cdd653e0aa035fc89e91acf771bd318
DIFF: https://github.com/llvm/llvm-project/commit/a73e0c306cdd653e0aa035fc89e91acf771bd318.diff

LOG: [AAPointerInfo] fix assertion at the pass-through use of a pointer

HandlePassthroughUser may sometimes create a new entry for the OffsetInfo of a
user in the OffsetInfoMap. This can invalidate outstanding references into the
map, including the one which needs to be copied into the new entry. This
produces invalid offset info that can trigger assertions.

Fixed this by not using references at this point. The bug was originally
introduced in commit ID 0dc0a441323d41b4860668f38d290579e0de130c.

Reviewed By: ronlieb

Differential Revision: https://reviews.llvm.org/D140837

Added: 
    

Modified: 
    llvm/lib/Transforms/IPO/AttributorAttributes.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
index 533afe4fa17c2..0af298bb58a3c 100644
--- a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
+++ b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
@@ -1435,11 +1435,20 @@ ChangeStatus AAPointerInfoFloating::updateImpl(Attributor &A) {
   DenseMap<Value *, OffsetInfo> OffsetInfoMap;
   OffsetInfoMap[&AssociatedValue].insert(0);
 
-  auto HandlePassthroughUser = [&](Value *Usr, const OffsetInfo &PtrOI,
-                                   bool &Follow) {
+  auto HandlePassthroughUser = [&](Value *Usr, Value *CurPtr, bool &Follow) {
+    // One does not simply walk into a map and assign a reference to a possibly
+    // new location. That can cause an invalidation before the assignment
+    // happens, like so:
+    //
+    //   OffsetInfoMap[Usr] = OffsetInfoMap[CurPtr]; /* bad idea! */
+    //
+    // The RHS is a reference that may be invalidated by an insertion caused by
+    // the LHS. So we ensure that the side-effect of the LHS happens first.
+    auto &UsrOI = OffsetInfoMap[Usr];
+    auto &PtrOI = OffsetInfoMap[CurPtr];
     assert(!PtrOI.isUnassigned() &&
            "Cannot pass through if the input Ptr was not visited!");
-    OffsetInfoMap[Usr] = PtrOI;
+    UsrOI = PtrOI;
     Follow = true;
     return true;
   };
@@ -1461,7 +1470,7 @@ ChangeStatus AAPointerInfoFloating::updateImpl(Attributor &A) {
 
     if (ConstantExpr *CE = dyn_cast<ConstantExpr>(Usr)) {
       if (CE->isCast())
-        return HandlePassthroughUser(Usr, OffsetInfoMap[CurPtr], Follow);
+        return HandlePassthroughUser(Usr, CurPtr, Follow);
       if (CE->isCompare())
         return true;
       if (!isa<GEPOperator>(CE)) {
@@ -1491,7 +1500,7 @@ ChangeStatus AAPointerInfoFloating::updateImpl(Attributor &A) {
     if (isa<PtrToIntInst>(Usr))
       return false;
     if (isa<CastInst>(Usr) || isa<SelectInst>(Usr) || isa<ReturnInst>(Usr))
-      return HandlePassthroughUser(Usr, OffsetInfoMap[CurPtr], Follow);
+      return HandlePassthroughUser(Usr, CurPtr, Follow);
 
     // For PHIs we need to take care of the recurrence explicitly as the value
     // might change while we iterate through a loop. For now, we give up if
@@ -1559,7 +1568,7 @@ ChangeStatus AAPointerInfoFloating::updateImpl(Attributor &A) {
         if (IsFirstPHIUser || BaseOI == UsrOI) {
           LLVM_DEBUG(dbgs() << "[AAPointerInfo] PHI is invariant " << *CurPtr
                             << " in " << *Usr << "\n");
-          return HandlePassthroughUser(Usr, PtrOI, Follow);
+          return HandlePassthroughUser(Usr, CurPtr, Follow);
         }
 
         LLVM_DEBUG(