[PATCH] D138083: AutoUpgrade: Fix assertion on invalid name mangling usage

[Matt Arsenault via Phabricator via llvm-commits]

arsenm created this revision.
arsenm added reviewers: craig.topper, pengfei, nikic, RKSimon, ab.
Herald added subscribers: StephenFan, hiraditya.
Herald added a project: All.
arsenm requested review of this revision.
Herald added a subscriber: wdng.
Herald added a project: LLVM.

This was trying to auto-upgrade a read_register call with missing type
mangling. This first would break since getCalledFunction checks the
callee type is consistent, so this would assert there. After that,
the replacement code would die on the type mismatch. Be more
defensive and let the verifier code produce an error that the IR
is broken.


https://reviews.llvm.org/D138083

Files:
  llvm/lib/IR/AutoUpgrade.cpp
  llvm/test/Assembler/autoupgrade-invalid-name-mangling.ll


Index: llvm/test/Assembler/autoupgrade-invalid-name-mangling.ll
===================================================================
--- /dev/null
+++ llvm/test/Assembler/autoupgrade-invalid-name-mangling.ll
@@ -0,0 +1,14 @@
+; RUN: not llvm-as < %s 2>&1 | FileCheck %s
+
+; CHECK: Intrinsic called with incompatible signature
+; CHECK-NEXT: %reg = call i32 @llvm.read_register.i64(
+; CHECK: Invalid user of intrinsic instruction!
+; CHECK-NEXT: %reg = call i32 @llvm.read_register.i64(
+define i32 @read_register_missing_mangling() {
+  %reg = call i32 @llvm.read_register(metadata !0)
+  ret i32 %reg
+}
+
+declare i64 @llvm.read_register(metadata)
+
+!0 = !{!"foo"}
Index: llvm/lib/IR/AutoUpgrade.cpp
===================================================================
--- llvm/lib/IR/AutoUpgrade.cpp
+++ llvm/lib/IR/AutoUpgrade.cpp
@@ -2042,13 +2042,17 @@
 /// Upgrade a call to an old intrinsic. All argument and return casting must be
 /// provided to seamlessly integrate with existing context.
 void llvm::UpgradeIntrinsicCall(CallBase *CI, Function *NewFn) {
-  Function *F = CI->getCalledFunction();
+  // Note dyn_cast to Function is not quite the same as getCalledFunction, which
+  // checks the callee's function type matches. It's likely we need to handle
+  // type changes here.
+  Function *F = dyn_cast<Function>(CI->getCalledOperand());
+  if (!F)
+    return;
+
   LLVMContext &C = CI->getContext();
   IRBuilder<> Builder(C);
   Builder.SetInsertPoint(CI->getParent(), CI->getIterator());
 
-  assert(F && "Intrinsic call is not direct?");
-
   if (!NewFn) {
     // Get the Function's name.
     StringRef Name = F->getName();
@@ -3909,21 +3913,29 @@
     }
 
     // This must be an upgrade from a named to a literal struct.
-    auto *OldST = cast<StructType>(CI->getType());
-    assert(OldST != NewFn->getReturnType() && "Return type must have changed");
-    assert(OldST->getNumElements() ==
-               cast<StructType>(NewFn->getReturnType())->getNumElements() &&
-           "Must have same number of elements");
-
-    SmallVector<Value *> Args(CI->args());
-    Value *NewCI = Builder.CreateCall(NewFn, Args);
-    Value *Res = PoisonValue::get(OldST);
-    for (unsigned Idx = 0; Idx < OldST->getNumElements(); ++Idx) {
-      Value *Elem = Builder.CreateExtractValue(NewCI, Idx);
-      Res = Builder.CreateInsertValue(Res, Elem, Idx);
+    if (auto *OldST = dyn_cast<StructType>(CI->getType())) {
+      assert(OldST != NewFn->getReturnType() &&
+             "Return type must have changed");
+      assert(OldST->getNumElements() ==
+                 cast<StructType>(NewFn->getReturnType())->getNumElements() &&
+             "Must have same number of elements");
+
+      SmallVector<Value *> Args(CI->args());
+      Value *NewCI = Builder.CreateCall(NewFn, Args);
+      Value *Res = PoisonValue::get(OldST);
+      for (unsigned Idx = 0; Idx < OldST->getNumElements(); ++Idx) {
+        Value *Elem = Builder.CreateExtractValue(NewCI, Idx);
+        Res = Builder.CreateInsertValue(Res, Elem, Idx);
+      }
+      CI->replaceAllUsesWith(Res);
+      CI->eraseFromParent();
+      return;
     }
-    CI->replaceAllUsesWith(Res);
-    CI->eraseFromParent();
+
+    // We're probably about to produce something invalid. Let the verifier catch
+    // it instead of dying here.
+    CI->setCalledOperand(
+        ConstantExpr::getPointerCast(NewFn, CI->getCalledOperand()->getType()));
     return;
   };
   CallInst *NewCall = nullptr;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D138083.475646.patch
Type: text/x-patch
Size: 3484 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221116/c0e76b4c/attachment.bin>