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

Fix FFI specs on release builds #12601

Merged
merged 3 commits into from
Oct 17, 2022

Conversation

HertzDevil
Copy link
Contributor

Should fix #12592.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:specs topic:compiler:interpreter labels Oct 12, 2022
@straight-shoota
Copy link
Member

Wouldn't it make sense to move that constant into the actual code in the FFI namespace?

@HertzDevil
Copy link
Contributor Author

Crystal::FFI is not public yet and the interpreter itself doesn't need this type, so all this does is requiring longer qualified type names in that spec file.

@straight-shoota
Copy link
Member

In the specs we could just use Int64 everywhere without needing ffi_arg at all. That should promote correctly.

@HertzDevil
Copy link
Contributor Author

In case someone else or ourselves later try to publicize FFI based on what is already in the repository, it is a good idea to remind them of ffi_arg's existence.

@straight-shoota
Copy link
Member

Yes, that's exactly why I suggested to move it out of the spec into the implementation.
The current solution feels some middle ground compromise partly acknowledging, but not entirely. I'd either go full and move ffi_sarg to the implementation, or avoid it entirely (but mention it in a comment).

@beta-ziliani
Copy link
Member

@straight-shoota am I getting it right that you meant to move the code for the Sarg type to ./src/compiler/crystal/ffi/ffi.cr, instead of being in the spec as it is now?

@straight-shoota
Copy link
Member

Yes, either that or remove it entirely (because it's only used in the spec and there we can simply just use the bigger type with Int64 and then there's no need for it).

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Regardless of that, I'm fine with merging it as is - or with changes in either of the directions I suggested.
I'd like to get this fix into 1.6.1 and not hold up the release for long.

@asterite
Copy link
Member

But this can be done after 1.6.1. Just don't run the specs in release mode. I think ffi has been there since 1.5.0

@HertzDevil
Copy link
Contributor Author

In theory we cannot take an Int64*, pass it to FFI as a Void*, and then have FFI reinterpret it as an Int32* because that would break on big-endian systems. So it is good practice to always use ffi_sarg and never assume Int64 fits everything.

@straight-shoota
Copy link
Member

I don't want to reinterpret Int64 as Int32. I just want to switch the return type to Int64 entirely.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Thank you ❤️

@straight-shoota straight-shoota added this to the 1.6.1 milestone Oct 14, 2022
@straight-shoota straight-shoota merged commit 41f861e into crystal-lang:master Oct 17, 2022
@HertzDevil HertzDevil deleted the bug/libffi-spec branch October 23, 2022 05:21
lbguilherme pushed a commit to lbguilherme/crystal that referenced this pull request Oct 24, 2022
@GeopJr GeopJr mentioned this pull request Nov 21, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:specs topic:compiler:interpreter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ffi_spec fails with --release
4 participants