[PATCH] D39864: Fix for CFI type tests lowering assert.

[Dmitry Mikulin via Phabricator via llvm-commits]

dmikulin created this revision.
Herald added a subscriber: hiraditya.

My attempt to fix https://bugs.llvm.org/show_bug.cgi?id=35252.

The problem is that current implementation of Value::replaceUsesExceptBlockAddr() uses UseList iterator to walk the list which keeps changing inside the loop. doRAUW() gets around it by checking if the list is empty to terminate. When we skip blockaddress uses, the list is never empty if there are any blockaddress uses. This change removes blockaddress uses from the UseList, places them in a temporary list and adds them back after all uses are processed.

I don't particularly like this fix, but couldn't find a better way. I have another version where the loop terminating condition checks if the only remaining uses are blockaddresses and skips leading blockaddresses on loop entry, but it requires repeated scans of UseList, so I liked it even less. If anyone has suggestions I'll be happy to explore.


https://reviews.llvm.org/D39864

Files:
  llvm/lib/IR/Value.cpp
  llvm/test/Transforms/LowerTypeTests/blockaddress-2.ll


Index: llvm/test/Transforms/LowerTypeTests/blockaddress-2.ll
===================================================================
--- /dev/null
+++ llvm/test/Transforms/LowerTypeTests/blockaddress-2.ll
@@ -0,0 +1,27 @@
+; RUN: opt -S %s -lowertypetests | FileCheck %s
+
+; CHECK: @badfileops = internal global %struct.f { i32 (i32*)* @bad_f, i32 (i32*)* @bad_f }
+; CHECK: @bad_f = internal alias i32 (i32*), bitcast (void ()* @.cfi.jumptable to i32 (i32*)*)
+; CHECK: define internal i32 @bad_f.cfi(i32* nocapture readnone) !type !0 {
+; CHECK-NEXT:  ret i32 9
+
+target triple = "x86_64-unknown-linux"
+
+%struct.f = type { i32 (i32*)*, i32 (i32*)* }
+ at llvm.used = external global [43 x i8*], section "llvm.metadata"
+ at badfileops = internal global %struct.f { i32 (i32*)* @bad_f, i32 (i32*)* @bad_f }, align 8
+
+declare i1 @llvm.type.test(i8*, metadata)
+
+define internal i32 @bad_f(i32* nocapture readnone) !type !1 {
+  ret i32 9
+}
+
+define internal fastcc i32 @do_f(i32, i32, i32*, i32, i64, i32) unnamed_addr !type !2 {
+  %7 = tail call i1 @llvm.type.test(i8* undef, metadata !"_ZTSFiP4fileP3uioP5ucrediP6threadE"), !nosanitize !3
+  ret i32 undef
+}
+
+!1 = !{i64 0, !"_ZTSFiP4fileP3uioP5ucrediP6threadE"}
+!2 = !{i64 0, !"_ZTSFiP6threadiP4fileP3uioliE"}
+!3 = !{}
Index: llvm/lib/IR/Value.cpp
===================================================================
--- llvm/lib/IR/Value.cpp
+++ llvm/lib/IR/Value.cpp
@@ -455,13 +455,20 @@
 }
 
 void Value::replaceUsesExceptBlockAddr(Value *New) {
-  use_iterator UI = use_begin(), E = use_end();
-  for (; UI != E;) {
-    Use &U = *UI;
-    ++UI;
+  SmallVector<std::pair<Use*, Value*>, 8> BA;
+  while (!use_empty()) {
+    Use &U = *UseList;
 
-    if (isa<BlockAddress>(U.getUser()))
+    // Don't process blockaddress uses. Remove them from UseList
+    // temporarily and place onto a separate list. We can't use
+    // UseList iterator here as the list is changing inside the loop.
+    // That's why we need to remove skiped uses from the list.
+    if (isa<BlockAddress>(U.getUser())) {
+      Value *V = U.get();
+      BA.push_back(std::make_pair(&U, V));
+      U.set(nullptr);
       continue;
+    }
 
     // Must handle Constants specially, we cannot call replaceUsesOfWith on a
     // constant because they are uniqued.
@@ -474,6 +481,14 @@
 
     U.set(New);
   }
+
+  // Go over the saved block address uses and add them back to UseList.
+  while (!BA.empty()) {
+    std::pair<Use*, Value*> P = BA.pop_back_val();
+    Use *U = P.first;
+    Value *V = P.second;
+    U->set(V);
+  }
 }
 
 namespace {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D39864.122309.patch
Type: text/x-patch
Size: 2583 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171109/fd68574e/attachment.bin>