[Feature] We should be able to walk through the operands of a call wi…

src/values/basic_value_use.rs

@@ -7,8 +7,6 @@ use crate::basic_block::BasicBlock;
 use crate::values::{AnyValueEnum, BasicValueEnum, MetadataValue};
 
 /// Either [BasicValueEnum], a [BasicBlock] or a [Metadata].
-///
-/// Note that [Metadata] variants are only constructed for LLVM 17+.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub enum Operand<'ctx> {
     /// Represents a [BasicValueEnum].

src/values/enums.rs

@@ -461,6 +461,7 @@ impl<'ctx> AggregateValueEnum<'ctx> {
 
 impl<'ctx> BasicMetadataValueEnum<'ctx> {
     pub(crate) unsafe fn new(value: LLVMValueRef) -> Self {
+        assert!(!value.is_null());
         match LLVMGetTypeKind(LLVMTypeOf(value)) {
             LLVMTypeKind::LLVMFloatTypeKind
             | LLVMTypeKind::LLVMFP128TypeKind

src/values/instruction_value.rs

@@ -3,20 +3,17 @@ use llvm_sys::core::LLVMGetGEPSourceElementType;
 use llvm_sys::core::{
     LLVMGetAlignment, LLVMGetAllocatedType, LLVMGetFCmpPredicate, LLVMGetICmpPredicate, LLVMGetIndices,
     LLVMGetInstructionOpcode, LLVMGetInstructionParent, LLVMGetMetadata, LLVMGetNextInstruction, LLVMGetNumIndices,
-    LLVMGetNumOperands, LLVMGetOperand, LLVMGetOperandUse, LLVMGetPreviousInstruction,
+    LLVMGetNumOperands, LLVMGetOperand, LLVMGetOperandUse, LLVMGetPreviousInstruction, LLVMGetTypeKind,
     LLVMGetVolatile, LLVMHasMetadata, LLVMInstructionClone, LLVMInstructionEraseFromParent,
     LLVMInstructionRemoveFromParent, LLVMIsAAllocaInst, LLVMIsABasicBlock, LLVMIsAGetElementPtrInst, LLVMIsALoadInst,
-    LLVMIsAStoreInst, LLVMIsATerminatorInst, LLVMIsConditional, LLVMIsTailCall,
-    LLVMSetAlignment, LLVMSetMetadata, LLVMSetOperand, LLVMSetVolatile, LLVMValueAsBasicBlock,
+    LLVMIsAStoreInst, LLVMIsATerminatorInst, LLVMIsConditional, LLVMIsTailCall, LLVMSetAlignment, LLVMSetMetadata,
+    LLVMSetOperand, LLVMSetVolatile, LLVMTypeOf, LLVMValueAsBasicBlock,
 };
 #[llvm_versions(10..)]
 use llvm_sys::core::{LLVMGetAtomicRMWBinOp, LLVMIsAAtomicCmpXchgInst, LLVMIsAAtomicRMWInst};
-#[llvm_versions(17..)]
-use llvm_sys::core::{LLVMGetTypeKind, LLVMIsAValueAsMetadata, LLVMTypeOf};
 use llvm_sys::core::{LLVMGetOrdering, LLVMSetOrdering};
 use llvm_sys::prelude::LLVMValueRef;
 use llvm_sys::LLVMOpcode;
-#[llvm_versions(17..)]
 use llvm_sys::LLVMTypeKind;
 
 use std::{ffi::CStr, fmt, fmt::Display};
@@ -694,36 +691,13 @@ impl<'ctx> InstructionValue<'ctx> {
             return Some(Operand::Block(bb.expect("BasicBlock should always be valid")));
         }
 
-        match self.try_ingest_metadata(operand) {
-            // This is indeed metadata.
-            Ok(Some(metadata)) => return Some(Operand::Metadata(metadata)),
-            // This is null metadata, which we can't convert to an operand.
-            Err(_) => return None,
-            // Not metadata at all.
-            Ok(None) => {}
+        if unsafe { LLVMGetTypeKind(LLVMTypeOf(operand)) == LLVMTypeKind::LLVMMetadataTypeKind } {
+            return Some(Operand::Metadata(MetadataValue::new(operand)));
         }
 
         Some(Operand::Value(unsafe { BasicValueEnum::new(operand) }))
     }
 
-    #[llvm_versions(17..)] // LLVMIsAValueAsMetadata was introduced un 17.0.
-    unsafe fn try_ingest_metadata(self, operand: LLVMValueRef) -> Result<Option<MetadataValue<'ctx>>, ()> {
-        assert!(!operand.is_null());
-        if unsafe { LLVMGetTypeKind(LLVMTypeOf(operand)) == LLVMTypeKind::LLVMMetadataTypeKind } {
-            // We may be dealing with null metadata, which would break memory invariants.
-            if LLVMIsAValueAsMetadata(operand).is_null() {
-                return Err(());
-            }
-            return Ok(Some(MetadataValue::new(operand)));
-        }
-        Ok(None)
-    }
-    #[llvm_versions(..17)] // LLVMIsAValueAsMetadata was introduced un 17.0.
-    unsafe fn try_ingest_metadata(self, _: LLVMValueRef) -> Result<Option<MetadataValue<'ctx>>, ()> {
-        Ok(None)
-    }
-
-
     /// Get an instruction value operand iterator.
     pub fn get_operands(self) -> OperandIter<'ctx> {
         OperandIter {

src/values/metadata_value.rs

@@ -101,7 +101,7 @@ impl<'ctx> MetadataValue<'ctx> {
 
     // SubTypes: Node only one day
     // REVIEW: BasicMetadataValueEnum only if you can put metadata in metadata...
-    pub fn get_node_values(self) -> Vec<BasicMetadataValueEnum<'ctx>> {
+    pub fn get_node_values(self) -> Vec<Option<BasicMetadataValueEnum<'ctx>>> {
         if self.is_string() {
             return Vec::new();
         }
@@ -117,7 +117,13 @@ impl<'ctx> MetadataValue<'ctx> {
         };
 
         vec.iter()
-            .map(|val| unsafe { BasicMetadataValueEnum::new(*val) })
+            .map(|val| {
+                if val.is_null() {
+                    None
+                } else {
+                    Some(unsafe { BasicMetadataValueEnum::new(*val) })
+                }
+            })
             .collect()
     }
 

tests/all/test_instruction_values.rs

@@ -662,7 +662,6 @@ fn test_metadata_kinds() {
     ]);
 }
 
-#[llvm_versions(17..)]
 #[test]
 fn test_metadata_as_operand() {
     // clang can introduce instructions such as
@@ -713,10 +712,13 @@ fn test_metadata_as_operand() {
     );
     let instruction = debug_info_builder.insert_declare_at_end(i32_ptr, Some(var_info), None, debug_loc, block);
     assert_eq!(instruction.get_num_operands(), 4);
-    assert!(instruction.get_operand(0).is_some());
-    assert!(instruction.get_operand(1).is_none());
-    assert!(instruction.get_operand(2).is_none());
-    assert!(instruction.get_operand(3).is_some());
+    assert_eq!(instruction.get_operands().count(), 4);
+    for (i, operand) in instruction.get_operands().enumerate() {
+        assert!(operand.is_some());
+        // A cheap (and certainly insufficient) test that we can walk through
+        // the operand without segfaulting.
+        eprintln!("operand {i} is {:?}", operand);
+    }
 }
 
 #[test]

tests/all/test_values.rs

@@ -917,7 +917,11 @@ fn test_metadata() {
     let md_node_child = context.metadata_node(&[bool_val.into(), f32_val.into()]);
     let md_node = context.metadata_node(&[bool_val.into(), f32_val.into(), md_string.into(), md_node_child.into()]);
 
-    let node_values = md_node.get_node_values();
+    let node_values: Vec<_> = md_node
+        .get_node_values()
+        .iter()
+        .map(|v| v.expect("Node value should not have been none"))
+        .collect();
 
     assert_eq!(md_node.get_string_value(), None);
     assert_eq!(node_values.len(), 4);
@@ -938,7 +942,11 @@ fn test_metadata() {
 
     assert_eq!(global_md.len(), 1);
 
-    let md = global_md[0].get_node_values();
+    let md: Vec<_> = md_node
+        .get_node_values()
+        .iter()
+        .map(|v| v.expect("Node value should not have been none"))
+        .collect();
 
     assert_eq!(md.len(), 4);
     assert_eq!(md[0].into_int_value(), bool_val);
@@ -992,7 +1000,11 @@ fn test_metadata() {
     assert!(ret_instr.has_metadata());
     assert!(ret_instr.get_metadata(1).is_none());
 
-    let md_node_values = ret_instr.get_metadata(2).unwrap().get_node_values();
+    let md_node_values: Vec<_> = md_node
+        .get_node_values()
+        .iter()
+        .map(|v| v.expect("Node value should not have been none"))
+        .collect();
 
     assert_eq!(md_node_values.len(), 1);
     assert_eq!(