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

[Yoric]

…th metadata arguments

Description

• We expand Operand to accept a new variant Metadata. This is probably a breaking change, so there may be a better way to do this.
• We expand Operand::get_operand_unchecked to create these variants when the operand is a metadata.
• Slightly expands the error messages in case of panic in AnyValueEnum::new and BasicValueEnum::new.
• Replaces the unreachable! of BasicValueEnum::new with a panic!, since it turns out to be reachable.
• Some testing.

There are still at least two unknowns in this version of the MR:

1. Right now, we return None for operands that are marked as LLVMMetadataTypeKind but that convert to null. Not sure that this is the right behavior.
2. So far, I've only ever used inkwell to inspect IR, not to produce it, so I'm not particularly confident about my test.

Related Issue

issue #633

How This Has Been Tested

New test test_metadata_as_operand.

Tests launched on macOS 12.7.6 with cargo test --features llvm18-1.

The following tests do not pass on my machine, but as far as I can tell, it's not related:

• test_object_file::test_symbol_iterator (assertion failed: has_symbol_a)
• test_targets::test_default_triple (unknown OS)

Waiting to see if it fares better in CI.

Option<Breaking Changes>

I didn't find any way to implement this change that did not introduce at least one variant to at least one enum.

Checklist

• [X] I have read the Contributing Guide

[ErisianArchitect]

Operand has some ease-of-use functions for the variants.

impl<'ctx> Operand<'ctx> {
    /// Determines if the [Operand] is a [BasicValueEnum].
    #[inline]
    #[must_use]
    pub fn is_value(self) -> bool {
        matches!(self, Self::Value(_))
    }

    /// Determines if the [Operand] is a [BasicBlock].
    #[inline]
    #[must_use]
    pub fn is_block(self) -> bool {
        matches!(self, Self::Block(_))
    }

    /// If the [Operand] is a [BasicValueEnum], map it into [Option::Some].
    #[inline]
    #[must_use]
    pub fn value(self) -> Option<BasicValueEnum<'ctx>> {
        match self {
            Self::Value(value) => Some(value),
            _ => None,
        }
    }

    /// If the [Operand] is a [BasicBlock], map it into [Option::Some].
    #[inline]
    #[must_use]
    pub fn block(self) -> Option<BasicBlock<'ctx>> {
        match self {
            Self::Block(block) => Some(block),
            _ => None,
        }
    }

    /// Expect [BasicValueEnum], panic with the message if it is not.
    #[inline]
    #[must_use]
    #[track_caller]
    pub fn expect_value(self, msg: &str) -> BasicValueEnum<'ctx> {
        match self {
            Self::Value(value) => value,
            _ => panic!("{msg}"),
        }
    }

    /// Expect [BasicBlock], panic with the message if it is not.
    #[inline]
    #[must_use]
    #[track_caller]
    pub fn expect_block(self, msg: &str) -> BasicBlock<'ctx> {
        match self {
            Self::Block(block) => block,
            _ => panic!("{msg}"),
        }
    }

    /// Unwrap [BasicValueEnum]. Will panic if it is not.
    #[inline]
    #[must_use]
    #[track_caller]
    pub fn unwrap_value(self) -> BasicValueEnum<'ctx> {
        self.expect_value("Called unwrap_value() on UsedValue::Block.")
    }

    /// Unwrap [BasicBlock]. Will panic if it is not.
    #[inline]
    #[must_use]
    #[track_caller]
    pub fn unwrap_block(self) -> BasicBlock<'ctx> {
        self.expect_block("Called unwrap_block() on UsedValue::Value.")
    }
}

Could you add one for the Metadata variant?

[Yoric]

Could you add one for the Metadata variant?

Done.

[TheDan64]

The code does not work across all LLVM versions and needs to be adjusted accordingly

[Yoric]

Apologies for the test failures, I'll need to install more versions of LLVM on my machine.

Given the speed of the beast, this will probably take the rest of the day 😓 .

[ErisianArchitect]

If you need it, I have a couple python scripts that can help simplify the process of building versions of LLVM from source. It works for me on Windows, might also work on other OSes as well.

[Yoric]

As far as I understand from looking at llvm-sys, the feature I depend on appeared with LLVM 17. So my tests now work with llvm16-0 and llvm18-1, I think that it should be sufficient.

Note that there are two tests broken on macOS with both versions, but this was the case already before my changes, afaict.

[ErisianArchitect]

Okay, I did some digging. Creating MetadataValue Operands is valid prior to LLVM 17, so this conditional code is invalid. I may be wrong, but you should be able to just conditionally compile out

            if LLVMIsAValueAsMetadata(operand).is_null() {
                return Err(());
            }

In versions prior to LLVM 17, and it should work as expected. I believe there are also other Metadata types beyond ValueAsMetadata, so this could also cause issues in cases where a Metadata type is not ValueAsMetadata.

[Yoric]

Okay, I did some digging. Creating MetadataValue Operands is valid prior to LLVM 17, so this conditional code is invalid. I may be wrong, but you should be able to just conditionally compile out

            if LLVMIsAValueAsMetadata(operand).is_null() {
                return Err(());
            }

In versions prior to LLVM 17, and it should work as expected. I believe there are also other Metadata types beyond ValueAsMetadata, so this could also cause issues in cases where a Metadata type is not ValueAsMetadata.

I'm not sure I follow. In my tests with LLVM 18, removing this check causes segfaults. Are you saying that it works with LLVM < 17?

[ErisianArchitect]

I'm saying that the ability for an Operand to be a MetadataValue, as far as I'm aware, predates 17 all the way to around version 3. Removing the check might cause segfaults because it might be a Value type (which is checked last) rather than a metadata type. That's my best guess. But I'm likely wrong.

If a Metadata Operand type is invalid prior to version 17, then none of the code involving Metadata Operand should be included in any version prior to 17. It all needs to be conditionally compiled out. But since Operands as MetadataValues predates version 17, I think that we need to go back to the drawing board here.

Although I should probably take a closer look.

[Yoric]

Alright, I've taken a deeper look at where the segfaults come from in my tests.

As it turns out, they show up in impl fmt::Debug for MetadataValue because we have null operands, which break somewhere in MetadataValue::get_node_values. I've plugged that particular hole (and simplified my code along the way), but I'm not entirely confident that it solves all the issues.

[ErisianArchitect]

It's going to be a pain, but I think this is something that is going to require a much closer look to figure out. I think that Dan would agree with me here.

[Yoric]

@ErisianArchitect Do you think the current implementation is worth pursuing? Otherwise, any suggestion?

[ErisianArchitect]

It's supported via llvm-sys, so it makes sense to have it. The problem with your code is that LLVMIsAValueAsMetadata was added in LLVM 17, but the ability to create Metadata values exists prior to LLVM 17.

That means that for versions prior to LLVM 17, this code:

        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)));
        }

would compile not compile. But it WOULD compile if you removed that inner if-statement that calls LLVMIsAValueAsMetadata. That's why I think this needs a closer look, because I wonder if it would be an error to attempt to return an Operand::Value when it would otherwise be Metadata.

[Yoric]

Note that I've removed LLVMIsAValueAsMetadata in the latest version of the PR.

[ErisianArchitect]

Ahh, I missed that. Does it work as expected for all three variants?