Add nullptr check to isLandingPad

[ptersilie]

Summary

Calling isLandingPad on an empty BasicBlock can lead to undefined behaviour, e.g. see the following program:

int main(void)
{
    // Start with a LLVM context.
    LLVMContext TheContext;

    // Make a module.
    Module *TheModule = new Module("mymod", TheContext);

    // Make a function
    std::vector<Type*> NoArgs = {};
    Type *u32 = Type::getInt32Ty(TheContext);
    FunctionType *FT = FunctionType::get(u32, NoArgs, false);
    Function *F = Function::Create(FT, Function::ExternalLinkage, "main", TheModule);

    // Make an empty block
    IRBuilder<> Builder(TheContext);
    BasicBlock *BB = BasicBlock::Create(TheContext, "entry", F);
    Builder.SetInsertPoint(BB);

    auto fnp = BB->getFirstNonPHI();
    assert(fnp == nullptr);

    // This can lead to UB!
    if (BB->isLandingPad()) {
        // do something
    }

    return 0;
}

This patch fixes this by using isa_and_nonnull instead of isa, which would result in isLandingPad always returning false when called on an empty BasicBlock. If there is a reason (e.g. performance) for not using isa_and_nonnull we could alternatively document that isLandingPad should not be called without checking getFirstNonPHI != nullptr first, which would avoid people making the same mistake I made.

[lebedev.ri]

No idea who would be the appropriate reviewer here.
I guess this makes sense, but here's identical problem
with very next function, BasicBlock::getLandingPadInst().

[fhahn]

Hm I think calling isLandingPad on an empty (=invalid) basic block might be a pre-condition violation we legitimately want to crash on (with assertions isa<> assert != 0)? I suspect there are a lot of functions that expect BBs with at least a valid terminator and would crash for empty basic blocks. Unless I am missing something I think it might be better to avoid calling it on invalid block in the client code.

[ptersilie]

isa does have an assertion that the argument must not be a nullptr. However, this assertion doesn't trigger when LLVM is compiled with assertions off (which as far as I can tell is the default), leading to undefined behaviour. I think at the very least we could add a comment to the documentation that isLandingPad should not be called without checking first that getFirstNonPHI() != nullptr. I'd be happy to update the PR if this is what you decide is the best course of action.

[lebedev.ri]

I suspect there are many functions that would need such comment.
Perhaps it would be best to place it somewhere in the docs, something along the lines of
"Warning: basic block with no terminator instruction is invalid, and must not be passed to certain API's",
but i think that might best go into a new patch.

llvm/lib/IR/BasicBlock.cpp

 
 void BasicBlock::replaceSuccessorsPhiUsesWith(BasicBlock *New) {
   this->replaceSuccessorsPhiUsesWith(this, New);
 }
 
 /// Return true if this basic block is a landing pad. I.e., it's
 /// the destination of the 'unwind' edge of an invoke instruction.
 bool BasicBlock::isLandingPad() const {
-  return isa<LandingPadInst>(getFirstNonPHI());
+  return isa_and_nonnull<LandingPadInst>(getFirstNonPHI());
 }
 
 /// Return the landingpad instruction associated with the landing pad.
 const LandingPadInst *BasicBlock::getLandingPadInst() const {
   return dyn_cast<LandingPadInst>(getFirstNonPHI());
 }
 
 Optional<uint64_t> BasicBlock::getIrrLoopHeaderWeight() const {