Allow null-valued function operands in getCalledFunction()

[dstenb]

Summary

Change the dynamic cast in CallBase::getCalledFunction() to allow
null-valued function operands.

This patch fixes a crash that occurred when a funtion operand of a
call instruction was dropped, and later on a metadata-carrying
instruction was printed out. When allocating the metadata slot numbers,
getCalledFunction() would be invoked on the call with the dropped
operand, resulting in a failed non-null assertion in isa<>.

This fixes PR38924, in which a printout in DBCE crashed due to this.

This aligns getCalledFunction() with getCalledValue(), as the latter
allows the operand to be null.

[vsk]

Thanks for the patch. The change to SlotTracker looks safe to me. I do think it's the cleanest way to address the kind of crash you're seeing -- as I pointed out in the PR, modifying BDCE to eagerly erase dead instructions may not go well. @hfinkel, any thoughts on this?

As for the test, I think a unit test might work better (see unittests/IR/MetadataTest.cpp). If something about debug info or BDCE changes, the test will remain useful. [Could be worth waiting for Duncan or Hal to approve the SlotTracker change before updating the patch.]

[dexonsmith]

The change looks correct, but I'm not sure it's the right one. Looking at Instructions.h at getCalledFunction():

/// Return the function called, or null if this is an
/// indirect function invocation.
///  
Function *getCalledFunction() const {
  return dyn_cast<Function>(Op<-3>());
}

it seems like we could just change this to dyn_cast_or_null, and that would also match the documentation comment.

I don't have a strong opinion; wouldn't mind @hfinkel's thoughts.

[dstenb]

• Changed so that getCalledFunction() allows null valued operands.
• Rewrote the test case to a unit test.

[dstenb]

In D52537#1246683, @vsk wrote:

As for the test, I think a unit test might work better (see unittests/IR/MetadataTest.cpp). If something about debug info or BDCE changes, the test will remain useful.

Thanks for the suggestion!

In D52537#1247366, @dexonsmith wrote:

The change looks correct, but I'm not sure it's the right one. Looking at Instructions.h at getCalledFunction():

/// Return the function called, or null if this is an
/// indirect function invocation.
///  
Function *getCalledFunction() const {
  return dyn_cast<Function>(Op<-3>());
}

it seems like we could just change this to dyn_cast_or_null, and that would also match the documentation comment.

I don't have a strong opinion; wouldn't mind @hfinkel's thoughts.

I went ahead and did this change in the latest revision.

(Your suggestion seems reasonable to me, but I don't really know enough about the LLVM framework to say something more concrete about this.)

[dstenb]

Any thoughts on this?

[vsk]

LGTM, but it'd be nice to have another +1 on this. Thanks.

[dstenb]

Does anyone more than @vsk have any thoughts on this?

[dexonsmith]

LGTM.

[dstenb]

Thanks for your reviews and suggestions! I'm planning on submitting this shortly.

llvm/trunk/include/llvm/IR/Instructions.h

public:
   template <int Idx_nocapture> const Use &Op() const {
     return User::OpFrom<Idx_nocapture>(this);
   }
 
   /// Return the function called, or null if this is an
   /// indirect function invocation.
   ///
   Function *getCalledFunction() const {
-    return dyn_cast<Function>(Op<-InstTy::ArgOffset>());
+    return dyn_cast_or_null<Function>(Op<-InstTy::ArgOffset>());
   }
 
   /// Determine whether this call has the given attribute.
   bool hasFnAttr(Attribute::AttrKind Kind) const {
     assert(Kind != Attribute::NoBuiltin &&
            "Use CallBase::isNoBuiltin() to check for Attribute::NoBuiltin");
     return hasFnAttrImpl(Kind);
   }

llvm/trunk/unittests/IR/MetadataTest.cpp

TEST_F(MDNodeTest, PrintFromMetadataAsValue) {
   ModuleSlotTracker MST(&M);
   EXPECT_PRINTER_EQ("!0 = distinct !{}", MAV0->print(OS, MST));
   EXPECT_PRINTER_EQ("!1 = distinct !{}", MAV1->print(OS, MST));
   EXPECT_PRINTER_EQ("!0", MAV0->printAsOperand(OS, false, MST));
   EXPECT_PRINTER_EQ("!1", MAV1->printAsOperand(OS, false, MST));
   EXPECT_PRINTER_EQ("metadata !0", MAV0->printAsOperand(OS, true, MST));
   EXPECT_PRINTER_EQ("metadata !1", MAV1->printAsOperand(OS, true, MST));
 }
+
+TEST_F(MDNodeTest, PrintWithDroppedCallOperand) {
+  Module M("test", Context);
+
+  auto *FTy = FunctionType::get(Type::getVoidTy(Context), false);
+  auto *F0 = Function::Create(FTy, GlobalValue::ExternalLinkage, "F0", &M);
+  auto *F1 = Function::Create(FTy, GlobalValue::ExternalLinkage, "F1", &M);
+  auto *BB0 = BasicBlock::Create(Context, "entry", F0);
+
+  CallInst *CI0 = CallInst::Create(F1, "", BB0);
+  CI0->dropAllReferences();
+
+  auto *R0 = ReturnInst::Create(Context, BB0);
+  auto *N0 = MDNode::getDistinct(Context, None);
+  R0->setMetadata("md", N0);
+
+  // Printing the metadata node would previously result in a failed assertion
+  // due to the call instruction's dropped function operand.
+  ModuleSlotTracker MST(&M);
+  EXPECT_PRINTER_EQ("!0 = distinct !{}", N0->print(OS, MST));
+}
 #undef EXPECT_PRINTER_EQ
 
 TEST_F(MDNodeTest, NullOperand) {
   // metadata !{}
   MDNode *Empty = MDNode::get(Context, None);
 
   // metadata !{metadata !{}}
   Metadata *Ops[] = {Empty};