[Inliner] Check number of operands in AddReturnAttributes

[erthalion]

The commit 2da4960 enabled noundef attributes propagation. It looks like ret void is considered to be noundef, thus ValidUB.hasAttributes now returns true for this type of instructions and everything proceed further to work with operands. The issue is that such instruction doesn't have operands, which means when accessing RI->getOperand(0) inliner pass crashes with an assert:

llvm/include/llvm/IR/Instructions.h:3420: llvm::Value* llvm::ReturnInst::getOperand(unsigned int) const:
Assertion `i_nocapture < OperandTraits<ReturnInst>::operands(this) && "getOperand() out of range!"' failed.

Fix that by verifying if the ReturnInst in fact has some operands to process.

/cc @goldsteinn as an author of the commit referenced in [3], and @chandlerc as mentioned in code_owners regarding inlining pass.

Fixes #86162

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Dmitrii Dolgov (erthalion)

Changes

The commit 2da4960 enabled noundef attributes propagation. It looks like ret void is considered to be noundef, thus ValidUB.hasAttributes now returns true for this type of instructions and everything proceed further to work with operands. The issue is that such instruction doesn't have operands, which means when accessing RI->getOperand(0) inliner pass crashes with an assert:

llvm/include/llvm/IR/Instructions.h:3420: llvm::Value* llvm::ReturnInst::getOperand(unsigned int) const:
Assertion `i_nocapture < OperandTraits<ReturnInst>::operands(this) && "getOperand() out of range!"' failed.

Fix that by verifying if the ReturnInst in fact has some operands to process.

/cc @goldsteinn as an author of the commit referenced in [3], and @chandlerc as mentioned in code_owners regarding inlining pass.

Fixes #86162

---

Full diff: https://github.com/llvm/llvm-project/pull/87093.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Utils/InlineFunction.cpp (+1-1)

diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
index 833dcbec228b88..d5cf6bd036e07c 100644
--- a/llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -1384,7 +1384,7 @@ static void AddReturnAttributes(CallBase &CB, ValueToValueMapTy &VMap) {
 
   for (auto &BB : *CalledFunction) {
     auto *RI = dyn_cast<ReturnInst>(BB.getTerminator());
-    if (!RI || !isa<CallBase>(RI->getOperand(0)))
+    if (!RI || RI->getNumOperands() == 0 || !isa<CallBase>(RI->getOperand(0)))
       continue;
     auto *RetVal = cast<CallBase>(RI->getOperand(0));
     // Check that the cloned RetVal exists and is a call, otherwise we cannot

[aeubanks]

could you add a test so it's clearer what the input IR causing the crash is? or at least just paste some reduced IR that crashes with opt -passes=inline (can reduce with llvm-reduce)

it'd seem like noundef on a function that returns void shouldn't be valid

[aeubanks]

yeah that's not allowed (https://godbolt.org/z/ffM1qnfrn) so I'm not sure exactly what you're seeing. perhaps the input IR is already broken (run it through the verifier with opt -passes=verify to check)

[erthalion]

yeah that's not allowed (https://godbolt.org/z/ffM1qnfrn) so I'm not sure exactly what you're seeing. perhaps the input IR is already broken (run it through the verifier with opt -passes=verify to check)

Interesting, let me verify the IR.

[erthalion]

With help from @macdice we most likely found the reason behind #86162. PostgreSQL generates the module passed to the inliner, and it turns out one part of the generator copies function attributes, eventually applying noudef where it shouldn't be applied. Adding verify pass shows it early on, thanks for the advice!

Unless you folks think that the extra check proposed in this PR makes sense as a general defense measure, I'm going to close the PR and the issue.