Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CallSiteValue::get_called_fn_value: return None on indirect calls #572

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

airwoodix
Copy link
Contributor

@airwoodix airwoodix commented Feb 21, 2025

Description

Fixes #571.

  • CallSiteValue::get_called_fn_value now matches the behavior of CallBase::getCalledFunction and returns None when the underlying instruction is an indirect call or a type inconsistency is detected (the latter only for LLVM >= 8).
  • The new CallSiteValue::get_called_fn_type corresponds to CallBase::getFunctionType and returns the type of the function called by the underlying call/invoke instruction. This is always well defined, whether the call is indirect or not. This new function only exists for LLVM >=8 because it depends on LLVMGetCalledFunctionType.

Related Issue

#571

How This Has Been Tested

Non-regression on #571 is tested by a dedicated test which uses a simplified version of the issue's reproduction code. For simplicity, this test is restricted to LLVM >= 15 (the test's IR snippet uses opaque pointers).

Doctests of CallSiteValue::get_called_fn_value and the new CallSiteValue::get_called_fn_type cover the usual-case, happy paths.

Breaking Changes

CallSiteValue::get_called_fn_value now returns an Option<FunctionValue> instead of a FunctionValue to model the fact that it may not be possible to retrieve the underlying function value (when the call is indirect).

Discussion

FunctionValue::new differs from most other type constructors (e.g. FunctionType::new) since it checks for the safety requirements and returns None if they are not met:

  • is it thus obsolete to have it marked unsafe?
  • is it the intention to have most type constructors behave this way, instead of being unsafe?

Checklist

@airwoodix airwoodix force-pushed the issue-571 branch 8 times, most recently from 181123f to a4229d5 Compare February 21, 2025 18:39
@airwoodix airwoodix marked this pull request as ready for review February 21, 2025 18:50
@TheDan64 TheDan64 self-requested a review February 22, 2025 23:43
#[llvm_versions(8..)]
#[inline]
fn get_called_fn_value_check_type_consistency(&self, fn_value: Option<FunctionValue<'ctx>>) -> Option<FunctionValue<'ctx>> {
fn_value.filter(|fn_value| fn_value.get_type() == self.get_called_fn_type())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use cargo fmt on your changes... it appears to be mixing in tabs and looks off

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I fixed formatting issues related to the changes in this PR. Other changes applied by rustfmt are in #575.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CallSiteValue::get_called_fn_value() fails it assert when the callee is a PointerValue
2 participants