This is an archive of the discontinued LLVM Phabricator instance.

Allow null-valued function operands in getCalledFunction()
ClosedPublic

Authored by dstenb on Sep 26 2018, 2:09 AM.

Details

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.

Diff Detail

Event Timeline

dstenb created this revision.Sep 26 2018, 2:09 AM
vsk added a subscriber: hfinkel.Sep 26 2018, 8:13 AM

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.]

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 updated this revision to Diff 167728.Oct 1 2018, 7:05 AM
dstenb retitled this revision from Ignore dropped call operands when allocating MD slot numbers to Allow null-valued function operands in getCalledFunction().
dstenb edited the summary of this revision. (Show Details)
dstenb added a reviewer: hfinkel.
  • Changed so that getCalledFunction() allows null valued operands.
  • Rewrote the test case to a unit test.
dstenb added a comment.Oct 1 2018, 7:12 AM
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!

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.)

Any thoughts on this?

vsk added a comment.Oct 11 2018, 9:41 AM

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

dstenb added a comment.Nov 1 2018, 3:37 AM

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

This revision is now accepted and ready to land.Nov 1 2018, 10:14 AM
dstenb added a comment.Nov 2 2018, 1:21 AM

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

This revision was automatically updated to reflect the committed changes.