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

Pass raw pointers in FFI, generalize generator type handling #352

Closed
wants to merge 9 commits into from

Conversation

MarijnS95
Copy link
Collaborator

Similar to #351 this change was already well in the works when a shift to a second generator was announced. Submitting a PR here just to not loose the concepts, even if it might end up not getting reviewed/merged.

The main goal of this PR is to unify handling of types between unsafe (FFI) types and their "safe" counterpart (builder functions), and remove some case-specific broken code introduced in previous PRs again. Vkxml arrays versus references are pretty troublesome to deal with, here's hoping that can be dealt with in the pure vk_parse implementation of the new generator :)

Finally, FFI signatures are adjusted to use *const instead of & which represents a "fat" pointer in certain scenarios (16-bytes).

Comment on lines 2445 to 2539
for elem in &mut spec.elements {
match elem {
vkxml::RegistryElement::Definitions(definitions) => {
for el in &mut definitions.elements {
match el {
vkxml::DefinitionsElement::Struct(_struct) => {
for mem in &mut _struct.elements {
match mem {
vkxml::StructElement::Member(field) => {
if matches!(field.array, Some(vkxml::ArrayType::Dynamic))
&& field
.size
.as_ref()
// TODO: Should rather check if no identifier is a member variable
.map_or(false, |s| s.contains('*'))
{
field.array = Some(vkxml::ArrayType::Static);
}
}
_ => {}
}
}
}
_ => {}
}
}
}
_ => {}
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This entire bit is here because vk.xml does not contain something like type_t (*myvariable)[sz], where [ is the only way to get it detected as a static-length array. Instead, pointers to static sized arrays are represented as a single pointer with static length (len= and altlen= fields in vk.xml).

It should be moved to vkxml/vk_parse.

@MaikKlein
Copy link
Member

Finally, FFI signatures are adjusted to use *const instead of & which represents a "fat" pointer in certain scenarios (16-bytes).

Uhh that is really bad. Can't really release a new version with that bug. I'll try to hammer out the generator in the next couple of days, otherwise we might have to get in the fix for the "old" generator.

@MarijnS95
Copy link
Collaborator Author

@MaikKlein It shouldn't be a problem here because fat pointers are only used for DSTs: slice and trait types. &[FragmentShadingRateCombinerOpKHR; 2] (and blend_constants: &[f32; 4]) are neither of those:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=88acb9736455f7262ecf4af2077f3a59

Blend constants are in the current release already.

We can possibly go through this PR and merge it after cleaning up the override (eg. move it to an ash fork of vkxml/vk_parse?) or remove it altogether for the time being. Anyway, this issue is trivial to fix with just a single line, I'll PR it.

@MaikKlein
Copy link
Member

Ah puh hehe. I just skimmed the source code and thought that would be slices instead of arrays. Okay then we are good for now.

@MarijnS95
Copy link
Collaborator Author

@MaikKlein Pushed a PR to clean it up (trivial fix) and to prevent such confusion in the future. My bad for writing it like that without clarifying what "certain scenarios" means, on master as well as in 358f480 the situation is clearly around static-sized arrays anyway.

@MarijnS95
Copy link
Collaborator Author

Most of these patches have made it in one way or another, only type unification which is just unnecessary noise in the generator that's set for a rewrite only. Closing.

@MarijnS95 MarijnS95 closed this Dec 22, 2021
@MarijnS95 MarijnS95 deleted the simplify branch December 22, 2021 01:08
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.

2 participants