Merge pull request rust-lang#121 from TheDan64/feature/safer-blocks

Add parent checking to BB methods. Fixes rust-lang#93

src/basic_block.rs

@@ -137,7 +137,7 @@ impl BasicBlock {
  if self.get_parent().is_none() {
  return None;
  }
- 
+
  let bb = unsafe {
  LLVMGetNextBasicBlock(self.basic_block)
  };
@@ -146,6 +146,7 @@ impl BasicBlock {
  }
 
  /// Prepends one `BasicBlock` before another.
+ /// It returns `Err(())` when either `BasicBlock` has no parent, as LLVM assumes they both have parents.
  ///
  /// # Example
  /// ```no_run
@@ -168,13 +169,21 @@ impl BasicBlock {
  /// assert_eq!(basic_block2.get_next_basic_block().unwrap(), basic_block1);
  /// ```
  // REVIEW: What happens if blocks are from different scopes?
- pub fn move_before(&self, basic_block: &BasicBlock) {
+ pub fn move_before(&self, basic_block: &BasicBlock) -> Result<(), ()> {
+ // This method is UB if the parent no longer exists, so we must check for parent (or encode into type system)
+ if self.get_parent().is_none() || basic_block.get_parent().is_none() {
+ return Err(());
+ }
+
  unsafe {
  LLVMMoveBasicBlockBefore(self.basic_block, basic_block.basic_block)
  }
+
+ Ok(())
  }
 
  /// Appends one `BasicBlock` after another.
+ /// It returns `Err(())` when either `BasicBlock` has no parent, as LLVM assumes they both have parents.
  ///
  /// # Example
  /// ```no_run
@@ -197,10 +206,17 @@ impl BasicBlock {
  /// assert_eq!(basic_block2.get_next_basic_block().unwrap(), basic_block1);
  /// ```
  // REVIEW: What happens if blocks are from different scopes?
- pub fn move_after(&self, basic_block: &BasicBlock) {
+ pub fn move_after(&self, basic_block: &BasicBlock) -> Result<(), ()> {
+ // This method is UB if the parent no longer exists, so we must check for parent (or encode into type system)
+ if self.get_parent().is_none() || basic_block.get_parent().is_none() {
+ return Err(());
+ }
+
  unsafe {
  LLVMMoveBasicBlockAfter(self.basic_block, basic_block.basic_block)
  }
+
+ Ok(())
  }
 
  /// Prepends a new `BasicBlock` before this one.
@@ -339,7 +355,8 @@ impl BasicBlock {
  Some(InstructionValue::new(value))
  }
 
- /// Removes this `BasicBlock` from its parent `FunctionValue`. Does nothing if it has no parent.
+ /// Removes this `BasicBlock` from its parent `FunctionValue`.
+ /// It returns `Err(())` when it has no parent to remove from.
  ///
  /// # Example
  /// ```no_run
@@ -364,16 +381,21 @@ impl BasicBlock {
  // by taking ownership of self (though BasicBlock's are not uniquely obtained...)
  // might have to make some methods do something like -> Result<..., BasicBlock<Orphan>> for BasicBlock<HasParent>
  // and would move_before/after make it no longer orphaned? etc..
- pub fn remove_from_function(&self) {
+ pub fn remove_from_function(&self) -> Result<(), ()> {
  // This method is UB if the parent no longer exists, so we must check for parent (or encode into type system)
- if self.get_parent().is_some() {
- unsafe {
- LLVMRemoveBasicBlockFromParent(self.basic_block)
- }
+ if self.get_parent().is_none() {
+ return Err(());
  }
+
+ unsafe {
+ LLVMRemoveBasicBlockFromParent(self.basic_block)
+ }
+
+ Ok(())
  }
 
  /// Removes this `BasicBlock` completely from memory. This is unsafe because you could easily have other references to the same `BasicBlock`.
+ /// It returns `Err(())` when it has no parent to delete from, as LLVM assumes it has a parent.
  ///
  /// # Example
  /// ```no_run
@@ -393,11 +415,15 @@ impl BasicBlock {
  /// }
  /// assert!(function.get_basic_blocks().is_empty());
  /// ```
- // REVIEW: Could potentially be unsafe if there are existing references. Might need a global ref counter
- pub unsafe fn delete(self) {
- // unsafe {
- LLVMDeleteBasicBlock(self.basic_block)
- // }
+ pub unsafe fn delete(self) -> Result<(), ()> {
+ // This method is UB if the parent no longer exists, so we must check for parent (or encode into type system)
+ if self.get_parent().is_none() {
+ return Err(());
+ }
+
+ LLVMDeleteBasicBlock(self.basic_block);
+
+ Ok(())
  }
 
  /// Obtains the `ContextRef` this `BasicBlock` belongs to.

tests/all/test_basic_block.rs

@@ -33,8 +33,8 @@ fn test_basic_block_ordering() {
  assert_eq!(basic_blocks[2], basic_block3);
  assert_eq!(basic_blocks[3], basic_block4);
 
- basic_block3.move_before(&basic_block2);
- basic_block.move_after(&basic_block4);
+ assert!(basic_block3.move_before(&basic_block2).is_ok());
+ assert!(basic_block.move_after(&basic_block4).is_ok());
 
  let basic_block5 = basic_block.prepend_basic_block("block5");
  let basic_blocks = function.get_basic_blocks();
@@ -62,7 +62,7 @@ fn test_basic_block_ordering() {
  assert!(basic_block3.get_previous_basic_block().is_none());
 
  unsafe {
- bb4.delete();
+ assert!(bb4.delete().is_ok());
  }
 
  let bb2 = basic_block5.get_previous_basic_block().unwrap();
@@ -153,21 +153,13 @@ fn test_no_parent() {
  assert_eq!(basic_block.get_parent().unwrap(), function);
  assert_eq!(basic_block.get_next_basic_block().unwrap(), basic_block2);
 
- // TODO: Test if this method is unsafe if parent function was hard deleted
- basic_block.remove_from_function();
+ assert!(basic_block.remove_from_function().is_ok());
 
  assert!(basic_block.get_next_basic_block().is_none());
  assert!(basic_block2.get_previous_basic_block().is_none());
 
- // The problem here is that calling the function more than once becomes UB
- // for some reason so we have to manually check for the parent function (in the call)
- // until we have SubTypes to solve this at compile time by doing something like:
- // impl BasicBlock<HasParent> { fn remove_from_function(self) -> BasicBlock<Orphan> }
- // though having to take ownership does raise some flags for when you just want to make
- // everything borrowable. I'm not sure it's possible to swap in place since they have
- // different subtypes
- basic_block.remove_from_function();
- basic_block.remove_from_function();
+ assert!(basic_block.remove_from_function().is_err());
+ assert!(basic_block.remove_from_function().is_err());
 
  assert!(basic_block.get_parent().is_none());
 }