[PATCH] D69977: [InstCombine] avoid crash from deleting an instruction that still has uses (PR43723)

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
spatel added reviewers: nlewycky, efriedma, lebedev.ri.
Herald added subscribers: hiraditya, mcrosier.
Herald added a project: LLVM.

This whole thing seems fragile, but I've never looked at this code before this.

We gather a set of white-listed instructions in isAllocSiteRemovable() and then replace/erase them. But we don't know in general if the instructions in the set have uses amongst themselves, so order of deletion makes a difference.

There's already a special-case for the llvm.objectsize intrinsic, so add another for llvm.invariant.end.

Should fix:
https://bugs.llvm.org/show_bug.cgi?id=43723


https://reviews.llvm.org/D69977

Files:
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/test/Transforms/InstCombine/builtin-object-size-ptr.ll


Index: llvm/test/Transforms/InstCombine/builtin-object-size-ptr.ll
===================================================================
--- llvm/test/Transforms/InstCombine/builtin-object-size-ptr.ll
+++ llvm/test/Transforms/InstCombine/builtin-object-size-ptr.ll
@@ -28,6 +28,26 @@
   ret i32 %conv
 }
 
+; This used to crash while erasing instructions:
+; https://bugs.llvm.org/show_bug.cgi?id=43723
+
+define void @PR43723() {
+; CHECK-LABEL: @PR43723(
+; CHECK-NEXT:    ret void
+;
+  %tab = alloca [10 x i8], align 16
+  %t0 = bitcast [10 x i8]* %tab to i8*
+  call void @llvm.memset.p0i8.i64(i8* align 16 %t0, i8 9, i64 10, i1 false)
+  %t1 = call {}* @llvm.invariant.start.p0i8(i64 10, i8* align 16 %t0)
+  call void @llvm.invariant.end.p0i8({}* %t1, i64 10, i8* align 16 %t0)
+  ret void
+
+  uselistorder i8* %t0, { 1, 0, 2 }
+}
+
 declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture) #1
 declare i64 @llvm.objectsize.i64.p0i8(i8*, i1) #2
 declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture) #1
+declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8, i64, i1 immarg) #0
+declare {}* @llvm.invariant.start.p0i8(i64 immarg, i8* nocapture) #0
+declare void @llvm.invariant.end.p0i8({}*, i64 immarg, i8* nocapture) #0
Index: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2377,14 +2377,13 @@
 
   if (isAllocSiteRemovable(&MI, Users, &TLI)) {
     for (unsigned i = 0, e = Users.size(); i != e; ++i) {
-      // Lowering all @llvm.objectsize calls first because they may
-      // use a bitcast/GEP of the alloca we are removing.
       if (!Users[i])
        continue;
 
       Instruction *I = cast<Instruction>(&*Users[i]);
-
       if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
+        // Lowering all @llvm.objectsize calls first because they may
+        // use a bitcast/GEP of the alloca we are removing.
         if (II->getIntrinsicID() == Intrinsic::objectsize) {
           Value *Result =
               lowerObjectSizeCall(II, DL, &TLI, /*MustSucceed=*/true);
@@ -2392,6 +2391,12 @@
           eraseInstFromFunction(*I);
           Users[i] = nullptr; // Skip examining in the next loop.
         }
+        // Erase llvm.invariant.end because we expect that it uses an
+        // llvm.invariant.start that we will remove below.
+        if (II->getIntrinsicID() == Intrinsic::invariant_end) {
+          eraseInstFromFunction(*I);
+          Users[i] = nullptr; // Skip examining in the next loop.
+        }
       }
     }
     for (unsigned i = 0, e = Users.size(); i != e; ++i) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D69977.228333.patch
Type: text/x-patch
Size: 2737 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191108/482767a6/attachment.bin>