improve support for detached instructions, basic blocks and functions

[nlewycky]

Describe the Bug
LLVM's memory management model is comprised of circular intrusive linked lists. The module owns an ordered list of global values, which may be Functions, GlobalVariables or GlobalAliases, a Function is a linked list of BasicBlocks, and a BasicBlock is a linked list of Instructions. The remove_from_parent() method detaches a node from its linked list parent leaving node with no concept of next or prev nodes.

From test_basic_block.rs:

    basic_block.remove_from_function();

    assert!(basic_block.get_next_basic_block().is_none());

At the moment, get_next_basic_block() doesn't check whether the block is detached, so it attempts to get the next block which is a misuse of LLVM API.

This also goes for similar functions get_previous_basic_block, move_before (self may be detached but basic_block may not), move_after (same) and prepend_basic_block.

Expected Behavior
It seems clear that get_next_basic_block() and get_previous_basic_block() should return None().

What should move_before(), move_after() and prepend_basic_block() do if asked to order something relative to a detached basic block?

LLVM Version (please complete the following information):

• LLVM Version: 7.0.1
• Inkwell Branch Used: master

Desktop (please complete the following information):

• OS: macOS 10.14.5

Additional Context
I've only looked at detached basic blocks, but the same rules apply for detached functions, detached instructions, and detached global values.

[TheDan64]

So I think get_next_basic_block and get_previous_basic_block were fixed by your recent changes which check for a parent, correct?

Is moving something relative to a detatched block safe to do in LLVM? If not we should make the return type Result<(), ()> for the remaining functions and return Err(()) if someone tries to move a bb relative to a detached bb?

[nlewycky]

It isn't safe, those functions will unconditionally dereference getParent(). Here's the entirety of moveBefore() for instance:

/// Unlink this basic block from its current function and
/// insert it into the function that MovePos lives in, right before MovePos.
void BasicBlock::moveBefore(BasicBlock *MovePos) {
  MovePos->getParent()->getBasicBlockList().splice(
      MovePos->getIterator(), getParent()->getBasicBlockList(), getIterator());
}

Looking at basic blocks only, the C functions affected are

• LLVMDeleteBasicBlock
• LLVMRemoveBasicBlockFromParent
• LLVMMoveBasicBlockBefore
• LLVMMoveBasicBlockAfter.

Result<(), ()> sounds right to me.