Skip to content

Added some sanity checks to function call compilation. This will catch *some* cases of ABI mismatch. #715

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.

Sign up for GitHub

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

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

Added some sanity checks to function call compilation. This will catch *some* cases of ABI mismatch. #715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,18 +270,11 @@ impl<'a, 'gcc, 'tcx> Builder<'a, 'gcc, 'tcx> {
actual_val.dereference(self.location).to_rvalue()
}
} else {
// FIXME: this condition seems wrong: it will pass when both types are not
// a vector.
// Check that the types are not "known to mismatch" or are vectors
assert!(
(!expected_ty.is_vector() || actual_ty.is_vector())
&& (expected_ty.is_vector() || !actual_ty.is_vector()),
"{:?} (is vector: {}) -> {:?} (is vector: {}), Function: {:?}[{}]",
actual_ty,
actual_ty.is_vector(),
expected_ty,
expected_ty.is_vector(),
func_ptr,
index
expected_ty.known_eq(&actual_ty, self.cx).unwrap_or(true)
|| expected_ty.is_vector() && actual_ty.is_vector(),
"{actual_ty:?} -> {expected_ty:?}, Function: {func_ptr:?}[{index}]"
);
// TODO(antoyo): perhaps use __builtin_convertvector for vector casting.
// TODO: remove bitcast now that vector types can be compared?
Expand Down
106 changes: 106 additions & 0 deletions src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,10 @@ pub trait TypeReflection<'gcc, 'tcx> {
fn is_u128(&self, cx: &CodegenCx<'gcc, 'tcx>) -> bool;

fn is_vector(&self) -> bool;
/// Checks if 2 types are "known to be equal". Returns Some(true) if types are equal(e.g. bool and bool),
/// Some(false) if they can't possibly be equal(e.g. a struct and a float), and None if there is no way
/// to check for their equality(struct and struct)
fn known_eq(&self, rhs: &Self, cx: &CodegenCx<'gcc, 'tcx>) -> Option<bool>;
}

impl<'gcc, 'tcx> TypeReflection<'gcc, 'tcx> for Type<'gcc> {
Expand Down Expand Up @@ -524,4 +528,106 @@ impl<'gcc, 'tcx> TypeReflection<'gcc, 'tcx> for Type<'gcc> {

false
}
fn known_eq(&self, other: &Self, cx: &CodegenCx<'gcc, 'tcx>) -> Option<bool> {
// "Happy" path: types represented using the same pointer
if self == other {
return Some(true);
}
if self.is_bool() {
return Some(other.is_bool());
} else if self.is_integral() {
// Int and.. something? Not equal.
if !other.is_integral() {
return Some(false);
}
// Do the intigers have the same size and sign?
return Some(
(self.get_size() == other.get_size())
&& (self.is_signed(cx) == other.is_signed(cx)),
);
} else if self.is_vector() {
// Vector and.. something? Different types.
if !other.is_vector() {
return Some(false);
}
// Both are vectors - try to get them directly
let (Some(lhs), Some(rhs)) = (self.dyncast_vector(), other.dyncast_vector()) else {
return None;
};
// Different element count - different types.
if lhs.get_num_units() != rhs.get_num_units() {
return Some(false);
}
// Same element - same type.
return lhs.get_element_type().known_eq(&rhs.get_element_type(), cx);
} else if let Some(lhs) = self.is_struct() {
// Struct and a not-struct? Different types.
let Some(rhs) = other.is_struct() else {
return Some(false);
};
// Different *field count*? Different types.
if lhs.get_field_count() != rhs.get_field_count() {
return Some(false);
}
// We can't get the type of a filed quite yet. So, we will say that we don't know if the structs are equal.
return None;
} else if let Some(s_ptr) = self.get_pointee() {
let Some(other_ptr) = other.get_pointee() else {
return Some(false);
};
return s_ptr.known_eq(&other_ptr, cx);
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it doesn't matter much since there's a check for pointers in the assert in the other file, but is this the expected behavior to check if the pointee types are equal since we can bitcast pointers of different types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code path calling this funcition in the assert should not be hit by pointers at all - it is meant to bitcast vectors.

The idea behind known_eq is to serve as a saner replacement for the current Eq implementation of Type.

As we discussed before, comparing Types / Rvalues on the libgccjit side is quite hard to do, and would require a lot of code to be added.

Here, I am seeing if I could possibly work around that issue, by doing some of that comparison logic on the Rust side.

Really, I am trying to see if I could implement the functions we require entirely on the Rust side, subverting the limitations of the GCC API.

One thing I am considering attempting is field-vise comparison of structs(so that structs with identical elements compare as equal).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be quite useful for our constant deduplication code. Since my last attempt at fixing that leak introduces non-determinism to the compiler, we will have to revert and replace that.

At one point, I suggested we compare the types of Rvalues before comparing them using string formatting.

I was told that will not work because we can't reliably compare types. known_eq solves that problem(once I get struct comparisons working). If 2 rvalues have different types, we know they can't be equal, so we don't do the string-based comparison.

I belive this would remove most of that memory leak in a sound and reliable way. I have other ways to aleviate that issue, but this will be the most impactfull.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still believe that using that using global_set_initializer() would be better than this approach. Was there any reason it would not work?

Also, I would be very wary of a struct comparison done like that: we need to take some attributes into account like packed and possibly some others: I much prefer to let that job to the gcc backend even though I'm not sure right now how that could work since it would involve the target part of the compiler.

Copy link
Contributor Author

@FractalFir FractalFir Jun 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

global_set_initializer(and globlal_set_initialized_rvalue) required globals to have unique names, and I think that messed with the string-based comparisons. I also belive could not get it to ever run right - global_set_initializer always errored out when I used it. I belive that was caused by some kind of type issue - it always complained about the type of the global and the type of the initializer rvalue being different. Maybe this is related to us calling get_aligned needlessly.

Also, I would be very wary of a struct comparison done like that: we need to take some attributes into account like packed and possibly some others: I much prefer to let that job to the gcc backend even though I'm not sure right now how that could work since it would involve the target part of the compiler.

Which is why known_eq will not return Some(true) for structs(at least for now). For structs, the current version only allows us to check that structs are unequal which is fine for our purposes.

Overall, the idea is for known_eq to return Some(true) if and only if the types are guaranteed to be equal, and return Some(false) if we can show that they are not equal.

For the global de-duplication, this comparison is simply an optimization. All it does skip the more costly comparison if we know for sure it will return false.

Additionally, for our purposes, catching most of the cases where types are not equal is already good enough.

If a sanity check can catch 99.9% of issues early, then it is worth it in my book.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also global_set_initializer will not help us with deduplication, at all. We will still have to manage the lvalues by ourselves, and deduplicate them ourselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also global_set_initializer will not help us with deduplication, at all.

Oh, sorry: I was mixing things up with the reduction of the memory usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

global_set_initializer(and globlal_set_initialized_rvalue) required globals to have unique names, and I think that messed with the string-based comparisons. I also belive could not get it to ever run right - global_set_initializer always errored out when I used it. I belive that was caused by some kind of type issue - it always complained about the type of the global and the type of the initializer rvalue being different. Maybe this is related to us calling get_aligned needlessly.

Please tell me if a fix to libgccjit related to global_set_initializer would help you with your fix the reduce the memory usage (e.g. remove the non-determinism). I could take a look.

} else if let Some(lhs_elem) = self.dyncast_array() {
// Array and not an array - not equal.
let Some(rhs_elem) = other.dyncast_array() else {
return Some(false);
};
// Mismatched elements - not equal
if !lhs_elem.known_eq(&rhs_elem, cx)? {
return Some(false);
}
return None;
} else if let Some(lhs_ptr) = self.dyncast_function_ptr_type() {
// Fn ptr and not fn ptr - not equal.
let Some(rhs_ptr) = other.dyncast_function_ptr_type() else {
return Some(false);
};
// Wrong argc
if lhs_ptr.get_param_count() != rhs_ptr.get_param_count() {
return Some(false);
}
// Wrong ret
if !lhs_ptr.get_return_type().known_eq(&rhs_ptr.get_return_type(), cx)? {
return Some(false);
}
// Wrong param count.
for idx in 0..lhs_ptr.get_param_count() {
if !lhs_ptr.get_param_type(idx).known_eq(&rhs_ptr.get_param_type(idx), cx)? {
return Some(false);
}
}
return None;
}
#[cfg(feature = "master")]
if self.is_floating_point() {
if !other.is_floating_point() {
return Some(false);
}
return Some(self.get_size() == other.get_size());
}
#[cfg(not(feature = "master"))]
{
fn is_floating_point<'gcc>(ty: &Type<'gcc>, cx: &CodegenCx<'gcc, '_>) -> bool {
ty.is_compatible_with(cx.context.new_type::<f32>())
|| ty.is_compatible_with(cx.context.new_type::<f64>())
}
if is_floating_point(self, cx) {
if !is_floating_point(self, cx) {
return Some(false);
}
return Some(self.get_size() == other.get_size());
}
}
// Unknown type...
None
}
}
Loading