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.

lib/IR/AsmWriter.cpp

for (auto &BB : F) {
     for (auto &I : BB)
       processInstructionMetadata(I);
   }
 }
 
 void SlotTracker::processInstructionMetadata(const Instruction &I) {
   // Process metadata used directly by intrinsics.
   if (const CallInst *CI = dyn_cast<CallInst>(&I))
+    if (CI->getCalledValue())
       if (Function *F = CI->getCalledFunction())
         if (F->isIntrinsic())
           for (auto &Op : I.operands())
             if (auto *V = dyn_cast_or_null<MetadataAsValue>(Op))
               if (MDNode *N = dyn_cast<MDNode>(V->getMetadata()))
                 CreateMetadataSlot(N);
 
   // Process metadata attached to this instruction.
   SmallVector<std::pair<unsigned, MDNode *>, 4> MDs;
   I.getAllMetadata(MDs);
   for (auto &MD : MDs)
     CreateMetadataSlot(MD.second);
 }
 

test/Transforms/BDCE/dump-crash.ll

+; RUN: opt -S -bdce -debug < %s 2>&1 | FileCheck %s
+
+; Reproducer for PR38924.
+;
+; Verify that it is possible to print the dbg.value when salvaging debug
+; information for the or instruction.
+
+; CHECK: SALVAGE: call void @llvm.dbg.value(metadata i32 123, metadata ![[VAR:[0-9]+]], metadata !DIExpression(DW_OP_constu, 456, DW_OP_or, DW_OP_stack_value)), !dbg ![[LOC:[0-9]+]]
+;
+; CHECK: call void @llvm.dbg.value(metadata i32 123, metadata ![[VAR]], metadata !DIExpression(DW_OP_constu, 456, DW_OP_or, DW_OP_stack_value)), !dbg ![[LOC]]
+;
+; CHECK-DAG: ![[VAR]] = !DILocalVariable(name: "local"
+; CHECK-DAG: ![[LOC]] = !DILocation(line: 234, column: 116
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline norecurse nounwind readnone
+declare fastcc i16 @foo() unnamed_addr #0
+
+; Function Attrs: noinline norecurse noreturn nounwind readonly
+define internal fastcc void @bar() unnamed_addr #1 !dbg !4 {
+entry:
+  %call = tail call fastcc i16 @foo()
+  %or = or i32 123, 456
+  call void @llvm.dbg.value(metadata i32 %or, metadata !7, metadata !DIExpression()), !dbg !6
+  ret void
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #2
+
+attributes #0 = { noinline norecurse nounwind readnone }
+attributes #1 = { noinline norecurse noreturn nounwind readonly }
+attributes #2 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 8.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !2)
+!1 = !DIFile(filename: "foo.c", directory: "/")
+!2 = !{}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 176, type: !5, isLocal: true, isDefinition: true, scopeLine: 177, flags: DIFlagPrototyped, isOptimized: false, unit: !0)
+!5 = !DISubroutineType(types: !2)
+!6 = !DILocation(line: 234, column: 116, scope: !4)
+!7 = !DILocalVariable(name: "local", scope: !4, file: !1, line: 221, type: !8)
+!8 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)